What issue this solves and before/after screenshots would be helpful for context!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jocrl and @THardy98)

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.fixture.ts, line 151 at r1 (raw file):

    statement: "SELECT city, id FROM vehicles WHERE city = $1",
    formattedStatement: "SELECT city, id FROM vehicles WHERE city = $1",

Could you modify this to demonstrate the difference between a normal statement and a formatted one?

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 165 at r1 (raw file):

    const formattedStatements = results.map(s => s.formatted_statement);
    const formattedStatement = formattedStatements.join();

I'm kinda confused what's going on here. Are you expecting multiple different formatted statements? I'm surprised you would format them nicely then just join everything with a comma.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @jocrl, and @THardy98)

pkg/server/combined_statement_stats.go, line 183 at r1 (raw file):

		alignMode := 2
		caseMode := 1
		metadata.Key.FormattedQuery, err = builtins.PrettyStatementCustomConfig(metadata.Key.Query, lineWidth, alignMode, caseMode)

When I mentioned that you can use the builtin within the RPC handler, I meant that you can embed the builtin directly within the SQL statements, instead of directly pulling the package in here.

So concretely, we currently fetch the statements as follow:

SELECT
  fingerprint_id,
  transaction_fingerprint_id,
  app_name,
  aggregated_ts,
  metadata,
  statistics,
  sampled_plan,
  aggregation_interval
FROM crdb_internal.statement_statistics

This will pull in metadata (which includes the query field) as is, but we can modify its query field by changing our SQL query using the builtin you just created

SELECT
  fingerprint_id,
  transaction_fingerprint_id,
  app_name,
  aggregated_ts,
  jsonb_set(
    metadata,
    array['query'],
    to_jsonb(
      prettify_statement(metadata ->> 'query')
    ))
  statistics,
  sampled_plan,
  aggregation_interval
FROM
  crdb_internal.statement_statistics;

Now rest of the RPC handler code can just remain unchanged and and the bulk of the work is left to the SQL engine.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @jocrl, and @THardy98)

pkg/sql/sem/builtins/builtins.go, line 8723 at r1 (raw file):

}

func PrettyStatementCustomConfig(

By using the new SQL query above, we don't have to export this function from the builtin package and we can just delete this.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @jocrl)

pkg/server/combined_statement_stats.go, line 183 at r1 (raw file):

ah, this makes more sense, my mistake I misunderstood.

in the case of an uncombined fetch, making a db query feels a bit counter-productive. Is importing the builtins logic in that scenario an acceptable solution, or would a db query okay?

pkg/sql/sem/builtins/builtins.go, line 8723 at r1 (raw file):

It's still used as part of the prettify_function builtin function. PrettyStatementCustomConfig allows for users to pass in custom parameters when calling prettify_function in the CLI to customize their config.

I will definitely un-export it, unless with that context in mind, you still think it doesn't provide much value.

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.fixture.ts, line 151 at r1 (raw file):

In this case, there is no difference (the statement string isn't long or complex enough to for the pretty-printer to change its formatting).

But I think this does highlight a potential issue.

If we decide to change our formatting config formattedStatement may not be represented correctly in this fixture.
I think the issue lies with where we store the config settings. If they are stored on the backend, then the console has no scope of potential config changes, which in turn means no visibility to how statements are formatted.

Not sure how we handle these kinds of settings.

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 165 at r1 (raw file):

I wasn't certain whether results would return multiple possible statements (although I think it only returns the one statement shown on the details page) so I thought a resolution would be to join the multiple formatted statements as a single formatted statement.

... it definitely shouldn't be comma-separated, but regardless, is it okay to assume that only 1 statement is returned here and we don't need to join anyway?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @jocrl)

pkg/sql/sem/builtins/builtins.go, line 8723 at r1 (raw file):

The custom config method is also currently being used when we fetch uncombined statements (formats on each statement iteration), restricting it from being un-exported.

That being said it could still at least be un-exported if we use the builtin function as you suggested for the combined logic, but I don't think the builtin function query for each statement iteration would be worth. Maybe a better solution would be to change the logic of how we add statements to the container, pre-populating the formatted statement field?

Another option could be to omit statement formatting for non-combined requests if the idea is to solely use combined requests going forward.

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 165 at r1 (raw file):

Removed the map and join, under the idea that a single statement is returned.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @jocrl, and @THardy98)

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.fixture.ts, line 151 at r1 (raw file):
Chatted at standup about the config storage; it'll just be a backend constant.

