roachpb/sql/ui: add mean rows written to Statement Details page #70377

lindseyjin · 2021-09-17T20:11:26Z

Resolves #67084

Previously, the Statement Details page did not show the mean rows
written to. We received feedback to surface rows affected.

To address this, we are adding "mean rows written" to non-SELECT
statements. This has been added as a row on the Statement Details
Overview page and on the Execution Stats "Other Execution Stats" table.
A future PR will also add this new metric to the column selector on
the Statements Overview page.

Release note (ui change): add mean rows written to statement details page

cockroach-teamcity · 2021-09-17T20:11:32Z

This change is

lindseyjin · 2021-09-17T20:13:41Z

hi! sorry y'all, I somehow closed my last PR when I was trying to rebase on upstream. I couldn't figure out how to recover that, so I've created a new one (although let me know if you have any tips if I run into this issue in the future!)

Azhng

Reviewed 1 of 14 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lindseyjin)

pkg/roachpb/app_stats.proto, line 90 at r1 (raw file):

  optional NumericStat rows_read = 16 [(gogoproto.nullable) = false];

  // RowsWritten collects the number of rows read from disk.

s/rows read from/rows written to/

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

      );
    const summary = summarize(statement);
    const notSelectStatement = summary.statement !== "select";

Reading the function of summarize(), I think this would also include statements like SET CLUSTER SETTINGS ... even though those statements does not always change stored data on disk.

Also, I feel this summarize() function doesn't capture the full picture. We also have UPSERT statements that does conditional update/insert operations.

Additionally, user can do PREPARE insert_stmt AS INSERT INTO table VALUES ($1) and then EXECUTE insert_stmt(...), the subsequent EXECUTE would also have mean rows written stats.

cc @maryliag any thoughts on handling these special cases ?

lindseyjin

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @lindseyjin and @maryliag)

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

Previously, Azhng (Archer Zhang) wrote…

Reading the function of summarize(), I think this would also include statements like SET CLUSTER SETTINGS ... even though those statements does not always change stored data on disk.

Also, I feel this summarize() function doesn't capture the full picture. We also have UPSERT statements that does conditional update/insert operations.

Additionally, user can do PREPARE insert_stmt AS INSERT INTO table VALUES ($1) and then EXECUTE insert_stmt(...), the subsequent EXECUTE would also have mean rows written stats.

cc @maryliag any thoughts on handling these special cases ?

Hmm, currently the ticket only mentions hiding this metric for SELECT statements. Is the problem that we don't want to show "mean rows written" for any statements that aren't able to modify rows?

lindseyjin

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)

pkg/roachpb/app_stats.proto, line 90 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

s/rows read from/rows written to/

Done.

maryliag

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

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

Previously, lindseyjin (Lindsey Jin) wrote…

Hmm, currently the ticket only mentions hiding this metric for SELECT statements. Is the problem that we don't want to show "mean rows written" for any statements that aren't able to modify rows?

I had left a previous comment on the other PR, for this here you need to checks: only show if statement type is DML, and if it is DML hide if is select type.
So I would change the notSelectStatement const to showRowsWritten and its value would be something like this (not sure the right parameter name for SQL Type)
const showRowsWritten = statement.sqltype === 'DML' and summary.statement !== 'statement'

and then below you can show {showRowsWritten && (...

lindseyjin

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

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

Previously, maryliag (Marylia Gutierrez) wrote…

I had left a previous comment on the other PR, for this here you need to checks: only show if statement type is DML, and if it is DML hide if is select type.
So I would change the notSelectStatement const to showRowsWritten and its value would be something like this (not sure the right parameter name for SQL Type)
const showRowsWritten = statement.sqltype === 'DML' and summary.statement !== 'statement'

and then below you can show {showRowsWritten && (...

Done!

Couldn't find if there was an existing enum for sql types, but let me know if one exists!

maryliag

Reviewed 10 of 14 files at r1, 1 of 1 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @lindseyjin, and @maryliag)

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx, line 581 at r4 (raw file):

                        <Text>
                          {formatNumberForDisplay(
                            stats.rows_written.mean,

nit: to account for cluster with version where this parameter might not exist, change this to stats.rows_written?.mean

pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.fixture.ts, line 92 at r4 (raw file):

  },
  rows_written: {
    mean: 10,

nit: use a different value from rows read, because during tests we want different values to make sure we're not using a value from a different parameter for example

pkg/ui/workspaces/cluster-ui/src/util/appStats/appStats.fixture.ts, line 208 at r4 (raw file):

        squared_diffs: 3.8399999999999994,
      },
      rows_written: {

nit: same as the other comment, change the values of rows written to some other random value different from rows read

pkg/ui/workspaces/db-console/src/util/appStats.ts, line 85 at r4 (raw file):

    bytes_read: addNumericStats(a.bytes_read, b.bytes_read, countA, countB),
    rows_read: addNumericStats(a.rows_read, b.rows_read, countA, countB),
    rows_written: addNumericStats(a.rows_read, b.rows_read, countA, countB),

this is suppose to be rows_written, you forgot to change the values inside the function

lindseyjin

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)

pkg/ui/workspaces/db-console/src/util/appStats.ts, line 85 at r4 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

this is suppose to be rows_written, you forgot to change the values inside the function

Done.

maryliag

Great work!

Reviewed 1 of 14 files at r1, 2 of 4 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng)

maryliag

Just realized that this metric should also be added to the Execution Stats Tab of the Statement Details. Can you add it there? Just below the read row on the "Other Execution Statistics". table

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng)

lindseyjin

got it! do I just need to add this into the "Other Execution Stats" table, or also the "Stats By Node" table?\

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @Azhng and @maryliag)

maryliag

That stats by node is based on the same statement table from the main page, so once you add there on your following task, it will get added on this table too. So don't worry about it on this PR, but on your next one make sure you test on this page

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @Azhng and @maryliag)

