ui: introduce schema insights page on db-console

[THardy98]

Resolves: #83828, #83829

This change introduces the schema insights page to the DB-Console.
Schema insights are fetched from schemaInsightsApi using the SQL-over
HTTP API and the corresponding component schemaInsightsView is
available from cluster-ui for future use in the CC console. The schema
insights page display a table of schema insights - currently different
types of index recommendations (i.e. drop/create/replace index
recommendations), with the intention to add different types of schema
insights in the future. Each schema insight row offers an actionable
button, offering the user the ability to execute the corresponding SQL
query that realizes their schema insight. Filters are available to
filter by database and the schema insight type, as well as search.

Loom: https://www.loom.com/share/29ac973730614968893c179f4974fc61
Updated Loom: https://www.loom.com/share/ee36842fa9594c8888523d9ce41e1607
(this demo shows a known bug in the duration formatting on the index details page when the duration is set to <1 hour, #85222)

Release note (ui change): Added new Schema Insights page to DB Console.
The Schema Insights page displays a table of schema insights - currently
different types of index recommendations (i.e. drop/create/replace index
recommendations). Each schema insight row offers the user the ability to
execute the corresponding SQL query that realizes their schema insight
via a clickable button. Filters are available to filter the surfaced
schema insights by database and insight type, as well as search.

Release justification: low risk, high benefit changes to existing
functionality

[cockroach-teamcity]

This change is 

[maryliag]

Reviewed 17 of 27 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @THardy98, and @xinhaoz)

---

pkg/ui/workspaces/cluster-ui/src/insights/types.ts line 83 at r1 (raw file):

export type SchemaInsightEventFilters = Omit<
  Filters,
  | "sqlType"

after you make the appNames optional, you can add it to the list

---

