-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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: simplify insights sagas, fix contention query filter #90784
ui: simplify insights sagas, fix contention query filter #90784
Conversation
78aed76
to
b5ef2a2
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.
since you closed #88830, you can update the PR description with the unblock on it
Nice work, glad to see it is working! I added a comment on the video, not sure if was related to your changes, but looks like the column selector didn't work. You can address this on this PR or on a following one.
Reviewed 14 of 14 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ericharmeling)
… selector This commit simplifies the insights sagas by moving the insights store refresh interval to the components and removing the root-level reset saga. The commit also updates the SQL query for the transaction contention events to not JOIN with the internal insights table, and to filter out all unresolved txn fingerprint IDs. The commit also properly handles the case in which no columns are selected. Fixes cockroachdb#90142. Release note: None
b5ef2a2
to
9bb0af5
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.
you can update the PR description with the unblock on it
Done
I added a comment on the video, not sure if was related to your changes, but looks like the column selector didn't work. You can address this on this PR or on a following one.
Good catch! The column selector was hooked up, but there was a bug in how the column selection was stored. In the case in which all columns were unselected, the selection was stored as null
, which resulted in the column selection showing the default columns. Fixed!
Here's a loom: https://www.loom.com/share/8857b4a22f2c4a129d58e315dc18f6c7
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @maryliag)
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.
Thanks for fixing that too!
Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ericharmeling)
bors r+ |
Build failed: |
bors retry |
Build succeeded: |
This PR simplifies the insights sagas by moving the insights store refresh interval to the components and removing the root-level reset saga. The commit also updates the SQL query for the transaction contention events to not JOIN with the internal insights table, and to filter out all unresolved txn fingerprint IDs.
Fixes #90142.
Release note: None
Loom: https://www.loom.com/share/f12685c712124560b326f211b44a06d1