server, ui: add sort and limit to sql activity pages, switch sql activity pages to read only from system table #98815

xinhaoz · 2023-03-16T20:51:56Z

See individual commits.

DB-Console: https://www.loom.com/share/73248244baa54cdf80389d0eec788447
DB-Console Details pages: https://www.loom.com/share/2d74c6cf082f4dcfb5e7f690166c2baf

cockroach-teamcity · 2023-03-16T20:52:14Z

This change is

maryliag

nit: You should update your PR title, since it has a lot more than that
Can you add looms showing the new behaviour working on all SQL Activity pages on both DB and CC?

Reviewed 3 of 3 files at r1, 21 of 21 files at r2, 3 of 3 files at r3, 5 of 5 files at r4, 18 of 18 files at r5, 12 of 12 files at r6, 24 of 24 files at r7, 34 of 34 files at r8.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)

-- commits line 79 at r4:
you should add a release note here, because documentation will need to be updated to show users they might not see data that was just executed

pkg/server/combined_statement_stats.go line 80 at r1 (raw file):

	var err error

	if req.FetchMode == nil || req.FetchMode.StatsType != serverpb.CombinedStatementsStatsRequest_StmtStatsOnly {

nit: can you use StatsType == TxnStatsOnly to make more clear? Is a little confusing this way

pkg/server/combined_statement_stats.go line 256 at r3 (raw file):

	aostClause := testingKnobs.GetAOSTClause()
	query := fmt.Sprintf(`
SELECT * FROM (

why you need this "wrapper" select?

pkg/server/combined_statement_stats.go line 264 at r4 (raw file):

    metadata,
    crdb_internal.merge_statement_stats(array_agg(statistics)) AS statistics
FROM crdb_internal.statement_statistics_persisted %s

there are other places on this file that should also be updated to the new tables, e.g. all the calls on the details. Basically all usages should be replaced with the new views, since there is also performance issues on details endpoints

pkg/ui/workspaces/cluster-ui/src/api/statementsApi.ts line 54 at r5 (raw file):

};

// THhe required fields to create a stmts request.

nit: The

pkg/ui/workspaces/cluster-ui/src/searchCriteria/searchCriteria.tsx line 1 at r7 (raw file):

// Copyright 2021 The Cockroach Authors.

nit: 2023

pkg/ui/workspaces/cluster-ui/src/store/transactionStats/index.ts line 1 at r2 (raw file):

// Copyright 2021 The Cockroach Authors.

nit: 2023

pkg/ui/workspaces/cluster-ui/src/store/transactionStats/txnStats.reducer.ts line 1 at r2 (raw file):

// Copyright 2021 The Cockroach Authors.

nit: 2023

pkg/ui/workspaces/cluster-ui/src/store/transactionStats/txnStats.reducer.ts line 41 at r2 (raw file):

};

// This is actually statements only, despite the SQLStatsState name.

this comment is not valid here

pkg/ui/workspaces/cluster-ui/src/store/transactionStats/txnStats.reducer.ts line 47 at r2 (raw file):

  initialState,
  reducers: {
    received: (state, action: PayloadAction<StatementsResponse>) => {

maybe you want to add a comment that is indeed StatementResponse for Transaction, might be confusing for someone looking into this in the future and asking why they don't match

pkg/ui/workspaces/cluster-ui/src/store/transactionStats/txnStats.sagas.ts line 1 at r2 (raw file):

// Copyright 2021 The Cockroach Authors.

nit: 2023

pkg/ui/workspaces/cluster-ui/src/store/transactionStats/txnStats.selector.ts line 1 at r2 (raw file):

// Copyright 2021 The Cockroach Authors.

nit: 2023

pkg/ui/workspaces/db-console/src/util/api.ts line 748 at r5 (raw file):

// getCombinedStatements returns statements the cluster has recently executed, and some stats about them.
export function getCombinedStatements(

curious of why this is no longer needed

pkg/ui/workspaces/db-console/src/views/transactions/transactionDetails.tsx line 39 at r5 (raw file):

import { selectTxnInsightsByFingerprint } from "src/views/insights/insightsSelectors";
import { selectHasAdminRole } from "src/redux/user";
import { limitSetting } from "./transactionsPage";

can you combine this with the import above with the other stuff coming from this file?

pkg/ccl/serverccl/statusccl/tenant_status_test.go line 440 at r4 (raw file):

		}

		for _, tenantStmt := range tenantCombinedStats.Statements {

why those tests were removed?

pkg/ccl/serverccl/statusccl/tenant_status_test.go line 447 at r4 (raw file):

	// ensure tests are deterministic.
	testCluster.TenantConn(0 /* idx */).
		Exec(t, "SET CLUSTER SETTING sql.stats.flush.interval = '24h'")

why can you just shut off the flush like it was done previously?

pkg/ui/workspaces/cluster-ui/src/store/transactionStats/txnStats.sagas.spec.ts line 1 at r2 (raw file):

// Copyright 2021 The Cockroach Authors.

nit: 2023

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

xinhaoz

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

pkg/server/combined_statement_stats.go line 80 at r1 (raw file):