From fefd36057afcde31d70710a065af1401a1f14b99 Mon Sep 17 00:00:00 2001 From: Matthew Todd Date: Tue, 20 Dec 2022 10:53:07 -0500 Subject: [PATCH] ui: degrade gracefully when regions aren't known Part of #89949 Previously, when a tenant SQL instance had spun down (leaving us no way to remember which region it had been in), the SQL Activity pages would claim that statements and transactions had occurred in an "undefined" region. This change moves from saying "undefined" to saying nothing at all, a slightly nicer user experience. This broader problem of losing the region mapping has been described in #93268; we'll begin addressing it shortly. Release note: None --- .../src/statementDetails/statementDetails.tsx | 4 +- .../statementsTable/statementsTable.spec.tsx | 60 +++++++++++++++++++ .../src/statementsTable/statementsTable.tsx | 11 ++-- .../src/transactionsPage/transactionsPage.tsx | 2 - .../src/transactionsPage/utils.spec.ts | 47 +++++++++++++++ .../cluster-ui/src/transactionsPage/utils.ts | 4 +- 6 files changed, 120 insertions(+), 8 deletions(-) create mode 100644 pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.spec.tsx diff --git a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx b/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx index 5ca0f8bb5de6..2f1e6816483c 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx @@ -527,7 +527,9 @@ export class StatementDetails extends React.Component< (stats.nodes || []).map(node => node.toString()), ).sort(); const regions = unique( - (stats.nodes || []).map(node => nodeRegions[node.toString()]), + (stats.nodes || []) + .map(node => nodeRegions[node.toString()]) + .filter(r => r), // Remove undefined / unknown regions. ).sort(); const lastExec = diff --git a/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.spec.tsx b/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.spec.tsx new file mode 100644 index 000000000000..3538dc847188 --- /dev/null +++ b/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.spec.tsx @@ -0,0 +1,60 @@ +// Copyright 2022 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +import { assert } from "chai"; +import Long from "long"; +import { + AggregateStatistics, + populateRegionNodeForStatements, +} from "./statementsTable"; + +describe("populateRegionNodeForStatements", () => { + function statementWithNodeIDs(...nodeIDs: number[]): AggregateStatistics { + return { + aggregatedFingerprintID: "", + aggregatedFingerprintHexID: "", + label: "", + summary: "", + aggregatedTs: 0, + aggregationInterval: 0, + implicitTxn: false, + fullScan: false, + database: "", + applicationName: "", + stats: { nodes: nodeIDs.map(id => Long.fromInt(id)) }, + }; + } + + it("maps nodes to regions, sorted", () => { + const statement = statementWithNodeIDs(1, 2); + populateRegionNodeForStatements([statement], { + "1": "gcp-us-west1", + "2": "gcp-us-east1", + }); + assert.deepEqual(["gcp-us-east1", "gcp-us-west1"], statement.regions); + }); + + it("handles statements without nodes", () => { + const statement = statementWithNodeIDs(); + populateRegionNodeForStatements([statement], { + "1": "gcp-us-west1", + "2": "gcp-us-east1", + }); + assert.deepEqual(statement.regions, []); + }); + + it("excludes nodes whose region we don't know", () => { + const statement = statementWithNodeIDs(1, 2); + populateRegionNodeForStatements([statement], { + "1": "gcp-us-west1", + }); + assert.deepEqual(statement.regions, ["gcp-us-west1"]); + }); +}); diff --git a/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx b/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx index c8f440c8b41d..b0b8b60472f0 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx @@ -341,10 +341,13 @@ export function populateRegionNodeForStatements( // E.g. {"gcp-us-east1" : [1,3,4]} if (stmt.stats.nodes) { stmt.stats.nodes.forEach(node => { - if (Object.keys(regions).includes(nodeRegions[node.toString()])) { - regions[nodeRegions[node.toString()]].add(longToInt(node)); - } else { - regions[nodeRegions[node.toString()]] = new Set([longToInt(node)]); + const region = nodeRegions[node.toString()]; + if (region) { + if (Object.keys(regions).includes(region)) { + regions[region].add(longToInt(node)); + } else { + regions[region] = new Set([longToInt(node)]); + } } }); } diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx index 21df50dc6328..c59b79dbed4c 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx @@ -404,8 +404,6 @@ export class TransactionsPage extends React.Component< const statements = data?.statements || []; const { filters } = this.state; - // If the cluster is a tenant cluster we don't show info - // about nodes/regions. const nodes = Object.keys(nodeRegions) .map(n => Number(n)) .sort(); diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.spec.ts b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.spec.ts index 1c23ce961822..1056c9275dfd 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.spec.ts +++ b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.spec.ts @@ -11,6 +11,7 @@ import { assert } from "chai"; import { filterTransactions, + generateRegion, getStatementsByFingerprintId, statementFingerprintIdsToText, } from "./utils"; @@ -19,6 +20,8 @@ import { data, nodeRegions } from "./transactions.fixture"; import Long from "long"; import * as protos from "@cockroachlabs/crdb-protobuf-client"; +type Statement = + protos.cockroach.server.serverpb.StatementsResponse.ICollectedStatementStatistics; type Transaction = protos.cockroach.server.serverpb.StatementsResponse.IExtendedCollectedTransactionStatistics; @@ -288,3 +291,47 @@ SELECT _`, ); }); }); + +describe("generateRegion", () => { + function transactionWithStatementFingerprintIDs( + ...ids: number[] + ): Transaction { + return { + stats_data: { + statement_fingerprint_ids: ids.map(id => Long.fromInt(id)), + }, + }; + } + + function statementWithFingerprintAndNodeIDs( + id: number, + ...nodeIDs: number[] + ): Statement { + return { + id: Long.fromInt(id), + stats: { nodes: nodeIDs.map(id => Long.fromInt(id)) }, + }; + } + + it("gathers up the list of regions for the transaction, sorted", () => { + assert.deepEqual( + generateRegion( + transactionWithStatementFingerprintIDs(42), + [statementWithFingerprintAndNodeIDs(42, 1, 2)], + { "1": "gcp-us-west1", "2": "gcp-us-east1" }, + ), + ["gcp-us-east1", "gcp-us-west1"], + ); + }); + + it("skips over nodes with unknown regions", () => { + assert.deepEqual( + generateRegion( + transactionWithStatementFingerprintIDs(42), + [statementWithFingerprintAndNodeIDs(42, 1, 2)], + { "1": "gcp-us-west1" }, + ), + ["gcp-us-west1"], + ); + }); +}); diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts index 1bd1e5363485..382e7004389c 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts +++ b/pkg/ui/workspaces/cluster-ui/src/transactionsPage/utils.ts @@ -273,7 +273,9 @@ export const generateRegion = ( }); }); - return Array.from(regions).sort(); + return Array.from(regions) + .filter(r => r) // Remove undefined / unknown regions. + .sort(); }; /**