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

cluster-ui: miscellaneous fixes and additions to statements/transactions page/details #222

Merged
merged 6 commits into from
Feb 27, 2021
Merged

Conversation

asubiotto
Copy link
Contributor

The first commit in this PR is #220, which should go in before this.

This PR addresses a lot of the nits observed in @awoods187' testing outlined in cockroachdb/cockroach#60001.

The last two commits add contention and max disk usage information to the pages based on previously agreed on designs by @Annebirzin. Here are some visuals:

Contention Time

image

Max scratch disk usage

image

@asubiotto
Copy link
Contributor Author

The commits are separated for ease of review. Let me know if you would prefer me to squash them.

@asubiotto
Copy link
Contributor Author

Need to update tests that assert the sort key.

@asubiotto
Copy link
Contributor Author

The PR this depends on has now been merged so this is ready for a review.

Copy link
Contributor

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

Reviewed 12 of 12 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @asubiotto, @nathanstilwell, and @nkodali)


packages/cluster-ui/src/statementsTable/statementsTable.tsx, line 85 at r1 (raw file):

      // TODO(asubiotto): Looks like none of these class names are defined?
      //  What's going on here?
      className: cx("statements-table__col-contention"),

I think you only need these if you want to override the widths of the column. We had a prior change where I pushed a commit eliminated most of these customer classes since they were unnecessary in most cases.

This column is no longer necessary because the transactions page exists.
The default sort order is specified using a column index. Previous commits that
rearranged these columns did not update the index, so an erroneous sort order
was observed.
…on details

This limits the number of decimal places to two, achieving better aesthetic
consistency.
This commit adds a contention time column that displays the amount of time
a statement spent contending and the cumulative time a transaction's
statements spent contending cumulatively in each respective page.
@asubiotto
Copy link
Contributor Author

Gotcha. Removed the undefined ones (that I think I added 😂 )

Copy link
Contributor

@dhartunian dhartunian left a comment

Choose a reason for hiding this comment

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

👍🏻

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.

2 participants