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: update label "CPU" to "SQL CPU" #116411

Merged
merged 1 commit into from Dec 14, 2023
Merged

Conversation

maryliag
Copy link
Contributor

@maryliag maryliag commented Dec 13, 2023

Previosuly, we were using the label "CPU Time" in the Console, which could be confusing since it was only SQL CPU Time.

This commit updates the title and tooltips to clarify that.

Fixes #116409

Updates on Insights and SQL Activity pages (Overview, Details, Search Criteria)
Screenshot 2023-12-13 at 7 36 06 PM

Screenshot 2023-12-13 at 7 36 24 PM Screenshot 2023-12-13 at 6 39 51 PM

Release note (ui change): Update "CPU Time" label to "SQL CPU Time" on the Console and add clarification to its tooltip.

@maryliag maryliag requested a review from a team as a code owner December 13, 2023 23:41
@maryliag maryliag added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Dec 13, 2023
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@maryliag maryliag added the backport-23.2.x Flags PRs that need to be backported to 23.2. label Dec 13, 2023
Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

:lgtm: Thanks!

Reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @florence-crl, @maryliag, and @mgartner)


pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/insightsColumns.tsx line 241 at r1 (raw file):

  cpu: (_: InsightExecEnum) => {
    return makeToolTip(
      <p>{`SQL CPU Time spent executing within the specified time interval. It 

nit: seems like a dangling space at the end.

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.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @florence-crl, @mgartner, and @yuzefovich)


pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/insightsColumns.tsx line 241 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: seems like a dangling space at the end.

fixed

Copy link

@florence-crl florence-crl left a comment

Choose a reason for hiding this comment

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

LGTM pending update of the tool tip to Alex's suggestion

@@ -238,7 +238,8 @@ export const insightsTableTitles: InsightsTableTitleType = {
},
cpu: (_: InsightExecEnum) => {
return makeToolTip(
<p>{`CPU Time spent executing within the specified time interval.`}</p>,
<p>{`SQL CPU Time spent executing within the specified time interval. It
excludes the time spent in the KV level as well as SQL planning time.`}</p>,

Choose a reason for hiding this comment

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

Alex's suggestion in slack sounds clearer:

Suggested change
excludes the time spent in the KV level as well as SQL planning time.`}</p>,
does not include SQL planning time nor KV execution.`}</p>,

indicates one standard deviation from the mean.
Average SQL CPU time spent executing within the specified time
interval. It excludes the time spent in the KV level as well as
SQL planning time. The gray bar indicates mean SQL CPU time. The

Choose a reason for hiding this comment

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

Suggested change
SQL planning time. The gray bar indicates mean SQL CPU time. The
interval. It does not include SQL planning time nor KV execution
time. The gray bar indicates mean SQL CPU time. The

Previosuly, we were using the label "CPU Time" in the
Console, which could be confusing since it was only SQL CPU Time.

This commit updates the title and tooltips to clarify that.

Fixes cockroachdb#116409

Release note (ui change): Update "CPU Time" label to "SQL CPU Time" on the
Console and add clarification to its tooltip.
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.

Tooltip and images updated! TFTRs!
Will merge once tests pass

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @mgartner and @yuzefovich)

@maryliag
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 14, 2023

Build succeeded:

@craig craig bot merged commit 25e400f into cockroachdb:master Dec 14, 2023
10 checks passed
Copy link

blathers-crl bot commented Dec 14, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 7f7eda7 to blathers/backport-release-23.1-116411: 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 23.1.x failed. See errors above.


error creating merge commit from 7f7eda7 to blathers/backport-release-23.2-116411: 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 23.2.x failed. See errors above.


error creating merge commit from 7f7eda7 to blathers/backport-release-23.2.0-rc-116411: 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 23.2.0-rc failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

update "CPU Time" labels to "SQL CPU Time"
4 participants