You mention that

If we decide to change our formatting config formattedStatement may not be represented correctly in this fixture.

The fixture is to use in storybook as an example of how the feature works. So similarly to how the onus is on you to add the field here, if the config changes the responsibility would be on the person who makes that change to also change the fixture example. You make a good point that there's no enforcement that ties this example together with what happens in the code. In this case I'm guessing you added the fixture field because typescript complained, but a change in the value of the string as you point out isn't a type change and won't be enforced.

I'd say that's an issue inherent with storybook - the point is to mock out what the back end provides the front, but that's only as realistic as the mock is accurate. So I'd file your concern under "well, there's a bigger general concern that we accept, so never mind 🙂".

That said, could you still instead change the string to something that's long or complex enough to demonstrate the difference? I see the point of storybook (and tests to an extent) as ways to demonstrate the scope of functionality including edge cases, different scenarios, and what the inputs and outputs look like. For storybook fixtures, the chosen statement should just be an arbitrary example so it should be fine to change the statement. If this specific select statement was demonstrating something I would expect a variable name or comment describing what it was demonstrating.

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 165 at r1 (raw file):

Err..., so the treatment of distSQL, node_id, and other fields in the return seems to imply that the filter statement above returns potentially more than one element in the results array 🙂.

I suspect that it would return multiple elements where the value of formattedStatement are all the same, but I suggest checking what actually happens. I think multiple elements for the same statement will happen if you lower the aggregation interval, so that there are multiple aggregation intervals with the same statement.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @jocrl, and @THardy98)

-- commits, line 2 at r4:
Since we also introduced a SQL builtin, it's a user-visible change within the SQL Shell, this would qualify this PR to also have sql change in its release note. Though it's probably better to split this into 2 commits

pkg/roachpb/app_stats.proto, line 190 at r4 (raw file):

  optional uint64 plan_hash = 10 [(gogoproto.nullable) = false];
  optional string query_summary = 12 [(gogoproto.nullable) = false];
  optional string formatted_query = 13 [(gogoproto.nullable) = false];

Any particular reason that we need to introduce a new protobuf field here? We can simply just overwrite the query field here with the prettified statement text before sending the protobuf response to the frontend.

Introducing a new field here means we are incurring additional storage cost in memory as well as on disk. Also, if we really do need to introduce a new field here, we also need to update our encoder/decoder accordingly (in pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil package) otherwise we won't be able to properly encode/decode this field.

pkg/server/combined_statement_stats.go, line 183 at r1 (raw file):

Hmmm good question 🤔 . My feeling here is that we can probably just leave the uncombined fetch alone (that is, do not prettify it). The reason behind that is that uncombined stats API really shouldn't be used by the frontend code at all. That was mostly left behind as an artifact of 21.1 and we are still in the process of cleaning it up. See #71829 for more details.

pkg/sql/conn_executor.go, line 518 at r4 (raw file):

	stmtStatsVisitor := func(_ context.Context, stat *roachpb.CollectedStatementStatistics) error {
		stat.Key.FormattedQuery, err = builtins.PrettyStatementCustomConfig(stat.Key.Query, lineWidth, alignMode, caseMode)

Since now the prettifying is handled by the API handler, do we still need to prettify the statement here?

pkg/sql/sem/builtins/builtins.go, line 8723 at r1 (raw file):

Maybe a better solution would be to change the logic of how we add statements to the container, pre-populating the formatted statement field?

I'm hesitant going down this route. There are a few reasons:

1. This incurs additional storage cost for an additional field.
2. This incurs additional compute cost for the write-path. Currently, SQL Stats's write path is on the hot path of query execution. I don't feel the additional benefit of the prettified statement text outweighs the worsened query latency

Another option could be to omit statement formatting for non-combined requests if the idea is to solely use combined requests going forward.

I like this idea a lot more. Then in this case perhaps we can just reuse the query field in the metadata protobuf messange. See my other comments for the reasoning.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @jocrl, and @THardy98)

