Skip to content

Commit

Permalink
ui: fix sql stats page data refreshing
Browse files Browse the repository at this point in the history 
This commit fixes a couple of bugs with sql stats page
data fetching.

First - we missed clearing the initial timeout to fetch data that was
created on mount - it should be cleared when a new time range is
selected on the page or when the page is unmounted. Currently, this
bug can lead to unexpeted data refetches of the incorrect (previously
selected) time range.

Second - with the introduction of new pages that use the time
picker, such as insights, sql stats pages are no longer the only
page that can change the time range. Previously, data was only
fetched on mount for sql stats pages if it was not previously fetched,
or a data refresh was scheduled based on last update time if the
current time range was not a custom (fixed) range. We should now also
refetch when the data is outdated due to a new time range being
selected on a different page. This commit introduces the `isDataValid`
prop to sql stats pages to determine this and refetch data accordingly.

Epic: none
  • Loading branch information
@xinhaoz
xinhaoz committed Jan 17, 2023
1 parent 6116ab4 commit af43742
Show file tree
Hide file tree
Showing 12 changed files with 108 additions and 77 deletions.
23 changes: 13 additions & 10 deletions pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.fixture.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,34 +217,34 @@ const statementStats: Required<IStatementStatistics> = {

const diagnosticsReports: StatementDiagnosticsReport[] = [
{
id: '594413966918975489',
id: "594413966918975489",
completed: true,
statement_fingerprint: "SHOW database",
statement_diagnostics_id: '594413981435920385',
statement_diagnostics_id: "594413981435920385",
requested_at: moment(1601471146),
},
{
id: '594413966918975429',
id: "594413966918975429",
completed: true,
statement_fingerprint: "SHOW database",
statement_diagnostics_id: '594413281435920385',
statement_diagnostics_id: "594413281435920385",
requested_at: moment(1601491146),
},
];

const diagnosticsReportsInProgress: StatementDiagnosticsReport[] = [
{
id: '594413966918975489',
id: "594413966918975489",
completed: false,
statement_fingerprint: "SHOW database",
statement_diagnostics_id: '594413981435920385',
statement_diagnostics_id: "594413981435920385",
requested_at: moment(1601471146),
},
{
id: '594413966918975429',
id: "594413966918975429",
completed: true,
statement_fingerprint: "SHOW database",
statement_diagnostics_id: '594413281435920385',
statement_diagnostics_id: "594413281435920385",
requested_at: moment(1601491146),
},
];
Expand Down Expand Up @@ -290,6 +290,7 @@ const statementsPagePropsFixture: StatementsPageProps = {
nodes: "",
},
lastUpdated,
isDataValid: true,
// Aggregate key values in these statements will need to change if implementation
// of 'statementKey' in appStats.ts changes.
statements: [
Expand Down Expand Up @@ -592,7 +593,8 @@ const statementsPagePropsFixture: StatementsPageProps = {
aggregatedFingerprintID: "6325213731862855938",
aggregatedFingerprintHexID:
Long.fromNumber(6325213731862855938).toString(16),
label: "INSERT INTO users VALUES ($1, $2, __more1_10__), (__more10_100__)",
label:
"INSERT INTO users VALUES ($1, $2, __more1_10__), (__more10_100__)",
summary: "INSERT INTO users VALUES",
aggregatedTs,
aggregationInterval,
Expand Down Expand Up @@ -904,7 +906,8 @@ const statementsPagePropsFixture: StatementsPageProps = {
aggregatedFingerprintID: "16819876564846676829",
aggregatedFingerprintHexID:
Long.fromNumber(16819876564846676829).toString(16),
label: "INSERT INTO vehicles VALUES ($1, $2, __more1_10__), (__more1_10__)",
label:
"INSERT INTO vehicles VALUES ($1, $2, __more1_10__), (__more1_10__)",
summary: "INSERT INTO vehicles",
aggregatedTs,
aggregationInterval,
Expand Down
7 changes: 7 additions & 0 deletions pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.selectors.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,13 @@ export const selectLastReset = createSelector(sqlStatsSelector, state => {
return formatDate(TimestampToMoment(state.data.last_reset));
});

export const selectStatementsDataValid = createSelector(
sqlStatsSelector,
(state: SQLStatsState): boolean => {
return state.valid;
},
);

export const selectStatements = createSelector(
sqlStatsSelector,
(_: AppState, props: RouteComponentProps) => props,
Expand Down
55 changes: 33 additions & 22 deletions pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPage.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ import {
const cx = classNames.bind(styles);
const sortableTableCx = classNames.bind(sortableTableStyles);

const POLLING_INTERVAL_MILLIS = 300000;

// Most of the props are supposed to be provided as connected props
// from redux store.
// StatementsPageDispatchProps, StatementsPageStateProps, and StatementsPageOuterProps interfaces
Expand Down Expand Up @@ -123,6 +125,7 @@ export interface StatementsPageDispatchProps {

export interface StatementsPageStateProps {
statements: AggregateStatistics[];
isDataValid: boolean;
lastUpdated: moment.Moment | null;
timeScale: TimeScale;
statementsError: Error | null;
Expand Down Expand Up @@ -151,10 +154,10 @@ export type StatementsPageProps = StatementsPageDispatchProps &
StatementsPageStateProps &
RouteComponentProps<unknown>;

function statementsRequestFromProps(
props: StatementsPageProps,
function stmtsRequestFromTimeScale(
ts: TimeScale,
): cockroach.server.serverpb.StatementsRequest {
const [start, end] = toRoundedDateRange(props.timeScale);
const [start, end] = toRoundedDateRange(ts);
return new cockroach.server.serverpb.StatementsRequest({
combined: true,
start: Long.fromNumber(start.unix()),
Expand Down Expand Up @@ -270,7 +273,7 @@ export class StatementsPage extends React.Component<
if (this.props.onTimeScaleChange) {
this.props.onTimeScaleChange(ts);
}
this.resetPolling(ts.key);
this.refreshStatements(ts);
this.setState({
startRequest: new Date(),
});
Expand All @@ -293,30 +296,31 @@ export class StatementsPage extends React.Component<
}
}

resetPolling(key: string): void {
resetPolling(ts: TimeScale): void {
this.clearRefreshDataTimeout();
if (key !== "Custom") {
if (ts.key !== "Custom") {
this.refreshDataTimeout = setTimeout(
this.refreshStatements,
300000, // 5 minutes
POLLING_INTERVAL_MILLIS, // 5 minutes
ts,
);
}
}

refreshStatements = (): void => {
const req = statementsRequestFromProps(this.props);
refreshStatements = (ts?: TimeScale): void => {
const time = ts ?? this.props.timeScale;
const req = stmtsRequestFromTimeScale(time);
this.props.refreshStatements(req);

this.resetPolling(this.props.timeScale.key);
this.resetPolling(time);
};

refreshDatabases = (): void => {
this.props.refreshDatabases();
this.resetPolling(this.props.timeScale.key);
};

resetSQLStats = (): void => {
const req = statementsRequestFromProps(this.props);
const req = stmtsRequestFromTimeScale(this.props.timeScale);
this.props.resetSQLStats(req);
this.setState({
startRequest: new Date(),
Expand All @@ -328,17 +332,24 @@ export class StatementsPage extends React.Component<
startRequest: new Date(),
});

// For the first data fetch for this page, we refresh if there are:
// For the first data fetch for this page, we refresh immediately if:
// - Last updated is null (no statements 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(
// - The data is not valid (time scale may have changed on other pages)
// - The time range selected is a moving window and the last udpated time
// is >= 5 minutes.
// Otherwise, we schedule a refresh at 5 mins from the lastUpdated time if
// the time range selected is a moving window (i.e. not custom).
const now = moment();
let nextRefresh = null;
if (this.props.lastUpdated == null || !this.props.isDataValid) {
nextRefresh = now;
} else if (this.props.timeScale.key !== "Custom") {
nextRefresh = this.props.lastUpdated
.clone()
.add(POLLING_INTERVAL_MILLIS, "milliseconds");
}
if (nextRefresh) {
this.refreshDataTimeout = setTimeout(
this.refreshStatements,
Math.max(0, nextRefresh.diff(now, "milliseconds")),
);
Expand Down
2 changes: 2 additions & 0 deletions pkg/ui/workspaces/cluster-ui/src/statementsPage/statementsPageConnected.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
selectDatabases,
selectLastReset,
selectStatements,
selectStatementsDataValid,
selectStatementsLastError,
selectTotalFingerprints,
selectColumns,
Expand Down Expand Up @@ -95,6 +96,7 @@ export const ConnectedStatementsPage = withRouter(
search: selectSearch(state),
sortSetting: selectSortSetting(state),
statements: selectStatements(state, props),
isDataValid: selectStatementsDataValid(state),
lastUpdated: selectStatementsLastUpdated(state),
statementsError: selectStatementsLastError(state),
totalFingerprints: selectTotalFingerprints(state),
Expand Down
1 change: 0 additions & 1 deletion ...rkspaces/cluster-ui/src/store/insights/transactionInsights/transactionInsights.reducer.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

import { createSlice, PayloadAction } from "@reduxjs/toolkit";
import { DOMAIN_NAME } from "src/store/utils";
import moment, { Moment } from "moment";
import { TxnContentionInsightEvent } from "src/insights";
import { ExecutionInsightsRequest } from "../../../api";
import { UpdateTimeScalePayload } from "../../sqlStats";
Expand Down
7 changes: 0 additions & 7 deletions pkg/ui/workspaces/cluster-ui/src/store/sqlStats/sqlStats.sagas.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,16 +52,9 @@ export function* updateSQLStatsTimeScaleSaga(
value: ts,
}),
);
const [start, end] = toRoundedDateRange(ts);
const req = new cockroach.server.serverpb.StatementsRequest({
combined: true,
start: Long.fromNumber(start.unix()),
end: Long.fromNumber(end.unix()),
});
yield put(sqlStatsActions.invalidated());
yield put(stmtInsightActions.invalidated());
yield put(txnInsightActions.invalidated());
yield put(sqlStatsActions.refresh(req));
}

export function* resetSQLStatsSaga(action: PayloadAction<StatementsRequest>) {
Expand Down
56 changes: 34 additions & 22 deletions pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPage.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ type IStatementsResponse = protos.cockroach.server.serverpb.IStatementsResponse;

const cx = classNames.bind(styles);

const POLLING_INTERVAL_MILLIS = 300000;
interface TState {
filters?: Filters;
pagination: ISortedTablePagination;
Expand All @@ -86,6 +87,7 @@ interface TState {
export interface TransactionsPageStateProps {
columns: string[];
data: IStatementsResponse;
isDataValid: boolean;
lastUpdated: moment.Moment | null;
timeScale: TimeScale;
error?: Error | null;
Expand Down Expand Up @@ -116,10 +118,10 @@ export type TransactionsPageProps = TransactionsPageStateProps &
TransactionsPageDispatchProps &
RouteComponentProps;

function statementsRequestFromProps(
props: TransactionsPageProps,
function stmtsRequestFromTimeScale(
ts: TimeScale,
): protos.cockroach.server.serverpb.StatementsRequest {
const [start, end] = toRoundedDateRange(props.timeScale);
const [start, end] = toRoundedDateRange(ts);
return new protos.cockroach.server.serverpb.StatementsRequest({
combined: true,
start: Long.fromNumber(start.unix()),
Expand Down Expand Up @@ -198,40 +200,50 @@ export class TransactionsPage extends React.Component<

// Schedule the next data request depending on the time
// range key.
resetPolling(key: string): void {
resetPolling(ts: TimeScale): void {
this.clearRefreshDataTimeout();
if (key !== "Custom") {
if (ts.key !== "Custom") {
this.refreshDataTimeout = setTimeout(
this.refreshData,
300000, // 5 minutes
POLLING_INTERVAL_MILLIS, // 5 minutes
ts,
);
}
}

refreshData = (): void => {
const req = statementsRequestFromProps(this.props);
refreshData = (ts?: TimeScale): void => {
const time = ts ?? this.props.timeScale;
const req = stmtsRequestFromTimeScale(time);
this.props.refreshData(req);
this.resetPolling(this.props.timeScale.key);

this.resetPolling(time);
};

resetSQLStats = (): void => {
const req = statementsRequestFromProps(this.props);
const req = stmtsRequestFromTimeScale(this.props.timeScale);
this.props.resetSQLStats(req);
this.resetPolling(this.props.timeScale.key);
this.resetPolling(this.props.timeScale);
};

componentDidMount(): void {
// For the first data fetch for this page, we refresh if there are:
// For the first data fetch for this page, we refresh immediately if:
// - Last updated is null (no statements 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(
// - The data is not valid (time scale may have changed on other pages)
// - The time range selected is a moving window and the last udpated time
// is >= 5 minutes.
// Otherwise, we schedule a refresh at 5 mins from the lastUpdated time if
// the time range selected is a moving window (i.e. not custom).
const now = moment();
let nextRefresh = null;
if (this.props.lastUpdated == null || !this.props.isDataValid) {
nextRefresh = now;
} else if (this.props.timeScale.key !== "Custom") {
nextRefresh = this.props.lastUpdated
.clone()
.add(POLLING_INTERVAL_MILLIS, "milliseconds");
}
if (nextRefresh) {
this.refreshDataTimeout = setTimeout(
this.refreshData,
Math.max(0, nextRefresh.diff(now, "milliseconds")),
);
Expand Down Expand Up @@ -387,7 +399,7 @@ export class TransactionsPage extends React.Component<
if (this.props.onTimeScaleChange) {
this.props.onTimeScaleChange(ts);
}
this.resetPolling(ts.key);
this.refreshData(ts);
};

render(): React.ReactElement {
Expand Down
6 changes: 5 additions & 1 deletion pkg/ui/workspaces/cluster-ui/src/transactionsPage/transactionsPageConnected.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ import {
} from "./transactionsPage.selectors";
import { selectIsTenant } from "../store/uiConfig";
import { nodeRegionsByIDSelector } from "../store/nodes";
import { selectStatementsLastUpdated } from "src/statementsPage/statementsPage.selectors";
import {
selectStatementsLastUpdated,
selectStatementsDataValid,
} from "src/statementsPage/statementsPage.selectors";
import { selectTimeScale } from "../store/utils/selectors";
import { StatementsRequest } from "src/api/statementsApi";
import { actions as localStorageActions } from "../store/localStorage";
Expand Down Expand Up @@ -71,6 +74,7 @@ export const TransactionsPageConnected = withRouter(
...props,
columns: selectTxnColumns(state),
data: selectTransactionsData(state),
isDataValid: selectStatementsDataValid(state),
lastUpdated: selectStatementsLastUpdated(state),
timeScale: selectTimeScale(state),
error: selectTransactionsLastError(state),
Expand Down
Loading

0 comments on commit af43742

Please sign in to comment.