ui/sql: show summarized statements in the statements table#71534
Conversation
|
This change is |
bfa603d to
d9480c2
Compare
lindseyjin
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained
a discussion (no related file):
@Annebirzin
Btw, this is what the Statements page looks like with the table character limit set to 30 instead:
Let me know if you prefer this to the current 15 character limit, or if you need a larger screenshot!
Annebirzin
left a comment
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, lindseyjin (Lindsey Jin) wrote…
Btw, this is what the Statements page looks like with the table character limit set to 30 instead:
Let me know if you prefer this to the current 15 character limit, or if you need a larger screenshot!
@lindseyjin For a simple SELECT I think we could leave at 30 characters (since there is nothing after the db.table name to show). But wondering if for SELECT + JOIN, we should shorten the db.table name to 15 characters so that users could see that there is a JOIN. Does that make sense?
lindseyjin
left a comment
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, Annebirzin (Anne Birzin) wrote…
@lindseyjin For a simple SELECT I think we could leave at 30 characters (since there is nothing after the db.table name to show). But wondering if for SELECT + JOIN, we should shorten the db.table name to 15 characters so that users could see that there is a JOIN. Does that make sense?
I haven't actually implemented the JOIN formatting logic, since I thought based on our last discussion we would just show everything after FROM up to 30 characters (which would likely cover the first JOIN and maybe the ON clause as well)
Would it be ok to keep that system for SELECT + JOINs, or would you like me to implement JOIN formatting rules? It might be a little complicated, but I can look into it if you think it's important UX wise
Annebirzin
left a comment
There was a problem hiding this comment.
Reviewable status:
a discussion (no related file):
Previously, lindseyjin (Lindsey Jin) wrote…
I haven't actually implemented the JOIN formatting logic, since I thought based on our last discussion we would just show everything after FROM up to 30 characters (which would likely cover the first JOIN and maybe the ON clause as well)
Would it be ok to keep that system for SELECT + JOINs, or would you like me to implement JOIN formatting rules? It might be a little complicated, but I can look into it if you think it's important UX wise
ah right, I see now. Let's stick to the 30 characters for now (if we see that a lot of tables have long names we could introduce the JOIN logic) thanks for clarifying!
Azhng
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go, line 52 at r2 (raw file):
implicitTxn bool database string anonymizedStmtSummary string
I don't think we should add the anonymizedStmtSummary into the planKey, the plan is determined independent of the the summary text. We can easily look up whether if we have sampled the explain plan for a statement without the summary text.
Also I'm expecting we will be working towards shrinking down this struct in the future since it's rather bloated right now. Ideally this would just be a simple uint64 hash, but welp 🤷
Annebirzin
left a comment
There was a problem hiding this comment.
Reviewable status:
Azhng
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go, line 285 at r2 (raw file):
stmtStats.mu.distSQLUsed = statistics[i].Key.KeyData.DistSQL stmtStats.mu.fullScan = statistics[i].Key.KeyData.FullScan stmtStats.mu.database = statistics[i].Key.KeyData.Database
The issue where the new field is not surfaced in the frontend when it is removed from the planKey is probably because it is not explicitly added here. There's a lot of metadata we don't have in the planKey so we just add them in the stmtStats object here.
Let me know if you have more troubles surfacing this metadata to the frontend.
d9480c2 to
c2d85cc
Compare
lindseyjin
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go, line 52 at r2 (raw file):
Previously, Azhng (Archer Zhang) wrote…
I don't think we should add the
anonymizedStmtSummaryinto theplanKey, the plan is determined independent of the the summary text. We can easily look up whether if we have sampled the explain plan for a statement without the summary text.Also I'm expecting we will be working towards shrinking down this struct in the future since it's rather bloated right now. Ideally this would just be a simple
uint64hash, but welp 🤷
Done.
pkg/sql/sqlstats/ssmemstorage/ss_mem_storage.go, line 285 at r2 (raw file):
Previously, Azhng (Archer Zhang) wrote…
The issue where the new field is not surfaced in the frontend when it is removed from the
planKeyis probably because it is not explicitly added here. There's a lot of metadata we don't have in theplanKeyso we just add them in thestmtStatsobject here.Let me know if you have more troubles surfacing this metadata to the frontend.
Done, thanks for the advice!
a3bbb38 to
07f92d8
Compare
Azhng
left a comment
There was a problem hiding this comment.
This is awesome! Just few more minor nits from me
Reviewed 2 of 23 files at r2, 8 of 11 files at r4.
Reviewable status:
pkg/ui/workspaces/db-console/src/views/statements/statementDetails.tsx, line 101 at r4 (raw file):
return { label: stmt.nodeId.toString(), summary: stmt.nodeId.toString(),
Hmm, why are we assigning nodeId to summary?
pkg/ui/workspaces/db-console/src/views/statements/statementsPage.fixture.ts, line 477 at r4 (raw file):
"CREATE TABLE IF NOT EXISTS users (id UUID NOT NULL, city VARCHAR NOT NULL, name VARCHAR NULL, address VARCHAR NULL, credit_card VARCHAR NULL, PRIMARY KEY (city ASC, id ASC))", summary: "CREATE TABLE IF NOT EXISTS users (id UUID NOT NULL, city VARCHAR NOT NULL, name VARCHAR NULL, address VARCHAR NULL, credit_card VARCHAR NULL, PRIMARY KEY (city ASC, id ASC))",
Hmm, is this summary correct for the label? It seems it's a bit too long
lindseyjin
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/db-console/src/views/statements/statementsPage.fixture.ts, line 477 at r4 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Hmm, is this summary correct for the label? It seems it's a bit too long
Currently, the back-end formatter returns the default statement as the summary if we don't have specific summary rules (aka same as the label). I account for this in the front-end by using the old summary function (regex) if it's not a supported statement type.
Not sure if it would be better to return a null type in this case?
07f92d8 to
c259c5b
Compare
Azhng
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/db-console/src/views/statements/statementsPage.fixture.ts, line 477 at r4 (raw file):
Previously, lindseyjin (Lindsey Jin) wrote…
Currently, the back-end formatter returns the default statement as the summary if we don't have specific summary rules (aka same as the label). I account for this in the front-end by using the old summary function (regex) if it's not a supported statement type.
Not sure if it would be better to return a null type in this case?
Ah I see. I didn't realize we are only doing summary for a subset of statements. Do we have plans on expending the summary to support other statement types ?
lindseyjin
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/db-console/src/views/statements/statementDetails.tsx, line 101 at r4 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Hmm, why are we assigning
nodeIdtosummary?
fixed!
maryliag
left a comment
There was a problem hiding this comment.
Reviewed 13 of 23 files at r2, 3 of 3 files at r3, 4 of 11 files at r4.
Reviewable status:
-- commits, line 19 at r4:
nit: you don't need to go in details of how you made the change on the release note. This part is used to created the docs later on and the part needed here is just explaining what are the ui change the user will notice, so it can skip the beginning you added, e.g.
Release note (ui change): Update statements summaries on the Statements.... and so on
pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.fixture.ts, line 459 at r4 (raw file):
'CREATE TABLE IF NOT EXISTS user_promo_codes (city VARCHAR NOT NULL, user_id UUID NOT NULL, code VARCHAR NOT NULL, "timestamp" TIMESTAMP NULL, usage_count INT8 NULL, PRIMARY KEY (city ASC, user_id ASC, code ASC))', summary: 'CREATE TABLE IF NOT EXISTS user_promo_codes (city VARCHAR NOT NULL, user_id UUID NOT NULL, code VARCHAR NOT NULL, "timestamp" TIMESTAMP NULL, usage_count INT8 NULL, PRIMARY KEY (city ASC, user_id ASC, code ASC))',
is this one this long because we don't have summaries for CREATE stataments?
pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx, line 206 at r4 (raw file):
// label is either shortStatement (StatementsPage) or nodeId (StatementDetails). label: string; summary: string;
nit: the comment about label is also valid for summary, update it accordingly
pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTableContent.tsx, line 184 at r4 (raw file):
const summary = summarize(statement); // current statements that we support summaries for from the backend. const summarizedStmts = new Set(["select", "insert", "upsert", "update"]);
make sure to create a new issue or update the current one detailing the supported statement types, so the remaining can be addressed too
pkg/ui/workspaces/cluster-ui/src/util/appStats/appStats.spec.ts, line 129 at r4 (raw file):
key_data: { query: "UPDATE foobar SET name = 'baz' WHERE id = 42", query_summary: "UPDATE foobar SET name = 'baz' WHERE id = 42",
can you add a few more examples with statement that would actually have changed on the summary compared to the query itself
lindseyjin
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/db-console/src/views/statements/statementsPage.fixture.ts, line 477 at r4 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Ah I see. I didn't realize we are only doing summary for a subset of statements. Do we have plans on expending the summary to support other statement types ?
I think eventually we might support more summarized formats (cc: @Annebirzin correct me if I'm wrong?)
But for now we're just supporting select, insert/upsert, and update summaries!
maryliag
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 67 at r5 (raw file):
return { label: stmt.nodeId.toString(), summary: stmt.nodeId.toString(),
shouldn't this be stmt.statement_summary ?
lindseyjin
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.fixture.ts, line 459 at r4 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
is this one this long because we don't have summaries for CREATE stataments?
Yup, I'm passing in the default statement if we don't have a summary format, and then replacing it with the old summary from the front-end if that's the case
Resolves cockroachdb#27021 Previously, statements on the statements page hid too much information. There were complaints that it was difficult to disambiguate between statements without having to view the full query on the tooltips. The first commit in this patch implemented back-end changes to add a new metadata field for summarized queries, as well as formatting functions. This second commit implements additional logic to pass that new metadata to the front-end and display it in the Statements Table. Currently, we only create summaries of SELECT, INSERT/UPSERT, and UPDATE statements in the back-end. For all other statement types, we will continue to use the existing summary system. Release note (ui change): Show new statement summaries on the Statements page. This applies for SELECT, INSERT/UPSERT, and UPDATE statements, and will enable them to be more detailed and less ambiguous than our previous formats.
c259c5b to
259cdd9
Compare
lindseyjin
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, maryliag (Marylia Gutierrez) wrote…
nit: you don't need to go in details of how you made the change on the release note. This part is used to created the docs later on and the part needed here is just explaining what are the ui change the user will notice, so it can skip the beginning you added, e.g.
Release note (ui change): Update statements summaries on the Statements.... and so on
Done!
pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts, line 67 at r5 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
shouldn't this be stmt.statement_summary ?
Fixed, thanks for pointing that out!
pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx, line 206 at r4 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
nit: the comment about label is also valid for summary, update it accordingly
Er, I'm not sure if that comment also applies for summary since we don't use nodeId (at least now that I've fixed that). I have added a more descriptive comment though, so let me know if that works!
pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTableContent.tsx, line 184 at r4 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
make sure to create a new issue or update the current one detailing the supported statement types, so the remaining can be addressed too
Not sure if you meant a github or Jira issue, or both? But I've created a new github issue here: #71667
Let me know if that looks good and if there's anything I should change!
lindseyjin
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, maryliag (Marylia Gutierrez) wrote…
nit: you don't need to go in details of how you made the change on the release note. This part is used to created the docs later on and the part needed here is just explaining what are the ui change the user will notice, so it can skip the beginning you added, e.g.
Release note (ui change): Update statements summaries on the Statements.... and so on
Done!
lindseyjin
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/util/appStats/appStats.spec.ts, line 129 at r4 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
can you add a few more examples with statement that would actually have changed on the summary compared to the query itself
Done.
maryliag
left a comment
There was a problem hiding this comment.
great work on this!
Reviewed 2 of 11 files at r4, 1 of 1 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx, line 206 at r4 (raw file):
Previously, lindseyjin (Lindsey Jin) wrote…
Er, I'm not sure if that comment also applies for summary since we don't use nodeId (at least now that I've fixed that). I have added a more descriptive comment though, so let me know if that works!
I wrote that before realizing the change and actually retracted, so it was indeed no longer valid, but your new comment was very much welcome and helpful :D
pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTableContent.tsx, line 184 at r4 (raw file):
Previously, lindseyjin (Lindsey Jin) wrote…
Not sure if you meant a github or Jira issue, or both? But I've created a new github issue here: #71667
Let me know if that looks good and if there's anything I should change!
github is enough, thanks!
|
bors r+ |
|
Build succeeded: |
Resolves #27021
Previously, statements on the statements page hid too much information.
There were complaints that it was difficult to disambiguate between
statements without having to view the full query on the tooltips.
The first commit in this patch implemented back-end changes to add a new
metadata field for summarized queries, as well as formatting functions.
This second commit implements additional logic to pass that new metadata
to the front-end and display it in the Statements Table.
Currently, we only create summaries of SELECT, INSERT/UPSERT, and UPDATE
statements in the back-end. For all other statement types, we will
continue to use the existing summary system.
Release note (ui change): Show new statement summaries on the Statements
page. This applies for SELECT, INSERT/UPSERT, and UPDATE statements, and
will enable them to be more detailed and less ambiguous than our
previous formats.