lindseyjin

Ok, sounds good!
Also, done!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @Azhng and @maryliag)

Azhng

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @Azhng and @maryliag)

maryliag

Reviewed 1 of 1 files at r8.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @maryliag)

Resolves cockroachdb#67084 Previously, the Statement Details page did not show the mean rows written to. We received feedback to surface rows affected. To address this, we are adding "mean rows written" to non-SELECT statements. This has been added as a row on the Statement Details Overview page and on the Execution Stats "Other Execution Stats" table. A future PR will also add this new metric to the column selector on the Statements Overview page. Release note (ui change): add mean rows written to statement details page

lindseyjin · 2021-09-22T14:45:03Z

^^ Pushing to resolve merge conflicts

lindseyjin · 2021-09-22T15:39:14Z

bors r+

craig · 2021-09-22T17:27:44Z

Build succeeded:

GitHub CI (Cockroach)

lindseyjin requested a review from a team as a code owner September 17, 2021 20:11

lindseyjin requested review from a team and removed request for a team September 17, 2021 20:14

lindseyjin force-pushed the add-mean-rows-written branch 2 times, most recently from 61c6631 to c769f07 Compare September 17, 2021 21:11

Azhng reviewed Sep 20, 2021

View reviewed changes

lindseyjin force-pushed the add-mean-rows-written branch from c769f07 to c167392 Compare September 20, 2021 16:25

lindseyjin commented Sep 20, 2021

View reviewed changes

lindseyjin force-pushed the add-mean-rows-written branch from c167392 to d503e34 Compare September 20, 2021 16:30

lindseyjin commented Sep 20, 2021

View reviewed changes

maryliag suggested changes Sep 20, 2021

View reviewed changes

lindseyjin force-pushed the add-mean-rows-written branch from d503e34 to fd309c9 Compare September 20, 2021 17:18

lindseyjin commented Sep 20, 2021

View reviewed changes

maryliag suggested changes Sep 21, 2021

View reviewed changes

lindseyjin force-pushed the add-mean-rows-written branch 3 times, most recently from fa237d1 to ac7c3ad Compare September 21, 2021 15:54

lindseyjin commented Sep 21, 2021

View reviewed changes

lindseyjin force-pushed the add-mean-rows-written branch 3 times, most recently from 0de6ade to 56f14ab Compare September 21, 2021 20:18

maryliag approved these changes Sep 21, 2021

View reviewed changes

maryliag suggested changes Sep 21, 2021

View reviewed changes

lindseyjin force-pushed the add-mean-rows-written branch from 56f14ab to 2372a0f Compare September 21, 2021 22:09

lindseyjin commented Sep 21, 2021

View reviewed changes

maryliag reviewed Sep 21, 2021

View reviewed changes

lindseyjin force-pushed the add-mean-rows-written branch from 2372a0f to 8aea9f5 Compare September 21, 2021 22:25

lindseyjin commented Sep 21, 2021

View reviewed changes

Azhng approved these changes Sep 22, 2021

View reviewed changes

maryliag approved these changes Sep 22, 2021

View reviewed changes

lindseyjin force-pushed the add-mean-rows-written branch from 8aea9f5 to d1568ad Compare September 22, 2021 14:44

craig bot merged commit ecf0ed8 into cockroachdb:master Sep 22, 2021

cockroach-teamcity mentioned this pull request Sep 22, 2021

roachpb/sql/ui: add mean rows written to Statement Details page cockroachdb/docs#11757

Closed

Azhng mentioned this pull request Sep 22, 2021

sql: current SQL Stats JSON decoder is not backward compatiable #70599

Closed

maryliag mentioned this pull request Oct 26, 2021

release-21.2: Add Rows Written to Transaction and Statement pages #72006

Merged

lindseyjin deleted the add-mean-rows-written branch November 16, 2021 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

roachpb/sql/ui: add mean rows written to Statement Details page #70377

roachpb/sql/ui: add mean rows written to Statement Details page #70377

lindseyjin commented Sep 17, 2021 •

edited

Loading

cockroach-teamcity commented Sep 17, 2021

lindseyjin commented Sep 17, 2021

Azhng left a comment

lindseyjin left a comment

lindseyjin left a comment

maryliag left a comment

lindseyjin left a comment

maryliag left a comment

lindseyjin left a comment

maryliag left a comment

maryliag left a comment

lindseyjin left a comment

maryliag left a comment

lindseyjin left a comment

Azhng left a comment

maryliag left a comment

lindseyjin commented Sep 22, 2021

lindseyjin commented Sep 22, 2021

craig bot commented Sep 22, 2021

roachpb/sql/ui: add mean rows written to Statement Details page #70377

roachpb/sql/ui: add mean rows written to Statement Details page #70377

Conversation

lindseyjin commented Sep 17, 2021 • edited Loading

cockroach-teamcity commented Sep 17, 2021

lindseyjin commented Sep 17, 2021

Azhng left a comment

Choose a reason for hiding this comment

lindseyjin left a comment

Choose a reason for hiding this comment

lindseyjin left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

lindseyjin left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

lindseyjin left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

lindseyjin left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

lindseyjin left a comment

Choose a reason for hiding this comment

Azhng left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

lindseyjin commented Sep 22, 2021

lindseyjin commented Sep 22, 2021

craig bot commented Sep 22, 2021

lindseyjin commented Sep 17, 2021 •

edited

Loading