cluster-ui: update statements page #178

asubiotto · 2021-01-22T15:35:05Z

This commit adds columns for execution stats that we want to display for 21.1.
Namely, this adds rows/bytes read, max memory, and network bytes sent. The
rows affected column has been removed.
The commit also reorders all column-related code to follow the graphical order.

Here is what that looks like with some example queries run against tpch:

Closes cockroachdb/cockroach#59294

Please let me know which tests to update and how. I have run into some issues with "accessing property mean of undefined" which is most likely due to unset proto fields. Is that something I need to handle in the protojs package in cockroach? If not, I'd appreciate if someone could point me to how to write tests for this and other cases.

I'm explicitly not adding code to handle cases when the stats haven't been collected. I leave this for the future if we decide to keep the sampling cluster setting. I focused on just getting the stats out there using preexisting components.

Checklist

Decide whether to remove the TXN Type column. The design doesn't have it, but want to double check cc @awoods187 @Annebirzin
I have written or updated test for the changes I made
I have updated the README of the package I'm working in to reflect my changes
I have added or updated Storybook if appropriate for my changes

cc @awoods187 @cockroachdb/sql-execution

asubiotto · 2021-01-22T15:39:21Z

Also, a note that I don't think this PR builds yet. To do so, I need to merge the protobuf updates I have in #59132, but I'd still appreciate a look. @dhartunian added you as a reviewer, let me know if I should tag someone else.

Annebirzin · 2021-01-22T17:53:02Z

@asubiotto I believe we are planning to remove the 'TXN type' column because we now have the transactions page (but we would still add txn type as a filter and include it on the statement detail page) @awoods187 can you confirm?

awoods187 · 2021-01-25T14:20:40Z

@Annebirzin that matches my understanding.

@asubiotto this is exciting!

dhartunian · 2021-01-25T21:19:48Z

@asubiotto I published a beta version of the CRDB client from this PR: cockroachdb/cockroach#59132 which you can use to get this to pass build.

If any of the endpoints could return a missing value, you can guard against that using patterns like:

const maxMemUsageBars = [
  bar("max-mem-usage", (d: StatementStatistics) => d.stats.max_mem_usage?.mean),
];

const networkBytesBars = [
  bar(
    "network-bytes",
    (d: StatementStatistics) => d.stats.bytes_sent_over_network?.mean,
  ),
];

With the ? on the field that may be undefined. Documented here: https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#optional-chaining.

I'll look through code in more detail as well.

dhartunian · 2021-01-25T21:21:30Z

@asubiotto use 0.0.5-beta.0 as the crdb-protobuf-client version in cluster-ui/package.json

dhartunian

Regarding testing, I think making sure that the Storybook pages for StatementsPage and StatementDetails page components render properly is a good place to start. You can adjust the fixtures in packages/cluster-ui/src/statementsPage/statementsPage.fixture.ts for example to make sure the pages have the input data they need for the new fields.

packages/cluster-ui/src/statementsTable/statementsTable.tsx

asubiotto · 2021-01-26T15:23:16Z

Added the fields to the storybook fixture and change the protobuf version to the beta you pushed (thanks). I also had a question about legend formatters. I haven't passed any into the bars I created, is that fine?

packages/cluster-ui/src/barCharts/barCharts.tsx

dhartunian · 2021-01-26T18:39:48Z

I think what we'll do is once your CRDB change is merged, we'll publish a new proto version that's non-beta, then we can merge this, and you can bump the cluster-ui dependency version in pkg/ui.

asubiotto · 2021-01-27T11:00:36Z

I think what we'll do is once your CRDB change is merged, we'll publish a new proto version that's non-beta, then we can merge this, and you can bump the cluster-ui dependency version in pkg/ui.

Sounds good.

I fixed the build issue which was caused by my deleting the rows affected title which is still used by the transactions page (for now). Added the .scss back in. Related to this, is there a way to get a tool to yell at you if you're referencing a css class that doesn't exist?

