ui: add Transaction and Statement fingerprint id #85464
Conversation
|
This change is |
There was a problem hiding this comment.
To clarify, the issues is to add the stmt/txn IDs to their respective overview pages, that is because the same row on the statement overview can have multiple transaction fingerprint IDs. So on this issue you should add only the Statement Fingerprint ID to the Statement overview table, and only the Transaction Fingerprint ID to the Transactions overview table.
Can you also add the ability to search on those fields?
Reviewable status:
complete! 0 of 0 LGTMs obtained
There was a problem hiding this comment.
Fixed in latest iteration
Reviewable status:
There was a problem hiding this comment.
just a few nits
Reviewed 7 of 8 files at r2.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx line 61 at r2 (raw file):
workloadPct: "% of All Runtime", lastExecTimestamp: "Last Execution Time (UTC)", statementFingerPrintId: "Statement Fingerprint ID",
nit: statementFingerprintID and transactionFingerprintID (and all usages like this in other locations)
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx line 226 at r2 (raw file):
style="tableTitle" placement="bottom" content={"The statement finger print id."}
nit: The statement fingerprint ID.
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx line 237 at r2 (raw file):
style="tableTitle" placement="bottom" content={"The transaction finger print id."}
nit: The transaction fingerprint ID.
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx line 226 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
nit: The statement fingerprint ID.
Please take a look at the expanded text.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status:
-- commits line 5 at r3:
To close the issue, this commit would also need the capability to search for the stmt/txn IDs on the search input of each page
from the comment here: #78509 (comment)
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx line 226 at r2 (raw file):
Previously, j82w wrote…
Please take a look at the expanded text.
Looks good!
There was a problem hiding this comment.
Reviewable status:
Previously, maryliag (Marylia Gutierrez) wrote…
To close the issue, this commit would also need the capability to search for the stmt/txn IDs on the search input of each page
from the comment here: #78509 (comment)
I added search support for the column. Do you think it would be better to add another field to AggregateStatistics to avoid needing to convert it back to a long then to hex?
There was a problem hiding this comment.
Reviewable status:
Previously, j82w wrote…
I added search support for the column. Do you think it would be better to add another field to AggregateStatistics to avoid needing to convert it back to a long then to hex?
I think is worth it, otherwise there is a lot of conversion going on.
pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx line 180 at r4 (raw file):
} return matchFingerPrintId.includes(search);
you can just do return matchString.includes(search) || matchFingerPrintId.includes(search); like you did below
There was a problem hiding this comment.
Reviewable status:
Previously, maryliag (Marylia Gutierrez) wrote…
I think is worth it, otherwise there is a lot of conversion going on.
Done.
pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx line 180 at r4 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
you can just do
return matchString.includes(search) || matchFingerPrintId.includes(search);like you did below
Done.
There was a problem hiding this comment.
Reviewed 6 of 7 files at r5.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.selectors.ts line 192 at r5 (raw file):
return { aggregatedFingerprintID: stmt.statementFingerprintID, aggregatedFingerprintHexID: stmt.statementFingerprintID,
you probably mean aggregatedFingerprintHexID: stmt.statementFingerprintHexID,
There was a problem hiding this comment.
Awesome! As long as tests are passing and you made sure the details page (both statement and transactions) are working properly
Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status:
j82w
changed the title
to SQL Activity overview page closes #78509 Release justification: Category 2: Bug fixes and low-risk updates to new functionality Release note (ui change): Adds Transaction and Statement fingerprint id to correlating SQL Activity overview pages. New columns are hidden by default.
|
bors r+ |
|
Build failed (retrying...): |
|
Build succeeded: |
|
blathers backport release-22.1 |
|
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 08f810f to blathers/backport-release-22.1-85464: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict [] you may need to manually resolve merge conflicts with the backport tool. Backport to branch release-22.1 failed. See errors above. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan. |
This adds the Transaction and Statement fingerprint id
to SQL Activity Statements overview page. The columns
are hidden by default.
closes #78509
Release justification: Category 2: Bug fixes and
low-risk updates to new functionality
Release note (ui change): Adds Transaction and Statement fingerprint
id to correlating SQL Activity overview pages. New columns are
hidden by default.