From 3177bc5673e0e4d919a9eb531d310da31278209e Mon Sep 17 00:00:00 2001 From: Xin Hao Zhang Date: Tue, 15 Nov 2022 16:49:15 -0500 Subject: [PATCH] cluster-ui: transaction insights bug fixes Fixes #91914 Fixes #91915 Fixes #91927 This commit fixes txn insights bugs and adds unit tests for utils used by the insights pages. Fixes for txn insights: - De-duplicate txn contention insights labels in overview page for high contention - Remove query column for statement details and single stmt transactions - Include statement query for a txn insight recommendation if there are multiple stmts in the txn being reported Release note (ui change): Query column in insight recs table removed. Instead, stmt is included in the description if the txn being reported has multiple stmts. --- .../cluster-ui/src/api/insightsApi.ts | 23 +- .../cluster-ui/src/insights/types.ts | 14 +- .../cluster-ui/src/insights/utils.spec.ts | 665 ++++++++++++++++++ .../cluster-ui/src/insights/utils.ts | 31 +- .../transactionInsightDetailsOverviewTab.tsx | 31 +- .../src/insightsTable/insightsTable.tsx | 95 ++- .../planDetails/planDetails.tsx | 2 +- .../workspaces/cluster-ui/src/util/format.ts | 12 +- 8 files changed, 797 insertions(+), 76 deletions(-) create mode 100644 pkg/ui/workspaces/cluster-ui/src/insights/utils.spec.ts diff --git a/pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts b/pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts index ccfb99c37386..af50ae40ec69 100644 --- a/pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts +++ b/pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts @@ -19,6 +19,7 @@ import { } from "./sqlApi"; import { BlockedContentionDetails, + dedupInsights, FlattenedStmtInsightEvent, getInsightFromCause, getInsightsFromProblemsAndCauses, @@ -564,10 +565,14 @@ function organizeExecutionInsightsResponseIntoTxns( return []; } - const txnByExecID = new Map(); + // Map of Transaction exec and fingerprint id -> txn. + const txnByIDs = new Map(); + const getTxnKey = (row: ExecutionInsightsResponseRow) => + row.txn_id.concat(row.txn_fingerprint_id); response.execution.txn_results[0].rows.forEach(row => { - let txnInsight: TxnInsightEvent = txnByExecID.get(row.txn_id); + const rowKey = getTxnKey(row); + let txnInsight: TxnInsightEvent = txnByIDs.get(rowKey); if (!txnInsight) { txnInsight = { @@ -587,7 +592,7 @@ function organizeExecutionInsightsResponseIntoTxns( insights: [], queries: [], }; - txnByExecID.set(row.txn_id, txnInsight); + txnByIDs.set(rowKey, txnInsight); } const start = moment.utc(row.start_time); @@ -630,15 +635,9 @@ function organizeExecutionInsightsResponseIntoTxns( ); }); - txnByExecID.forEach(txn => { + txnByIDs.forEach(txn => { // De-duplicate top-level txn insights. - const insightsSeen = new Set(); - txn.insights = txn.insights.reduce((insights, i) => { - if (insightsSeen.has(i.name)) return insights; - insightsSeen.add(i.name); - insights.push(i); - return insights; - }, []); + txn.insights = dedupInsights(txn.insights); // Sort stmt insights for each txn by start time. txn.statementInsights.sort((a, b) => { @@ -648,7 +647,7 @@ function organizeExecutionInsightsResponseIntoTxns( }); }); - return Array.from(txnByExecID.values()); + return Array.from(txnByIDs.values()); } type InsightQuery = { diff --git a/pkg/ui/workspaces/cluster-ui/src/insights/types.ts b/pkg/ui/workspaces/cluster-ui/src/insights/types.ts index ab3d5b94c5f0..52dbb13820ea 100644 --- a/pkg/ui/workspaces/cluster-ui/src/insights/types.ts +++ b/pkg/ui/workspaces/cluster-ui/src/insights/types.ts @@ -179,7 +179,7 @@ export type ContentionEvent = { execType: InsightExecEnum; }; -const highContentionInsight = ( +export const highContentionInsight = ( execType: InsightExecEnum, latencyThreshold?: number, contentionDuration?: number, @@ -207,9 +207,9 @@ const highContentionInsight = ( }; }; -const slowExecutionInsight = ( +export const slowExecutionInsight = ( execType: InsightExecEnum, - latencyThreshold: number, + latencyThreshold?: number, ): Insight => { let threshold = latencyThreshold + "ms"; if (!latencyThreshold) { @@ -226,7 +226,7 @@ const slowExecutionInsight = ( }; }; -const planRegressionInsight = (execType: InsightExecEnum): Insight => { +export const planRegressionInsight = (execType: InsightExecEnum): Insight => { const description = `This ${execType} was slow because we picked the wrong plan, ` + `possibly due to outdated statistics, the statement using different literals or ` + @@ -240,7 +240,7 @@ const planRegressionInsight = (execType: InsightExecEnum): Insight => { }; }; -const suboptimalPlanInsight = (execType: InsightExecEnum): Insight => { +export const suboptimalPlanInsight = (execType: InsightExecEnum): Insight => { const description = `This ${execType} was slow because a good plan was not available, whether ` + `due to outdated statistics or missing indexes.`; @@ -253,7 +253,7 @@ const suboptimalPlanInsight = (execType: InsightExecEnum): Insight => { }; }; -const highRetryCountInsight = (execType: InsightExecEnum): Insight => { +export const highRetryCountInsight = (execType: InsightExecEnum): Insight => { const description = `This ${execType} has being retried more times than the value of the ` + `'sql.insights.high_retry_count.threshold' cluster setting.`; @@ -266,7 +266,7 @@ const highRetryCountInsight = (execType: InsightExecEnum): Insight => { }; }; -const failedExecutionInsight = (execType: InsightExecEnum): Insight => { +export const failedExecutionInsight = (execType: InsightExecEnum): Insight => { const description = `This ${execType} execution failed completely, due to contention, resource ` + `saturation, or syntax errors.`; diff --git a/pkg/ui/workspaces/cluster-ui/src/insights/utils.spec.ts b/pkg/ui/workspaces/cluster-ui/src/insights/utils.spec.ts new file mode 100644 index 000000000000..56afb8a6353e --- /dev/null +++ b/pkg/ui/workspaces/cluster-ui/src/insights/utils.spec.ts @@ -0,0 +1,665 @@ +// 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 moment from "moment"; +import { + mergeTxnContentionAndStmtInsights, + filterTransactionInsights, + getAppsFromTransactionInsights, + filterStatementInsights, + getAppsFromStatementInsights, + getInsightsFromProblemsAndCauses, + flattenTxnInsightsToStmts, + mergeTxnInsightDetails, + dedupInsights, +} from "./utils"; +import { + TxnInsightEvent, + StatementInsightEvent, + InsightNameEnum, + failedExecutionInsight, + FlattenedStmtInsightEvent, + InsightExecEnum, + TxnContentionInsightEvent, + highContentionInsight, + slowExecutionInsight, + planRegressionInsight, + suboptimalPlanInsight, + highRetryCountInsight, + BlockedContentionDetails, + TxnInsightDetails, +} from "./types"; + +const INTERNAL_APP_PREFIX = "$ internal"; + +const txnContentionEventMock: TxnContentionInsightEvent = { + transactionID: "execution", + transactionFingerprintID: "fingerprint", + queries: ["select 1"], + insights: [highContentionInsight(InsightExecEnum.TRANSACTION)], + startTime: moment(), + contentionDuration: moment.duration(100, "millisecond"), + contentionThreshold: 100, + application: "sql_obs_fun_times", + execType: InsightExecEnum.TRANSACTION, +}; + +const blockedContentionMock: BlockedContentionDetails = { + collectionTimeStamp: moment(), + blockingExecutionID: "execution", + blockingTxnFingerprintID: "block", + blockingQueries: ["select 1"], + contendedKey: "key", + schemaName: "schema", + databaseName: "db", + tableName: "table", + indexName: "index", + contentionTimeMs: 500, +}; + +function mockTxnContentionInsightEvent( + fields: Partial = {}, +): TxnContentionInsightEvent { + return { ...txnContentionEventMock, ...fields }; +} + +const statementInsightMock: StatementInsightEvent = { + statementExecutionID: "execution", + statementFingerprintID: "fingerprint", + startTime: moment(), + isFullScan: false, + elapsedTimeMillis: 100, + totalContentionTime: moment.duration(100, "millisecond"), + endTime: moment(), + rowsRead: 4, + rowsWritten: 1, + causes: ["FailedExecution"], + problem: "SlowExecution", + query: "select 1", + insights: [failedExecutionInsight(InsightExecEnum.STATEMENT)], + indexRecommendations: [], + planGist: "gist", +}; + +function mockStmtInsight(fields: Partial) { + return { ...statementInsightMock, ...fields }; +} + +function mockFlattenedStmtInsightEvent( + fields: Partial = {}, +): FlattenedStmtInsightEvent { + return { + ...statementInsightMock, + transactionExecutionID: "transactionExecution", + transactionFingerprintID: "fingerprintExecution", + implicitTxn: false, + sessionID: "sessionID", + databaseName: "defaultdb", + username: "sql-obs", + priority: "high", + retries: 0, + application: "coolApp", + ...fields, + }; +} + +const txnInsightEventMock: TxnInsightEvent = { + databaseName: "defaultDb", + username: "craig", + priority: "high", + retries: 0, + implicitTxn: false, + sessionID: "123", + transactionExecutionID: "execution", + transactionFingerprintID: "fingerprint", + application: "sql_obs_fun_times", + lastRetryReason: null, + contention: null, + statementInsights: [statementInsightMock], + insights: [failedExecutionInsight(InsightExecEnum.TRANSACTION)], + queries: ["select 1"], +}; + +function mockTxnInsightEvent( + fields: Partial = {}, +): TxnInsightEvent { + return { ...txnInsightEventMock, ...fields }; +} + +describe("test workload insights utils", () => { + describe("filterTransactionInsights", () => { + const txns = [ + mockTxnInsightEvent({ application: "hello" }), + mockTxnInsightEvent({ application: "world" }), + mockTxnInsightEvent({ application: "cat" }), + mockTxnInsightEvent({ application: "frog" }), + mockTxnInsightEvent({ application: "cockroach" }), + mockTxnInsightEvent({ application: "cockroach" }), + mockTxnInsightEvent({ application: "db" }), + mockTxnInsightEvent({ application: "db" }), + ]; + + it("should filter out txns not matching provided filters", () => { + const filters = { app: "cockroach,db" }; + const filtered = filterTransactionInsights( + txns, + filters, + INTERNAL_APP_PREFIX, + ); + expect(filtered.length).toEqual(4); + }); + + it("should filter out or include internal txns depending on filters", () => { + const txnsWithInternal = [ + ...txns, + mockTxnInsightEvent({ application: "$ internal-my-app" }), + mockTxnInsightEvent({ application: "$ internal-my-app" }), + ]; + // If internal app name not included in filter, internal apps should be + // filtered out. + const filters = { app: "" }; + let filtered = filterTransactionInsights( + txnsWithInternal, + filters, + INTERNAL_APP_PREFIX, + ); + expect(filtered.length).toEqual(txns.length); + + // Now they should be included. + filters.app = INTERNAL_APP_PREFIX; + filtered = filterTransactionInsights( + txnsWithInternal, + filters, + INTERNAL_APP_PREFIX, + ); + expect(filtered.length).toEqual(2); + }); + + it("should filter out txns not matching search", () => { + const txnsWithQueries = [ + mockTxnInsightEvent({ queries: ["select foo", "update bar"] }), + mockTxnInsightEvent({ queries: ["hello", "world", "foo"] }), + ]; + + let filtered = filterTransactionInsights( + txnsWithQueries, + { app: "" }, + INTERNAL_APP_PREFIX, + "foo", + ); + expect(filtered.length).toEqual(2); + + filtered = filterTransactionInsights( + txnsWithQueries, + { app: "" }, + INTERNAL_APP_PREFIX, + "update", + ); + expect(filtered.length).toEqual(1); + + filtered = filterTransactionInsights( + txnsWithQueries, + { app: "" }, + INTERNAL_APP_PREFIX, + "no results", + ); + expect(filtered.length).toEqual(0); + }); + + it("should filter txns given a mix of requirements", () => { + const txnsMixed = [ + // This should be the only txn remaining. + mockTxnInsightEvent({ + application: "myApp", + queries: ["select foo"], + }), + // No required search term. + mockTxnInsightEvent({ application: "myApp", queries: ["update bar"] }), + // No required app. + mockTxnInsightEvent({ queries: ["hello", "world", "select foo"] }), + // Internal app should be filtered out. + mockTxnInsightEvent({ + application: INTERNAL_APP_PREFIX, + queries: ["select foo"], + }), + ]; + + const filtered = filterTransactionInsights( + txnsMixed, + { app: "myApp" }, + INTERNAL_APP_PREFIX, + "select foo", + ); + expect(filtered.length).toEqual(1); + }); + }); + + describe("getAppsFromTransactionInsights", () => { + const appNames = ["one", "two", "three"]; + const txns = appNames.map(app => mockTxnInsightEvent({ application: app })); + + // Multiple internal app names should all become the internal + // app name prefix. + txns.push( + mockTxnInsightEvent({ + application: "$ internal-app", + }), + mockTxnInsightEvent({ + application: "$ internal-another-app", + }), + ); + + const appsFromTxns = getAppsFromTransactionInsights( + txns, + INTERNAL_APP_PREFIX, + ); + expect(appsFromTxns.length).toEqual(appNames.length + 1); + appNames.forEach(app => expect(appsFromTxns.includes(app)).toBeTruthy()); + }); + + describe("filterStatementInsights", () => { + const stmts = [ + mockFlattenedStmtInsightEvent({ application: "hello" }), + mockFlattenedStmtInsightEvent({ application: "world" }), + mockFlattenedStmtInsightEvent({ application: "cat" }), + mockFlattenedStmtInsightEvent({ application: "frog" }), + mockFlattenedStmtInsightEvent({ application: "cockroach" }), + mockFlattenedStmtInsightEvent({ application: "cockroach" }), + mockFlattenedStmtInsightEvent({ application: "db" }), + mockFlattenedStmtInsightEvent({ application: "db" }), + ]; + + it("should filter out stmts not matching provided filters", () => { + const filters = { app: "cockroach,db" }; + const filtered = filterStatementInsights( + stmts, + filters, + INTERNAL_APP_PREFIX, + ); + expect(filtered.length).toEqual(4); + }); + + it("should filter out or include internal stmts depending on filters", () => { + const stmtsWithInternal = [ + ...stmts, + mockFlattenedStmtInsightEvent({ application: "$ internal-my-app" }), + mockFlattenedStmtInsightEvent({ application: "$ internal-my-app" }), + ]; + // If internal app name not included in filter, internal apps should be + // filtered out. + const filters = { app: "" }; + let filtered = filterStatementInsights( + stmtsWithInternal, + filters, + INTERNAL_APP_PREFIX, + ); + expect(filtered.length).toEqual(stmts.length); + + // Now they should be included. + filters.app = INTERNAL_APP_PREFIX; + filtered = filterStatementInsights( + stmtsWithInternal, + filters, + INTERNAL_APP_PREFIX, + ); + expect(filtered.length).toEqual(2); + }); + + it("should filter out txns not matching search", () => { + const stmtsWithQueries = [ + mockFlattenedStmtInsightEvent({ query: "select foo" }), + mockFlattenedStmtInsightEvent({ query: "hello" }), + ]; + + let filtered = filterStatementInsights( + stmtsWithQueries, + { app: "" }, + INTERNAL_APP_PREFIX, + "foo", + ); + expect(filtered.length).toEqual(1); + + filtered = filterStatementInsights( + stmtsWithQueries, + { app: "" }, + INTERNAL_APP_PREFIX, + "hello", + ); + expect(filtered.length).toEqual(1); + + filtered = filterStatementInsights( + stmtsWithQueries, + { app: "" }, + INTERNAL_APP_PREFIX, + "no results", + ); + expect(filtered.length).toEqual(0); + }); + + it("should filter txns given a mix of requirements", () => { + const stmtsMixed = [ + // This should be the only txn remaining. + mockFlattenedStmtInsightEvent({ + application: "myApp", + query: "select foo", + }), + // No required search term. + mockFlattenedStmtInsightEvent({ + application: "myApp", + query: "update bar", + }), + // No required app. + mockFlattenedStmtInsightEvent({ + query: "hello world", + }), + // Internal app should be filtered out. + mockFlattenedStmtInsightEvent({ + application: INTERNAL_APP_PREFIX, + query: "select foo", + }), + ]; + + const filtered = filterStatementInsights( + stmtsMixed, + { app: "myApp" }, + INTERNAL_APP_PREFIX, + "select foo", + ); + expect(filtered.length).toEqual(1); + }); + it; + }); + + describe("getAppsFromStatementInsights", () => { + const appNames = ["one", "two", "three"]; + const stmts = appNames.map(app => + mockFlattenedStmtInsightEvent({ application: app }), + ); + + // Internal app name should all be consolidated to the prefix.. + stmts.push( + mockFlattenedStmtInsightEvent({ + application: "$ internal-name", + }), + mockFlattenedStmtInsightEvent({ + application: "$ internal-another-app", + }), + ); + + const appsFromStmts = getAppsFromStatementInsights( + stmts, + INTERNAL_APP_PREFIX, + ); + expect(appsFromStmts.length).toEqual(appNames.length + 1); + appNames.forEach(app => expect(appsFromStmts.includes(app)).toBeTruthy()); + }); + + describe("getInsightsFromProblemsAndCauses", () => { + const createTestCases = (execType: InsightExecEnum) => [ + { + problem: "FailedExecution", + causes: [InsightNameEnum.failedExecution], + expectedInsights: [failedExecutionInsight(execType)], + }, + { + problem: "SlowExecution", + causes: [InsightNameEnum.failedExecution], + expectedInsights: [failedExecutionInsight(execType)], + }, + { + problem: "SlowExecution", + causes: [], + expectedInsights: [slowExecutionInsight(execType)], + }, + { + problem: "SlowExecution", + causes: [ + InsightNameEnum.planRegression, + InsightNameEnum.suboptimalPlan, + InsightNameEnum.highRetryCount, + InsightNameEnum.highContention, + ], + expectedInsights: [ + planRegressionInsight(execType), + suboptimalPlanInsight(execType), + highRetryCountInsight(execType), + highContentionInsight(execType), + ], + }, + { + problem: "random", + causes: [InsightNameEnum.failedExecution], + expectedInsights: [], + }, + ]; + + [InsightExecEnum.STATEMENT, InsightExecEnum.TRANSACTION].forEach(type => { + createTestCases(type).forEach(tc => { + const insights = getInsightsFromProblemsAndCauses( + tc.problem, + tc.causes, + type, + ); + expect(insights.length).toEqual(tc.expectedInsights.length); + insights.forEach((insight, i) => { + expect(insight.name).toEqual(tc.expectedInsights[i].name); + expect(insight.description).toEqual( + tc.expectedInsights[i].description, + ); + }); + }); + }); + }); + + describe("flattenTxnInsightsToStmts", () => { + // Mock transactions, where each txn will have 2 stmt insights + // with problems and 2 with no problems. + // The 2 with no problems should NOT be included in the + // flattened array. + const txns = new Array(4).map((_, i) => + mockTxnInsightEvent({ + transactionExecutionID: `exec${i}`, + statementInsights: [ + mockStmtInsight({ statementExecutionID: `exec${i * 2}` }), + mockStmtInsight({ statementExecutionID: `exec${i * 2 + 1}` }), + mockStmtInsight({ insights: [] }), // should be excluded + mockStmtInsight({ insights: [] }), // should be excluded + ], + }), + ); + + const numStmts = txns.reduce( + (sum, txn) => (sum += txn.statementInsights.length), + 0, + ); + + const flattened = flattenTxnInsightsToStmts(txns); + expect(flattened.length).toEqual(numStmts); + + txns.forEach((txn, ti) => { + txn.statementInsights.forEach((stmt, si) => { + const flattenedStmt = flattened[ti * 2 + si]; + expect(stmt.statementExecutionID).toEqual( + flattenedStmt.statementExecutionID, + ); + expect(stmt.elapsedTimeMillis).toEqual( + flattenedStmt.statementExecutionID, + ); + expect(stmt.startTime.unix()).toEqual(flattenedStmt.startTime.unix()); + expect(stmt.endTime.unix()).toEqual(flattenedStmt.endTime.unix()); + expect(txn.transactionFingerprintID).toEqual( + flattenedStmt.transactionExecutionID, + ); + expect(txn.transactionExecutionID).toEqual( + flattenedStmt.transactionExecutionID, + ); + expect(txn.sessionID).toEqual(flattenedStmt.sessionID); + expect(txn.application).toEqual(flattenedStmt.application); + expect(txn.databaseName).toEqual(flattenedStmt.databaseName); + expect(txn.implicitTxn).toEqual(flattenedStmt.implicitTxn); + expect(txn.username).toEqual(flattenedStmt.username); + expect(txn.priority).toEqual(flattenedStmt.priority); + expect(txn.retries).toEqual(flattenedStmt.retries); + }); + }); + }); + + describe("mergeTxnContentionAndStmtInsights", () => { + const txnInsights = [ + mockTxnInsightEvent({ + transactionExecutionID: "hello", + transactionFingerprintID: "world", + insights: [...txnContentionEventMock.insights], + }), + mockTxnInsightEvent({ + transactionExecutionID: "cockroach", + transactionFingerprintID: "labs", + }), + ]; + + const txnContentionInsights = [ + mockTxnContentionInsightEvent({ + // This entry should be merged with above. + transactionID: "hello", + transactionFingerprintID: "world", + insights: txnContentionEventMock.insights, + }), + mockTxnContentionInsightEvent({ + transactionID: "just", + transactionFingerprintID: "by myself", + }), + ]; + + const merged = mergeTxnContentionAndStmtInsights( + txnInsights, + txnContentionInsights, + ); + + expect(merged.length).toEqual(3); + + const mergedTxn = merged.find( + txn => txn.transactionExecutionID === "hello", + ); + + expect(mergedTxn.contention.asMilliseconds()).toEqual( + txnContentionEventMock.contentionDuration.asMilliseconds(), + ); + + // These fields are not available on the contention event but are on + // the txn insight event from stmts. + expect(mergedTxn.sessionID).toEqual(txnInsightEventMock.sessionID); + expect(mergedTxn.databaseName).toEqual(txnInsightEventMock.databaseName); + expect(mergedTxn.username).toEqual(txnInsightEventMock.username); + expect(mergedTxn.priority).toEqual(txnInsightEventMock.priority); + expect(mergedTxn.retries).toEqual(txnInsightEventMock.retries); + expect(mergedTxn.implicitTxn).toEqual(txnInsightEventMock.implicitTxn); + expect(mergedTxn.sessionID).toEqual(txnInsightEventMock.sessionID); + + // Check insights are de-duplicated. + expect(mergedTxn.insights.length).toEqual(1); + }); + + describe("mergeTxnInsightDetails", () => { + const txnInsightFromStmts = mockTxnInsightEvent({ + insights: [slowExecutionInsight(InsightExecEnum.TRANSACTION)], + }); + const txnContentionDetails = { + transactionExecutionID: txnInsightEventMock.transactionExecutionID, + queries: txnInsightEventMock.queries, + insights: [ + highContentionInsight(InsightExecEnum.TRANSACTION), + slowExecutionInsight(InsightExecEnum.TRANSACTION), + ], + startTime: moment(), + totalContentionTimeMs: 500, + contentionThreshold: 100, + application: txnInsightEventMock.application, + transactionFingerprintID: txnInsightEventMock.transactionFingerprintID, + blockingContentionDetails: [blockedContentionMock], + execType: InsightExecEnum.TRANSACTION, + insightName: "HighContention", + }; + + const testMergedAgainstContentionFields = (merged: TxnInsightDetails) => { + expect(merged.startTime.unix()).toEqual( + txnContentionDetails.startTime.unix(), + ); + expect(merged.contentionThreshold).toEqual( + txnContentionDetails.contentionThreshold, + ); + expect(merged.blockingContentionDetails).toEqual( + txnContentionDetails.blockingContentionDetails, + ); + expect(merged.totalContentionTimeMs).toEqual( + txnContentionDetails.totalContentionTimeMs, + ); + }; + + const testMergedAgainstTxnFromInsights = (merged: TxnInsightDetails) => { + expect(merged.databaseName).toEqual(txnInsightFromStmts.databaseName); + expect(merged.retries).toEqual(txnInsightFromStmts.retries); + expect(merged.implicitTxn).toEqual(txnInsightFromStmts.implicitTxn); + expect(merged.priority).toEqual(txnInsightFromStmts.priority); + expect(merged.username).toEqual(txnInsightFromStmts.username); + expect(merged.sessionID).toEqual(txnInsightFromStmts.sessionID); + }; + + it("should merge objects when both are present", () => { + const merged = mergeTxnInsightDetails( + txnInsightFromStmts, + txnContentionDetails, + ); + testMergedAgainstContentionFields(merged); + testMergedAgainstTxnFromInsights(merged); + // Insights should be de-duped + const insightNamesUniqe = new Set( + txnContentionDetails.insights + .map(i => i.name) + .concat(txnInsightFromStmts.insights.map(i => i.name)), + ); + expect(merged.insights.length).toEqual(insightNamesUniqe.size); + }); + + it("should return details when contention details aren't present", () => { + const merged = mergeTxnInsightDetails(txnInsightFromStmts, null); + testMergedAgainstTxnFromInsights(merged); + expect(merged.insights.length).toBe(txnInsightFromStmts.insights.length); + }); + + it("should return details when txn insights from stmts aren't present", () => { + const merged = mergeTxnInsightDetails(null, txnContentionDetails); + testMergedAgainstContentionFields(merged); + expect(merged.insights.length).toBe(txnContentionDetails.insights.length); + }); + }); + + describe("dedupInsights", () => { + const e = InsightExecEnum.STATEMENT; + const insights = [ + highContentionInsight(e), + highRetryCountInsight(e), + highRetryCountInsight(e), + slowExecutionInsight(e), + slowExecutionInsight(e), + highRetryCountInsight(e), + ]; + const expected = [ + highContentionInsight(e), + highRetryCountInsight(e), + slowExecutionInsight(e), + ]; + + const deduped = dedupInsights(insights); + expect(deduped.length).toEqual(expected.length); + deduped.forEach((insight, i) => { + expect(insight.name).toEqual(expected[i].name); + }); + }); +}); diff --git a/pkg/ui/workspaces/cluster-ui/src/insights/utils.ts b/pkg/ui/workspaces/cluster-ui/src/insights/utils.ts index 61cf42f68aa4..66ec795adb2e 100644 --- a/pkg/ui/workspaces/cluster-ui/src/insights/utils.ts +++ b/pkg/ui/workspaces/cluster-ui/src/insights/utils.ts @@ -275,11 +275,11 @@ export function getInsightsFromProblemsAndCauses( /** * flattenTxnInsightsToStmts flattens the txn insights array - * into its stmt insights. Only stmts with non-empty insights - * array will be included. + * into its stmt insights, including the txn level ifnormation. + * Only stmts with non-empty insights array will be included. * @param txnInsights array of transaction insights * @returns An array of FlattenedStmtInsightEvent where each elem - * includes stmt and txn info. Only includes stmts with non-empty + * includes stmt and txn info. All elements have a non-empty * insights array. */ export function flattenTxnInsightsToStmts( @@ -343,7 +343,9 @@ export function mergeTxnContentionAndStmtInsights( ...txn, contention: existingContentionEvent.contention, startTime: existingContentionEvent.startTime, - insights: txn.insights.concat(existingContentionEvent.insights), + insights: dedupInsights( + txn.insights.concat(existingContentionEvent.insights), + ), }; return; // Continue } @@ -371,9 +373,15 @@ export function mergeTxnInsightDetails( application: txnContentionDetails.application ?? txnDetailsFromStmts?.application, lastRetryReason: txnDetailsFromStmts?.lastRetryReason, + sessionID: txnDetailsFromStmts?.sessionID, + retries: txnDetailsFromStmts?.retries, + databaseName: txnDetailsFromStmts?.databaseName, + implicitTxn: txnDetailsFromStmts?.implicitTxn, + username: txnDetailsFromStmts?.username, + priority: txnDetailsFromStmts?.priority, statementInsights: txnDetailsFromStmts?.statementInsights, - insights: txnContentionDetails.insights.concat( - txnDetailsFromStmts?.insights ?? [], + insights: dedupInsights( + txnContentionDetails.insights.concat(txnDetailsFromStmts?.insights ?? []), ), queries: txnContentionDetails.queries, startTime: txnContentionDetails.startTime, @@ -490,3 +498,14 @@ export function getTxnInsightRecommendations( return recs; } + +export function dedupInsights(insights: Insight[]): Insight[] { + // De-duplicate top-level txn insights. + const insightsSeen = new Set(); + return insights.reduce((deduped, i) => { + if (insightsSeen.has(i.name)) return deduped; + insightsSeen.add(i.name); + deduped.push(i); + return deduped; + }, []); +} diff --git a/pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/transactionInsightDetailsOverviewTab.tsx b/pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/transactionInsightDetailsOverviewTab.tsx index 32b86a2f3d49..c2dd74410c73 100644 --- a/pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/transactionInsightDetailsOverviewTab.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/transactionInsightDetailsOverviewTab.tsx @@ -59,7 +59,11 @@ export const TransactionInsightDetailsOverviewTab: React.FC = ({ const insightQueries = insightDetails?.queries?.join("") || "Insight not found."; - const insightsColumns = makeInsightsColumns(isCockroachCloud); + const insightsColumns = makeInsightsColumns( + isCockroachCloud, + insightDetails.queries?.length > 1, + true, + ); const blockingExecutions: ContentionEvent[] = insightDetails?.blockingContentionDetails?.map(x => { @@ -89,10 +93,11 @@ export const TransactionInsightDetailsOverviewTab: React.FC = ({ const insightRecs = getTxnInsightRecommendations(insightDetails); const rowsRead = - stmtInsights?.reduce((count, stmt) => (count += stmt.rowsRead), 0) ?? "N/A"; + stmtInsights?.reduce((count, stmt) => (count += stmt.rowsRead), 0) ?? + "no samples"; const rowsWritten = stmtInsights?.reduce((count, stmt) => (count += stmt.rowsWritten), 0) ?? - "N/A"; + "no samples"; return (
@@ -111,21 +116,21 @@ export const TransactionInsightDetailsOverviewTab: React.FC = ({ label="Start Time" value={ insightDetails.startTime?.format(DATE_FORMAT_24_UTC) ?? - "N/A" + "no samples" } /> stmt.isFullScan) - ?.toString() ?? "N/A" + ?.toString() ?? "no samples" } /> @@ -134,15 +139,17 @@ export const TransactionInsightDetailsOverviewTab: React.FC = ({ - + {insightDetails.lastRetryReason && ( + + )} insightColumnLabels.query, + query: () => {insightColumnLabels.query}, insights: () => { return ( {value}
; } +const StatementExecution = ({ + rec, + disableLink, +}: { + rec: InsightRecommendation; + disableLink: boolean; +}) => ( +
+ Statement: {" "} + {disableLink ? ( +
+ + {computeOrUseStmtSummary( + rec.execution?.statement, + rec.execution?.summary, + )} + +
+ ) : ( + + )} +
+); + function descriptionCell( insightRec: InsightRecommendation, - disableStmtLink?: boolean, + showQuery: boolean, + disableStmtLink: boolean, ): React.ReactElement { + const stmtLink = + showQuery || isIndexRec(insightRec) ? ( + + ) : null; + const clusterSettingsLink = ( <> {"This threshold can be configured in "} @@ -90,38 +125,13 @@ function descriptionCell( {"."} ); - const summary = computeOrUseStmtSummary( - insightRec.execution?.statement, - insightRec.execution?.summary, - ); switch (insightRec.type) { case "CreateIndex": case "ReplaceIndex": case "AlterIndex": return ( <> -
- Statement Fingerprint: {" "} - {disableStmtLink && ( -
- - {summary} - -
- )} - {!disableStmtLink && ( - - )} -
+ {stmtLink}
Recommendation: {" "} {insightRec.query} @@ -162,6 +172,7 @@ function descriptionCell( Time Spent Waiting: {" "} {Duration(insightRec.details.duration * 1e6)}
+ {stmtLink}
Description: {" "} {insightRec.details.description} {clusterSettingsLink} @@ -175,6 +186,7 @@ function descriptionCell( Retries: {" "} {insightRec.execution.retries}
+ {stmtLink}
Description: {" "} {insightRec.details.description} {clusterSettingsLink} @@ -188,6 +200,7 @@ function descriptionCell( case "SuboptimalPlan": return ( <> + {stmtLink}
Description: {" "} {insightRec.details.description} @@ -203,6 +216,7 @@ function descriptionCell( case "PlanRegression": return ( <> + {stmtLink}
Description: {" "} {insightRec.details.description} @@ -212,6 +226,7 @@ function descriptionCell( case "FailedExecution": return ( <> + {stmtLink}
This execution has failed.
@@ -224,6 +239,7 @@ function descriptionCell( Elapsed Time: {Duration(insightRec.details.duration * 1e6)}
+ {stmtLink}
Description: {" "} {insightRec.details.description} {clusterSettingsLink} @@ -284,8 +300,20 @@ function actionCell( return <>; } +const isIndexRec = (rec: InsightRecommendation) => { + switch (rec.type) { + case "AlterIndex": + case "CreateIndex": + case "DropIndex": + return true; + default: + return false; + } +}; + export function makeInsightsColumns( hideAction: boolean, + showQuery?: boolean, disableStmtLink?: boolean, ): ColumnDescriptor[] { return [ @@ -299,16 +327,9 @@ export function makeInsightsColumns( name: "details", title: insightsTableTitles.details(), cell: (item: InsightRecommendation) => - descriptionCell(item, disableStmtLink), + descriptionCell(item, showQuery, disableStmtLink), sort: (item: InsightRecommendation) => item.type, }, - { - name: "query", - title: insightsTableTitles.query(), - cell: (item: InsightRecommendation) => - limitText(item.execution?.statement, 100) ?? "N/A", - sort: (item: InsightRecommendation) => item.execution?.statement, - }, { name: "action", title: insightsTableTitles.actions(), diff --git a/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/planDetails.tsx b/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/planDetails.tsx index e9c84c402514..52dd042bf3c4 100644 --- a/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/planDetails.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/planDetails.tsx @@ -217,7 +217,7 @@ export function Insights({ onChangeSortSetting, }: InsightsProps): React.ReactElement { const hideAction = useContext(CockroachCloudContext) && database?.length == 0; - const insightsColumns = makeInsightsColumns(hideAction, true); + const insightsColumns = makeInsightsColumns(hideAction, false, true); const data = formatIdxRecommendations( idxRecommendations, database, diff --git a/pkg/ui/workspaces/cluster-ui/src/util/format.ts b/pkg/ui/workspaces/cluster-ui/src/util/format.ts index 11a4ed2a884e..55a227491c4c 100644 --- a/pkg/ui/workspaces/cluster-ui/src/util/format.ts +++ b/pkg/ui/workspaces/cluster-ui/src/util/format.ts @@ -225,7 +225,17 @@ export const limitStringArray = (arr: string[], limit: number): string => { return limitText(arr[0], limit); } - return arr[0].concat("..."); + let str = arr[0]; + for (let next = 1; next < arr.length; ++next) { + const charsLeft = limit - str.length; + if (charsLeft < arr[next].length) { + str += arr[next].substring(0, charsLeft).concat("..."); + break; + } + str += arr[next]; + } + + return str; }; function add(a: string, b: string): string {