Skip to content

Commit

Permalink
ui: use cluster setting from redux on schema insights
Browse files Browse the repository at this point in the history
Part Of #108373

Use the value of the cluster setting
`sql.index_recommendation.drop_unused_duration` from redux,
instead of adding as part of the select.
With this change, now users with VIEWACTIVITY or
VIEWACTIVITYREDACTED can see index recommendations on the console,
without the need the view cluster settings permission.

This commit changes the Schema Insights Api.

Release note (ui change): Users without `VIEWCLUSTERSETTINGS` permission
but with `VIEWACTIVITY` or `VIEWACTIVITYREDACTED` can now see
index recommendations.
  • Loading branch information
maryliag committed Aug 24, 2023
1 parent ad2dcb4 commit bc9e1b0
Show file tree
Hide file tree
Showing 23 changed files with 178 additions and 93 deletions.
3 changes: 2 additions & 1 deletion pkg/ui/workspaces/cluster-ui/src/api/databaseDetailsApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { Format, Identifier, QualifiedIdentifier } from "./safesql";
import moment from "moment-timezone";
import { fromHexString, withTimeout } from "./util";
import { cockroach } from "@cockroachlabs/crdb-protobuf-client";
import { getLogger } from "../util";
import { getLogger, indexUnusedDuration } from "../util";