pkg/roachpb/app_stats.proto, line 190 at r4 (raw file):

I wasn't sure if we were using the statement field for other reasons than displaying it on the details page, and didn't want to break anything by changing its contents (I think we retrieve it from the URL on the details page, not sure if we wanted the formatted statement in the URL).

If it is the case that it's okay to be replaced, I can make that change.

pkg/server/combined_statement_stats.go, line 183 at r1 (raw file):

Sweet. I'll remove it then.

pkg/sql/conn_executor.go, line 518 at r4 (raw file):

Nope. Will remove.

pkg/sql/sem/builtins/builtins.go, line 8723 at r1 (raw file):

Will remove. Left a comment regarding the query field in the protobuf message.

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.fixture.ts, line 151 at r1 (raw file):

You guess right :)

Thanks for the explanation, I'll definitely change the query string to something that warrants some formatting changes, good idea.

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 165 at r1 (raw file):

I'll revert it back.

Simple solution would be to create a set of formatted statements, then join them with a newline character.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @jocrl, and @THardy98)

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 165 at r1 (raw file):

Simple solution would be to create a set of formatted statements, then join them with a newline character.

I think it's worth taking a look at what the actual input looks like (hence the suggestion to lower the aggregation to recreate the input) before deciding what to do... since as you mention, I expect there to be only one statement, and so only one formatted statement. Maybe the same formatted statement is repeated in the array, so if you just join them with a newline you might get the same thing repeated a bunch.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @jocrl, and @THardy98)

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.fixture.ts, line 151 at r1 (raw file):

Oo, I should also mention that you can run storybook for db console and cluster ui by running yarn run storybook in their respective directories.

Storybook as mentioned is useful for enumerating and testing various cases without having to recreate the in the back end. E.g., if you wanted to test how a component displaying an error message behaves if the error message is super long, but you don't want to have to bother recreating a real error with a super long message in the db.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @jocrl, and @THardy98)

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 165 at r1 (raw file):

Taken a look. From my testing on an aggregate interval of 10s, it only returns one statement even if the statement is executed multiple times within the same aggregate interval.

That being said, I think the aforementioned implementation using a set still stands (a set will only keep unique statements so duplicates should not be an issue) and would cover the case where multiple results are returned if changes in functionality in the future allow for it.

If covering this case is overkill, it can easily be removed.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @jocrl)

-- commits, line 2 at r4:

added sql change to the release note, but have kept it as a single commit
is splitting into multiple commits common for a PR?

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.fixture.ts, line 151 at r1 (raw file):

Sweet :)

I've made changes to the statement details and statements page fixtures to show formatted statements, definitely better for transparency.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @jocrl, @maryliag, and @THardy98)

pkg/server/index_usage_stats.go, line 233 at r6 (raw file):

Not at all, I just replicated the pattern we used in combined_statement_stats.go, though using append in makeSQLQuery should've been intuitive...

changed to use makeSQLQuery.

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 163 at r12 (raw file):

Yup, forgot to update, nice catch :)

pkg/ui/workspaces/db-console/src/views/statements/statementDetails.tsx, line 193 at r12 (raw file):

updated :)

All lgtm! Just waiting for marylia to see if it's fine for us to change the semantics of statementAttr query params.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng, @jocrl, @maryliag, and @THardy98)

pkg/server/index_usage_stats.go, line 233 at r6 (raw file):

