Skip to content

roachpb, server, ui: add fingerprint ID to details pages#91885

Merged
craig[bot] merged 3 commits intocockroachdb:masterfrom
maryliag:fingerprint-details
Nov 15, 2022
Merged

roachpb, server, ui: add fingerprint ID to details pages#91885
craig[bot] merged 3 commits intocockroachdb:masterfrom
maryliag:fingerprint-details

Conversation

@maryliag
Copy link
Copy Markdown
Contributor

Commit 1
ui: use format from cluster-ui
The format.js file was duplicated on db console,
making it hard to maintain.
This commit replaces all usages of the format
functions on db-console package, to the ones
from cluster-ui.

Epic: None

Release note: None

Commit 2
ui: add leading zeros to hex value with less than 16 chars
Previously, some fingerprint IDs in hex format would have
a leading 0, that was not being returning from the DB calls.
This was causing confusion when seeing the value on the UI
and trying to find the same fingerprint on our tables.

This commits checks the hex values used and add the missing
leading zeros back.

Part Of: #91763

Release note (bug fix): Add leading zeros to fingerprint IDs
with less than 16 characters.

Commit 3
roachpb, server, ui: add fingerprint ID to details pages
Previously, there was no easy way to get a statement or
fingerprint id from their respective details pages,
allowing then the value to be found on our internal tables
using cli.
This commit adds the fingerprint id on the response of
the statement details page.
It also adds the fingerprint ID to the Statement Details
page and Transaction Details page.

Fixes #91763

Screen Shot 2022-11-14 at 6 53 40 PM

Screen Shot 2022-11-14 at 6 53 58 PM

Release note (ui change): Add fingerprint ID in hex format
to the Statement Details page and Transaction Details page.

The format.js file was duplicated on db console,
making it hard to maintain.
This commit replaces all usages of the format
functions on db-console package, to the ones
from cluster-ui.

Epic: None

Release note: None
Previously, some fingerprint IDs in hex format would have
a leading 0, that was not being returning from the DB calls.
This was causing confusion when seeing the value on the UI
and trying to find the same fringerprint on our tables.

This commits checks the hex values used and add the missing
leading zeros back.

Part Of: cockroachdb#91763

Release note (bug fix): Add leading zeros to fingerprint IDs
with less than 16 characters.
@maryliag maryliag requested review from a team and dongniwang November 15, 2022 00:39
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@maryliag maryliag force-pushed the fingerprint-details branch 2 times, most recently from 0ca0b8a to be2cf04 Compare November 15, 2022 02:00
Copy link
Copy Markdown
Contributor

@j82w j82w left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 16 of 16 files at r1, 5 of 6 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dongniwang and @maryliag)


pkg/ui/workspaces/cluster-ui/src/util/format.ts line 265 at r2 (raw file):

    return "";
  }
  if (s?.length === 16) {

Is this check needed? The while will just get skipped if it is 16.


pkg/ui/workspaces/cluster-ui/src/util/format.ts line 261 at r3 (raw file):

// CheckHexValue adds the leading 0 on strings with hex value that
// have a length < 16.
export function CheckHexValue(s: string): string {

Should it be named more like FixFingerprintHexValue? Can the comment also be updated to say that .toString(16) removes the 0s and it needs to be added back to match the sql tables?

Previously, there was no easy way to get a statement or
fingerprint id from their respective details pages,
allowing then the value to be found on our internal tables
using cli.
This commit adds the fingerprint id on the response of
the statement details page.
It also adds the fingeprint ID to the Statement Details
page and Transaction Details page.

Fixes cockroachdb#91763

Release note (ui change): Add fingerprint ID in hex format
to the Statement Details page and Transaction Details page.
@maryliag maryliag force-pushed the fingerprint-details branch from be2cf04 to 66730b8 Compare November 15, 2022 14:41
Copy link
Copy Markdown
Contributor Author

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @dongniwang and @j82w)


pkg/ui/workspaces/cluster-ui/src/util/format.ts line 265 at r2 (raw file):

Previously, j82w wrote…

Is this check needed? The while will just get skipped if it is 16.

Good point! I was doing it slightly different initially and forgot to remove that part! thanks for the catch!


pkg/ui/workspaces/cluster-ui/src/util/format.ts line 261 at r3 (raw file):

Previously, j82w wrote…

Should it be named more like FixFingerprintHexValue? Can the comment also be updated to say that .toString(16) removes the 0s and it needs to be added back to match the sql tables?

Done

@maryliag
Copy link
Copy Markdown
Contributor Author

TFTR!
bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Nov 15, 2022

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add human-readable query identifiers in crdb_internal.statement_statistics and DB Console Statements page

3 participants