ui: link insights to fingerprint details #90403
Conversation
|
This change is |
There was a problem hiding this comment.
Looks great! I have a couple comments relating to the timestamp we use for the start time of transaction contention events, but I can probably address those in #90397.
Reviewed 23 of 23 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @maryliag)
pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 105 at r1 (raw file):
transactionID: row.waiting_txn_id, fingerprintID: row.waiting_txn_fingerprint_id, startTime: moment(row.collection_ts),
I think I made a mistake here in labeling the collection_ts as the "start time" of the transaction. We should be labeling this variable "collectedAt and the corresponding table column "Contention Collected At" (or something like that). I'm not sure we have a transaction start time readily available for contended transactions in our internal stats tables, since most of the info in those tables is aggregated across executions, but I could be wrong?
I call this out in #90397.
I suspect this could have caused problems with the link to the fingerprint details page, but it looks like you've gotten it working anyways.
Code quote:
startTime: moment(row.collection_ts),pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/detailsLinks.tsx line 29 at r1 (raw file):
const timeScale: TimeScale = { windowSize: moment.duration(1, "hour"), fixedWindowEnd: startTime.add(1, "hour"),
Because the transaction "start time" passed in from the transaction insights store is actually the contention collection time, would we want to use that value to create the window end?
Code quote:
fixedWindowEnd: startTime.add(1, "hour")
maryliag
force-pushed
the
add-link-details
branch
from
33380e1 to
f5d1b8e
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 105 at r1 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
I think I made a mistake here in labeling the collection_ts as the "start time" of the transaction. We should be labeling this variable "
collectedAtand the corresponding table column "Contention Collected At" (or something like that). I'm not sure we have a transaction start time readily available for contended transactions in our internal stats tables, since most of the info in those tables is aggregated across executions, but I could be wrong?I call this out in #90397.
I suspect this could have caused problems with the link to the fingerprint details page, but it looks like you've gotten it working anyways.
the reaosn it works is because it always go back to the X:00 hour, so I'm probably adding the missing time.
I'll leave your PR to fix the start time and focus this one on the links
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/detailsLinks.tsx line 29 at r1 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
Because the transaction "start time" passed in from the transaction insights store is actually the contention collection time, would we want to use that value to create the window end?
we don't have another value to use at this point, do we? I can adjust the windows size, so the start value would actually be a little earlier
maryliag
force-pushed
the
add-link-details
branch
from
f5d1b8e to
2575c6b
Compare
There was a problem hiding this comment.
Reviewed 1 of 7 files at r2.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 105 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
the reaosn it works is because it always go back to the X:00 hour, so I'm probably adding the missing time.
I'll leave your PR to fix the start time and focus this one on the links
SGTM!
pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/detailsLinks.tsx line 29 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
we don't have another value to use at this point, do we? I can adjust the windows size, so the start value would actually be a little earlier
SGTM!
Now from the Insights page, clicking on the statement or fingerprint ids, it will bring you to their respective details page. This commit also filter out transaction insights that didn't have their value set yet (meaning they're still 0). Finally, this commit fixes the start/end values being passed to the combined statement endpoint, to the correct rounded values, aligning what we say on the UI. Fixes cockroachdb#87750 Release note (ui change): The fingerprint id values for statement and transactions on the insights pages are links that open the respective details page on the time period of the execution of that statement/transaction. Release note (bug fix): Sendind the proper start/end values to the endpoint used on SQL Activity page, now returning the full hour as described on the UI.
maryliag
force-pushed
the
add-link-details
branch
from
2575c6b to
62b4f58
Compare
|
TFTR! |
|
Build failed: |
|
bors r+ |
|
Build succeeded: |
Now from the Insights page, clicking on the statement or fingerprint ids, it will bring you to their respective details page.
This commit also filter out transaction insights that didn't have their value set yet (meaning they're still 0).
Finally, this commit fixes the start/end values being passed to the combined statement endpoint, to the correct rounded values, aligning what we say on the UI.
Fixes #87750
https://www.loom.com/share/d9d6de0c78af4496b538a905732c745b
Note to reviewers: the bug displayed on the video will be addressed on #90397
Release note (ui change): The fingerprint id values for statement and transactions on the insights pages are links that open the respective details page on the time period of the execution of that statement/transaction.
Release note (bug fix): Sendind the proper start/end values to the endpoint used on SQL Activity page, now returning the full hour as described on the UI.