pkg/ui/workspaces/cluster-ui/src/insights/utils.ts line 157 at r1 (raw file):

    filteredSchemaInsights = filteredSchemaInsights.filter(
      schemaInsight =>
        schemaInsight.query?.includes(search) ||

you made the search lower case, but you also need to do all the other things you're comparing it to also lower case, so your search can be case insensitive

---

pkg/ui/workspaces/cluster-ui/src/insights/schemaInsights/indexUsageStatsRec.ts line 46 at r1 (raw file):

function formatDuration(duration: moment.Duration): string {
  //Get Days and subtract from duration

nit: add period to comments (a few others on the same file)

---

pkg/ui/workspaces/cluster-ui/src/insights/schemaInsights/schemaInsightsView.tsx line 156 at r1 (raw file):

  const countActiveFilters = calculateActiveFilters(filters);
  // TODO(thomas): do we want/need this? are we filtering by app ?

we're not filtering by app, you can remove all mentions of app on this page

---

pkg/ui/workspaces/cluster-ui/src/insights/schemaInsights/schemaInsightsView.tsx line 183 at r1 (raw file):

            onSubmitFilters={onSubmitFilters}
            // TODO(thomas): don't want to include app names?
            appNames={[]}

after you make the parameter optional, you can remove this

---

pkg/ui/workspaces/cluster-ui/src/queryFilter/filter.tsx line 37 at r1 (raw file):

  onSubmitFilters: (filters: Filters) => void;
  smth?: string;
  appNames: string[];

you can make this parameter optional, so you can hide it from your page

---

pkg/ui/workspaces/cluster-ui/src/store/localStorage/localStorage.reducer.ts line 85 at r1 (raw file):

const defaultFiltersSchemaInsights = {
  app: defaultFilters.app,

you won't have app filter

---

pkg/ui/workspaces/cluster-ui/src/store/schemaInsights/schemaInsights.selectors.ts line 15 at r1 (raw file):

export const selectSchemaInsights = createSelector(adminUISelector, adminUiState => {
  // TODO(thomas): return empty array or null?

empty array

---

pkg/ui/workspaces/db-console/src/redux/apiReducers.ts line 409 at r1 (raw file):

  clusterUiApi.getSchemaInsightEventState,
  "schemaInsights",
  moment.duration(5, "minute"),

considering the refresh will happen every 5min, make sure the issue that was happening on the sql activity pages is not happening here (where we had a loading showing up and no real updates)

---

pkg/ui/workspaces/db-console/src/views/insights/insightsSelectors.ts line 47 at r1 (raw file):

  InsightEventFilters
  >("filters/SchemaInsightsPage", (state: AdminUIState) => state.localSettings, {
    // TODO(thomas): remove ?

yes, you can remove

[THardy98]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @maryliag, and @xinhaoz)

---

pkg/ui/workspaces/cluster-ui/src/insights/types.ts line 83 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

after you make the appNames optional, you can add it to the list

Added the hideAppNames filter flag, added app to the omit list.

---

pkg/ui/workspaces/cluster-ui/src/insights/utils.ts line 157 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

you made the search lower case, but you also need to do all the other things you're comparing it to also lower case, so your search can be case insensitive

Lowercased all strings I'm comparing to.

---

pkg/ui/workspaces/cluster-ui/src/insights/schemaInsights/indexUsageStatsRec.ts line 46 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: add period to comments (a few others on the same file)

Done.

---

pkg/ui/workspaces/cluster-ui/src/insights/schemaInsights/schemaInsightsView.tsx line 156 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

we're not filtering by app, you can remove all mentions of app on this page

Removed all the TODOs initially added to this PR.
Removed the commented app filter logic.

---

pkg/ui/workspaces/cluster-ui/src/insights/schemaInsights/schemaInsightsView.tsx line 183 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

after you make the parameter optional, you can remove this

Hmm, originally we'd discussed this and opted to leave the appNames field as non-optional as the filter for schema insights was the only place where we wouldn't filter by app. Instead, we opted to add the hideAppNames flag to hide the app filter field.

Just want to double-check whether we want to make this optional or not.
Removed the comment.

---

pkg/ui/workspaces/cluster-ui/src/queryFilter/filter.tsx line 37 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

you can make this parameter optional, so you can hide it from your page

Ditto from previous comment on appNames optional/non-optional.

---

pkg/ui/workspaces/cluster-ui/src/store/localStorage/localStorage.reducer.ts line 85 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

you won't have app filter

Removed, added defaults for the schemaInsightType and database filters.

---

pkg/ui/workspaces/cluster-ui/src/store/schemaInsights/schemaInsights.selectors.ts line 15 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

empty array

Removed comment, returning empty array.

---

pkg/ui/workspaces/db-console/src/redux/apiReducers.ts line 409 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

considering the refresh will happen every 5min, make sure the issue that was happening on the sql activity pages is not happening here (where we had a loading showing up and no real updates)

Judging from the fix in the corresponding PR (https://github.com/cockroachdb/cockroach/pull/85772/files#diff-90aed7889a716bcd02ff73d766f11c8cf8420afc0cca421e780e2705ce7b5316), I think we can avoid the issue by simply removing the receivedSchemaInsightsSaga saga, unless I'm mistaken.

Followed the referenced PR for reducer changes (adding the lastUpdated field).

---

pkg/ui/workspaces/db-console/src/views/insights/insightsSelectors.ts line 47 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

yes, you can remove

Done.

[THardy98]

Branch should be in a good state. Some remaining items to do:

• page continually re-renders, not sure what's causing it (some troubleshooting & debugging required, shouldn't be a huge issue for anyone building on top of the page)
• needs tests (for indexUsageStatsRec.ts and schemaInsights.saga.ts in particular)
• add the time picker
• CSS to colour some description text blue in the table rows is being overridden by the row-wrapper class
• add table-level insights (this will likely be done in a follow-up PR)
• need an "empty" placeholder for schema insights table

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @maryliag, and @xinhaoz)

[maryliag]

Reviewed 1 of 27 files at r1, 5 of 12 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @maryliag, @THardy98, and @xinhaoz)

---

pkg/ui/workspaces/cluster-ui/src/insights/schemaInsights/schemaInsightsView.tsx line 183 at r1 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

Hmm, originally we'd discussed this and opted to leave the appNames field as non-optional as the filter for schema insights was the only place where we wouldn't filter by app. Instead, we opted to add the hideAppNames flag to hide the app filter field.

Just want to double-check whether we want to make this optional or not.
Removed the comment.

I think is worth making it optional, so you don't need to pass empty values that won't be used. To clarify, still keep the hideApps flag

[maryliag]

add the time picker

adding a time picker doesn't make sense on this page, you won't need to add that

add table-level insights

this is also not on the scope of this task and won't be added this release

page continually re-renders, not sure what's causing it (some troubleshooting & debugging)

maybe @xinhaoz can help out with this one, since she just fixed the issue on sql activity page, so could be similar

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @maryliag, @THardy98, and @xinhaoz)

[THardy98]

Added a loom to show its current state.
One bug I noticed while demoing:

• Navigation to the statement detail page from the "CREATE INDEX" recommendation is broken. We fetch the raw fingerprint_id in the query, but it looks like we need to transform it:
  

  cockroach/pkg/server/combined_statement_stats.go

  Line 182 in e1d36c1

  if statementFingerprintID, err = sqlstatsutil.DatumToUint64(row[0]); err != nil {

  
  When we encode the raw fingerprint_id, navigation to the statement details page crashes.

[maryliag]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @maryliag, @THardy98, and @xinhaoz)

---

pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts line 86 at r5 (raw file):

        statement: row.query,
        summary: row.querysummary,
        fingerprintID: row.fingerprint_id,

can you try replacing this with parseInt(row.fingerprint_id) to check if the link redirects to the right place

[ericharmeling]

Looks great! As you said the other day, there are a lot of patterns in common with the transaction workload page and API, so I don't have a lot of things to add.

A couple things to note (apologies if some of this is obvious or unhelpful 😅):

• I, for whatever reason, added all of the cluster-ui redux stuff to ui: add insights overview page v1 #84612, but none of that is tested or hooked up to a connected component in cluster-ui. Really just ground work for CC console. So IDK if you need to add tests for any of that (but couldn't hurt 🤷).
• I think I touch some of the same files here: ui: add workload insight details page v1 #86325. So there might be some conflicts to resolve.
• I think we should probably talk offline about having a follow-up PR where we consolidate some of the types that we are using across our components and in our SQL API files, since some of these are pretty much identical (e.g., SchemaInsightQuery and InsightQuery; InsightType and InsightNameEnum).

Reviewed 9 of 27 files at r1, 6 of 12 files at r2, 1 of 1 files at r3, 4 of 4 files at r4, 2 of 2 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag, @THardy98, and @xinhaoz)

---

pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts line 42 at r4 (raw file):

  | CreateIndexRecommendationsResponse;
type SchemaInsightQuery<RowType> = {
  name: InsightType;

FYI: I updated InsightQuery in https://github.com/cockroachdb/cockroach/pull/86325/files to pass the state type as a type argument, in case you wanted to have multiple state types? We could then reuse this type across schema and workload insight APIs. Work for another follow-up PR tho I think.

---

pkg/ui/workspaces/cluster-ui/src/insights/types.ts line 81 at r1 (raw file):

>;

export type SchemaInsightEventFilters = Omit<

nit: it might be better to Pick here instead of Omit with all these fields (I should do the same for InsightEventFilters)

[xinhaoz]

Reviewed 1 of 27 files at r1, 2 of 12 files at r2, 1 of 1 files at r3, 1 of 2 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @THardy98)

---

pkg/ui/workspaces/cluster-ui/src/insights/schemaInsights/indexUsageStatsRec.ts line 57 at r5 (raw file):

}

function formatDuration(duration: moment.Duration): string {

If the goal of this function was to make it more human readable, you might want to check out moment's humanize function. Feel free to ignore this comment if the formatting was meant to be this specific though.

---

pkg/ui/workspaces/db-console/src/redux/apiReducers.ts line 409 at r1 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

Judging from the fix in the corresponding PR (https://github.com/cockroachdb/cockroach/pull/85772/files#diff-90aed7889a716bcd02ff73d766f11c8cf8420afc0cca421e780e2705ce7b5316), I think we can avoid the issue by simply removing the receivedSchemaInsightsSaga saga, unless I'm mistaken.

Followed the referenced PR for reducer changes (adding the lastUpdated field).

Since we're polling in the component itself, I think we are safe to just set this as 'null', otherwise I'm worried about the syncing of the timeout from the cachedReducer obj and the interval.

[maryliag]

Reviewed 1 of 27 files at r1, 3 of 12 files at r2, 1 of 1 files at r3, 2 of 4 files at r4, 1 of 2 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @maryliag, @THardy98, and @xinhaoz)

---

pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts line 84 at r7 (raw file):

    row.index_recommendations.forEach(rec => {
      results.push({
        type: "CREATE_INDEX",

the type coming here won't always be a create, so you need to make a slight change:

let idxType: InsightType;
txn_result.rows.forEach(row => {
    row.index_recommendations.forEach(rec => {
      const t = rec.split(" : ")[0];

    switch (t) {
      case "creation":
        idxType = "CREATE_INDEX";
        break;
      case "replacement":
        idxType = "REPLACE_INDEX";
        break;
      case "drop":
        idxType = "DROP_INDEX";
        break;
    }

      results.push({
        type: idxType,
        database: row.db,
        execution: {
          statement: row.query,
          summary: row.querysummary,
          fingerprintID: HexStringToInt64String(row.fingerprint_id),
          implicit: row.implicittxn,
        },
        query: rec.split(" : ")[1],
      });
    });
  });

---

pkg/ui/workspaces/cluster-ui/src/store/schemaInsights/schemaInsights.reducer.ts line 14 at r5 (raw file):

import { DOMAIN_NAME, noopReducer } from "../utils";
import moment, { Moment } from "moment";
import {InsightRecommendation} from "../../insightsTable/insightsTable";

you will get lint error without space between {, so change to import { InsightRecommendations } from...

---

pkg/ui/workspaces/cluster-ui/src/store/schemaInsights/schemaInsights.reducer.ts line 27 at r5 (raw file):

  lastUpdated: null,
  lastError: null,
  valid: false,

isn't this suppose to be true?

[THardy98]

Although not used yet (until schema insights becomes a part of CC console), I've added schemaInsights.sagas.spec.ts, to test the schemaInsights saga.

Resolved the merge conflicts between the few insights PRs recently, moved some common types/utility functions to insights/utils.ts and insights/types.ts but like @ericharmeling mentioned, there's still work to unify these types/variables that would be nice in a follow-up PR.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @maryliag, and @xinhaoz)

---

pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts line 42 at r4 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

FYI: I updated InsightQuery in https://github.com/cockroachdb/cockroach/pull/86325/files to pass the state type as a type argument, in case you wanted to have multiple state types? We could then reuse this type across schema and workload insight APIs. Work for another follow-up PR tho I think.

Ah nice! Very reasonably looks like something that could be unified across the APIs in a follow-up PR.

---

pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts line 86 at r5 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

can you try replacing this with parseInt(row.fingerprint_id) to check if the link redirects to the right place

Using the new HexStringToInt64String method.

---

pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts line 84 at r7 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

the type coming here won't always be a create, so you need to make a slight change:

let idxType: InsightType;
txn_result.rows.forEach(row => {
    row.index_recommendations.forEach(rec => {
      const t = rec.split(" : ")[0];

    switch (t) {
      case "creation":
        idxType = "CREATE_INDEX";
        break;
      case "replacement":
        idxType = "REPLACE_INDEX";
        break;
      case "drop":
        idxType = "DROP_INDEX";
        break;
    }

      results.push({
        type: idxType,
        database: row.db,
        execution: {
          statement: row.query,
          summary: row.querysummary,
          fingerprintID: HexStringToInt64String(row.fingerprint_id),
          implicit: row.implicittxn,
        },
        query: rec.split(" : ")[1],
      });
    });
  });

Updated with this logic.

---

pkg/ui/workspaces/cluster-ui/src/insights/types.ts line 81 at r1 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

nit: it might be better to Pick here instead of Omit with all these fields (I should do the same for InsightEventFilters)

Using Pick instead of Omit.

---

pkg/ui/workspaces/cluster-ui/src/insights/schemaInsights/indexUsageStatsRec.ts line 57 at r5 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

If the goal of this function was to make it more human readable, you might want to check out moment's humanize function. Feel free to ignore this comment if the formatting was meant to be this specific though.

I tried using humanize but didn't like the ambiguity with some of these human-readable options (i.e. "a few seconds", or "over a minute", etc.). I ended up re-writing the logic anyway (previous logic didn't handle durations < 1 hour well) to use simple rounding and modulos.

---

pkg/ui/workspaces/cluster-ui/src/insights/schemaInsights/schemaInsightsView.tsx line 183 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

I think is worth making it optional, so you don't need to pass empty values that won't be used. To clarify, still keep the hideApps flag

Made appNames optional, kept the hideAppNames flag.

---

pkg/ui/workspaces/cluster-ui/src/store/schemaInsights/schemaInsights.reducer.ts line 14 at r5 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

you will get lint error without space between {, so change to import { InsightRecommendations } from...

Fixed all lint errors.

---

pkg/ui/workspaces/cluster-ui/src/store/schemaInsights/schemaInsights.reducer.ts line 27 at r5 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

isn't this suppose to be true?

I followed the reducer logic for sqlStats, which has sets the initial state of valid to false. My understanding is that we are delegating responsibility to the component to request/poll for schema insights, so it's up to the component to populate its corresponding redux state. As such, we can set the initial valid state to false, knowing that the component is responsible for making the request for this saga call that will lead to this being true (or error).

Either way, I'm not sure if the starting value of valid matters too much if the component makes a request on startup (but I could very well be wrong).

---

pkg/ui/workspaces/db-console/src/redux/apiReducers.ts line 409 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Since we're polling in the component itself, I think we are safe to just set this as 'null', otherwise we might be double calling.

Good call, set the timeout to null.

[maryliag]

Reviewed 13 of 23 files at r6, 9 of 14 files at r8, 1 of 1 files at r9, 8 of 8 files at r10, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling and @xinhaoz)

[THardy98]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @maryliag, and @xinhaoz)

---

-- commits line 16 at r11:

Previously, maryliag (Marylia Gutierrez) wrote…

can you add a little more info to help the docs team? Say what are the types on recommendations on this page

Added to the release note, more descriptive as to what is offered by the Schema Insights page.

---

pkg/ui/workspaces/cluster-ui/src/api/schemaInsightsApi.ts line 159 at r11 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

let's be a little conservative here, can you add a filter for the last week only? So
FROM crdb_internal.statement_statistics) WHERE aggregated_ts >= NOW() - INTERVAL '1 week'

Added.

[maryliag]

Awesome work on this page!

Reviewed 1 of 1 files at r12, 1 of 1 files at r13, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ericharmeling, @maryliag, and @xinhaoz)

[kevin-v-ngo]

Looks awesome! Small typos on the pop-up when clicking on CREATE and DROP index.

"....by executing a DROP INDEX statements..."

"statements" should be singular on for both pop-ups

[THardy98]

Looks awesome! Small typos on the pop-up when clicking on CREATE and DROP index.

"....by executing a DROP INDEX statements..."

"statements" should be singular on for both pop-ups

Fixed in #86747

[THardy98]

going to be bors-ing this, CI failures are all changefeedccl

[THardy98]

TYFR :)

[THardy98]

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[maryliag]

bors r-

[craig]

Canceled.

[THardy98]

Fixed merge conflicts, CI fails but on an unrelated streamingccl test. Going to merge this

[THardy98]

bors r+

[craig]

This PR was included in a batch that timed out, it will be automatically retried

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)