Skip to content
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: add CPU column on Insights #96279

Merged
merged 1 commit into from
Feb 2, 2023
Merged

Conversation

maryliag
Copy link
Contributor

@maryliag maryliag commented Jan 31, 2023

Part Of #87213

Adds CPU Time column on Statement and Transaction
Insights pages and their respective details pages.

Transaction Overview
Screen Shot 2023-01-31 at 12 37 20 PM

Transaction Details
Screen Shot 2023-01-31 at 12 37 52 PM

Statements Overview
Screen Shot 2023-01-31 at 12 37 36 PM

Statement Details
Screen Shot 2023-01-31 at 12 37 43 PM

Release note (ui change): Add CPU Time to Statement and
Transaction Insights.

@maryliag maryliag requested review from a team January 31, 2023 17:39
@maryliag maryliag requested a review from a team as a code owner January 31, 2023 17:39
@maryliag maryliag requested review from a team and removed request for a team January 31, 2023 17:39
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@gtr gtr 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 7 of 7 files at r1, 24 of 24 files at r2, 9 of 9 files at r3, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)


pkg/ui/workspaces/cluster-ui/src/insights/types.ts line 33 at r3 (raw file):

  application: string;
  contentionTime?: moment.Duration;
  cpuNanos: number;

Just to clarify, the change to cpuSQLNanos was only to be reflected on the sql package for crdb_internal and system tables for stmt statistics right? And when it comes to the naming in the UI package, cpuNanos should remain as-is?

@maryliag
Copy link
Contributor Author

Just to clarify, the change to cpuSQLNanos was only to be reflected on the sql package for crdb_internal and system tables for stmt statistics right? And when it comes to the naming in the UI package, cpuNanos should remain as-is?

Explained offline, but yes, the naming change is to reflect that the stats saved are for sql cpu only, but on the UI we will continue to call it just CPU

@matthewtodd
Copy link
Contributor

Explained offline, but yes, the naming change is to reflect that the stats saved are for sql cpu only, but on the UI we will continue to call it just CPU

I might suggest still calling it cpuSqlNanos in InsightEventBase, just to avoid a little micro-confusion comparing the frontend model code with the database, WDYT?

Part Of cockroachdb#87213

Adds CPU Time column on Statement and Transaction
Insights pages and their respective details pages.

Release note (ui change): Add CPU Time to Statement and
Transaction Insights.
Copy link
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.

Done

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @gtr)

Copy link
Contributor

@gtr gtr 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 8 of 8 files at r5, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @maryliag)

@maryliag
Copy link
Contributor Author

maryliag commented Feb 1, 2023

TFTR!
bors r+

@craig
Copy link
Contributor

craig bot commented Feb 1, 2023

This PR was included in a batch that was canceled, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Feb 2, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Feb 2, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Feb 2, 2023

Build succeeded:

@craig craig bot merged commit 6321c86 into cockroachdb:master Feb 2, 2023
@maryliag maryliag deleted the cpu-insight-ui branch February 2, 2023 14:43
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.

4 participants