ui: add rows written to statement and transaction tables #70593
Conversation
|
This change is |
| <Anchor href={statementsTimeInterval} target="_blank"> | ||
| time interval | ||
| </Anchor> | ||
| . The gray bar indicates the mean number of rows written to disk. |
There was a problem hiding this comment.
I noticed that other components split this text up into two paragraphs. However, it seems that there is no space after the period with that format, since the linter complains if you add a space.
Two solutions I've found around this are:
- adding a   after the period
- putting everything in one paragraph and adding a space after the period
Wondering if my current approach is fine, and if I should also apply this to the other tooltips? (or if it's not that important of an issue?)
There was a problem hiding this comment.
I'd prefer the approach with 2 paragraphs because it makes the description more clear.
| let contentModifier = ""; | ||
| const fingerprintModifier = ""; | ||
| switch (statType) { | ||
| case "transaction": |
There was a problem hiding this comment.
question: should I remove this case if we're not supporting rowsWritten as a column on the Transactions page? Also, should I be adding this column onto the Transactions page, either in this PR or a future one?
lindseyjin
force-pushed
the
column-selector-rows-written
branch
from
1d78a72
to
f0006ab
Compare
There was a problem hiding this comment.
Reviewed 3 of 6 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ianjevans, @lindseyjin, and @maryliag)
pkg/ui/workspaces/cluster-ui/src/barCharts/barCharts.tsx, line 38 at r2 (raw file):
const rowsWrittenBars = [ bar("rows-written", (d: StatementStatistics) => d.stats.rows_written.mean),
since this is a new parameter, we should add the check here
d.stats.rows_written?.mean
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 299 at r1 (raw file):
Previously, lindseyjin (Lindsey Jin) wrote…
question: should I remove this case if we're not supporting rowsWritten as a column on the Transactions page? Also, should I be adding this column onto the Transactions page, either in this PR or a future one?
You should add the column on Transaction page, you can do it on this PR, so this case is still valid and shouldn't be removed from here
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 322 at r1 (raw file):
Previously, lindseyjin (Lindsey Jin) wrote…
I noticed that other components split this text up into two paragraphs. However, it seems that there is no space after the period with that format, since the linter complains if you add a space.
Two solutions I've found around this are:
- adding a   after the period
- putting everything in one paragraph and adding a space after the period
Wondering if my current approach is fine, and if I should also apply this to the other tooltips? (or if it's not that important of an issue?)
When you need to string to keep the spaces you can add the text inside {}. You can see in a few other places on the same file. So in your case you would have
</Anchor>
{" . The gray bar indicates the mean number of ro..."}
pkg/ui/workspaces/cluster-ui/src/util/docs.ts, line 64 at r1 (raw file):
Previously, lindseyjin (Lindsey Jin) wrote…
wondering if this is the right article to link, or if I should just keep the "readFromDisk" link?
@ianjevans can you confirm what is the appropriate doc for rows written? Thanks!
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 299 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
You should add the column on Transaction page, you can do it on this PR, so this case is still valid and shouldn't be removed from here
hi, I realized that this change would involve adding a new field to the transactions proto, since in my last PR I only updated the statements proto. Would you still like me to do this in this PR?
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 299 at r1 (raw file):
Previously, lindseyjin (Lindsey Jin) wrote…
hi, I realized that this change would involve adding a new field to the transactions proto, since in my last PR I only updated the statements proto. Would you still like me to do this in this PR?
Yes :)
lindseyjin
force-pushed
the
column-selector-rows-written
branch
3 times, most recently
from
1e3ab38
to
ad5ff98
Compare
lindseyjin
changed the title
lindseyjin
force-pushed
the
column-selector-rows-written
branch
2 times, most recently
from
5764046
to
48ac747
Compare
There was a problem hiding this comment.
Note: rebased on top of my other commit to fix tooltip text, although that should be merged into master soon! #70639
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/barCharts/barCharts.tsx, line 38 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
since this is a new parameter, we should add the check here
d.stats.rows_written?.mean
Done.
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 299 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
Yes :)
Done!
There was a problem hiding this comment.
You can rebase with master now, since the other one was merged. This way this PR will have only one commit (currently still showing the other one here)
Reviewed 15 of 15 files at r4, 3 of 3 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status:
pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/json_encoding_test.go, line 197 at r6 (raw file):
"sqDiff": {{.Float}} } "rowsWritten": {
nit: this seems to have an extra space here, make sure it's aligned
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 304 at r6 (raw file):
case "statement": contentModifier = contentModifiers.statements; break;
you need the case transactionDetails, which is the tooltip used on the statements table inside the transaction details page, which is why you need the fingerprintModifier parameter
lindseyjin
force-pushed
the
column-selector-rows-written
branch
2 times, most recently
from
634b2e9
to
4ebd600
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 304 at r6 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
you need the case
transactionDetails, which is the tooltip used on the statements table inside the transaction details page, which is why you need thefingerprintModifierparameter
Done!
Although I was wondering how to view this on the UI? When I click into a transaction, the table only shows me one column (statements) with no column selector. Is that supposed to be the case?
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 304 at r6 (raw file):
Previously, lindseyjin (Lindsey Jin) wrote…
Done!
Although I was wondering how to view this on the UI? When I click into a transaction, the table only shows me one column (statements) with no column selector. Is that supposed to be the case?
ooh also, should I update the transaction details to show mea rows written to be consistent with statement details?
There was a problem hiding this comment.
Reviewed 2 of 3 files at r7, all commit messages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 304 at r6 (raw file):
Previously, lindseyjin (Lindsey Jin) wrote…
ooh also, should I update the transaction details to show mea rows written to be consistent with statement details?
The majority of columns on Transaction Details is hidden because of an issue Archer is currently working on. Once that is resolved, the remaining columns will show, including the new one you just added :) This is why you need that other case
And yes, add the info to the transaction details under Transaction resource usage
Resolves cockroachdb#67084 Previously, the Statements and Transactions tables did not have a "Rows Written" column. This was inadequate as we received feedback to surface information on Rows Affected for statements on the db console. Since we've already added "Rows Written" as a metric on the Statement Details pages, this commit also adds this metric as a column on the Statements Overview table. This commit also adds this metric as a field on the Transaction Statistics proto, in order to add this as a column on the Transactions table and as a field on the Transaction Details page. The table columns will not show up by default, although they are selectable from the column selector. Release note (ui change): A new column, "Rows Written" has been added onto the Statements and Transactions tables. This column will not show up by default, but can be selected from the column selector. A new metric "Mean rows written" has also been added to the Transaction Details page.
lindseyjin
force-pushed
the
column-selector-rows-written
branch
from
4ebd600
to
b76149d
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 304 at r6 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
The majority of columns on Transaction Details is hidden because of an issue Archer is currently working on. Once that is resolved, the remaining columns will show, including the new one you just added :) This is why you need that other case
And yes, add the info to the transaction details under Transaction resource usage
Done.
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 317 at r8 (raw file):
style="tableTitle" content={ <>
noob question: what's these empty <></> tags for?
There was a problem hiding this comment.
Awesome work!
Reviewed 1 of 3 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 317 at r8 (raw file):
Previously, Azhng (Archer Zhang) wrote…
noob question: what's these empty
<></>tags for?
The tooltip component expects only one element for the content, so we add everything to one element, but we don't want to use any specific element, because they came with their respective styles, sizes etc, so we use the empty type, meaning it won't affect anything
There was a problem hiding this comment.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx, line 317 at r8 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
The tooltip component expects only one element for the content, so we add everything to one element, but we don't want to use any specific element, because they came with their respective styles, sizes etc, so we use the empty type, meaning it won't affect anything
Ah I see! Thanks!
|
bors r+ |
|
Build succeeded: |
Resolves #67084
Previously, the Statements and Transactions tables did not have a
"Rows Written" column. This was inadequate as we received feedback
to surface information on Rows Affected for statements on the db
console.
Since we've already added "Rows Written" as a metric on the Statement
Details pages, this commit also adds this metric as a column on the
Statements Overview table. This commit also adds this metric as a field
on the Transaction Statistics proto, in order to add this as a column
on the Transactions table and as a field on the Transaction Details page.
The table columns will not show up by default, although they are
selectable from the column selector.
Column Selector:
Statements Overview table:
Stats By Node table:
Transactions table:
Transaction Details:
Release note (ui change): A new column, "Rows Written" has been added
onto the Statements and Transactions tables. This column will not show
up by default, but can be selected from the column selector. A new
metric "Mean rows written" has also been added to the Transaction
Details page.