Skip to content

Commit

Permalink
ui: unify cluster-ui stores for Statement and Transaction Page
Browse files Browse the repository at this point in the history
Previously, on CC Conosle, Statement Page and Transaction Page were backed by
their own Redux stores and their own Redux Sagas. However, both Statement
and Transaction Page were backed by a single API endpoint, namely
`/_status/statements`. This resulted in unnecessary API calls and bugs
like #72009, which was caused by Reset SQL Stats sagas failing to
reset the Transaction Page Redux store.

This commit unifies the following Redux store and sagas into a single
store / sagas:
1. Statement Page store / sagas
2. Transaciton Page store / sagas
3. Reset SQL Stats store / sagas

This greatly removed the code duplication and test duplication and simplify
the logic. Statement Page and Transaction Page can reuse the same store
by their own selectors.

Resolves #72009

Release note: None
  • Loading branch information
Azhng committed Nov 22, 2021
1 parent 459aff5 commit f35c4a8
Show file tree
Hide file tree
Showing 21 changed files with 335 additions and 616 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -142,16 +142,15 @@ function filterByRouterParamsPredicate(
}

export const selectStatement = createSelector(
(state: AppState) => state.adminUI.statements,
(state: AppState) => state.adminUI.sqlStats,
(_state: AppState, props: RouteComponentProps) => props,
(statementsState, props) => {
const statements = statementsState.data?.statements;
(sqlStatsState, props) => {
const statements = sqlStatsState.data?.statements;
if (!statements) {
return null;
}

const internalAppNamePrefix =
statementsState.data?.internal_app_name_prefix;
const internalAppNamePrefix = sqlStatsState.data?.internal_app_name_prefix;
const flattened = flattenStatementStats(statements);
const results = _.filter(
flattened,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
nodeDisplayNameByIDSelector,
nodeRegionsByIDSelector,
} from "../store/nodes";
import { actions as statementsActions } from "src/store/statements";
import { actions as sqlStatsActions } from "src/store/sqlStats";
import {
actions as statementDiagnosticsActions,
selectDiagnosticsReportsByStatementFingerprint,
Expand All @@ -44,7 +44,7 @@ const mapStateToProps = (state: AppState, props: StatementDetailsProps) => {
const statementFingerprint = statement?.statement;
return {
statement,
statementsError: state.adminUI.statements.lastError,
statementsError: state.adminUI.sqlStats.lastError,
dateRange: selectDateRange(state),
nodeNames: selectIsTenant(state) ? {} : nodeDisplayNameByIDSelector(state),
nodeRegions: selectIsTenant(state) ? {} : nodeRegionsByIDSelector(state),
Expand All @@ -62,7 +62,7 @@ const mapStateToProps = (state: AppState, props: StatementDetailsProps) => {
const mapDispatchToProps = (
dispatch: Dispatch,
): StatementDetailsDispatchProps => ({
refreshStatements: () => dispatch(statementsActions.refresh()),
refreshStatements: () => dispatch(sqlStatsActions.refresh()),
refreshStatementDiagnosticsRequests: () =>
dispatch(statementDiagnosticsActions.refresh()),
refreshNodes: () => dispatch(nodesActions.refresh()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,10 @@ import { cockroach } from "@cockroachlabs/crdb-protobuf-client";
import { RouteComponentProps } from "react-router-dom";

import { AppState } from "src/store";
import { StatementsState } from "../store/statements";
import { selectDiagnosticsReportsPerStatement } from "../store/statementDiagnostics";
import { AggregateStatistics } from "../statementsTable";
import { sqlStatsSelector } from "../store/sqlStats/sqlStats.selector";
import { SQLStatsState } from "../store/sqlStats";

type ICollectedStatementStatistics = cockroach.server.serverpb.StatementsResponse.ICollectedStatementStatistics;
export interface StatementsSummaryData {
Expand All @@ -47,65 +48,55 @@ export const adminUISelector = createSelector(
adminUiState => adminUiState,
);

export const statementsSelector = createSelector(
adminUISelector,
adminUiState => adminUiState.statements,
);

export const localStorageSelector = createSelector(
adminUISelector,
adminUiState => adminUiState.localStorage,
);

// selectApps returns the array of all apps with statement statistics present
// in the data.
export const selectApps = createSelector(
statementsSelector,
statementsState => {
if (!statementsState.data) {
return [];
}
export const selectApps = createSelector(sqlStatsSelector, sqlStatsState => {
if (!sqlStatsState.data || !sqlStatsState.valid) {
return [];
}

let sawBlank = false;
let sawInternal = false;
const apps: { [app: string]: boolean } = {};
statementsState.data.statements.forEach(
(statement: ICollectedStatementStatistics) => {
if (
statementsState.data.internal_app_name_prefix &&
statement.key.key_data.app.startsWith(
statementsState.data.internal_app_name_prefix,
)
) {
sawInternal = true;
} else if (statement.key.key_data.app) {
apps[statement.key.key_data.app] = true;
} else {
sawBlank = true;
}
},
);
return []
.concat(
sawInternal ? [statementsState.data.internal_app_name_prefix] : [],
)
.concat(sawBlank ? ["(unset)"] : [])
.concat(Object.keys(apps));
},
);
let sawBlank = false;
let sawInternal = false;
const apps: { [app: string]: boolean } = {};
sqlStatsState.data.statements.forEach(
(statement: ICollectedStatementStatistics) => {
if (
sqlStatsState.data.internal_app_name_prefix &&
statement.key.key_data.app.startsWith(
sqlStatsState.data.internal_app_name_prefix,
)
) {
sawInternal = true;
} else if (statement.key.key_data.app) {
apps[statement.key.key_data.app] = true;
} else {
sawBlank = true;
}
},
);
return []
.concat(sawInternal ? [sqlStatsState.data.internal_app_name_prefix] : [])
.concat(sawBlank ? ["(unset)"] : [])
.concat(Object.keys(apps));
});

// selectDatabases returns the array of all databases with statement statistics present
// in the data.
export const selectDatabases = createSelector(
statementsSelector,
statementsState => {
if (!statementsState.data) {
sqlStatsSelector,
sqlStatsState => {
if (!sqlStatsState.data) {
return [];
}

return Array.from(
new Set(
statementsState.data.statements.map(s =>
sqlStatsState.data.statements.map(s =>
s.key.key_data.database ? s.key.key_data.database : "(unset)",
),
),
Expand All @@ -116,7 +107,7 @@ export const selectDatabases = createSelector(
// selectTotalFingerprints returns the count of distinct statement fingerprints
// present in the data.
export const selectTotalFingerprints = createSelector(
statementsSelector,
sqlStatsSelector,
state => {
if (!state.data) {
return 0;
Expand All @@ -128,7 +119,7 @@ export const selectTotalFingerprints = createSelector(

// selectLastReset returns a string displaying the last time the statement
// statistics were reset.
export const selectLastReset = createSelector(statementsSelector, state => {
export const selectLastReset = createSelector(sqlStatsSelector, state => {
if (!state.data) {
return "";
}
Expand All @@ -137,11 +128,11 @@ export const selectLastReset = createSelector(statementsSelector, state => {
});

export const selectStatements = createSelector(
statementsSelector,
sqlStatsSelector,
(_: AppState, props: RouteComponentProps) => props,
selectDiagnosticsReportsPerStatement,
(
state: StatementsState,
state: SQLStatsState,
props: RouteComponentProps<any>,
diagnosticsReportsPerStatement,
): AggregateStatistics[] => {
Expand Down Expand Up @@ -214,7 +205,7 @@ export const selectStatements = createSelector(
);

export const selectStatementsLastError = createSelector(
statementsSelector,
sqlStatsSelector,
state => state.lastError,
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,10 @@ import { Dispatch } from "redux";
import { Moment } from "moment";

import { AppState } from "src/store";
import { actions as statementsActions } from "src/store/statements";
import { actions as statementDiagnosticsActions } from "src/store/statementDiagnostics";
import { actions as analyticsActions } from "src/store/analytics";
import { actions as localStorageActions } from "src/store/localStorage";
import { actions as resetSQLStatsActions } from "src/store/sqlStats";
import { actions as sqlStatsActions } from "src/store/sqlStats";
import {
StatementsPage,
StatementsPageDispatchProps,
Expand Down Expand Up @@ -62,18 +61,18 @@ export const ConnectedStatementsPage = withRouter(
}),
(dispatch: Dispatch) => ({
refreshStatements: (req?: StatementsRequest) =>
dispatch(statementsActions.refresh(req)),
dispatch(sqlStatsActions.refresh(req)),
onDateRangeChange: (start: Moment, end: Moment) => {
dispatch(
statementsActions.updateDateRange({
sqlStatsActions.updateDateRange({
start: start.unix(),
end: end.unix(),
}),
);
},
refreshStatementDiagnosticsRequests: () =>
dispatch(statementDiagnosticsActions.refresh()),
resetSQLStats: () => dispatch(resetSQLStatsActions.request()),
resetSQLStats: () => dispatch(sqlStatsActions.reset()),
dismissAlertMessage: () =>
dispatch(
localStorageActions.update({
Expand Down
8 changes: 4 additions & 4 deletions pkg/ui/workspaces/cluster-ui/src/store/reducers.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,22 @@
import { assert } from "chai";
import { createStore } from "redux";
import { rootActions, rootReducer } from "./reducers";
import { actions as statementsActions } from "./statements";
import { actions as sqlStatsActions } from "./sqlStats";

describe("rootReducer", () => {
it("resets redux state on RESET_STATE action", () => {
const store = createStore(rootReducer);
const initState = store.getState();
const error = new Error("oops!");
store.dispatch(statementsActions.failed(error));
store.dispatch(sqlStatsActions.failed(error));
const changedState = store.getState();
store.dispatch(rootActions.resetState());
const resetState = store.getState();

assert.deepEqual(initState, resetState);
assert.notDeepEqual(
resetState.statements.lastError,
changedState.statements.lastError,
resetState.sqlStats.lastError,
changedState.sqlStats.lastError,
);
});
});
9 changes: 3 additions & 6 deletions pkg/ui/workspaces/cluster-ui/src/store/reducers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

import { combineReducers, createStore } from "redux";
import { createAction, createReducer } from "@reduxjs/toolkit";
import { StatementsState, reducer as statements } from "./statements";
import { LocalStorageState, reducer as localStorage } from "./localStorage";
import {
StatementDiagnosticsState,
Expand All @@ -19,24 +18,23 @@ import {
import { NodesState, reducer as nodes } from "./nodes";
import { LivenessState, reducer as liveness } from "./liveness";
import { SessionsState, reducer as sessions } from "./sessions";
import { TransactionsState, reducer as transactions } from "./transactions";
import {
TerminateQueryState,
reducer as terminateQuery,
} from "./terminateQuery";
import { UIConfigState, reducer as uiConfig } from "./uiConfig";
import { DOMAIN_NAME } from "./utils";
import { SQLStatsState, reducer as sqlStats } from "./sqlStats";

export type AdminUiState = {
statements: StatementsState;
statementDiagnostics: StatementDiagnosticsState;
localStorage: LocalStorageState;
nodes: NodesState;
liveness: LivenessState;
sessions: SessionsState;
transactions: TransactionsState;
terminateQuery: TerminateQueryState;
uiConfig: UIConfigState;
sqlStats: SQLStatsState;
};

export type AppState = {
Expand All @@ -46,13 +44,12 @@ export type AppState = {
export const reducers = combineReducers<AdminUiState>({
localStorage,
statementDiagnostics,
statements,
nodes,
liveness,
sessions,
transactions,
terminateQuery,
uiConfig,
sqlStats,
});

export const rootActions = {
Expand Down
4 changes: 0 additions & 4 deletions pkg/ui/workspaces/cluster-ui/src/store/sagas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,20 @@ import { all, fork } from "redux-saga/effects";

import { localStorageSaga } from "./localStorage";
import { statementsDiagnosticsSagas } from "./statementDiagnostics";
import { statementsSaga } from "./statements";
import { nodesSaga } from "./nodes";
import { livenessSaga } from "./liveness";
import { sessionsSaga } from "./sessions";
import { transactionsSaga } from "./transactions";
import { terminateSaga } from "./terminateQuery";
import { notifificationsSaga } from "./notifications";
import { sqlStatsSaga } from "./sqlStats";

export function* sagas(cacheInvalidationPeriod?: number) {
yield all([
fork(localStorageSaga),
fork(statementsSaga, cacheInvalidationPeriod),
fork(statementsDiagnosticsSagas, cacheInvalidationPeriod),
fork(nodesSaga, cacheInvalidationPeriod),
fork(livenessSaga, cacheInvalidationPeriod),
fork(sessionsSaga),
fork(transactionsSaga),
fork(notifificationsSaga),
fork(sqlStatsSaga),
]);
Expand Down
29 changes: 18 additions & 11 deletions pkg/ui/workspaces/cluster-ui/src/store/sqlStats/sqlStats.reducer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,26 +11,32 @@
import { createSlice, PayloadAction } from "@reduxjs/toolkit";
import { cockroach } from "@cockroachlabs/crdb-protobuf-client";
import { DOMAIN_NAME, noopReducer } from "../utils";
import { StatementsRequest } from "src/api/statementsApi";

type ResetSQLStatsResponse = cockroach.server.serverpb.ResetSQLStatsResponse;
type StatementsResponse = cockroach.server.serverpb.StatementsResponse;

export type ResetSQLStatsState = {
data: ResetSQLStatsResponse;
export type SQLStatsState = {
data: StatementsResponse;
lastError: Error;
valid: boolean;
};

const initialState: ResetSQLStatsState = {
const initialState: SQLStatsState = {
data: null,
lastError: null,
valid: true,
};

const resetSQLStatsSlice = createSlice({
name: `${DOMAIN_NAME}/resetsqlstats`,
export type UpdateDateRangePayload = {
start: number;
end: number;
};

const sqlStatsSlice = createSlice({
name: `${DOMAIN_NAME}/sqlstats`,
initialState,
reducers: {
received: (state, action: PayloadAction<ResetSQLStatsResponse>) => {
received: (state, action: PayloadAction<StatementsResponse>) => {
state.data = action.payload;
state.valid = true;
state.lastError = null;
Expand All @@ -42,10 +48,11 @@ const resetSQLStatsSlice = createSlice({
invalidated: state => {
state.valid = false;
},
// Define actions that don't change state
refresh: noopReducer,
request: noopReducer,
refresh: (_, action?: PayloadAction<StatementsRequest>) => {},
request: (_, action?: PayloadAction<StatementsRequest>) => {},
updateDateRange: (_, action: PayloadAction<UpdateDateRangePayload>) => {},
reset: _ => {},
},
});

export const { reducer, actions } = resetSQLStatsSlice;
export const { reducer, actions } = sqlStatsSlice;
Loading

0 comments on commit f35c4a8

Please sign in to comment.