release-22.2: server,ui: add limit and sort to sql activity, split stmt and txns storage in redux #99403
Conversation
xinhaoz
changed the title
xinhaoz
changed the title
|
This change is |
xinhaoz
force-pushed
the
backport22.2-98815
branch
4 times, most recently
from
33a3e4a
to
4ea0b6f
Compare
xinhaoz
force-pushed
the
backport22.2-98815
branch
from
4ea0b6f
to
2cc1f87
Compare
There was a problem hiding this comment.
Great! There is one extra thing you will need to do because is a backport.
Imagine your changes go on 22.2.8, but as soon as a new cluster-ui version is created, it will use the new UI, so your current UI should work with all crdb versions < 22.2.8.
Take advantage that our staging cluster doesn't have your backend changes to test if is working. It will ignore your new parameter and return 20k results, so then you should extra the extra steps of checking (on the ui side), the amount of rows returned, if that is bigger than your request you know is using an older crdb version for the backend, so you should order the result by the column selected and filter our to return just the top X selected. This will have to be done on the frontend.
Make sure once this is merged, you update our staging cluster with the nightly image and test right away, so we can catch any issues before an actual release is created.
Reviewed 3 of 3 files at r1, 22 of 22 files at r2, 3 of 3 files at r3, 2 of 2 files at r4, 20 of 20 files at r5, 12 of 12 files at r6, 24 of 24 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)
pkg/ui/workspaces/cluster-ui/src/pageConfig/pageConfig.tsx line 24 at r7 (raw file):
export function PageConfig(props: PageConfigProps): React.ReactElement { const isCockroachCloud = useContext(CockroachCloudContext);
have you checked on Cloud? because this was added to change to a white background on cloud, which is grey in DB console
|
Previously, maryliag (Marylia Gutierrez) wrote…
Oops, this was deleted by accident during merge conflict resolution, I'll add it back. |
There was a problem hiding this comment.
Yeah I've already verified that on CC it does just throw away the new parms, returning the results as in the old version. Are we deciding to take this approach with mixed versions over just showing a message that the page will not be working as expected until upgrade is complete? Even if we do the sort and limit on the client side, it's still going to be using a subset of the data that may not contain all the queries they expected -- are we okay with that? Is there a way to determine if an upgrade is ongoing on the CC side -- maybe we can hide the limit and sort knobs until it's finalized.
Reviewable status:
There was a problem hiding this comment.
I'm not even talking about mixed version. Someone could be on 22.2.0 and never upgrade, but they will see this new UI. So what we should do on that case?
Reviewable status:
There was a problem hiding this comment.
Ah right, I forgot it'll just pick up the latest cluster-ui anyways. Sounds good, I can check in the request response directly and do the sort/limit.
Reviewable status:
xinhaoz
force-pushed
the
backport22.2-98815
branch
2 times, most recently
from
cfdd4eb
to
acd9fcf
Compare
xinhaoz
force-pushed
the
backport22.2-98815
branch
from
acd9fcf
to
42a67ed
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r9, 1 of 1 files at r10, 3 of 3 files at r11, all commit messages.
Reviewable status:
pkg/ui/workspaces/cluster-ui/src/api/statementsApi.ts line 214 at r11 (raw file):
// Filter out stmts not belonging to txns response. res.statements = res.statements.filter(stmt =>
at the beginning of this function you did res.statements = []; so you won't have anything to filter out here
Code quote:
res.statements = [];
xinhaoz
force-pushed
the
backport22.2-98815
branch
from
42a67ed
to
7bf5642
Compare
|
Previously, maryliag (Marylia Gutierrez) wrote…
Ah nice catch. Fixed and added more tests to verify this part of the behaviour. |
xinhaoz
force-pushed
the
backport22.2-98815
branch
from
7bf5642
to
05f4c52
Compare
xinhaoz
force-pushed
the
backport22.2-98815
branch
2 times, most recently
from
7c20f36
to
6e18961
Compare
This commit adds a new field, fetch_mode, which is a message containing an enum specifying what kind of stats to return from the combined stmts endpoint. The options are - StmtStatsOnly - TxnStatsOnly Leaving this field null will fetch both stmts and txns. If `TxnStatsOnly` is specified, we will also include the stmt stats for the stms in each txn in the returned txn payload. This is because in the client app we need the stmt stats to properly build the txn fingerprint pages. In the future, we will split this API instead of using this fetch mode flag, but this is currently preferred to make it easier to backport for perf improvements. Epic: none Part of: cockroachdb#97252 Part of: cockroachdb#97875 Release note: None
Previously, both the stmt and txns fingerprints pages were using the same api response from the /statements. This was because we need the stmts response to build txns pages. Howevever having to fetch both types of stats can slow down the request. Now that we can specify the fetch_mode as part of the request, we can split the 2 into their own api calls and redux fields. Epic: none Part of: cockroachdb#97875 Release note: None
This commit adds limit and sort fields to the combined statements request. These params can be used to specify how much data is returned and the priority at which to return data (e.g. top-k). The current sort options are: - service latency - contention time - execution count Epic: none Part of: cockroachdb#97876 Part of: cockroachdb#97875 Release note: None
This commit changes the table used for the combined stats endpoint from the view combining persisted and in-memory stats to the view that is just a wrapper around the system table. Thus, we are now reading only persisted stats from the system table for the combined stats endpoint. This commit updates tests using the combined api to flush stats before using the api. Epic: None Release note (ui change): On the SQL Activity fingerprints pages, users will not see stats that have not yet been flushed to disk.
xinhaoz
force-pushed
the
backport22.2-98815
branch
from
6e18961
to
f2dd021
Compare
This commit adds new knobs to the sql stats fingerprint pages. Users can now specify a limit and sort priority with their sql stats fingerprints requests. Closes: cockroachdb#97876 Part of: cockroachdb#97875 Release note: None
Previously, the calculatation for the `% of runtime` column for the sql activity pages, was performed on the client app by summing the (execCount* avgAvcLat) from all statements returned. This was okay when we were returning a large number of stmts (20,000) but now that we have reduced that limit significantly (default 100, with a max provided option of 500), performing this calculation on the client side won't give us the full picture of total runtime of the workload in the requested time interval. This commit modifies the statements API to return the total estimated runtime of the workload so that we can continue to show the '% of runtime` column. We also now provide this as a server sort option, so that users can request the top-k stmts or txns by % of runtime. Epic: none Release note (ui change): Users can request top-k stmts by % runtime in sql activity fingerprints pages.
Create new Search Criteria component, and add to Statements and Transactions page. This commit also updates the UI, with the new position of filters and reset sql stats. Part Of cockroachdb#98891 Release note (ui change): Add Search Criteria to Statements and Transactions pages, updates UX with improvements.
Previously, in our db-console the route for a txn fingerprint detail page used the aggregated timestamp. That is no longer the ase, and the routing test to the page should be updated to reflec that. This change also adds some missing props to the txnDetailsPage story file. Epic: none Release note: None
When we are on a new cluster-ui version that sends user configured limit and sort values in the sql stats request, we may be talking to an older server version that does not support those request params. In that scenario, The server response will be that of the request without limit and sort. This commit checks the response from the server so that if we find that the payload limit is not equal to the requested limit, we manually sort and truncate the response to match the parameters the user sees on the UI. Epic: none Release note: None
Previously, the `MetricsDataProvider` component queried the redux store for the `TimeScale` object which contained details of the currently active time window. This piece of state was assumed to update to account for the "live" moving window that metrics show when pre-set lookback time windows are selected. A recent PR: cockroachdb#98331 removed the feature that polled new data from SQL pages, which also disabled polling on metrics pages due to the re-use of `TimeScale`. This commit modifies the `MetricsDataProvider` to instead read the `metricsTime` field of the `TimeScaleState` object. This object was constructed for use by the `MetricsDataProvider` but was not wired up to the component. Resolves cockroachdb#99524 Epic: None Release note: None
xinhaoz
force-pushed
the
backport22.2-98815
branch
from
f2dd021
to
2b64c3f
Compare
There was a problem hiding this comment.
Awesome work!
Reviewed 7 of 34 files at r13, 23 of 24 files at r14, 5 of 5 files at r15, 2 of 2 files at r16, all commit messages.
Reviewable status:
…rForCachedDataField` Note that the `testUtils` file in the api folder is a forward port of testing utilities created for cockroachdb#99403 This commit adds testing for functions in `sqlActivity/util.tsx` and adds some checks to those functions to make them more safe to use. The param `nodeRegions` is removed from `filteredStatementsData` as it was unused. Epic: none Release note: None
…rForCachedDataField` Note that the `testUtils` file in the api folder is a forward port of testing utilities created for cockroachdb#99403 This commit adds testing for functions in `sqlActivity/util.tsx` and adds some checks to those functions to make them more safe to use. The param `nodeRegions` is removed from `filteredStatementsData` as it was unused. Epic: none Release note: None
…rForCachedDataField` Note that the `testUtils` file in the api folder is a forward port of testing utilities created for cockroachdb#99403 This commit adds testing for functions in `sqlActivity/util.tsx` and adds some checks to those functions to make them more safe to use. The param `nodeRegions` is removed from `filteredStatementsData` as it was unused. Epic: none Release note: None
Backport 7/7 commits from #98815.
Backport 1/1 commits from #99712.
See individual commits.
DB-Console (22.2): https://www.loom.com/share/d13568d08c0443d7a0e51f065f30bea4
DB-Console Details pages (22.2): https://www.loom.com/share/8f84030cf3904877b06a4b3d244b09f1
/cc @cockroachdb/release
Release justification: low-risk updates to existing functionality (code yellow fixes)