Merge pull request #101235 from maryliag/backport23.1.0-99719-101226

release-23.1.0: updates on aggregation for SQL Activity

docs/generated/http/full.md

@@ -4116,6 +4116,7 @@ tenant pods.
 | key | [StatementsResponse.ExtendedStatementStatisticsKey](#cockroach.server.serverpb.StatementsResponse-cockroach.server.serverpb.StatementsResponse.ExtendedStatementStatisticsKey) |  |  | [reserved](#support-status) |
 | id | [uint64](#cockroach.server.serverpb.StatementsResponse-uint64) |  |  | [reserved](#support-status) |
 | stats | [cockroach.sql.StatementStatistics](#cockroach.server.serverpb.StatementsResponse-cockroach.sql.StatementStatistics) |  |  | [reserved](#support-status) |
+| txn_fingerprint_ids | [uint64](#cockroach.server.serverpb.StatementsResponse-uint64) | repeated | In 23.1 we expect the response to only group on fingerprint_id and app_name in the overview page. We now return the aggregated list of unique txn fingerprint ids, leaving the txn_fingerprint_id field in the key empty. | [reserved](#support-status) |
 
 
 
@@ -4227,6 +4228,7 @@ Support status: [reserved](#support-status)
 | key | [StatementsResponse.ExtendedStatementStatisticsKey](#cockroach.server.serverpb.StatementsResponse-cockroach.server.serverpb.StatementsResponse.ExtendedStatementStatisticsKey) |  |  | [reserved](#support-status) |
 | id | [uint64](#cockroach.server.serverpb.StatementsResponse-uint64) |  |  | [reserved](#support-status) |
 | stats | [cockroach.sql.StatementStatistics](#cockroach.server.serverpb.StatementsResponse-cockroach.sql.StatementStatistics) |  |  | [reserved](#support-status) |
+| txn_fingerprint_ids | [uint64](#cockroach.server.serverpb.StatementsResponse-uint64) | repeated | In 23.1 we expect the response to only group on fingerprint_id and app_name in the overview page. We now return the aggregated list of unique txn fingerprint ids, leaving the txn_fingerprint_id field in the key empty. | [reserved](#support-status) |
 
 
 

pkg/server/combined_statement_stats.go

@@ -100,14 +100,13 @@ func getCombinedStatementStats(
 	}
 
 	if req.FetchMode != nil && req.FetchMode.StatsType == serverpb.CombinedStatementsStatsRequest_TxnStatsOnly {
-		// Change the whereClause for the statements to those matching the txn_fingerprint_ids in the
-		// transactions response that are within the desired interval. We also don't need the order and
-		// limit anymore.
-		orderAndLimit = ""
-		whereClause, args = buildWhereClauseForStmtsByTxn(req, transactions, testingKnobs)
+		// If we're fetching for txns, the client still expects statement stats for
+		// stmts in the txns response.
+		statements, err = collectStmtsForTxns(ctx, ie, req, transactions, testingKnobs, tableSuffix)
+	} else {
+		statements, err = collectCombinedStatements(ctx, ie, whereClause, args, orderAndLimit, testingKnobs, tableSuffix)
 	}
 
-	statements, err = collectCombinedStatements(ctx, ie, whereClause, args, orderAndLimit, testingKnobs, tableSuffix)
 	if err != nil {
 		return nil, serverError(ctx, err)
 	}
@@ -407,17 +406,15 @@ func collectCombinedStatements(
 SELECT * FROM (
 SELECT
     fingerprint_id,
-    transaction_fingerprint_id,
+    array_agg(distinct transaction_fingerprint_id),
     app_name,
     max(aggregated_ts) as aggregated_ts,
-    metadata,
+    crdb_internal.merge_stats_metadata(array_agg(metadata)) as metadata,
     crdb_internal.merge_statement_stats(array_agg(statistics)) AS statistics
 FROM %s%s %s
 GROUP BY
     fingerprint_id,
-    transaction_fingerprint_id,
-    app_name,
-    metadata
+    app_name
 ) %s
 %s`
 
@@ -475,12 +472,18 @@ GROUP BY
 			return nil, serverError(ctx, err)
 		}
 
-		var transactionFingerprintID uint64
-		if transactionFingerprintID, err = sqlstatsutil.DatumToUint64(row[1]); err != nil {
-			return nil, serverError(ctx, err)
+		var txnFingerprintID uint64
+		txnFingerprintDatums := tree.MustBeDArray(row[1])
+		txnFingerprintIDs := make([]appstatspb.TransactionFingerprintID, 0, txnFingerprintDatums.Array.Len())
+		for _, idDatum := range txnFingerprintDatums.Array {
+			if txnFingerprintID, err = sqlstatsutil.DatumToUint64(idDatum); err != nil {
+				return nil, serverError(ctx, err)
+			}
+			txnFingerprintIDs = append(txnFingerprintIDs, appstatspb.TransactionFingerprintID(txnFingerprintID))
 		}
 
 		app := string(tree.MustBeDString(row[2]))
+
 		aggregatedTs := tree.MustBeDTimestampTZ(row[3]).Time
 
 		var metadata appstatspb.CollectedStatementStatistics
@@ -490,8 +493,6 @@ GROUP BY
 		}
 
 		metadata.Key.App = app
-		metadata.Key.TransactionFingerprintID =
-			appstatspb.TransactionFingerprintID(transactionFingerprintID)
 
 		statsJSON := tree.MustBeDJSON(row[5]).JSON
 		if err = sqlstatsutil.DecodeStmtStatsStatisticsJSON(statsJSON, &metadata.Stats); err != nil {
@@ -503,8 +504,9 @@ GROUP BY
 				KeyData:      metadata.Key,
 				AggregatedTs: aggregatedTs,
 			},
-			ID:    appstatspb.StmtFingerprintID(statementFingerprintID),
-			Stats: metadata.Stats,
+			ID:                appstatspb.StmtFingerprintID(statementFingerprintID),
+			Stats:             metadata.Stats,
+			TxnFingerprintIDs: txnFingerprintIDs,
 		}
 
 		statements = append(statements, stmt)
@@ -535,13 +537,12 @@ SELECT
     app_name,
     max(aggregated_ts) as aggregated_ts,
     fingerprint_id,
-    metadata,
+    max(metadata),
     crdb_internal.merge_transaction_stats(array_agg(statistics)) AS statistics
 FROM %s%s %s
 GROUP BY
     app_name,
-    fingerprint_id,
-    metadata
+    fingerprint_id
 ) %s
 %s`
 
@@ -596,6 +597,7 @@ GROUP BY
 		}
 
 		app := string(tree.MustBeDString(row[0]))
+
 		aggregatedTs := tree.MustBeDTimestampTZ(row[1]).Time
 		fingerprintID, err := sqlstatsutil.DatumToUint64(row[2])
 		if err != nil {
@@ -633,6 +635,121 @@ GROUP BY
 	return transactions, nil
 }
 
+func collectStmtsForTxns(
+	ctx context.Context,
+	ie *sql.InternalExecutor,
+	req *serverpb.CombinedStatementsStatsRequest,
+	transactions []serverpb.StatementsResponse_ExtendedCollectedTransactionStatistics,
+	testingKnobs *sqlstats.TestingKnobs,
+	tableSuffix string,
+) ([]serverpb.StatementsResponse_CollectedStatementStatistics, error) {
+
+	whereClause, args := buildWhereClauseForStmtsByTxn(req, transactions, testingKnobs)
+
+	queryFormat := `
+SELECT
+    fingerprint_id,
+    transaction_fingerprint_id,
+    crdb_internal.merge_stats_metadata(array_agg(metadata)) AS metadata,
+    crdb_internal.merge_statement_stats(array_agg(statistics)) AS statistics,
+    app_name
+FROM %s%s %s
+GROUP BY
+    fingerprint_id,
+    transaction_fingerprint_id,
+    app_name
+`
+
+	const expectedNumDatums = 5
+
+	query := fmt.Sprintf(queryFormat, "crdb_internal.statement_statistics_persisted", tableSuffix, whereClause)
+
+	it, err := ie.QueryIteratorEx(ctx, "stmts-for-txn", nil,
+		sessiondata.NodeUserSessionDataOverride, query, args...)
+
+	if err != nil {
+		return nil, serverError(ctx, err)
+	}
+
+	// If there are no results from the persisted table, retrieve the data from the combined view
+	// with data in-memory.
+	if !it.HasResults() {
+		err = closeIterator(it, err)
+
+		query = fmt.Sprintf(queryFormat, `crdb_internal.statement_statistics`, "", whereClause)
+
+		it, err = ie.QueryIteratorEx(ctx, "stmts-for-txn-with-memory", nil,
+			sessiondata.NodeUserSessionDataOverride, query, args...)
+
+		if err != nil {
+			return nil, serverError(ctx, err)
+		}
+	}
+
+	defer func() {
+		closeErr := it.Close()
+		if closeErr != nil {
+			err = errors.CombineErrors(err, closeErr)
+		}
+	}()
+
+	var statements []serverpb.StatementsResponse_CollectedStatementStatistics
+	var ok bool
+	for ok, err = it.Next(ctx); ok; ok, err = it.Next(ctx) {
+		var row tree.Datums
+		if row = it.Cur(); row == nil {
+			return nil, errors.New("unexpected null row on collectStmtsForTxns")
+		}
+
+		if row.Len() != expectedNumDatums {
+			return nil, errors.Newf("expected %d columns, received %d on collectStmtsForTxns", expectedNumDatums)
+		}
+
+		var statementFingerprintID uint64
+		if statementFingerprintID, err = sqlstatsutil.DatumToUint64(row[0]); err != nil {
+			return nil, serverError(ctx, err)
+		}
+
+		var txnFingerprintID uint64
+		if txnFingerprintID, err = sqlstatsutil.DatumToUint64(row[1]); err != nil {
+			return nil, serverError(ctx, err)
+		}
+
+		var metadata appstatspb.CollectedStatementStatistics
+		metadataJSON := tree.MustBeDJSON(row[2]).JSON
+		if err = sqlstatsutil.DecodeStmtStatsMetadataJSON(metadataJSON, &metadata); err != nil {
+			return nil, serverError(ctx, err)
+		}
+
+		metadata.Key.TransactionFingerprintID = appstatspb.TransactionFingerprintID(txnFingerprintID)
+
+		statsJSON := tree.MustBeDJSON(row[3]).JSON
+		if err = sqlstatsutil.DecodeStmtStatsStatisticsJSON(statsJSON, &metadata.Stats); err != nil {
+			return nil, serverError(ctx, err)
+		}
+
+		app := string(tree.MustBeDString(row[4]))
+		metadata.Key.App = app
+
+		stmt := serverpb.StatementsResponse_CollectedStatementStatistics{
+			Key: serverpb.StatementsResponse_ExtendedStatementStatisticsKey{
+				KeyData: metadata.Key,
+			},
+			ID:    appstatspb.StmtFingerprintID(statementFingerprintID),
+			Stats: metadata.Stats,
+		}
+
+		statements = append(statements, stmt)
+
+	}
+
+	if err != nil {
+		return nil, serverError(ctx, err)
+	}
+
+	return statements, nil
+}
+
 func (s *statusServer) StatementDetails(
 	ctx context.Context, req *serverpb.StatementDetailsRequest,
 ) (*serverpb.StatementDetailsResponse, error) {

pkg/server/serverpb/status.proto

@@ -1,4 +1,4 @@
-// Copyright 2016 The Cockroach Authors.
+ // Copyright 2016 The Cockroach Authors.
 //
 // Use of this software is governed by the Business Source License
 // included in the file licenses/BSL.txt.
@@ -1595,6 +1595,11 @@ message StatementsResponse {
     uint64 id = 3 [(gogoproto.customname) = "ID",
       (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/appstatspb.StmtFingerprintID"];
     cockroach.sql.StatementStatistics stats = 2 [(gogoproto.nullable) = false];
+    // In 23.1 we expect the response to only group on fingerprint_id and app_name
+    // in the overview page. We now return the aggregated list of unique txn fingerprint ids,
+    // leaving the txn_fingerprint_id field in the key empty.
+   repeated uint64 txn_fingerprint_ids = 4 [(gogoproto.customname) = "TxnFingerprintIDs",
+     (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/sql/appstatspb.TransactionFingerprintID"];
   }
 
   repeated CollectedStatementStatistics statements = 1 [(gogoproto.nullable) = false];

pkg/server/status_test.go

@@ -2087,7 +2087,9 @@ func TestStatusAPICombinedStatements(t *testing.T) {
 			}
 
 			statementsInResponse = append(statementsInResponse, respStatement.Key.KeyData.Query)
-			expectedTxnFingerprints[respStatement.Key.KeyData.TransactionFingerprintID] = struct{}{}
+			for _, txnFingerprintID := range respStatement.TxnFingerprintIDs {
+				expectedTxnFingerprints[txnFingerprintID] = struct{}{}
+			}
 		}
 
 		for _, respTxn := range resp.Transactions {
@@ -2102,6 +2104,8 @@ func TestStatusAPICombinedStatements(t *testing.T) {
 				expectedStmts, statementsInResponse, pretty.Sprint(resp), path)
 		}
 		if hasTxns {
+			// We expect that expectedTxnFingerprints is now empty since
+			// we should have removed them all.
 			assert.Empty(t, expectedTxnFingerprints)
 		} else {
 			assert.Empty(t, resp.Transactions)

pkg/ui/workspaces/cluster-ui/src/insights/types.ts

@@ -330,6 +330,7 @@ export interface indexDetails {
 
 // These are the fields used for workload insight recommendations.
 export interface ExecutionDetails {
+  application?: string;
   databaseName?: string;
   elapsedTimeMillis?: number;
   contentionTimeMs?: number;

pkg/ui/workspaces/cluster-ui/src/insights/utils.ts

@@ -382,6 +382,7 @@ export function getStmtInsightRecommendations(
   if (!insightDetails) return [];
 
   const execDetails: ExecutionDetails = {
+    application: insightDetails.application,
     statement: insightDetails.query,
     fingerprintID: insightDetails.statementFingerprintID,
     retries: insightDetails.retries,
@@ -409,6 +410,7 @@ export function getTxnInsightRecommendations(
   if (!insightDetails) return [];
 
   const execDetails: ExecutionDetails = {
+    application: insightDetails.application,
     transactionExecutionID: insightDetails.transactionExecutionID,
     retries: insightDetails.retries,
     contentionTimeMs: insightDetails.contentionTime.asMilliseconds(),

pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/statementInsightDetailsOverviewTab.tsx

@@ -159,6 +159,7 @@ export const StatementInsightDetailsOverviewTab: React.FC<
               label="Transaction Fingerprint ID"
               value={TransactionDetailsLink(
                 insightDetails.transactionFingerprintID,
+                insightDetails.application,
               )}
             />
             <SummaryCardItem

pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/transactionInsightDetailsOverviewTab.tsx

@@ -201,6 +201,7 @@ the maximum number of statements was reached in the console.`;
                       label="Transaction Fingerprint ID"
                       value={TransactionDetailsLink(
                         txnDetails.transactionFingerprintID,
+                        txnDetails.application,
                       )}
                     />
                   </SummaryCard>

pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/transactionInsights/transactionInsightsTable.tsx

@@ -67,7 +67,8 @@ export function makeTransactionInsightsColumns(): ColumnDescriptor<TxnInsightEve
     {
       name: "fingerprintID",
       title: insightsTableTitles.fingerprintID(execType),
-      cell: item => TransactionDetailsLink(item.transactionFingerprintID),
+      cell: item =>
+        TransactionDetailsLink(item.transactionFingerprintID, item.application),
       sort: item => item.transactionFingerprintID,
     },
     {

pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/detailsLinks.tsx

@@ -13,14 +13,20 @@ import React from "react";
 import { HexStringToInt64String } from "../../../util";
 import { Link } from "react-router-dom";
 import { StatementLinkTarget } from "../../../statementsTable";
+import { TransactionLinkTarget } from "../../../transactionsTable";
 
 export function TransactionDetailsLink(
   transactionFingerprintID: string,
+  application?: string,
 ): React.ReactElement {
   const txnID = HexStringToInt64String(transactionFingerprintID);
-  const path = `/transaction/${txnID}`;
   return (
-    <Link to={path}>
+    <Link
+      to={TransactionLinkTarget({
+        transactionFingerprintId: txnID,
+        application,
+      })}
+    >
       <div>{String(transactionFingerprintID)}</div>
     </Link>
   );

pkg/ui/workspaces/cluster-ui/src/insightsTable/insightsTable.tsx

@@ -124,6 +124,7 @@ const StatementExecution = ({
         </div>
       ) : (
         <StatementLink
+          appNames={[rec.execution.application]}
           statementFingerprintID={rec.execution.fingerprintID}
           statement={rec.execution.statement}
           statementSummary={rec.execution.summary}