Awesome! Yeah we haven't been consistent with our SQL statement formatting throughout the code base (i'm partially responsible too 😅) but it's nice that we start to pay attentions to things like this.

The change on query params are okay. The code looks good, but I want to do some testing and then I will give the approval (hopefully later today)

Reviewed 1 of 18 files at r5, 4 of 21 files at r6, 1 of 16 files at r13, 15 of 15 files at r14, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng and @THardy98)

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 152 at r12 (raw file):

That change was necessary because of cases where the fingerprint was huge and would end up breaking the size, with the id we don't have this type of issue anymore and is what we're also using the transaction page. We don't need to worry about the case you mentioned.

Ooo, good point about the backwards compatibility of URLs. @maryliag, does this mean that we generally don't have to worry about backwards compatibility of URLs, as long as they are consistent (i.e. no broken links) within a version?

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng and @THardy98)

In general we do worry about URL compatibility, this is why sometimes we create redirection of routes.
For example, when I created the new SQL Activity page, I didn't remove the previous Statement and Transactions routes, instead I redirected them to the new SQL Activity route with the right tab selected.

For this particular case I don't think we should worry because those urls are very ephemeral, in the sense those stats are cleanup with time and is not something people would tend to bookmark, so adding all the extra code to convert from one type to the other is a lot of checks for something small, but I'm open to different opinions if you all things we should be handling that case.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng and @THardy98)

Great work on this!

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @Azhng and @THardy98)

🔥 🚀

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @Azhng and @THardy98)

