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: tenants use sqlstats-supplied regions #98410

Merged
merged 1 commit into from
Mar 14, 2023
Merged

cluster-ui: tenants use sqlstats-supplied regions #98410

merged 1 commit into from
Mar 14, 2023

Conversation

matthewtodd
Copy link
Contributor

@matthewtodd matthewtodd commented Mar 10, 2023

Fixes #98056.

As of #95449, the SQL Activity pages in the DB Console can draw regions information directly from the sqlstats tables, rather than having to translate node IDs to regions on page load.

Here, we make that switch, but for non-system tenants only, because:

  1. The ephemeral nature of serverless nodes made this view-time mapping especially problematic in that context. (See further notes in sqlstats: include regions in statement_statistics #95449.)

  2. The system-tenant views also include KV node IDs in a special Regions/Nodes column, which we are unable to recreate given the backend storage structure. (Future design work might suggest removing these node IDs altogether, for a unified UI.)

Screenshots!

Statements, with and without regions filter

statements

statements - filtered

Statement details

statement details

Transactions, with and without regions filter

transactions

transactions - filtered

Transaction details

transaction details

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@matthewtodd matthewtodd changed the title cluster-ui: use backend-supplied regions for transactions cluster-ui: tenants use sqlstats-supplied regions Mar 11, 2023
@matthewtodd matthewtodd requested a review from a team March 14, 2023 14:32
@matthewtodd matthewtodd marked this pull request as ready for review March 14, 2023 14:33
@matthewtodd matthewtodd requested a review from a team March 14, 2023 14:33
Fixes #98056.

As of #95449, the SQL Activity pages in the DB Console can draw regions
information directly from the sqlstats tables, rather than having to
translate node IDs to regions on page load.

Here, we make that switch, but for non-system tenants only, because:

1. The ephemeral nature of serverless nodes made this view-time mapping
   especially problematic in that context. (See further notes in #95449.)

2. The system-tenant views also include KV node IDs in a special
   Regions/Nodes column, which we are unable to recreate given the
   backend storage structure. (Future design work might suggest removing
   these node IDs altogether, for a unified UI.)

Release note: None
@matthewtodd
Copy link
Contributor Author

@maryliag we'll want to coordinate when to put this into a cluster-ui release.

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

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

we'll want to coordinate when to put this into a cluster-ui release.

what is the concern of what should be added together?

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @matthewtodd)

@matthewtodd
Copy link
Contributor Author

Thanks, @maryliag!

bors r+

@craig
Copy link
Contributor

craig bot commented Mar 14, 2023

Build failed (retrying...):

@craig craig bot merged commit 421d136 into cockroachdb:master Mar 14, 2023
@craig
Copy link
Contributor

craig bot commented Mar 14, 2023

Build succeeded:

@matthewtodd matthewtodd deleted the sqlstats-regions branch March 14, 2023 19:02
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.

cluster-ui: use backend-supplied regions in SQL Activity
3 participants