Awesome!! Some comments from me! Hopefully I looked at all the right files 😄

Reviewed 3 of 32 files at r1, 32 of 40 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling and @maryliag)

pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 40 at r2 (raw file):

                app_name
              FROM
                crdb_internal.transaction_contention_events AS tce

here you can add the WHERE to filter only the ones you care about, minimizing the joins you need to make

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

const HIGH_WAIT_CONTENTION_THRESHOLD = 2;

const isHighWait = (

I think the responsibility of deciding if this is high wait should be on the api, so it would only return the high wait ones and the UI doesn't have to worry about it.

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

export const InsightTypes = [
  { name: "highWaitTime", insight: highWaitTimeInsight, check: isHighWait },

if you follow my previous comment, then the check won't be necessary here

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

export type InsightEventFilters = Omit<
  ActiveStatementFilters,

it can be tricky to use the ActiveStatementFilters as a base, instead of Filters, because if you make a change on that one that you don't want to reflect here, people won't probably associate with active execution with the insights page or realize they will need to change here.
Probably safer to use Filters

Also, probably no for this PR since there is only one type, but we will need to introduce a filter for insight type

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

  const insights: Insight[] = [];
  InsightTypes.forEach(insight => {
    if (insight.check(insightState)) {

you won't need this check here anymore

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

    }
  });
  console.log(insights);

nit: don't forget to remove this :)

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

    const insightsForTxn = getInsights(txn);
    if (insightsForTxn.length < 1) {
      console.log("no insights for txn");

nit: this one too

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/sqlInsightsOverview.tsx line 26 at r2 (raw file):

  transactionInsightsProps,
}: SqlInsightsOverviewProps): React.ReactElement => {
  const sqlInsightsOptions: Option[] = [

the selection between statement and transaction is a dropdown select, not a bullet

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/sqlInsightsTable/emptyInsightsTablePlaceholder.tsx line 38 at r2 (raw file):

        title: "No insight events since this page was last refreshed.",
        icon: emptyTableResultsImg,
        message: "Insight events are cleared every hour.",

I'm not sure if that is the case for both the contention table and for the insights table. You might want to use something more generic or remove the "message" completely, leaving just the title which is enough for this case.

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/sqlInsightsTable/insightsColumns.tsx line 20 at r2 (raw file):

  insights: "Insights",
  startTime: "Start Time (UTC)",
  elapsedTime: "Elapsed Time (ms)",

don't add the ms to the column title, we have a formatter that adds the unit, and makes the adjustment for each case to decide between ms, s, ns, and so on

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/sqlInsightsTable/insightsColumns.tsx line 79 at r2 (raw file):

        content={
          <p>
            The category of insight identified as the cause of a slow {execType}{" "}

I don't think we can say is slow, because we will be adding different things that might be causing other issues besides being slow. Maybe something more generic:
The category of insight identifies for the {execType} execution.

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/transactionInsights/transactionInsightsTable.tsx line 41 at r2 (raw file):

        <Link to={`/insights/${item.executionID}`}>
          {String(item.executionID)}
        </Link>

while the details page is not created, better keep this as a label, so we don't have the state where it crashes when people click on the link. Once you create the details page, you can add the link back.

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/transactionInsights/transactionInsightsTable.tsx line 44 at r2 (raw file):

      ),
      sort: (item: TransactionInsight) => String(item.executionID),
      alwaysShow: true,

since you're not adding a column picker, you don't need the alwaysShow. That is used when the page has a column picker and we don't want that column to be on the list, meaning it can never be unselected.

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/transactionInsights/transactionInsightsTable.tsx line 51 at r2 (raw file):

      cell: (item: TransactionInsight) => item.query,
      sort: (item: TransactionInsight) => item.query,
      alwaysShow: true,

same as above

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/transactionInsights/transactionInsightsTable.tsx line 72 at r2 (raw file):

      name: "elapsedTime",
      title: insightsTableTitles.elapsedTime(execType),
      cell: (item: TransactionInsight) => `${item.elapsedTime} ms`,

use the utils function we have for Duration, which already takes care of the unit. Also, that function expects the value to be passed in nanoseconds, so make sure you're doing the right conversion

TFTR!

I did a lot of refactoring around the types and the SQL API to address your comments and to further accommodate adding statements and new insight types. At a certain point we can't reuse things though (like queries) until we have a dedicated insights table, so we'll have to either make new endpoints or make the one I added here a little more sophisticated. PTAL!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 40 at r2 (raw file):

only the ones you care about

I assume you are referring to the ones with a contention duration higher than a given threshold, correct?

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

Done.

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

Done.

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

it can be tricky to use the ActiveStatementFilters as a base, instead of Filters, because if you make a change on that one that you don't want to reflect here, people won't probably associate with active execution with the insights page or realize they will need to change here.
Probably safer to use Filters

Great point! Done. I also refactored some other dependencies on SqlActivity things (e.g., SQLActivityError)

Also, probably no for this PR since there is only one type, but we will need to introduce a filter for insight type

SGTM

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

Removed

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

😁 Removed.

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

Removed.

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/sqlInsightsOverview.tsx line 26 at r2 (raw file):

I created a small dropdown component to use as a new pageconfig for the execution type selection here.

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/sqlInsightsTable/emptyInsightsTablePlaceholder.tsx line 38 at r2 (raw file):

Removed the message.

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/sqlInsightsTable/insightsColumns.tsx line 20 at r2 (raw file):

Done.

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/sqlInsightsTable/insightsColumns.tsx line 79 at r2 (raw file):

Done.

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/transactionInsights/transactionInsightsTable.tsx line 41 at r2 (raw file):

Done.

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/transactionInsights/transactionInsightsTable.tsx line 44 at r2 (raw file):

Removed.

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/transactionInsights/transactionInsightsTable.tsx line 51 at r2 (raw file):

Removed.

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/transactionInsights/transactionInsightsTable.tsx line 72 at r2 (raw file):

Done.

Awesome improvements! Just a few comments from me!

Reviewed 1 of 40 files at r2, 13 of 19 files at r3, 6 of 8 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling and @maryliag)

pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 40 at r2 (raw file):

Yes, that's correct. You don't need to add this extra select, you can do the WHERE at the end.

SELECT
                 blocking_txn_id,
                 blocking_queries,
                 collection_ts,
                 contention_duration,
                 app_name
               FROM
                 crdb_internal.transaction_contention_events AS tce
                   JOIN (
                   SELECT
                     transaction_fingerprint_id,
                     app_name,
                     array_agg(metadata ->> 'query') AS blocking_queries
                   FROM
                     crdb_internal.statement_statistics
                   GROUP BY
                     transaction_fingerprint_id,
                     app_name
                 ) AS bqs ON bqs.transaction_fingerprint_id = tce.blocking_txn_fingerprint_id
             WHERE
                     contention_duration > INTERVAL '${HIGH_WAIT_CONTENTION_THRESHOLD.toISOString()}'

if you ran both this and the one you have with EXPLAIN SELECT...., you will see it generates the same plan

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

// The only insight we currently report is "High Wait Time", which is the insight
// associated with each row in the crdb_internal.transaction_contention_events table

nit: add period to comments

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/sqlInsightsTable/insightTable.module.scss line 32 at r5 (raw file):

  color: $colors--link;
  display: flex;
  line-height: 24px;

you can use $line-height--medium here

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/transactionInsights/transactionInsightsView.tsx line 191 at r5 (raw file):

      <PageConfig>
        <PageConfigItem>
          <DropDownSelect

since this is not changing the data, comment this part out and as soon as we have statements data to show, we can add it back with the proper handle.
You can comment and add a TODO

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 40 at r2 (raw file):

Ooo very nice. Done.

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

Done.

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/sqlInsightsTable/insightTable.module.scss line 32 at r5 (raw file):

Done

pkg/ui/workspaces/cluster-ui/src/insights/sqlInsights/transactionInsights/transactionInsightsView.tsx line 191 at r5 (raw file):

Done.

Great! You probably will need to wait for Xin Hao PR to get merged in first, and do the rename Kevin mentioned, otherwise

Reviewed 4 of 4 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @maryliag)

Great work on this! Just want to make sure, is your loom the most up to date state of the page?
Just had a few nits

Reviewed 15 of 15 files at r7, 19 of 25 files at r8, 1 of 5 files at r9, 14 of 14 files at r10, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ericharmeling and @maryliag)

pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/workloadInsightsError.tsx line 21 at r10 (raw file):

};

export const WorkloadInsightsError = (

can't you just use the errorComponent we already have? It also has the statsType that you can just pass the stmt/txn text

pkg/ui/workspaces/cluster-ui/src/store/insights/insights.reducer.ts line 47 at r10 (raw file):

      state.valid = false;
    },
    // Define actions that don't change state

nit: period

Just want to make sure, is your loom the most up to date state of the page?

Just updated the loom link in that first comment (https://www.loom.com/share/2682ad5c9b00439d986a596df2d5f098).

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @maryliag)

pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/workloadInsightsError.tsx line 21 at r10 (raw file):
I originally was using the SQLActivityError component, but decided to make one for workload insights so as not to have a dependency on a SQLActivity component, following our earlier exchange.

it can be tricky to use the ActiveStatementFilters as a base, instead of Filters, because if you make a change on that one that you don't want to reflect here, people won't probably associate with active execution with the insights page or realize they will need to change here.
Probably safer to use Filters

Great point! Done. I also refactored some other dependencies on SqlActivity things (e.g., SQLActivityError)

I can definitely change it back though if you think that's the better move.

pkg/ui/workspaces/cluster-ui/src/store/insights/insights.reducer.ts line 47 at r10 (raw file):

Done.

Just one small nit that I just realize about the search, otherwise

Reviewed 1 of 25 files at r8, 2 of 2 files at r11, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ericharmeling and @maryliag)

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

      txn =>
        !search ||
        txn.executionID?.includes(search) ||

we want the search to be case insensitive, so add toLowerCase to the search, txn.executionID and query

pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/workloadInsightsError.tsx line 21 at r10 (raw file):

I didn't realize that one was on the sqlActivity. Never mind, you can keep this one