-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ui: add workload insight details page v1 #86325
ui: add workload insight details page v1 #86325
Conversation
7ba51b2
to
b928466
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Added a few comments
Reviewed 21 of 29 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling and @maryliag)
-- commits
line 9 at r1:
since this PR won't add all the info, can you create another issue to list things that would be missing so we can keep a track to add later on?
pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts
line 130 at r1 (raw file):
} // Details
nit: period
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/insightDetailsTables.tsx
line 30 at r1 (raw file):
title: insightsTableTitles.executionID(execType), cell: (item: EventExecution) => String(item.executionID), sort: (item: EventExecution) => String(item.executionID),
you don't need to convert to string for the sort (same thing below)
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/transactionInsights/transactionInsightsTable.tsx
line 42 at r1 (raw file):
</Link> ), sort: (item: InsightEvent) => String(item.executionID),
you can remove the string from this sort too
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/insightCell.tsx
line 33 at r1 (raw file):
return ( <Tooltip key={crypto.randomBytes(16).toString("hex")}
is this to get rid of the warning about several elements with same key? if so, seems like you're using more complex things and doing conversion without the need, maybe you can try just Math.random()
or something like that (the key can be a string or number, so no conversion necessary)
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/queriesCell.tsx
line 42 at r1 (raw file):
<div> {transactionQueries.map((query, idx, arr) => ( <div key={crypto.randomBytes(16).toString("hex")}>
similar to my previous comment
pkg/ui/workspaces/cluster-ui/src/insightsTable/insightsTable.tsx
line 34 at r1 (raw file):
query?: string; execution?: executionDetails; details?: insightDetails;
will this work for statement and transactions? just thinking if you should be specific with transactionDetails or is okay to keep the name as just details
pkg/ui/workspaces/cluster-ui/src/store/insightDetails/insightDetails.sagas.ts
line 40 at r1 (raw file):
export function* receivedInsightDetailsSaga(delayMs: number) { yield delay(delayMs);
what is this for?
pkg/ui/workspaces/db-console/src/redux/apiReducers.ts
line 414 at r1 (raw file):
"insightDetails", insightRequestKey, moment.duration(10, "s"),
does it make sense to have a cache duration for the details? do we expect it to change?
also, can you confirm if we stay on the page, we don't hit that error we were hitting on the sql activity page, where a loading would show up and make the page get stuck
pkg/ui/workspaces/db-console/src/views/app/components/layoutSidebar/index.tsx
line 50 at r1 (raw file):
path: "/insights", text: "Insights", activeFor: [],
don't you need the item on the list here?
This commit adds the v1 Workload Insight Details page to the DB Console, via the cluster-ui package. The v1 Workload Insight Details page only includes details about a specific High Wait Time transaction insight event, populated with information served a new "endpoint" built on top of the SQL-over-HTTP API. Fixes cockroachdb#83775. Release note (ui change): Added new Workload Insight Details page to DB Console Release justification: low-risk updates to new functionality
b928466
to
bb2052a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TFTR! I think I got to all your feedback.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @maryliag)
Previously, maryliag (Marylia Gutierrez) wrote…
since this PR won't add all the info, can you create another issue to list things that would be missing so we can keep a track to add later on?
Done! #86405
pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts
line 130 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
nit: period
Done.
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/insightDetailsTables.tsx
line 30 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
you don't need to convert to string for the sort (same thing below)
Done.
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/transactionInsights/transactionInsightsTable.tsx
line 42 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
you can remove the string from this sort too
Done.
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/insightCell.tsx
line 33 at r1 (raw file):
is this to get rid of the warning about several elements with same key?
Yes!
you can try just Math.random() or something like that
Done.
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/queriesCell.tsx
line 42 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
similar to my previous comment
Done.
pkg/ui/workspaces/cluster-ui/src/insightsTable/insightsTable.tsx
line 34 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
will this work for statement and transactions? just thinking if you should be specific with transactionDetails or is okay to keep the name as just
details
It's the same for all execution types. It's just some additional details related to the insight type (and not the execution type).
pkg/ui/workspaces/cluster-ui/src/store/insightDetails/insightDetails.sagas.ts
line 40 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
what is this for?
I just removed this function and all of the cache invalidation stuff from cluster-ui for insight details. When we actually add connected components to cluster-ui for CC console, we can revisit the cluster-ui store stuff.
pkg/ui/workspaces/db-console/src/redux/apiReducers.ts
line 414 at r1 (raw file):
does it make sense to have a cache duration for the details? do we expect it to change?
I think it's possible that the number of queries and blocked queries increases? but none of the other information is going to change really. So maybe we just don't have a cache duration for insight details.
also, can you confirm if we stay on the page, we don't hit that error we were hitting on the sql activity page, where a loading would show up and make the page get stuck
Confirmed. No error as far as I can tell.
pkg/ui/workspaces/db-console/src/views/app/components/layoutSidebar/index.tsx
line 50 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
don't you need the item on the list here?
Reverted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 29 files at r1, 11 of 11 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @ericharmeling)
bors r+ |
Build failed (retrying...): |
Going to try borsing this again... |
bors retry |
Already running a review |
Build succeeded: |
This commit adds the v1 Workload Insight Details page to the DB Console, via the
cluster-ui
package. The v1 Workload Insight Details page only includes details about a specific High Wait Time transaction insight event, populated with information served a new "endpoint" built on top of the SQL-over-HTTP API.Note that this v1 page only includes information available on contention for individual transaction executions recorded in the
crdb_internal.transaction_contention_events
table. As most transaction statistics are aggregated across multiple transaction executions, there are some data missing in this v1, most notably: end time, rows processed, priority, full scan, retry count, last retry reason, session id.Fixes #83775.
https://www.loom.com/share/3f7dbd1326954c4ca7949ed8539ae4e9
Release note (ui change): Added new Workload Insight Details page to DB Console
Release justification: low-risk updates to new functionality