Skip to content

server,ui: transactionDetail page now shows statement stats scoped by transaction fingerprint ID#71967

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
Azhng:unhide-statements-for-txn-detail
Oct 26, 2021
Merged

server,ui: transactionDetail page now shows statement stats scoped by transaction fingerprint ID#71967
craig[bot] merged 1 commit intocockroachdb:masterfrom
Azhng:unhide-statements-for-txn-detail

Conversation

@Azhng
Copy link
Copy Markdown
Contributor

@Azhng Azhng commented Oct 25, 2021

Previously, transactionDetail page shows statement statistics aggregated
across all executions of that statement fingerprint. This led to
confusing UX and the statement statistics were subsequently removed
from transactionDetail page.
This commit reintroduces statement statistics on transactionDetail
page. The statistics now are only aggregated across the statement
fingerprints that are part of the selected transaction fingerprint.

Resolves #59205 #65106

Release note (ui change): transactionDetail page now shows statement
statistics scoped by the transaction fingeprint.

demo.mov

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@Azhng Azhng changed the title server,ui: transactionDetail page now shows statement stats scoped by server,ui: transactionDetail page now shows statement stats scoped by transaction fingerprint ID Oct 26, 2021
@Azhng Azhng force-pushed the unhide-statements-for-txn-detail branch from 57e2bc9 to 03cc84d Compare October 26, 2021 15:55
@Azhng Azhng requested a review from a team October 26, 2021 15:56
@Azhng Azhng marked this pull request as ready for review October 26, 2021 15:56
@Azhng Azhng force-pushed the unhide-statements-for-txn-detail branch from 03cc84d to a5a2175 Compare October 26, 2021 15:59
Copy link
Copy Markdown

@matthewtodd matthewtodd left a comment

Choose a reason for hiding this comment

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

Nice! Just a few nits and a question.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @Azhng)


-- commits, line 5 at r1:
nit: s/transactionDetail/the transactionDetail/


-- commits, line 5 at r1:
nit: s/shows/showed


-- commits, line 9 at r1:
nit: s/transactionDetail/the transactionDetail/


pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetails.tsx, line 147 at r1 (raw file):

                s.key.key_data.transaction_fingerprint_id.comp(
                  transactionFingerprintId,
                ) === 0,

Interesting, is there a reason not to use Long.equals here?

…d by

transaction fingerprint ID

Previously, the transactionDetail page showed statement statistics aggregated
across **all** executions of that statement fingerprint. This led to
confusing UX and the statement statistics were subsequently removed
from transactionDetail page.
This commit reintroduces statement statistics on the transactionDetail
page. The statistics now are only aggregated across the statement
fingerprints that are part of the selected transaction fingerprint.

Resolves cockroachdb#59205 cockroachdb#65106

Release note (ui change): transactionDetail page now shows statement
statistics scoped by the transaction fingeprint.
@Azhng Azhng force-pushed the unhide-statements-for-txn-detail branch from a5a2175 to d798002 Compare October 26, 2021 20:20
Copy link
Copy Markdown
Contributor Author

@Azhng Azhng 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 @matthewtodd)


-- commits, line 5 at r1:

Previously, matthewtodd (Matthew Todd) wrote…

nit: s/transactionDetail/the transactionDetail/

Done.


-- commits, line 5 at r1:

Previously, matthewtodd (Matthew Todd) wrote…

nit: s/shows/showed

Done.


-- commits, line 9 at r1:

Previously, matthewtodd (Matthew Todd) wrote…

nit: s/transactionDetail/the transactionDetail/

Done.


pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetails.tsx, line 147 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Interesting, is there a reason not to use Long.equals here?

TIL, changed to use Long.equals

Copy link
Copy Markdown

@matthewtodd matthewtodd left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @Azhng)

@Azhng
Copy link
Copy Markdown
Contributor Author

Azhng commented Oct 26, 2021

TFTR!

bors r=matthewtodd

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Oct 26, 2021

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.

ui: Transaction page inconsistent grouping of statement stats

3 participants