Merge cockroachdb#92596

92596: ui: fix polling for statement and transaction details pages r=ericharmeling a=ericharmeling This commit moves polling to the statement and transaction details page components from the cached data reducer and sagas, using the same approach as cockroachdb#85772. Fixes cockroachdb#91297. Loom (DB Console and CC Console): https://www.loom.com/share/d3002ad54463448aaa3b8c9d74e31d10 Release note (ui change): The statement fingerprint details page in the Console no longer infinitely loads after 5 minutes. Co-authored-by: Eric Harmeling <[email protected]>
AlexTalks · Jan 10, 2023 · b5cffc0 · b5cffc0
2 parents 1738c19 + e36a054
commit b5cffc0
Show file tree

Hide file tree

Showing 13 changed files with 172 additions and 68 deletions.
diff --git a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.fixture.ts b/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.fixture.ts
@@ -17,6 +17,8 @@ import { StatementDetailsResponse } from "../api";
 
 const history = createMemoryHistory({ initialEntries: ["/statements"] });
 
+const lastUpdated = moment("Nov 28 2022 01:30:00 GMT");
+
 const statementDetailsNoData: StatementDetailsResponse = {
   statement: {
     metadata: {
@@ -806,6 +808,7 @@ export const getStatementDetailsPropsFixture = (
     },
   },
   isLoading: false,
+  lastUpdated: lastUpdated,
   timeScale: {
     windowSize: moment.duration(5, "day"),
     sampleSize: moment.duration(5, "minutes"),

diff --git a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts b/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.selectors.ts
@@ -22,6 +22,8 @@ import {
 import { cockroach } from "@cockroachlabs/crdb-protobuf-client";
 import { TimeScale, toRoundedDateRange } from "../timeScaleDropdown";
 import { selectTimeScale } from "../statementsPage/statementsPage.selectors";
+import moment from "moment";
+
 type StatementDetailsResponseMessage =
   cockroach.server.serverpb.StatementDetailsResponse;
 
@@ -41,6 +43,7 @@ export const selectStatementDetails = createSelector(
     statementDetails: StatementDetailsResponseMessage;
     isLoading: boolean;
     lastError: Error;
+    lastUpdated: moment.Moment | null;
   } => {
     // Since the aggregation interval is 1h, we want to round the selected timeScale to include
     // the full hour. If a timeScale is between 14:32 - 15:17 we want to search for values
@@ -59,9 +62,15 @@ export const selectStatementDetails = createSelector(
         statementDetails: statementDetailsStatsData[key].data,
         isLoading: statementDetailsStatsData[key].inFlight,
         lastError: statementDetailsStatsData[key].lastError,
+        lastUpdated: statementDetailsStatsData[key].lastUpdated,
       };
     }
-    return { statementDetails: null, isLoading: true, lastError: null };
+    return {
+      statementDetails: null,
+      isLoading: true,
+      lastError: null,
+      lastUpdated: null,
+    };
   },
 );
 

diff --git a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx b/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx
@@ -132,6 +132,7 @@ export interface StatementDetailsStateProps {
   statementDetails: StatementDetailsResponse;
   isLoading: boolean;
   statementsError: Error | null;
+  lastUpdated: moment.Moment | null;
   timeScale: TimeScale;
   nodeRegions: { [nodeId: string]: string };
   diagnosticsReports: StatementDiagnosticsReport[];
@@ -147,15 +148,13 @@ const cx = classNames.bind(styles);
 const summaryCardStylesCx = classNames.bind(summaryCardStyles);
 const timeScaleStylesCx = classNames.bind(timeScaleStyles);
 
-function getStatementDetailsRequest(
-  timeScale: TimeScale,
-  statementFingerprintID: string,
-  location: Location,
+function getStatementDetailsRequestFromProps(
+  props: StatementDetailsProps,
 ): cockroach.server.serverpb.StatementDetailsRequest {
-  const [start, end] = toRoundedDateRange(timeScale);
+  const [start, end] = toRoundedDateRange(props.timeScale);
   return new cockroach.server.serverpb.StatementDetailsRequest({
-    fingerprint_id: statementFingerprintID,
-    app_names: queryByName(location, appNamesAttr)?.split(","),
+    fingerprint_id: props.statementFingerprintID,
+    app_names: queryByName(props.location, appNamesAttr)?.split(","),
     start: Long.fromNumber(start.unix()),
     end: Long.fromNumber(end.unix()),
   });
@@ -200,6 +199,7 @@ export class StatementDetails extends React.Component<
   StatementDetailsState
 > {
   activateDiagnosticsRef: React.RefObject<ActivateDiagnosticsModalRef>;
+  refreshDataTimeout: NodeJS.Timeout;
 
   constructor(props: StatementDetailsProps) {
     super(props);
@@ -218,7 +218,7 @@ export class StatementDetails extends React.Component<
     // where the value 10/30 min is selected on the Metrics page.
     const ts = getValidOption(this.props.timeScale, timeScale1hMinOptions);
     if (ts !== this.props.timeScale) {
-      this.props.onTimeScaleChange(ts);
+      this.changeTimeScale(ts);
     }
   }
 
@@ -234,17 +234,33 @@ export class StatementDetails extends React.Component<
   hasDiagnosticReports = (): boolean =>
     this.props.diagnosticsReports.length > 0;
 
-  refreshStatementDetails = (
-    timeScale: TimeScale,
-    statementFingerprintID: string,
-    location: Location,
-  ): void => {
-    const req = getStatementDetailsRequest(
-      timeScale,
-      statementFingerprintID,
-      location,
-    );
+  changeTimeScale = (ts: TimeScale): void => {
+    if (this.props.onTimeScaleChange) {
+      this.props.onTimeScaleChange(ts);
+    }
+    this.resetPolling(ts.key);
+  };
+
+  clearRefreshDataTimeout() {
+    if (this.refreshDataTimeout !== null) {
+      clearTimeout(this.refreshDataTimeout);
+    }
+  }
+
+  resetPolling(key: string) {
+    this.clearRefreshDataTimeout();
+    if (key !== "Custom") {
+      this.refreshDataTimeout = setTimeout(
+        this.refreshStatementDetails,
+        300000, // 5 minutes
+      );
+    }
+  }
+
+  refreshStatementDetails = (): void => {
+    const req = getStatementDetailsRequestFromProps(this.props);
     this.props.refreshStatementDetails(req);
+    this.resetPolling(this.props.timeScale.key);
   };
 
   handleResize = (): void => {
@@ -260,13 +276,24 @@ export class StatementDetails extends React.Component<
   };
 
   componentDidMount(): void {
+    this.refreshStatementDetails();
     window.addEventListener("resize", this.handleResize);
     this.handleResize();
-    this.refreshStatementDetails(
-      this.props.timeScale,
-      this.props.statementFingerprintID,
-      this.props.location,
-    );
+    // For the first data fetch for this page, we refresh if there are:
+    // - Last updated is null (no statement details fetched previously)
+    // - The time interval is not custom, i.e. we have a moving window
+    // in which case we poll every 5 minutes. For the first fetch we will
+    // calculate the next time to refresh based on when the data was last
+    // updated.
+    if (this.props.timeScale.key !== "Custom" || !this.props.lastUpdated) {
+      const now = moment();
+      const nextRefresh =
+        this.props.lastUpdated?.clone().add(5, "minutes") || now;
+      setTimeout(
+        this.refreshStatementDetails,
+        Math.max(0, nextRefresh.diff(now, "milliseconds")),
+      );
+    }
     this.props.refreshUserSQLRoles();
     this.props.refreshNodes();
     if (!this.props.isTenant) {
@@ -284,11 +311,7 @@ export class StatementDetails extends React.Component<
       prevProps.statementFingerprintID != this.props.statementFingerprintID ||
       prevProps.location != this.props.location
     ) {
-      this.refreshStatementDetails(
-        this.props.timeScale,
-        this.props.statementFingerprintID,
-        this.props.location,
-      );
+      this.refreshStatementDetails();
     }
 
     this.props.refreshNodes();
@@ -754,7 +777,7 @@ export class StatementDetails extends React.Component<
             <TimeScaleDropdown
               options={timeScale1hMinOptions}
               currentScale={this.props.timeScale}
-              setTimeScale={this.props.onTimeScaleChange}
+              setTimeScale={this.changeTimeScale}
             />
           </PageConfigItem>
         </PageConfig>

diff --git a/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetailsConnected.ts b/pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetailsConnected.ts
@@ -48,16 +48,15 @@ import { getMatchParamByName, statementAttr } from "../util";
 // For tenant cases, we don't show information about node, regions and
 // diagnostics.
 const mapStateToProps = (state: AppState, props: RouteComponentProps) => {
-  const { statementDetails, isLoading, lastError } = selectStatementDetails(
-    state,
-    props,
-  );
+  const { statementDetails, isLoading, lastError, lastUpdated } =
+    selectStatementDetails(state, props);
   const statementFingerprint = statementDetails?.statement.metadata.query;
   return {
     statementFingerprintID: getMatchParamByName(props.match, statementAttr),
     statementDetails,
     isLoading: isLoading,
     statementsError: lastError,
+    lastUpdated: lastUpdated,
     timeScale: selectTimeScale(state),
     nodeRegions: nodeRegionsByIDSelector(state),
     diagnosticsReports:

diff --git a/pkg/ui/workspaces/cluster-ui/src/store/statementDetails/statementDetails.reducer.ts b/pkg/ui/workspaces/cluster-ui/src/store/statementDetails/statementDetails.reducer.ts
@@ -17,12 +17,14 @@ import {
   StatementDetailsResponseWithKey,
 } from "src/api/statementsApi";
 import { generateStmtDetailsToID } from "../../util";
+import moment from "moment";
 
 export type SQLDetailsStatsState = {
   data: StatementDetailsResponse;
   lastError: Error;
   valid: boolean;
   inFlight: boolean;
+  lastUpdated: moment.Moment | null;
 };
 
 export type SQLDetailsStatsReducerState = {
@@ -48,6 +50,7 @@ const sqlDetailsStatsSlice = createSlice({
         valid: true,
         lastError: null,
         inFlight: false,
+        lastUpdated: moment.utc(),
       };
     },
     failed: (state, action: PayloadAction<ErrorWithKey>) => {
@@ -56,6 +59,7 @@ const sqlDetailsStatsSlice = createSlice({
         valid: false,
         lastError: action.payload.err,
         inFlight: false,
+        lastUpdated: moment.utc(),
       };
     },
     invalidated: (state, action: PayloadAction<{ key: string }>) => {
@@ -81,6 +85,7 @@ const sqlDetailsStatsSlice = createSlice({
         valid: false,
         lastError: null,
         inFlight: true,
+        lastUpdated: null,
       };
     },
     request: (state, action: PayloadAction<StatementDetailsRequest>) => {
@@ -97,6 +102,7 @@ const sqlDetailsStatsSlice = createSlice({
         valid: false,
         lastError: null,
         inFlight: true,
+        lastUpdated: null,
       };
     },
   },

diff --git a/pkg/ui/workspaces/cluster-ui/src/store/statementDetails/statementDetails.sagas.spec.ts b/pkg/ui/workspaces/cluster-ui/src/store/statementDetails/statementDetails.sagas.spec.ts
@@ -28,10 +28,24 @@ import {
   reducer,
   SQLDetailsStatsReducerState,
 } from "./statementDetails.reducer";
+
+import moment from "moment";
+
+const lastUpdated = moment();
+
 export type StatementDetailsRequest =
   cockroach.server.serverpb.StatementDetailsRequest;
 
 describe("SQLDetailsStats sagas", () => {
+  let spy: jest.SpyInstance;
+  beforeAll(() => {
+    spy = jest.spyOn(moment, "utc").mockImplementation(() => lastUpdated);
+  });
+
+  afterAll(() => {
+    spy.mockRestore();
+  });
+
   const action: PayloadAction<StatementDetailsRequest> = {
     payload: cockroach.server.serverpb.StatementDetailsRequest.create({
       fingerprint_id: "SELECT * FROM crdb_internal.node_build_info",
@@ -665,6 +679,7 @@ describe("SQLDetailsStats sagas", () => {
                 lastError: null,
                 valid: true,
                 inFlight: false,
+                lastUpdated: lastUpdated,
               },
           },
         })
@@ -690,6 +705,7 @@ describe("SQLDetailsStats sagas", () => {
                 lastError: error,
                 valid: false,
                 inFlight: false,
+                lastUpdated: lastUpdated,
               },
           },
         })

diff --git a/pkg/ui/workspaces/cluster-ui/src/store/statementDetails/statementDetails.sagas.ts b/pkg/ui/workspaces/cluster-ui/src/store/statementDetails/statementDetails.sagas.ts
@@ -9,15 +9,14 @@
 // licenses/APL.txt.
 
 import { PayloadAction } from "@reduxjs/toolkit";
-import { all, call, put, delay, takeLatest } from "redux-saga/effects";
+import { all, call, put, takeLatest } from "redux-saga/effects";
 import {
   ErrorWithKey,
   getStatementDetails,
   StatementDetailsRequest,
   StatementDetailsResponseWithKey,
 } from "src/api/statementsApi";
 import { actions as sqlDetailsStatsActions } from "./statementDetails.reducer";
-import { CACHE_INVALIDATION_PERIOD } from "src/store/utils";
 import { generateStmtDetailsToID } from "../../util";
 
 export function* refreshSQLDetailsStatsSaga(
@@ -53,28 +52,9 @@ export function* requestSQLDetailsStatsSaga(
   }
 }
 
-export function receivedSQLDetailsStatsSagaFactory(delayMs: number) {
-  return function* receivedSQLDetailsStatsSaga(
-    action: PayloadAction<StatementDetailsResponseWithKey>,
-  ) {
-    yield delay(delayMs);
-    yield put(
-      sqlDetailsStatsActions.invalidated({
-        key: action?.payload.key,
-      }),
-    );
-  };
-}
-
-export function* sqlDetailsStatsSaga(
-  cacheInvalidationPeriod: number = CACHE_INVALIDATION_PERIOD,
-) {
+export function* sqlDetailsStatsSaga() {
   yield all([
     takeLatest(sqlDetailsStatsActions.refresh, refreshSQLDetailsStatsSaga),
     takeLatest(sqlDetailsStatsActions.request, requestSQLDetailsStatsSaga),
-    takeLatest(
-      sqlDetailsStatsActions.received,
-      receivedSQLDetailsStatsSagaFactory(cacheInvalidationPeriod),
-    ),
   ]);
 }