asubiotto · 2021-01-27T11:02:36Z

packages/cluster-ui/src/statementsTable/statementsTable.module.scss

@@ -24,7 +24,7 @@
  }


Do I need to change anything here?

Nope, these are overrides for specific column widths in case you need them.

asubiotto · 2021-01-27T11:58:17Z

Looks like there are some test issues which I believe cockroachdb/cockroach#59132 fixes. I'll wait for that to merge.

Also, I see that I'm getting some lint errors and would like to run eslint --fix on save. Have you by any chance set that up in Goland? I tried using an automatic configuration which didn't work due to not being able to find an eslint package. I then tried to link it to both eslint and typescript-eslint in the parent node_modules folder but it complains about the absence of an lib/options folder.

dhartunian · 2021-01-27T18:16:51Z

I think what we'll do is once your CRDB change is merged, we'll publish a new proto version that's non-beta, then we can merge this, and you can bump the cluster-ui dependency version in pkg/ui.

Sounds good.

I fixed the build issue which was caused by my deleting the rows affected title which is still used by the transactions page (for now). Added the .scss back in. Related to this, is there a way to get a tool to yell at you if you're referencing a css class that doesn't exist?

I don't think such a feature exists. The bar chart code in particular is a bit ugly so it's easier to make mistakes in there. Maybe @nathanstilwell has thoughts.

dhartunian · 2021-01-27T18:20:07Z

@asubiotto I have this setup for eslint in Goland and it works for this project. You should only have to run eslint and it takes care of using prettier for formatting and parsing typescript.

dhartunian

I pushed a small update with CSS edits that hopefully help, and I ran yarn lint:fix in a second commit.

dhartunian · 2021-01-27T22:42:22Z

packages/cluster-ui/src/statementsTable/statementsTable.module.scss

@@ -24,7 +24,7 @@
  }
 }

-.statements-table__col-count, .statements-table__col-retries, .statements-table__col-rows {
+.statements-table__col-count, .statements-table__col-rows, .statements-table__col-rows-read, .statements-table__col-bytes-read , .statements-table__col-max-mem-usage, .statements-tabe__col-network-bytes, .statements-table__col-retries {


there's a tabe -> table spelling error here. I will push a small commit to your branch that cleans this up. It's quite annoying to deal with and we should only need to use a single selector here.

dhartunian · 2021-01-27T22:42:25Z

packages/cluster-ui/src/statementsTable/statementsTable.module.scss

@@ -24,7 +24,7 @@
  }


Nope, these are overrides for specific column widths in case you need them.

asubiotto · 2021-01-28T11:50:53Z

Thanks for the edits. Re eslint, I did try that but for some reason it doesn't work (regardless of wheter I'm using ui or cluster-ui project root) and asks me to specify a path to the eslint package. This does work in cockroachdb/cockroach. Will keep on fiddling with it. edit: looks like running yarn remove eslint in cluster-ui has fixed it. I think GoLand might've tried installing its own version which somehow messed things up?

This commit adds columns for execution stats that we want to display for 21.1. Namely, this adds rows/bytes read, max memory, and network bytes sent. The rows affected column has been removed. The commit also reorders all column-related code to follow the graphical order.

Make bar chart wrapper apply a simple set of uniform styles instead of mandating a long list of carefully selected selectors. Adjust existing width overrides of certain columns to not refer to the "name"+`--bar-chart` selector and instead target the bar charts inside of the specific table cells. This reduces the need for named bar chart selectors inside each column.

asubiotto · 2021-02-01T10:24:48Z

New version that points to v0.0.5 protobuf changes and fixes CI errors. I think this should be good to go! PTAL

dhartunian

LGTM.

Once you merge this, an automated build will create and publish the next version of the cluster-ui package. You can then bump the version in CRDB to get the latest version of the statements page. Latest version will be pushed as a git commit with a version number to the repo, or can be seen in npm: https://www.npmjs.com/package/@cockroachlabs/cluster-ui