since stmt.statement_fingerprint_id is a new field, you need to check for its existing on the frontend, for the cases where we have an older cluster with this ui. So I added some comments just about modifying to check the value before using

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @Azhng and @THardy98)

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 55 at r14 (raw file):

    if (!(key in statsKey)) {
      statsKey[key] = {
        statementFingerprintID: stmt.statement_fingerprint_id.toString(),

stmt.statement_fingerprint_id?.toString()

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 72 at r14 (raw file):

    const stmt = statsKey[key];
    return {
      aggregateFingerprintID: stmt.statementFingerprintID,

nit: aggregatedFingerprintID

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 117 at r14 (raw file):

  const filterByKeys = (stmt: ExecutionStatistics) =>
    stmt.statement_fingerprint_id.toString() === statementFingerprintID &&

stmt.statement_fingerprint_id?.toString()

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.selectors.ts, line 177 at r14 (raw file):

      if (!(key in statsByStatementKey)) {
        statsByStatementKey[key] = {
          statementFingerprintID: stmt.statement_fingerprint_id.toString(),

stmt.statement_fingerprint_id?.toString()

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.selectors.ts, line 194 at r14 (raw file):

      const stmt = statsByStatementKey[key];
      return {
        aggregateFingerprintID: stmt.statementFingerprintID,

nit: aggregatedFingerprintID

pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx, line 206 at r14 (raw file):

export interface AggregateStatistics {
  aggregateFingerprintID: string;

nit: aggregatedFingerprintID

pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts, line 103 at r14 (raw file):

    if (!(key in statsKey)) {
      statsKey[key] = {
        aggregateFingerprintID: s.statement_fingerprint_id.toString(),

stmt.statement_fingerprint_id?.toString()

nit: aggregatedFingerprintID

pkg/ui/workspaces/cluster-ui/src/util/appStats/appStats.ts, line 262 at r14 (raw file):

export function statementKey(stmt: ExecutionStatistics): string {
  return (
    stmt.statement_fingerprint_id.toString() +

stmt.statement_fingerprint_id?.toString()

pkg/ui/workspaces/db-console/src/views/statements/statementDetails.tsx, line 83 at r14 (raw file):

    if (!(key in statsKey)) {
      statsKey[key] = {
        statementFingerprintID: stmt.statement_fingerprint_id.toString(),

stmt.statement_fingerprint_id?.toString()

pkg/ui/workspaces/db-console/src/views/statements/statementDetails.tsx, line 100 at r14 (raw file):

    const stmt = statsKey[key];
    return {
      aggregateFingerprintID: stmt.statementFingerprintID,

nit: aggregatedFingerprintID

pkg/ui/workspaces/db-console/src/views/statements/statementDetails.tsx, line 145 at r14 (raw file):

  const filterByKeys = (stmt: ExecutionStatistics) =>
    stmt.statement_fingerprint_id.toString() === statementFingerprintID &&

stmt.statement_fingerprint_id?.toString()

pkg/ui/workspaces/db-console/src/views/statements/statementsPage.tsx, line 122 at r14 (raw file):

      if (!(key in statsByStatementKey)) {
        statsByStatementKey[key] = {
          statementFingerprintID: stmt.statement_fingerprint_id.toString(),

stmt.statement_fingerprint_id?.toString()

pkg/ui/workspaces/db-console/src/views/statements/statementsPage.tsx, line 139 at r14 (raw file):

      const stmt = statsByStatementKey[key];
      return {
        aggregateFingerprintID: stmt.statementFingerprintID,

nit: aggregatedFingerprintID

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.fixture.ts, line 166 at r14 (raw file):

    byNode: [
      {
        aggregateFingerprintID: fingerprintID,

nit: aggregatedFingerprintID

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.fixture.ts, line 289 at r14 (raw file):

  statements: [
    {
      aggregateFingerprintID: "1253500548539870016",

nit: aggregatedFingerprintID

@maryliag, if we have an older cluster with this UI, would it mean that the UI wouldn't work? (and is that fine?). Your changes make it so that it wouldn't give a hard error from trying to call toString() on null, but we still have a situation where statementFingerprintID is null, and it's supposed be used to identify statements in the details page, etc.

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @Azhng and @THardy98)

I hadn't considered this. I have the changes committed locally, but this raises a good point - we'll have an issue where multiple statements will match a null value if this field doesn't exist.

I thought that the statement fingerprint ID was provided by the backend but just not used. By that logic, we're using information that's already been provided to older clusters, which in turn should be able to support it. Or am I misunderstood and providing the statement fingerprint ID is a recent addition?

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @Azhng and @THardy98)

@THardy98 you're correct. We were already providing the id value from the backend, I mistakenly read the code as you adding the new value also on the backend, not just on the frontend code. This means that new versions of the ui running on older cluster should work fine.
I would still like you to add those checks just to be safe though. :)

Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @Azhng and @THardy98)

@maryliag just pushed changes :)

Reviewable status: complete! 0 of 0 LGTMs obtained (and 2 stale) (waiting on @Azhng, @maryliag, and @THardy98)

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 55 at r14 (raw file):

Done.

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 117 at r14 (raw file):

Done.

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.selectors.ts, line 177 at r14 (raw file):

Done.

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.selectors.ts, line 194 at r14 (raw file):

Done.

pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx, line 206 at r14 (raw file):

Done.

pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts, line 103 at r14 (raw file):

Done.

pkg/ui/workspaces/cluster-ui/src/util/appStats/appStats.ts, line 262 at r14 (raw file):

Done.

pkg/ui/workspaces/db-console/src/views/statements/statementDetails.tsx, line 83 at r14 (raw file):

Done.

pkg/ui/workspaces/db-console/src/views/statements/statementDetails.tsx, line 100 at r14 (raw file):

Done.

pkg/ui/workspaces/db-console/src/views/statements/statementDetails.tsx, line 145 at r14 (raw file):

Done.

pkg/ui/workspaces/db-console/src/views/statements/statementsPage.tsx, line 122 at r14 (raw file):

Done.

pkg/ui/workspaces/db-console/src/views/statements/statementsPage.tsx, line 139 at r14 (raw file):

Done.

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.fixture.ts, line 166 at r14 (raw file):

Done.

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.fixture.ts, line 289 at r14 (raw file):

Done.

awesome! 🚀

Reviewed 10 of 10 files at r15, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @Azhng and @THardy98)