ui: add contention info to insight statement details #91668
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @j82w)
pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 665 at r1 (raw file):
// We only surface the most recently observed problem for a given statement. query: `SELECT insight.*, prettify_statement(non_prettified_query, 108, 2, 1) AS query
this value was change to 1 because of issue #91197. Using 2 could cause a problem, so you should keep the option 1 here
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/insightDetailsTables.tsx line 147 at r1 (raw file):
const columns = makeInsightStatementContentionColumns(); return ( <SortedTable className="statements-table" columns={columns} {...props} />
you're missing the props sortSetting and onChangeSortSetting. (which is also missing on the table above)
|
Previously, maryliag (Marylia Gutierrez) wrote…
Done. |
There was a problem hiding this comment.
great!
just a few smaller changes, otherwise
Reviewed 2 of 4 files at r2, 1 of 1 files at r3, 7 of 7 files at r4, all commit messages.
Reviewable status:
pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding.go line 304 at r4 (raw file):
// "schemaName": { "type": "string" }, // "databaseName": { "type": "string" }, // "tableName": { "type": "string" },
nit: the alignment is wrong (probably a mix of spaces and tabs)
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/statementInsightDetailsOverviewTab.tsx line 156 at r4 (raw file):
if (insightDetails.contentionEvents != null) { contentionTable = ( <Row>
you need some extra space at the bottom for smaller screens
<Row gutter={24} className={tableCx("margin-bottom")}>
| @@ -64,6 +64,15 @@ export type TransactionInsightEventDetails = { | |||
| execType: InsightExecEnum; | |||
| }; | |||
|
|
|||
| export type InsightContentionEvent = { | |||
There was a problem hiding this comment.
Can we clarify this is for stmts right now? Since we have a BlockedContentionDetails for transactions already it can get a little confusing
There was a problem hiding this comment.
Ideally we'd be able to merge the 2 in the future but I'm fine with keeping it separate right now since the other is based off the contention registry response
|
Previously, xinhaoz (Xin Hao Zhang) wrote…
Sure, what about BlockedStatementContentionDetails? |
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/insightDetailsTables.tsx line 147 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
you're missing the props
sortSettingandonChangeSortSetting. (which is also missing on the table above)
Done.
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/statementInsightDetailsOverviewTab.tsx line 156 at r4 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
you need some extra space at the bottom for smaller screens
<Row gutter={24} className={tableCx("margin-bottom")}>
Done.
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/insights/types.ts line 67 at r4 (raw file):
Previously, j82w wrote…
Sure, what about BlockedStatementContentionDetails?
Sounds good to me!
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/insightDetailsTables.tsx line 148 at r5 (raw file):
InsightContentionTableProps > = props => { const columns = makeInsightStatementContentionColumns();
Since this is actually creating the same object every time can we either lift this call out of the component or make the makeInsightStatementContentionColumns just an object instead of a func?
This changes the crdb_internal.cluster_execution_insights to store the names of contention key instead of the ids. This avoids complex join logic so the ui can display a eas format. This does not currently contain the blocking transaction fingerprint or the blocking statement information. The blocking fingerprint will be added in issue #91665 closes: #91187 Release note (ui change): Add the contention time, schema, database, table, and index info to insights statement details page.
|
Previously, xinhaoz (Xin Hao Zhang) wrote…
Making it a const causes the dev ui watch initial page to not load. I'm going to leave it as a function for now. If there is a solution please create a github issue, and I'll do a follow up pr. |
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/insights/types.ts line 67 at r4 (raw file):
Previously, xinhaoz (Xin Hao Zhang) wrote…
Sounds good to me!
Done.
|
bors r+ |
|
Build succeeded: |
This changes the crdb_internal.cluster_execution_insights to store
the names of contention key instead of
the ids. This avoids complex join logic so the
ui can display a eas format.
This does not currently contain the blocking
transaction fingerprint or the blocking statement
information. The blocking fingerprint will be added in issue #91665
closes: #91187
https://www.loom.com/share/03e08c5a473844b3a637b9eb618b7866
Release note (ui change): Add the contention
time, schema, database, table, and index info
to insights statement details page.