asubiotto · 2021-02-02T12:04:06Z

Thanks for your guidance!

asubiotto requested a review from dhartunian January 22, 2021 15:35

asubiotto requested review from nathanstilwell and nkodali as code owners January 22, 2021 15:35

dhartunian reviewed Jan 25, 2021

View reviewed changes

packages/cluster-ui/src/statementsTable/statementsTable.tsx Outdated Show resolved Hide resolved

dhartunian reviewed Jan 26, 2021

View reviewed changes

packages/cluster-ui/src/barCharts/barCharts.tsx Outdated Show resolved Hide resolved

asubiotto commented Jan 27, 2021

View reviewed changes

asubiotto requested review from j-low and laurenbarker as code owners January 27, 2021 11:32

dhartunian reviewed Jan 27, 2021

View reviewed changes

asubiotto and others added 3 commits February 1, 2021 11:22

cluster-ui: fix lint errors in barCharts.tsx

041326e

dhartunian approved these changes Feb 2, 2021

View reviewed changes

asubiotto merged commit 449b51c into cockroachdb:master Feb 2, 2021

asubiotto mentioned this pull request Feb 4, 2021

cluster-ui: update transactions page with execution stats #193

Merged

3 tasks

asubiotto deleted the statements-page branch February 9, 2021 14:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cluster-ui: update statements page #178

cluster-ui: update statements page #178

asubiotto commented Jan 22, 2021 •

edited

Loading

asubiotto commented Jan 22, 2021 •

edited

Loading

Annebirzin commented Jan 22, 2021

awoods187 commented Jan 25, 2021

dhartunian commented Jan 25, 2021

dhartunian commented Jan 25, 2021

dhartunian left a comment

asubiotto commented Jan 26, 2021

dhartunian commented Jan 26, 2021

asubiotto commented Jan 27, 2021

asubiotto Jan 27, 2021

dhartunian Jan 27, 2021

asubiotto commented Jan 27, 2021

dhartunian commented Jan 27, 2021

dhartunian commented Jan 27, 2021

dhartunian left a comment

dhartunian Jan 27, 2021

dhartunian Jan 27, 2021

asubiotto commented Jan 28, 2021 •

edited

Loading

asubiotto commented Feb 1, 2021

dhartunian left a comment

asubiotto commented Feb 2, 2021

cluster-ui: update statements page #178

cluster-ui: update statements page #178

Conversation

asubiotto commented Jan 22, 2021 • edited Loading

Checklist

asubiotto commented Jan 22, 2021 • edited Loading

Annebirzin commented Jan 22, 2021

awoods187 commented Jan 25, 2021

dhartunian commented Jan 25, 2021

dhartunian commented Jan 25, 2021

dhartunian left a comment

Choose a reason for hiding this comment

asubiotto commented Jan 26, 2021

dhartunian commented Jan 26, 2021

asubiotto commented Jan 27, 2021

asubiotto Jan 27, 2021

Choose a reason for hiding this comment

dhartunian Jan 27, 2021

Choose a reason for hiding this comment

asubiotto commented Jan 27, 2021

dhartunian commented Jan 27, 2021

dhartunian commented Jan 27, 2021

dhartunian left a comment

Choose a reason for hiding this comment

dhartunian Jan 27, 2021

Choose a reason for hiding this comment

dhartunian Jan 27, 2021

Choose a reason for hiding this comment

asubiotto commented Jan 28, 2021 • edited Loading

asubiotto commented Feb 1, 2021

dhartunian left a comment

Choose a reason for hiding this comment

asubiotto commented Feb 2, 2021

asubiotto commented Jan 22, 2021 •

edited

Loading

asubiotto commented Jan 22, 2021 •

edited

Loading

asubiotto commented Jan 28, 2021 •

edited

Loading