Merge #76301 #76511

76301: server, ui: aggregate statements on period selected r=maryliag a=maryliag Previously, the aggregation timestamp was used as part of the key of a statement. The new query to collect statement now ignores aggregation timestamp as keys. This commit also removes the aggregated timestamp value on the UI, since it no longer applies. Fixes #74513 Release note (ui change): We don't show information about aggregations timestamp in Statements overview and Details pages, since now all the statement fingerprints are grouped inside the same time selection. 76511: pgwire: fix flaky cancel tests r=RichardJCai a=rafiss fixes #76494 fixes #76493 One required a nil guard, and the other required the telemetry counter to be incremented before the connection is closed. Release note: None Co-authored-by: Marylia Gutierrez <[email protected]> Co-authored-by: Rafi Shamim <[email protected]>
cockroachdb · Feb 14, 2022 · 3be8121 · 3be8121
3 parents 66dfdac + 71ef6b8 + 055f9bf
commit 3be8121
Show file tree

Hide file tree

Showing 7 changed files with 15 additions and 68 deletions.
diff --git a/pkg/server/combined_statement_stats.go b/pkg/server/combined_statement_stats.go
@@ -131,8 +131,8 @@ func collectCombinedStatements(
 				fingerprint_id,
 				transaction_fingerprint_id,
 				app_name,
-				aggregated_ts,
-				max(metadata) AS metadata,
+				max(aggregated_ts) as aggregated_ts,
+				metadata,
 				crdb_internal.merge_statement_stats(array_agg(statistics)) AS statistics,
 				max(sampled_plan) AS sampled_plan,
 				aggregation_interval
@@ -141,7 +141,7 @@ func collectCombinedStatements(
 				fingerprint_id,
 				transaction_fingerprint_id,
 				app_name,
-				aggregated_ts,
+				metadata,
 				aggregation_interval
 		%s`, whereClause, orderAndLimit)
 
@@ -244,13 +244,18 @@ func collectCombinedTransactions(
 	query := fmt.Sprintf(
 		`SELECT
 				app_name,
-				aggregated_ts,
+				max(aggregated_ts) as aggregated_ts,
+				fingerprint_id,
+				metadata,
+				crdb_internal.merge_transaction_stats(array_agg(statistics)) AS statistics,
+				aggregation_interval
+			FROM crdb_internal.transaction_statistics %s
+			GROUP BY
+				app_name,
 				fingerprint_id,
 				metadata,
-				statistics,
 				aggregation_interval
-			FROM crdb_internal.transaction_statistics
-			%s %s`, whereClause, orderAndLimit)
+			%s`, whereClause, orderAndLimit)
 
 	const expectedNumDatums = 6
 

diff --git a/pkg/server/status.go b/pkg/server/status.go
@@ -2550,7 +2550,6 @@ func (s *statusServer) CancelQueryByKey(
 	ctx context.Context, req *serverpb.CancelQueryByKeyRequest,
 ) (resp *serverpb.CancelQueryByKeyResponse, retErr error) {
 	local := req.SQLInstanceID == s.sqlServer.SQLInstanceID()
-	resp = &serverpb.CancelQueryByKeyResponse{}
 
 	// Acquiring the semaphore here helps protect both the source and destination
 	// nodes. The source node is protected against an attacker causing too much
@@ -2571,13 +2570,14 @@ func (s *statusServer) CancelQueryByKey(
 		// If we acquired the semaphore but the cancellation request failed, then
 		// hold on to the semaphore for longer. This helps mitigate a DoS attack
 		// of random cancellation requests.
-		if !resp.Canceled {
+		if err != nil || (resp != nil && !resp.Canceled) {
 			time.Sleep(1 * time.Second)
 		}
 		alloc.Release()
 	}()
 
 	if local {
+		resp = &serverpb.CancelQueryByKeyResponse{}
 		resp.Canceled, err = s.sessionRegistry.CancelQueryByKey(req.CancelQueryKey)
 		if err != nil {
 			resp.Error = err.Error()

diff --git a/pkg/sql/pgwire/server.go b/pkg/sql/pgwire/server.go
@@ -741,12 +741,12 @@ func (s *Server) ServeConn(ctx context.Context, conn net.Conn, socketType Socket
 // attacker) will not know if the cancellation attempt succeeded. Errors are
 // logged so that an operator can be aware of any possibly malicious requests.
 func (s *Server) handleCancel(ctx context.Context, conn net.Conn, buf *pgwirebase.ReadBuffer) {
+	telemetry.Inc(sqltelemetry.CancelRequestCounter)
 	var err error
 	defer func() {
 		if err != nil {
 			log.Sessions.Warningf(ctx, "unexpected while handling pgwire cancellation request: %v", err)
 		}
-		telemetry.Inc(sqltelemetry.CancelRequestCounter)
 	}()
 
 	var backendKeyDataBits uint64

diff --git a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx b/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx
@@ -708,20 +708,6 @@ export class StatementDetails extends React.Component<
             <Col className="gutter-row" span={12}>
               <SummaryCard className={cx("summary-card")}>
                 <Heading type="h5">Statement details</Heading>
-                <div className={summaryCardStylesCx("summary--card__item")}>
-                  <Text>Aggregation Interval (UTC)</Text>
-                  <Text>
-                    {intervalStartTime.format("MMM D, h:mm A")} -{" "}
-                    {intervalEndTime.format(
-                      `${
-                        intervalStartTime.isSame(intervalEndTime, "day")
-                          ? ""
-                          : "MMM D,"
-                      }h:mm A`,
-                    )}
-                  </Text>
-                </div>
-
                 {!isTenant && (
                   <div>
                     <div className={summaryCardStylesCx("summary--card__item")}>

diff --git a/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx b/pkg/ui/workspaces/cluster-ui/src/statementsTable/statementsTable.tsx
@@ -92,17 +92,6 @@ function makeCommonColumns(
   const retryBar = retryBarChart(statements, defaultBarChartOptions);
 
   return [
-    {
-      name: "aggregationInterval",
-      title: statisticsTableTitles.aggregationInterval(statType),
-      className: cx("statements-table__interval_time"),
-      cell: (stmt: AggregateStatistics) =>
-        formatAggregationIntervalColumn(
-          stmt.aggregatedTs,
-          stmt.aggregationInterval,
-        ),
-      sort: (stmt: AggregateStatistics) => stmt.aggregatedTs,
-    },
     {
       name: "executionCount",
       title: statisticsTableTitles.executionCount(statType),

diff --git a/pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx b/pkg/ui/workspaces/cluster-ui/src/statsTableUtil/statsTableUtil.tsx
@@ -35,7 +35,6 @@ export const statisticsColumnLabels = {
   database: "Database",
   diagnostics: "Diagnostics",
   executionCount: "Execution Count",
-  aggregationInterval: "Aggregation Interval (UTC)",
   maxMemUsage: "Max Memory",
   networkBytes: "Network",
   regionNodes: "Regions/Nodes",
@@ -140,27 +139,6 @@ export const statisticsTableTitles: StatisticTableTitleType = {
       </Tooltip>
     );
   },
-  aggregationInterval: () => {
-    return (
-      <Tooltip
-        placement="bottom"
-        style="tableTitle"
-        content={
-          <div>
-            <p>
-              The time interval of the statement execution. By default,
-              statements are configured to aggregate over an hour interval.
-              <br />
-              For example, if a statement is executed at 1:23PM it will fall in
-              the 1:00PM - 2:00PM time interval.
-            </p>
-          </div>
-        }
-      >
-        {getLabel("aggregationInterval")}
-      </Tooltip>
-    );
-  },
   executionCount: (statType: StatisticType) => {
     let contentModifier = "";
     let fingerprintModifier = "";

diff --git a/pkg/ui/workspaces/cluster-ui/src/transactionsTable/transactionsTable.tsx b/pkg/ui/workspaces/cluster-ui/src/transactionsTable/transactionsTable.tsx
@@ -162,17 +162,6 @@ export function makeTransactionsColumns(
         ),
       alwaysShow: true,
     },
-    {
-      name: "aggregationInterval",
-      title: statisticsTableTitles.aggregationInterval("transaction"),
-      cell: (item: TransactionInfo) =>
-        formatAggregationIntervalColumn(
-          TimestampToNumber(item.stats_data?.aggregated_ts),
-          DurationToNumber(item.stats_data?.aggregation_interval),
-        ),
-      sort: (item: TransactionInfo) =>
-        TimestampToNumber(item.stats_data?.aggregated_ts),
-    },
     {
       name: "executionCount",
       title: statisticsTableTitles.executionCount(statType),