From 6deea40dece097d1288f67d0bbef1e9d08a3daac Mon Sep 17 00:00:00 2001 From: Xin Hao Zhang Date: Mon, 19 Sep 2022 17:26:53 -0400 Subject: [PATCH] ui/cluster-ui: fix no most recent stmt for active txns Fixes #87738 Previously, active txns could have an empty 'Most Recent Statement' column, even if their executed statement count was non-zero. This was due to the most recent query text being populated by the active stmt, which could be empty at the time of querying. This commit populates the last statement text for a txn even when it is not currently executing a query. This commit also removes the `isFullScan` field from active txn pages, as we cannot fill this field out without all stmts in the txn. Release note (ui change): Full scan field is removed from active txn details page. Release note (bug fix): active txns with non-zero executed statement count now always have populated stmt text, even when no stmt is being executed. --- .../activeStatementUtils.spec.ts | 304 ++++++++++-------- .../activeExecutions/activeStatementUtils.ts | 32 +- .../activeTransactionsTable.tsx | 21 +- .../cluster-ui/src/activeExecutions/types.ts | 6 +- .../activeTransactionDetails.tsx | 5 - 5 files changed, 202 insertions(+), 166 deletions(-) diff --git a/pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.spec.ts b/pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.spec.ts index e9009f039488..5398091083db 100644 --- a/pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.spec.ts +++ b/pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.spec.ts @@ -85,7 +85,6 @@ function makeActiveTxn( statementID: defaultActiveStatement.statementID, retries: 3, lastAutoRetryReason: null, - isFullScan: defaultActiveStatement.isFullScan, priority: "Normal", statementCount: 5, status: "Executing", @@ -184,67 +183,186 @@ describe("test activeStatementUtils", () => { }); describe("getActiveExecutionsFromSessions", () => { - const activeQueries = [1, 2, 3, 4].map(num => - makeActiveQuery({ id: num.toString() }), - ); + it("should convert sessions response to active statements result", () => { + const sessionsResponse: SessionsResponse = { + sessions: [ + { + id: new Uint8Array(), + username: "bar", + application_name: "application", + client_address: "clientAddress", + active_queries: [makeActiveQuery({ id: "1" })], + }, + { + id: new Uint8Array(), + username: "foo", + application_name: "application2", + client_address: "clientAddress2", + active_queries: [makeActiveQuery({ id: "2" })], + }, + { + id: new Uint8Array(), + username: "closed", + status: SessionStatusType.CLOSED, + // Closed sessions should not appear in active stmts. + application_name: "application2", + client_address: "clientAddress2", + active_queries: [makeActiveQuery({ id: "3" })], + }, + ], + errors: [], + internal_app_name_prefix: INTERNAL_APP_NAME_PREFIX, + toJSON: () => ({}), + }; + + const statements = getActiveExecutionsFromSessions( + sessionsResponse, + LAST_UPDATED, + ).statements; + + expect(statements.length).toBe(2); + + statements.forEach(stmt => { + if (stmt.user === "bar") { + expect(stmt.application).toBe("application"); + expect(stmt.clientAddress).toBe("clientAddress"); + } else if (stmt.user === "foo") { + expect(stmt.application).toBe("application2"); + expect(stmt.clientAddress).toBe("clientAddress2"); + } else { + fail(`stmt user should be foo or bar, got ${stmt.user}`); + } + // expect(stmt.transactionID).toBe(defaultActiveStatement.transactionID); + expect(stmt.status).toBe("Executing"); + expect(stmt.elapsedTimeMillis).toBe( + LAST_UPDATED.diff(MOCK_START_TIME, "ms"), + ); + expect(stmt.start.unix()).toBe( + TimestampToMoment(defaultActiveQuery.start).unix(), + ); + expect(stmt.query).toBe(defaultActiveStatement.query); + }); + }); - const sessionsResponse: SessionsResponse = { - sessions: [ - { - id: new Uint8Array(), - username: "bar", - application_name: "application", - client_address: "clientAddress", - active_queries: activeQueries, - }, + it("should convert sessions response to active transactions result", () => { + const txns = [ { id: new Uint8Array(), - username: "foo", - application_name: "application2", - client_address: "clientAddress2", - active_queries: activeQueries, + start: new Timestamp({ + seconds: Long.fromNumber(MOCK_START_TIME.unix()), + }), + num_auto_retries: 3, + num_statements_executed: 4, }, { id: new Uint8Array(), - username: "foo", - status: SessionStatusType.CLOSED, - application_name: "application2", - client_address: "clientAddress2", - active_queries: activeQueries, + start: new Timestamp({ + seconds: Long.fromNumber(MOCK_START_TIME.unix()), + }), + num_auto_retries: 4, + num_statements_executed: 3, }, - ], - errors: [], - internal_app_name_prefix: INTERNAL_APP_NAME_PREFIX, - toJSON: () => ({}), - }; - - const statements = getActiveExecutionsFromSessions( - sessionsResponse, - LAST_UPDATED, - ).statements; - - expect(statements.length).toBe(activeQueries.length * 2); - - statements.forEach(stmt => { - if (stmt.user === "bar") { - expect(stmt.application).toBe("application"); - expect(stmt.clientAddress).toBe("clientAddress"); - } else if (stmt.user === "foo") { - expect(stmt.application).toBe("application2"); - expect(stmt.clientAddress).toBe("clientAddress2"); - } else { - fail(`stmt user should be foo or bar, got ${stmt.user}`); - } - // expect(stmt.transactionID).toBe(defaultActiveStatement.transactionID); - expect(stmt.status).toBe("Executing"); - expect(stmt.elapsedTimeMillis).toBe( - LAST_UPDATED.diff(MOCK_START_TIME, "ms"), - ); - expect(stmt.start.unix()).toBe( - TimestampToMoment(defaultActiveQuery.start).unix(), + ]; + + const sessionsResponse: SessionsResponse = { + sessions: [ + { + id: new Uint8Array(), + username: "bar", + application_name: "application", + client_address: "clientAddress", + active_queries: [makeActiveQuery()], + active_txn: txns[0], + }, + { + id: new Uint8Array(), + username: "foo", + application_name: "application2", + client_address: "clientAddress2", + active_queries: [makeActiveQuery()], + active_txn: txns[1], + }, + { + id: new Uint8Array(), + username: "foo", + status: SessionStatusType.CLOSED, + application_name: "closed_application", + client_address: "clientAddress2", + active_queries: [makeActiveQuery()], + active_txn: txns[1], + }, + ], + errors: [], + internal_app_name_prefix: INTERNAL_APP_NAME_PREFIX, + toJSON: () => ({}), + }; + + const activeTransactions = getActiveExecutionsFromSessions( + sessionsResponse, + LAST_UPDATED, + ).transactions; + + // Should filter out the txn from closed session. + expect(activeTransactions.length).toBe(2); + + activeTransactions.forEach((txn: ActiveTransaction, i) => { + expect(txn.application).toBe( + sessionsResponse.sessions[i].application_name, + ); + expect(txn.elapsedTimeMillis).toBe( + LAST_UPDATED.diff(MOCK_START_TIME, "ms"), + ); + expect(txn.status).toBe("Executing"); + expect(txn.query).toBeTruthy(); + expect(txn.start.unix()).toBe( + TimestampToMoment(defaultActiveQuery.start).unix(), + ); + }); + }); + + it("should populate txn latest query when there is no active stmt for txns with at least 1 stmt", () => { + const lastActiveQueryText = "SELECT 1"; + const sessionsResponse: SessionsResponse = { + sessions: [ + { + id: new Uint8Array(), + last_active_query: lastActiveQueryText, + active_queries: [], + active_txn: { + id: new Uint8Array(), + start: new Timestamp({ + seconds: Long.fromNumber(MOCK_START_TIME.unix()), + }), + num_auto_retries: 0, + num_statements_executed: 1, + }, + }, + { + id: new Uint8Array(), + last_active_query: lastActiveQueryText, + active_queries: [], + active_txn: { + id: new Uint8Array(), + start: new Timestamp({ + seconds: Long.fromNumber(MOCK_START_TIME.unix()), + }), + num_auto_retries: 0, + num_statements_executed: 0, + }, + }, + ], + errors: [], + internal_app_name_prefix: INTERNAL_APP_NAME_PREFIX, + toJSON: () => ({}), + }; + + const activeExecs = getActiveExecutionsFromSessions( + sessionsResponse, + LAST_UPDATED, ); - // expect(stmt.sessionID).toBe(defaultActiveStatement.sessionID); - expect(stmt.query).toBe(defaultActiveStatement.query); + + expect(activeExecs.transactions[0].query).toBe(lastActiveQueryText); + expect(activeExecs.transactions[1].query).toBeFalsy(); }); }); @@ -262,84 +380,6 @@ describe("test activeStatementUtils", () => { expect(apps).toEqual(["app1", "app2", "app3", "app4"]); }); - describe("getActiveExecutionsFromSessions transactions result", () => { - const txns = [ - { - id: new Uint8Array(), - start: new Timestamp({ - seconds: Long.fromNumber(MOCK_START_TIME.unix()), - }), - num_auto_retries: 3, - num_statements_executed: 4, - }, - { - id: new Uint8Array(), - start: new Timestamp({ - seconds: Long.fromNumber(MOCK_START_TIME.unix()), - }), - num_auto_retries: 4, - num_statements_executed: 3, - }, - ]; - - const sessionsResponse: SessionsResponse = { - sessions: [ - { - id: new Uint8Array(), - username: "bar", - application_name: "application", - client_address: "clientAddress", - active_queries: [makeActiveQuery()], - active_txn: txns[0], - }, - { - id: new Uint8Array(), - username: "foo", - application_name: "application2", - client_address: "clientAddress2", - active_queries: [makeActiveQuery()], - active_txn: txns[1], - }, - { - id: new Uint8Array(), - username: "foo", - status: SessionStatusType.CLOSED, - application_name: "closed_application", - client_address: "clientAddress2", - active_queries: [makeActiveQuery()], - active_txn: txns[1], - }, - ], - errors: [], - internal_app_name_prefix: INTERNAL_APP_NAME_PREFIX, - toJSON: () => ({}), - }; - - const activeTransactions = getActiveExecutionsFromSessions( - sessionsResponse, - LAST_UPDATED, - ).transactions; - - // Should filter out the txn from closed session. - expect(activeTransactions.length).toBe(2); - - expect(activeTransactions.length).toBe(txns.length); - - activeTransactions.forEach((txn: ActiveTransaction, i) => { - expect(txn.application).toBe( - sessionsResponse.sessions[i].application_name, - ); - expect(txn.elapsedTimeMillis).toBe( - LAST_UPDATED.diff(MOCK_START_TIME, "ms"), - ); - expect(txn.status).toBe("Executing"); - expect(txn.query).toBeTruthy(); - expect(txn.start.unix()).toBe( - TimestampToMoment(defaultActiveQuery.start).unix(), - ); - }); - }); - describe("filterActiveTransactions", () => { it("should filter out txns that do not match filters", () => { const txns: ActiveTransaction[] = [ diff --git a/pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts b/pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts index ef8f276d80bb..fe00e78fb538 100644 --- a/pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts +++ b/pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeStatementUtils.ts @@ -89,7 +89,6 @@ export function getActiveExecutionsFromSessions( return { statements: [], transactions: [] }; const time = lastUpdated || moment.utc(); - const activeStmtByTxnID: Record = {}; const statements: ActiveStatement[] = []; const transactions: ActiveTransaction[] = []; @@ -102,9 +101,12 @@ export function getActiveExecutionsFromSessions( .forEach(session => { const sessionID = byteArrayToUuid(session.id); - session.active_queries.forEach(query => { + let activeStmt: ActiveStatement = null; + if (session.active_queries.length) { + // There will only ever be one query in this array. + const query = session.active_queries[0]; const queryTxnID = byteArrayToUuid(query.txn_id); - const stmt: ActiveStatement = { + activeStmt = { statementID: query.id, transactionID: queryTxnID, sessionID, @@ -122,9 +124,8 @@ export function getActiveExecutionsFromSessions( isFullScan: query.is_full_scan || false, // Or here is for conversion in case the field is null. }; - statements.push(stmt); - activeStmtByTxnID[queryTxnID] = stmt; - }); + statements.push(activeStmt); + } const activeTxn = session.active_txn; if (!activeTxn) return; @@ -132,30 +133,23 @@ export function getActiveExecutionsFromSessions( transactions.push({ transactionID: byteArrayToUuid(activeTxn.id), sessionID, - query: null, - statementID: null, + query: + activeStmt?.query ?? + (activeTxn.num_statements_executed + ? session.last_active_query + : null), + statementID: activeStmt?.statementID, status: "Executing" as ExecutionStatus, start: TimestampToMoment(activeTxn.start), elapsedTimeMillis: time.diff(TimestampToMoment(activeTxn.start), "ms"), application: session.application_name, retries: activeTxn.num_auto_retries, statementCount: activeTxn.num_statements_executed, - isFullScan: session.active_queries.some(query => query.is_full_scan), lastAutoRetryReason: activeTxn.last_auto_retry_reason, priority: activeTxn.priority, }); }); - // Find most recent statement for each txn. - transactions.map(txn => { - const mostRecentStmt = activeStmtByTxnID[txn.transactionID]; - if (!mostRecentStmt) return txn; - txn.query = mostRecentStmt.query; - txn.statementID = mostRecentStmt.statementID; - txn.isFullScan = mostRecentStmt.isFullScan; - return txn; - }); - return { transactions, statements, diff --git a/pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeTransactionsTable/activeTransactionsTable.tsx b/pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeTransactionsTable/activeTransactionsTable.tsx index 419a86deaee2..a55590f6556c 100644 --- a/pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeTransactionsTable/activeTransactionsTable.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/activeExecutions/activeTransactionsTable/activeTransactionsTable.tsx @@ -31,13 +31,20 @@ export function makeActiveTransactionsColumns( { name: "mostRecentStatement", title: executionsTableTitles.mostRecentStatement(execType), - cell: (item: ActiveTransaction) => ( - - - {limitText(item.query || "", 70)} - - - ), + cell: (item: ActiveTransaction) => { + const queryText = limitText(item.query || "", 70); + return ( + + {item.statementID ? ( + + {queryText} + + ) : ( + queryText + )} + + ); + }, sort: (item: ActiveTransaction) => item.query, }, activeTransactionColumnsFromCommon.status, diff --git a/pkg/ui/workspaces/cluster-ui/src/activeExecutions/types.ts b/pkg/ui/workspaces/cluster-ui/src/activeExecutions/types.ts index a575d6548081..d5045a677346 100644 --- a/pkg/ui/workspaces/cluster-ui/src/activeExecutions/types.ts +++ b/pkg/ui/workspaces/cluster-ui/src/activeExecutions/types.ts @@ -25,22 +25,22 @@ export const SessionStatusType = protos.cockroach.server.serverpb.Session.Status; export interface ActiveExecution { - statementID?: string; // This may not be present for a transaction. + statementID?: string; // Empty for transactions not currently executing a statement. transactionID: string; sessionID: string; status: ExecutionStatus; start: Moment; elapsedTimeMillis: number; application: string; - query?: string; // Possibly empty for a transaction. + query?: string; // For transactions, this is the latest query executed. timeSpentWaiting?: moment.Duration; - isFullScan: boolean; } export type ActiveStatement = ActiveExecution & Required> & { user: string; clientAddress: string; + isFullScan: boolean; }; export type ActiveTransaction = ActiveExecution & { diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionDetails/activeTransactionDetails.tsx b/pkg/ui/workspaces/cluster-ui/src/transactionDetails/activeTransactionDetails.tsx index acb445a5805b..20b4e196eb0e 100644 --- a/pkg/ui/workspaces/cluster-ui/src/transactionDetails/activeTransactionDetails.tsx +++ b/pkg/ui/workspaces/cluster-ui/src/transactionDetails/activeTransactionDetails.tsx @@ -60,7 +60,6 @@ export const ActiveTxnInsightsLabels = { LAST_STATEMENT_EXEC_ID: "Most Recent Statement Execution ID", SESSION_ID: "Session ID", PRIORITY: "Priority", - FULL_SCAN: "Full Scan", }; export const RECENT_STATEMENT_NOT_FOUND_MESSAGE = @@ -143,10 +142,6 @@ export const ActiveTransactionDetails: React.FC< label={ActiveTxnInsightsLabels.PRIORITY} value={capitalize(transaction.priority)} /> -