const { ZoneConfig } = cockroach.config.zonepb;
const { ZoneConfigurationLevel } = cockroach.server.serverpb;
Expand Down Expand Up @@ -413,6 +413,7 @@ type DatabaseIndexUsageStatsResponse = {

const getDatabaseIndexUsageStats: DatabaseDetailsQuery<IndexUsageStatistic> = {
createStmt: (dbName: string, csIndexUnusedDuration: string) => {
csIndexUnusedDuration = csIndexUnusedDuration ?? indexUnusedDuration;
return {
sql: Format(
`SELECT * FROM (SELECT
Expand Down
43 changes: 27 additions & 16 deletions pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {
InsightType,
recommendDropUnusedIndex,
} from "../insights";
import { HexStringToInt64String } from "../util";
import { HexStringToInt64String, indexUnusedDuration } from "../util";
import { QuoteIdentifier } from "./safesql";

// Export for db-console import from clusterUiApi.
Expand Down Expand Up @@ -51,12 +51,16 @@ type CreateIndexRecommendationsResponse = {
index_recommendations: string[];
};

export type SchemaInsightReqParams = {
csIndexUnusedDuration: string;
};

type SchemaInsightResponse =
| ClusterIndexUsageStatistic
| CreateIndexRecommendationsResponse;
type SchemaInsightQuery<RowType> = {
name: InsightType;
query: string;
query: string | ((csIndexUnusedDuration: string) => string);
toSchemaInsight: (response: SqlTxnResult<RowType>) => InsightRecommendation[];
};

Expand Down Expand Up @@ -142,12 +146,9 @@ function createIndexRecommendationsToSchemaInsight(
// and want to return the most used ones as a priority.
const dropUnusedIndexQuery: SchemaInsightQuery<ClusterIndexUsageStatistic> = {
name: "DropIndex",
query: `WITH cs AS (
SELECT value
FROM crdb_internal.cluster_settings
WHERE variable = 'sql.index_recommendation.drop_unused_duration'
)
SELECT * FROM (SELECT us.table_id,
query: (csIndexUnusedDuration: string) => {
csIndexUnusedDuration = csIndexUnusedDuration ?? indexUnusedDuration;
return `SELECT * FROM (SELECT us.table_id,
us.index_id,
us.last_read,
us.total_reads,
Expand All @@ -157,18 +158,18 @@ const dropUnusedIndexQuery: SchemaInsightQuery<ClusterIndexUsageStatistic> = {
t.parent_id as database_id,
t.database_name,
t.schema_name,
cs.value as unused_threshold,
cs.value::interval as interval_threshold,
'${csIndexUnusedDuration}' as unused_threshold,
'${csIndexUnusedDuration}'::interval as interval_threshold,
now() - COALESCE(us.last_read AT TIME ZONE 'UTC', COALESCE(ti.created_at, '0001-01-01')) as unused_interval
FROM "".crdb_internal.index_usage_statistics AS us
JOIN "".crdb_internal.table_indexes as ti
ON us.index_id = ti.index_id AND us.table_id = ti.descriptor_id
JOIN "".crdb_internal.tables as t
ON t.table_id = ti.descriptor_id and t.name = ti.descriptor_name
CROSS JOIN cs
WHERE t.database_name != 'system' AND ti.is_unique IS false)
WHERE unused_interval > interval_threshold
ORDER BY total_reads DESC;`,
ORDER BY total_reads DESC;`;
},
toSchemaInsight: clusterIndexUsageStatsToSchemaInsight,
};

Expand Down Expand Up @@ -211,14 +212,24 @@ const schemaInsightQueries: SchemaInsightQuery<SchemaInsightResponse>[] = [
createIndexRecommendationsQuery,
];

function getQuery(
csIndexUnusedDuration: string,
query: string | ((csIndexUnusedDuration: string) => string),
): string {
if (typeof query == "string") {
return query;
}
return query(csIndexUnusedDuration);
}

// getSchemaInsights makes requests over the SQL API and transforms the corresponding
// SQL responses into schema insights.
export async function getSchemaInsights(): Promise<
SqlApiResponse<InsightRecommendation[]>
> {
export async function getSchemaInsights(
params: SchemaInsightReqParams,
): Promise<SqlApiResponse<InsightRecommendation[]>> {
const request: SqlExecutionRequest = {
statements: schemaInsightQueries.map(insightQuery => ({
sql: insightQuery.query,
sql: getQuery(params.csIndexUnusedDuration, insightQuery.query),
})),
execute: true,
max_result_size: LARGE_RESULT_SIZE,
Expand Down
3 changes: 2 additions & 1 deletion pkg/ui/workspaces/cluster-ui/src/api/tableDetailsApi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import { fromHexString, withTimeout } from "./util";
import { Format, Identifier, Join, SQL } from "./safesql";
import { cockroach } from "@cockroachlabs/crdb-protobuf-client";
import { IndexUsageStatistic, recommendDropUnusedIndex } from "../insights";
import { getLogger } from "../util";
import { getLogger, indexUnusedDuration } from "../util";

const { ZoneConfig } = cockroach.config.zonepb;
const { ZoneConfigurationLevel } = cockroach.server.serverpb;
Expand Down Expand Up @@ -481,6 +481,7 @@ const getTableIndexUsageStats: TableDetailsQuery<IndexUsageStatistic> = {
[new Identifier(dbName), new SQL(tableName)],
new SQL("."),
);
csIndexUnusedDuration = csIndexUnusedDuration ?? indexUnusedDuration;
return {
sql: Format(
`WITH tableId AS (SELECT $1::regclass::int as table_id)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,15 @@ import {
import * as H from "history";
import moment from "moment-timezone";
import { defaultFilters } from "src/queryFilter";
import { indexUnusedDuration } from "src/util/constants";
const history = H.createHashHistory();

const withLoadingIndicator: DatabaseDetailsPageProps = {
loading: true,
loaded: false,
lastError: undefined,
showIndexRecommendations: false,
csIndexUnusedDuration: "168h",
csIndexUnusedDuration: indexUnusedDuration,
name: randomName(),
tables: [],
viewMode: ViewMode.Tables,
Expand Down Expand Up @@ -69,7 +70,7 @@ const withoutData: DatabaseDetailsPageProps = {
loaded: true,
lastError: null,
showIndexRecommendations: false,
csIndexUnusedDuration: "168h",
csIndexUnusedDuration: indexUnusedDuration,
name: randomName(),
tables: [],
viewMode: ViewMode.Tables,
Expand Down Expand Up @@ -131,7 +132,7 @@ const withData: DatabaseDetailsPageProps = {
loaded: true,
lastError: null,
showIndexRecommendations: true,
csIndexUnusedDuration: "168h",
csIndexUnusedDuration: indexUnusedDuration,
name: randomName(),
tables: [createTable()],
viewMode: ViewMode.Tables,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
import { DatabaseTablePage, DatabaseTablePageProps } from "./databaseTablePage";
import moment from "moment-timezone";
import * as H from "history";
import { indexUnusedDuration } from "src/util/constants";
const history = H.createHashHistory();

const withLoadingIndicator: DatabaseTablePageProps = {
Expand All @@ -30,7 +31,7 @@ const withLoadingIndicator: DatabaseTablePageProps = {
indexUsageStatsEnabled: false,
showIndexRecommendations: false,
automaticStatsCollectionEnabled: true,
csIndexUnusedDuration: "168h",
csIndexUnusedDuration: indexUnusedDuration,
details: {
loading: true,
loaded: false,
Expand Down Expand Up @@ -77,7 +78,7 @@ const withData: DatabaseTablePageProps = {
indexUsageStatsEnabled: true,
showIndexRecommendations: true,
automaticStatsCollectionEnabled: true,
csIndexUnusedDuration: "168h",
csIndexUnusedDuration: indexUnusedDuration,
details: {
loading: false,
loaded: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import { DatabasesPage, DatabasesPageProps } from "./databasesPage";

import * as H from "history";
import { defaultFilters } from "src/queryFilter";
import { indexUnusedDuration } from "src/util/constants";
const history = H.createHashHistory();

const withLoadingIndicator: DatabasesPageProps = {
Expand All @@ -26,7 +27,7 @@ const withLoadingIndicator: DatabasesPageProps = {
lastError: undefined,
automaticStatsCollectionEnabled: true,
indexRecommendationsEnabled: false,
csIndexUnusedDuration: "168h",
csIndexUnusedDuration: indexUnusedDuration,
databases: [],
sortSetting: {
ascending: false,
Expand Down Expand Up @@ -55,7 +56,7 @@ const withoutData: DatabasesPageProps = {
lastError: null,
automaticStatsCollectionEnabled: true,
indexRecommendationsEnabled: false,
csIndexUnusedDuration: "168h",
csIndexUnusedDuration: indexUnusedDuration,
databases: [],
sortSetting: {
ascending: false,
Expand Down Expand Up @@ -85,7 +86,7 @@ const withData: DatabasesPageProps = {
showNodeRegionsColumn: true,
automaticStatsCollectionEnabled: true,
indexRecommendationsEnabled: true,
csIndexUnusedDuration: "168h",
csIndexUnusedDuration: indexUnusedDuration,
sortSetting: {
ascending: false,
columnTitle: "name",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

import { indexUnusedDuration } from "src/util/constants";
import { SchemaInsightsViewProps } from "./schemaInsightsView";

export const SchemaInsightsPropsFixture: SchemaInsightsViewProps = {
Expand Down Expand Up @@ -71,6 +72,7 @@ export const SchemaInsightsPropsFixture: SchemaInsightsViewProps = {
schemaInsightType: "",
},
hasAdminRole: true,
csIndexUnusedDuration: indexUnusedDuration,
refreshSchemaInsights: () => {},
onSortChange: () => {},
onFiltersChange: () => {},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { actions as localStorageActions } from "../../store/localStorage";
import { Dispatch } from "redux";
import { selectHasAdminRole } from "../../store/uiConfig";
import { actions as analyticsActions } from "../../store/analytics";
import { selectDropUnusedIndexDuration } from "src/store/clusterSettings/clusterSettings.selectors";

const mapStateToProps = (
state: AppState,
Expand All @@ -45,6 +46,7 @@ const mapStateToProps = (
sortSetting: selectSortSetting(state),
hasAdminRole: selectHasAdminRole(state),
maxSizeApiReached: selectSchemaInsightsMaxApiSizeReached(state),
csIndexUnusedDuration: selectDropUnusedIndexDuration(state),
});

const mapDispatchToProps = (
Expand Down Expand Up @@ -82,8 +84,8 @@ const mapDispatchToProps = (
}),
);
},
refreshSchemaInsights: () => {
dispatch(actions.refresh());
refreshSchemaInsights: (csIndexUnusedDuration: string) => {
dispatch(actions.refresh({ csIndexUnusedDuration }));
},
refreshUserSQLRoles: () => dispatch(uiConfigActions.refreshUserSQLRoles()),
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,14 @@ export type SchemaInsightsViewStateProps = {
filters: SchemaInsightEventFilters;
sortSetting: SortSetting;
hasAdminRole: boolean;
csIndexUnusedDuration: string;
maxSizeApiReached?: boolean;
};

export type SchemaInsightsViewDispatchProps = {
onFiltersChange: (filters: SchemaInsightEventFilters) => void;
onSortChange: (ss: SortSetting) => void;
refreshSchemaInsights: () => void;
refreshSchemaInsights: (csIndexUnusedDuration: string) => void;
refreshUserSQLRoles: () => void;
};

Expand All @@ -83,6 +84,7 @@ export const SchemaInsightsView: React.FC<SchemaInsightsViewProps> = ({
onFiltersChange,
onSortChange,
maxSizeApiReached,
csIndexUnusedDuration,
}: SchemaInsightsViewProps) => {
const isCockroachCloud = useContext(CockroachCloudContext);
const [pagination, setPagination] = useState<ISortedTablePagination>({
Expand All @@ -95,13 +97,17 @@ export const SchemaInsightsView: React.FC<SchemaInsightsViewProps> = ({
);

useEffect(() => {
const refreshSchema = (): void => {
refreshSchemaInsights(csIndexUnusedDuration);
};

// Refresh every 1 minute.
refreshSchemaInsights();
const interval = setInterval(refreshSchemaInsights, 60 * 1000);
refreshSchema();
const interval = setInterval(refreshSchema, 60 * 1000);
return () => {
clearInterval(interval);
};
}, [refreshSchemaInsights]);
}, [refreshSchemaInsights, csIndexUnusedDuration]);

useEffect(() => {
// Refresh every 5 minutes.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
// licenses/APL.txt.

import { AppState } from "../reducers";
import { greaterOrEqualThanVersion } from "../../util";
import { greaterOrEqualThanVersion, indexUnusedDuration } from "../../util";

export const selectAutomaticStatsCollectionEnabled = (
state: AppState,
Expand Down Expand Up @@ -43,9 +43,10 @@ export const selectIndexUsageStatsEnabled = (state: AppState): boolean => {
export const selectDropUnusedIndexDuration = (state: AppState): string => {
const settings = state.adminUI?.clusterSettings.data?.key_values;
if (!settings) {
return "168h";
return indexUnusedDuration;
}
return (
settings["sql.index_recommendation.drop_unused_duration"]?.value || "168h"
settings["sql.index_recommendation.drop_unused_duration"]?.value ||
indexUnusedDuration
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,11 @@ import {
KeyedDatabaseDetailsState,
reducer,
} from "./databaseDetails.reducer";
import { DatabasesListState, refreshDatabasesListSaga } from "../databasesList";
import { indexUnusedDuration } from "src/util/constants";

describe("DatabaseDetails sagas", () => {
const database = "test_db";
const csIndexUnusedDuration = "168h";
const csIndexUnusedDuration = indexUnusedDuration;
const requestAction: PayloadAction<DatabaseDetailsReqParams> = {
payload: { database, csIndexUnusedDuration },
type: "request",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,14 @@ import {
reducer,
} from "./tableDetails.reducer";
import moment from "moment";
import { generateTableID } from "../../util";
import { generateTableID, indexUnusedDuration } from "../../util";

describe("TableDetails sagas", () => {
const database = "test_db";
const table = "test_table";
const csIndexUnusedDuration = "168h";
const key = generateTableID(database, table);
const requestAction: PayloadAction<TableDetailsReqParams> = {
payload: { database, table, csIndexUnusedDuration },
payload: { database, table, csIndexUnusedDuration: indexUnusedDuration },
type: "request",
};
const tableDetailsResponse: SqlApiResponse<TableDetailsResponse> = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@
// licenses/APL.txt.

import { createSlice, PayloadAction } from "@reduxjs/toolkit";
import { DOMAIN_NAME, noopReducer } from "../utils";
import { DOMAIN_NAME } from "../utils";
import moment, { Moment } from "moment-timezone";
import { InsightRecommendation } from "../../insights";
import { SqlApiResponse } from "src/api";
import { SchemaInsightReqParams, SqlApiResponse } from "src/api";

export type SchemaInsightsState = {
data: SqlApiResponse<InsightRecommendation[]>;
Expand Down Expand Up @@ -50,9 +50,8 @@ const schemaInsightsSlice = createSlice({
state.valid = false;
state.lastUpdated = moment.utc();
},
// Define actions that don't change state.
refresh: noopReducer,
request: noopReducer,
refresh: (_, _action: PayloadAction<SchemaInsightReqParams>) => {},
request: (_, _action: PayloadAction<SchemaInsightReqParams>) => {},
},
});

Expand Down
Loading

0 comments on commit bc9e1b0

Please sign in to comment.