ui,server: Table stats collection details added to Database pages #76168
Conversation
|
This change is |
ericharmeling
force-pushed
the
add-table-stats-dbconsole
branch
from
064f4ec to
ee6b079
Compare
ericharmeling
changed the title
ericharmeling
force-pushed
the
add-table-stats-dbconsole
branch
from
ee6b079 to
364fd1f
Compare
|
I am having trouble getting the ui tests to pass for the pages I updated... And the error messages aren't super helpful to me. |
ericharmeling
changed the title
There was a problem hiding this comment.
Looking really good! I added a few comments, still need to understand what's wrong on your tests.
Also, on PRs that have UI changes we like to add images or videos of the changes
Reviewed 16 of 23 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling and @maryliag)
-- commits, line 6 at r1:
nit: release justification is only for backport, you don't need on a regular PR
pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx, line 299 at r1 (raw file):
<a className={booleanSettingCx("crl-hover-text__link-text")} href="https://www.cockroachlabs.com/docs/stable/cost-based-optimizer#control-automatic-statistics"
we don't hard coded the full path in the url in case it changes, we use Anchor for those cases, for example:
https://github.com/cockroachdb/cockroach/blob/85913ddabbdfd7475d5314401270382834147cb4/pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx#L457-L459
pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 399 at r1 (raw file):
"crl-hover-text__link-text", )} href="https://www.cockroachlabs.com/docs/stable/cost-based-optimizer#control-automatic-statistics"
similar comment as above
pkg/ui/workspaces/cluster-ui/src/settings/booleanSetting.tsx, line 27 at r1 (raw file):
export function BooleanSetting(props: BooleanSettingProps) { const { text, enabled, tooltipText } = props; if (enabled) {
the two possible returned values are very similar, so create a variable for the label and another for the class and use that instead on just one return (I added a similar comment below and noticed this one after :) )
pkg/ui/workspaces/cluster-ui/src/summaryCard/index.tsx, line 59 at r1 (raw file):
className, }) => value ? (
the options are very similar with just the difference on enable/disable, so you can create a variable for the class and label and use it here instead
e.g.
...}) => {
const label = value ? "Enabled" : "Disabled"
const class = value ? "bool-setting-icon__enabled" : "bool-setting-icon__disabled"
return (
<div .....
>
{label}
</Tooltip>...
)
}
pkg/ui/workspaces/db-console/src/views/databases/databasesPage/redux.ts, line 35 at r1 (raw file):
} from "src/redux/nodes"; import { getNodesByRegionString } from "../utils"; import { selectAutomaticStatsCollectionEnabled } from "oss/src/redux/clusterSettings";
nit: don't add oss to imports (it does that by default, so you need to always keep an eye)
`... from "src/redux..."
pkg/ui/workspaces/db-console/src/views/databases/databaseTablePage/redux.ts, line 35 at r1 (raw file):
import { getNodesByRegionString } from "../utils"; import { resetIndexUsageStatsAction } from "src/redux/indexUsageStats"; import { selectAutomaticStatsCollectionEnabled } from "oss/src/redux/clusterSettings";
similar as above, remove the oss
pkg/ui/workspaces/db-console/src/views/databases/databaseDetailsPage/redux.spec.ts, line 31 at r1 (raw file):
import { mapStateToProps, mapDispatchToProps } from "./redux"; import moment from "moment"; import { makeTimestamp } from "oss/src/views/databases/utils";
nit: same as above
There was a problem hiding this comment.
Nice work! I left a few more comments. Happy to help pair on the tests if they get gnarly.
Reviewed 2 of 23 files at r1, all commit messages.
Reviewable status:
pkg/server/admin.go, line 881 at r1 (raw file):
sessiondata.InternalExecutorOverride{User: userName}, fmt.Sprintf("SELECT max(created) AS created FROM [SHOW STATISTICS FOR TABLE %s]", escQualTable), )
I think this can be
row, cols, err = s.server.sqlServer.internalExecutor.QueryRowExWithCols(
ctx, "admin-show-statistics", nil, /* txn */
sessiondata.InternalExecutorOverride{User: userName},
"SELECT max(created) AS created FROM [SHOW STATISTICS FOR TABLE $1]",
escQualTable,
)
pkg/server/admin.go, line 894 at r1 (raw file):
if !noTableStats { var createdTs time.Time if err := scanner.Scan(row, createdCol, &createdTs); err != nil {
Looks like you can pass a pointer to a pointer to createdTs here to avoid the scanner.IsNull check.
pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx, line 367 at r1 (raw file):
), cell: table => table.details.statsLastUpdated.isSame(moment.utc("0001-01-01"))
Can statsLastUpdated be undefined?
pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 376 at r1 (raw file):
value={this.props.stats.rangeCount} /> {!this.props.details.statsLastUpdated.isSame(
ditto, can statsLastUpdated be undefined?
pkg/ui/workspaces/db-console/src/views/databases/databaseDetailsPage/redux.ts, line 153 at r1 (raw file):
statsLastUpdated: util.TimestampToMoment( details?.data?.stats_last_created_at, moment.utc("0001-01-01"),
I wonder if we want to use undefined here instead of this "magic" value. Or maybe make a constant for it?
pkg/ui/workspaces/db-console/src/views/databases/databaseTablePage/redux.ts, line 112 at r1 (raw file):
statsLastUpdated: util.TimestampToMoment( details?.data?.stats_last_created_at, moment.utc("0001-01-01"),
ditto, consider using either undefined or a constant?
ericharmeling
force-pushed
the
add-table-stats-dbconsole
branch
from
364fd1f to
24169b0
Compare
There was a problem hiding this comment.
TFTRs!
I've updated the PR in response to your feedback. PTAL at the latest commit.
@matthewtodd Would be nice to go over those tests! Thanks for offering to help. I'll follow-up offline.
Reviewable status:
Previously, maryliag (Marylia Gutierrez) wrote…
nit: release justification is only for backport, you don't need on a regular PR
Done.
pkg/server/admin.go, line 881 at r1 (raw file):
"SELECT max(created) AS created FROM [SHOW STATISTICS FOR TABLE $1]",
escQualTable,
Specifying the statement like this returns a SQL syntax error. I tried this instead:
`SELECT max(created) AS created FROM [SHOW STATISTICS FOR TABLE $1]`,
escQualTable,
And still get a SQL syntax error. FWIW, the surrounding statements (e.g., admin-show-create) use fmt.Sprintf (e.g., fmt.Sprintf("SHOW CREATE %s", escQualTable),).
Is there an advantage to not using fmt.Sprintf?
pkg/server/admin.go, line 894 at r1 (raw file):
Previously, matthewtodd (Matthew Todd) wrote…
Looks like you can pass a pointer to a pointer to
createdTshere to avoid thescanner.IsNullcheck.
Done.
pkg/ui/workspaces/cluster-ui/src/databaseDetailsPage/databaseDetailsPage.tsx, line 367 at r1 (raw file):
Previously, matthewtodd (Matthew Todd) wrote…
Can
statsLastUpdatedbe undefined?
Done.
pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx, line 299 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
we don't hard coded the full path in the url in case it changes, we use Anchor for those cases, for example:
https://github.com/cockroachdb/cockroach/blob/85913ddabbdfd7475d5314401270382834147cb4/pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx#L457-L459
Done.
On a related note, there is some inconsistency over which Tooltip component is being used on the Database pages and the SQL Activity pages, which is reflected in the font styling of the tooltip text between those pages.
- https://github.com/cockroachdb/cockroach/blob/85913ddabbdfd7475d5314401270382834147cb4/pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx#L15
- https://github.com/cockroachdb/cockroach/blob/85913ddabbdfd7475d5314401270382834147cb4/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx#L13
I started updating the Databases pages in this PR to use import { Tooltip } from "@cockroachlabs/ui-components";, but the styling got a little too complicated. Might be best for another PR. Should I open an issue or just leave it as it is?
pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 376 at r1 (raw file):
Previously, matthewtodd (Matthew Todd) wrote…
ditto, can
statsLastUpdatedbe undefined?
Done.
pkg/ui/workspaces/cluster-ui/src/databaseTablePage/databaseTablePage.tsx, line 399 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
similar comment as above
Done.
pkg/ui/workspaces/cluster-ui/src/settings/booleanSetting.tsx, line 27 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
the two possible returned values are very similar, so create a variable for the label and another for the class and use that instead on just one return (I added a similar comment below and noticed this one after :) )
Done.
pkg/ui/workspaces/cluster-ui/src/summaryCard/index.tsx, line 59 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
the options are very similar with just the difference on enable/disable, so you can create a variable for the class and label and use it here instead
e.g....}) => { const label = value ? "Enabled" : "Disabled" const class = value ? "bool-setting-icon__enabled" : "bool-setting-icon__disabled" return ( <div ..... > {label} </Tooltip>... ) }
Done.
pkg/ui/workspaces/db-console/src/views/databases/databaseDetailsPage/redux.ts, line 153 at r1 (raw file):
Previously, matthewtodd (Matthew Todd) wrote…
I wonder if we want to use undefined here instead of this "magic" value. Or maybe make a constant for it?
Done.
pkg/ui/workspaces/db-console/src/views/databases/databasesPage/redux.ts, line 35 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
nit: don't add
ossto imports (it does that by default, so you need to always keep an eye)
`... from "src/redux..."
Done.
pkg/ui/workspaces/db-console/src/views/databases/databaseTablePage/redux.ts, line 35 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
similar as above, remove the
oss
Done.
pkg/ui/workspaces/db-console/src/views/databases/databaseTablePage/redux.ts, line 112 at r1 (raw file):
Previously, matthewtodd (Matthew Todd) wrote…
ditto, consider using either undefined or a constant?
Done.
pkg/ui/workspaces/db-console/src/views/databases/databaseDetailsPage/redux.spec.ts, line 31 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
nit: same as above
Done.
Okay! I've added a video to the PR overview. |
There was a problem hiding this comment.
Thanks for the video! I just have one small nit regarding the commits, otherwise looking good! :D
Something seems off with your test failures, because it seems some of the tests itself failed to run, not that they returned an error (e.g. not a single test was parsed message). I can see Matthew will help you, you might also get help from dev-inf for some of the tests too.
Another thing: we do have the same Database page in the managed repo (PR just landed this week https://github.com/cockroachlabs/managed-service/pull/7708), so you will need to create a PR very similar (if not equal) to this one, for the managed repo, so CC Console can have those changes too.
Reviewed 3 of 4 files at r2, 11 of 13 files at r3.
Reviewable status:
-- commits, line 19 at r3:
nit: we want to keep just one commit, so you can squash those two into a single one
pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx, line 299 at r1 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
Done.
On a related note, there is some inconsistency over which Tooltip component is being used on the Database pages and the SQL Activity pages, which is reflected in the font styling of the tooltip text between those pages.
- https://github.com/cockroachdb/cockroach/blob/85913ddabbdfd7475d5314401270382834147cb4/pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx#L15
- https://github.com/cockroachdb/cockroach/blob/85913ddabbdfd7475d5314401270382834147cb4/pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx#L13
I started updating the Databases pages in this PR to use
import { Tooltip } from "@cockroachlabs/ui-components";, but the styling got a little too complicated. Might be best for another PR. Should I open an issue or just leave it as it is?
Best for another PR, feel free to open an issue so we keep track of it.
ericharmeling
force-pushed
the
add-table-stats-dbconsole
branch
from
24169b0 to
3dfa7bf
Compare
ericharmeling
changed the title
There was a problem hiding this comment.
Another thing: we do have the same Database page in the managed repo (PR just landed this week cockroachlabs/managed-service#7708), so you will need to create a PR very similar (if not equal) to this one, for the managed repo, so CC Console can have those changes too.
Got it. I'll start working on implementing these changes in managed-service once this PR is merged.
Question: Should I have this PR not resolve #67510 and instead have a PR into managed-service resolve that issue, or should I just make the managed-service PR fast-follow work? It doesn't look like https://github.com/cockroachlabs/managed-service/pull/7708 directly resolves any open GH issues.
Reviewable status:
Previously, maryliag (Marylia Gutierrez) wrote…
nit: we want to keep just one commit, so you can squash those two into a single one
Done.
pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx, line 299 at r1 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
Best for another PR, feel free to open an issue so we keep track of it.
Looks like someone beat me to it :)
There was a problem hiding this comment.
Since issue #67510 specifies DB Console, you can make this PR resolve that issue and create a follow issue and PR for the managed changes.
Issues that are specific for CC Console can be created just on jira. For example: the PR you mentioned have a [CRDB-8616] in the title, and that's the issue is solves (https://cockroachlabs.atlassian.net/browse/CRDB-8616). For that repo you add the issue on the title but it should also have the link of the issue on the description, to help.
Reviewed 1 of 13 files at r3, 8 of 8 files at r4, all commit messages.
Reviewable status:
ericharmeling
force-pushed
the
add-table-stats-dbconsole
branch
2 times, most recently
from
637a75f to
a41e8c8
Compare
There was a problem hiding this comment.
Looks great!
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status:
|
Thanks for the approval, @maryliag! I think I got to all of your feedback, @matthewtodd. I also pushed all of the updates we worked on on 2/11/22. If you're good with the changes, I'll go ahead and merge. :) |
There was a problem hiding this comment.
Looks great, @ericharmeling! Thank you! 🎉
Reviewed 6 of 23 files at r1, 3 of 4 files at r2, 9 of 13 files at r3, 8 of 8 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status:
pkg/server/admin.go, line 881 at r1 (raw file):
Previously, ericharmeling (Eric Harmeling) wrote…
"SELECT max(created) AS created FROM [SHOW STATISTICS FOR TABLE $1]",
escQualTable,Specifying the statement like this returns a SQL syntax error. I tried this instead:
`SELECT max(created) AS created FROM [SHOW STATISTICS FOR TABLE $1]`,
escQualTable,And still get a SQL syntax error. FWIW, the surrounding statements (e.g.,
admin-show-create) usefmt.Sprintf(e.g.,fmt.Sprintf("SHOW CREATE %s", escQualTable),).Is there an advantage to not using
fmt.Sprintf?
Ugh, sorry for sending you into this. I was generally looking to avoid manually building SQL strings like this in a "defend against SQL injection" spirit, but given the escaping of the table name (as well as the pattern in the surrounding code), I think you're right that we're fine here.
|
bors r+ |
75451: backupccl,spanconfig,kvserver: ExportRequest noops on ephemeral ranges r=adityamaru a=adityamaru This change is the first of two changes that gets us to the goal of backup ignoring certain table row data, and not holding up GC on these ranges. This change does a few things: - It sets up the transport of the exclude_data_from_backup bit set on a table descriptor, to the span configuration applied in KV. - It teaches ExportRequest on a range marked as excluded to return an empty ExportResponse. In this way, a backup processor will receive no row data to backup up for an ephemeral table. - A follow up change will also teach the SQLTranslator to not populate the protected timestamp field on the SpanConfig for such tables. This way, a long running backup will not hold up GC on such high-churn tables. With no protection on such ranges, it is possible that an ExportRequest targetting the range has a StartTime below the range's GCThreshold. To avoid the returned BatchTimestampBeforeGCError from failing the backup we decorate the the error with information about the range being excluded from backup and handle the error in the backup processor. Informs: #73536 Release note (sql change): BACKUP of a table marked with `exclude_data_from_backup` via `ALTER TABLE ... SET (exclude_data_from_backup = true)` will no longer backup that table's row data. The backup will continue to backup the table's descriptor and related metadata, and so on restore we will end up with an empty version of the backed up table. 76168: ui,server: Table stats collection details added to Database pages r=ericharmeling a=ericharmeling Closes: #67510 Release justification: low-risk, high benefit changes to existing functionality Release note (ui change): - Added status of automatic statistics collection to the Database and Database table pages on the DB Console. - Added timestamp of last statistics collection to the Database details and Database table pages on the DB Console. Here's a video of the changes in the UI: https://user-images.githubusercontent.com/27286675/153521703-9ed19ed0-5fe9-4cb6-a537-1d227aeb0a0b.mov 76444: bazel: upgrade to bazel 5.0.0 r=rail a=rickystewart Closes #75796. Release note: None Co-authored-by: Aditya Maru <[email protected]> Co-authored-by: Eric Harmeling <[email protected]> Co-authored-by: Ricky Stewart <[email protected]>
There was a problem hiding this comment.
Reviewable status:
pkg/server/admin.go, line 881 at r1 (raw file):
All good!
I was generally looking to avoid manually building SQL strings like this in a "defend against SQL injection" spirit, but given the escaping of the table name (as well as the pattern in the surrounding code), I think you're right that we're fine here.
Ah - that makes a lot of sense! 👍
|
Build failed (retrying...): |
|
Build failed (retrying...): |
|
bors r- Sadly this needs a I'm working on removing the tracking of these files in #76521. Sorry |
|
Canceled. |
Closes: cockroachdb#67510 Release note (ui change): - Added status of automatic statistics collection to the Database and Database table pages on the DB Console. - Added timestamp of last statistics collection to the Database details and Database table pages on the DB Console.
ericharmeling
force-pushed
the
add-table-stats-dbconsole
branch
from
a41e8c8 to
366bf32
Compare
No worries! Just rebased on master, reran |
|
@ajwerner This passed GH CI after my last update - good to retry the merge? |
|
bors r+ |
|
Build succeeded: |
76168: ui,server: Table stats collection details added to Database pages r=ericharmeling a=ericharmeling Closes: cockroachdb#67510 Release justification: low-risk, high benefit changes to existing functionality Release note (ui change): - Added status of automatic statistics collection to the Database and Database table pages on the DB Console. - Added timestamp of last statistics collection to the Database details and Database table pages on the DB Console. Here's a video of the changes in the UI: https://user-images.githubusercontent.com/27286675/153521703-9ed19ed0-5fe9-4cb6-a537-1d227aeb0a0b.mov Co-authored-by: Eric Harmeling <[email protected]>
ghost
mentioned this pull request
Closes: #67510
Release note (ui change):
and Database table pages on the DB Console.
details and Database table pages on the DB Console.
Here's a video of the changes in the UI:
surfacetablestats.mov