ui: add statement fingerprint to insights#96872
ui: add statement fingerprint to insights#96872craig[bot] merged 1 commit intocockroachdb:masterfrom j82w:91655_3
Conversation
|
This change is |
THardy98
left a comment
There was a problem hiding this comment.
Just watched the demo.
On the page listing the details of a transaction execution insight, would it be possible to link the Statement Waiting Execution ID to its corresponding statement execution insight page?
Reviewable status:
complete! 0 of 0 LGTMs obtained
maryliag
left a comment
There was a problem hiding this comment.
Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status:
-- commits line 7 at r1:
what happens if a user renames one of these items?
pkg/sql/crdb_internal.go line 6459 at r1 (raw file):
contention_duration INTERVAL NOT NULL, contending_key BYTES NOT NULL, contending_pretty_key STRING NOT NULL,
nit: spacing
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/insightDetailsTables.tsx line 61 at r1 (raw file):
InsightExecEnum.STATEMENT, ), cell: (item: ContentionEvent) => item.waitingStmtFingerprintID,
can you make this be a link to the details page for the fingerprint?
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/insightDetailsTables.tsx line 160 at r1 (raw file):
name: "databaseName", title: insightsTableTitles.databaseName(execType), cell: (item: ContentionDetails) => item.databaseName,
should we also add link for the database/table/index page? If we do, you might have to save the ids instead of the names, in case people rename and you might get a database not found of even if another get renamed with the same name, users would get confused
pkg/sql/crdb_internal_test.go line 986 at r1 (raw file):
waiting_txn_fingerprint_id, 'hex' ) AS waiting_txn_fingerprint_id, contending_pretty_key,
nit: spacing
|
To create a link for the statement fingerprint it is required to know if it is implicit or not. It should be possible to get the information, but figured it would be better to do it in a follow up PR since this is already a large PR. |
Previously, maryliag (Marylia Gutierrez) wrote…
It gets the name when the table is generated. It's possible if the user deletes a table or index that it will be empty string. |
|
Previously, maryliag (Marylia Gutierrez) wrote…
When the virtual table is generated the contention key is parsed to the get the ids. It then gets the names based on the ids. It will always have the correct name even if it is renamed. The one issue is if users delete a table or index. If they deleted then there shouldn't be a reason to investigate a contention event. |
|
I added the link in the latest iteration. |
|
Previously, maryliag (Marylia Gutierrez) wrote…
I figured it would be better to do in a follow up PR. We don't have if the statement is implicit or not which is required to create the link. |
|
Previously, j82w (Jake) wrote…
Done |
maryliag
left a comment
There was a problem hiding this comment.
Nice!
Reviewed 8 of 8 files at r2, all commit messages.
Reviewable status:
Previously, j82w (Jake) wrote…
It gets the name when the table is generated. It's possible if the user deletes a table or index that it will be empty string.
Ah okay!
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/insightDetailsTables.tsx line 61 at r1 (raw file):
Previously, j82w (Jake) wrote…
Done
Awesome!
xinhaoz
left a comment
There was a problem hiding this comment.
Nice! Just some small nits and questions.
...aces/cluster-ui/src/insights/workloadInsightDetails/transactionInsightDetailsOverviewTab.tsx
Outdated
Show resolved
Hide resolved
j82w
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/api/stmtInsightsApi.ts line 180 at r4 (raw file):
Previously, xinhaoz (Xin Hao Zhang) wrote…
Do we surface these executions?
It looks like they all get filtered out. I'll remove the check.
pkg/ui/workspaces/cluster-ui/src/api/txnInsightsApi.ts line 216 at r4 (raw file):
Previously, xinhaoz (Xin Hao Zhang) wrote…
Can we extract this into its own file? I'm surprised there is no circular dependency between stmtInsightsApi and txnInsightsApi
Done.
1. Removes contention events from insights. This avoids tracking and storing duplicate information. 2. Add database_name, schema_name, table_name, index_name, contending_pretty_key to crdb_internal.transaction_contention_events. This avoids needing to join with multiple tables and makes it easier for users to understand. 3. Add waiting statement info to the insights transaction details page. This way if users have a large transaction with multiple statements with multiple contention events they can see which specific statement was waited on. 4. Add blocking transaction fingerprint to the insight statement details page. The user can now see which transaction is blocking the statement ,and it has a link to the blocking transaction activity page. closes: #91665 Release note (ui change): Add waiting statement id and fingerprint to insights transaction details page. Add blocking transaction id and fingerprint to the insights statement page.
|
Previously, xinhaoz (Xin Hao Zhang) wrote…
Fixed |
|
bors r+ |
|
Build succeeded: |
Removes contention events from insights. This avoids tracking and storing duplicate information.
Add database_name, schema_name, table_name, index_name, contending_pretty_key to crdb_internal.transaction_contention_events. This avoids needing to join with multiple tables and makes it easier for users to understand.
Add waiting statement info to the insights transaction details page. This way if users have a large transaction with multiple statements with multiple contention events they can see which specific statement was waited on.
Add blocking transaction fingerprint to the insight statement details page. The user can now see which transaction is blocking the statement ,and it has a link to the blocking transaction activity page.
https://www.loom.com/share/de37b2e3007d48c2a1e3ad39ed334e20
closes: #91665
Release note (ui change): Add waiting statement id and fingerprint to insights transaction details page. Add blocking transaction id and fingerprint to the insights statement page.