ui: add execution insights to statement and transaction fingerprint details #96440

ericharmeling · 2023-02-02T16:34:48Z

This commit adds execution insights to the Statement Fingerprint and Transaction Fingerprint Details pages.

Part of #83780.

Loom: https://www.loom.com/share/98d2023b672e43fa8016829aa641a829

Note that the SQL queries against the *_execution_insights tables are updated to SELECT DISTINCT ON (*_fingerprint_id, problems, causes) (equivalent to GROUP BY (*_fingerprint_id, problems, causes)) from the latest results in the tables, rather than row_number() OVER ( PARTITION BY stmt_fingerprint_id, problem, causes ORDER BY end_time DESC ) AS rank... WHERE rank = 1. Both patterns return the same result, but one uses aggregation and the other uses a window function. I find the DISTINCT ON/GROUP BY pattern easier to understand, I'm not seeing much difference in the planning/execution time between the two over the same set of data, and I'm seeing DISTINCT ON/GROUP BY coming up as more performant in almost all the secondary sources I've encountered.

Release note (ui change): Added execution insights to the Statement Fingerprint Details and Transaction Fingerprint Details Pages.

cockroach-teamcity · 2023-02-02T16:35:01Z

This change is

gtr

Looking good :) Just some nits and a couple suggestions from me.

Reviewed 37 of 37 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling)

pkg/ui/workspaces/cluster-ui/src/api/stmtInsightsApi.ts line 129 at r1 (raw file):

  return `
SELECT ${stmtColumns} FROM

nit: trailing space

pkg/ui/workspaces/cluster-ui/src/api/txnInsightsApi.ts line 464 at r1 (raw file):

      *
    FROM
      ${TXN_INSIGHTS_TABLE_NAME}

nit: trailing space

pkg/ui/workspaces/cluster-ui/src/api/txnInsightsApi.ts line 517 at r1 (raw file):

  };
  filters.execID = req?.txnExecutionID ? req.txnExecutionID : null;
  filters.fingerprintID = req?.txnFingerprintID ? req.txnFingerprintID : null;

Nit suggestion but can we move these expressions inside the above block like so?

  const filters: TxnQueryFilters = {
    start: req?.start,
    end: req?.end,
    execID: req?.txnExecutionID ? req.txnExecutionID : null,
    fingerprintID: req?.txnFingerprintID ? req.txnFingerprintID : null,
  };

Code quote:

  filters.execID = req?.txnExecutionID ? req.txnExecutionID : null;
  filters.fingerprintID = req?.txnFingerprintID ? req.txnFingerprintID : null;

pkg/ui/workspaces/cluster-ui/src/insightsTable/insightsTable.tsx line 83 at r1 (raw file):

        placement="bottom"
        content={
          "The latest execution of the statement fingerprint with an insight within the selected time period."

The Loom video shows an ID in this column - should this tooltip say The latest execution ID...?

Also, I'm not entirely sure if the ...within the selected time period. bit is necessary since I haven't seen any other tooltips specify this but perhaps others can chime in here.

pkg/ui/workspaces/cluster-ui/src/store/insights/statementFingerprintInsights/index.ts line 1 at r1 (raw file):

// Copyright 2022 The Cockroach Authors.

nit: 2023

pkg/ui/workspaces/cluster-ui/src/store/insights/statementFingerprintInsights/statementFingerprintInsights.reducer.ts line 1 at r1 (raw file):

// Copyright 2022 The Cockroach Authors.

nit: 2023

pkg/ui/workspaces/cluster-ui/src/store/insights/statementFingerprintInsights/statementFingerprintInsights.sagas.ts line 1 at r1 (raw file):

// Copyright 2022 The Cockroach Authors.

nit: 2023

pkg/ui/workspaces/cluster-ui/src/store/insights/statementFingerprintInsights/statementFingerprintInsights.selectors.ts line 1 at r1 (raw file):

// Copyright 2022 The Cockroach Authors.

nit: 2023

ericharmeling

TFTR! I think I got to all of your comments.

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

pkg/ui/workspaces/cluster-ui/src/api/stmtInsightsApi.ts line 129 at r1 (raw file):

Previously, gtr (Gerardo Torres Castro) wrote…

nit: trailing space

Done.

pkg/ui/workspaces/cluster-ui/src/api/txnInsightsApi.ts line 464 at r1 (raw file):

Previously, gtr (Gerardo Torres Castro) wrote…

nit: trailing space

Done.

pkg/ui/workspaces/cluster-ui/src/api/txnInsightsApi.ts line 517 at r1 (raw file):

Previously, gtr (Gerardo Torres Castro) wrote…

Nit suggestion but can we move these expressions inside the above block like so?

  const filters: TxnQueryFilters = {
    start: req?.start,
    end: req?.end,
    execID: req?.txnExecutionID ? req.txnExecutionID : null,
    fingerprintID: req?.txnFingerprintID ? req.txnFingerprintID : null,
  };

Definitely! Done.

pkg/ui/workspaces/cluster-ui/src/insightsTable/insightsTable.tsx line 83 at r1 (raw file):

Previously, gtr (Gerardo Torres Castro) wrote…

The Loom video shows an ID in this column - should this tooltip say The latest execution ID...?

Also, I'm not entirely sure if the ...within the selected time period. bit is necessary since I haven't seen any other tooltips specify this but perhaps others can chime in here.

Done.

pkg/ui/workspaces/cluster-ui/src/store/insights/statementFingerprintInsights/index.ts line 1 at r1 (raw file):

Previously, gtr (Gerardo Torres Castro) wrote…

nit: 2023

Done

pkg/ui/workspaces/cluster-ui/src/store/insights/statementFingerprintInsights/statementFingerprintInsights.reducer.ts line 1 at r1 (raw file):

Previously, gtr (Gerardo Torres Castro) wrote…

nit: 2023

Done

pkg/ui/workspaces/cluster-ui/src/store/insights/statementFingerprintInsights/statementFingerprintInsights.sagas.ts line 1 at r1 (raw file):

Previously, gtr (Gerardo Torres Castro) wrote…

nit: 2023

Done.

pkg/ui/workspaces/cluster-ui/src/store/insights/statementFingerprintInsights/statementFingerprintInsights.selectors.ts line 1 at r1 (raw file):

Previously, gtr (Gerardo Torres Castro) wrote…

nit: 2023

Done.

maryliag

can you should the network tab? I want to confirm the cache is working and it's not spamming refresh calls

Reviewed 24 of 37 files at r1, 10 of 10 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling and @gtr)

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

export const LONG_TIMEOUT = "300s";
export const LARGE_RESULT_SIZE = 50000; // 50 kib
export const VERY_LARGE_RESULT_SIZE = 500000; // 500 kib

I don't see this used

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

};

export const selectStatementInsightDetailsCombinerByFingerprint = (

did you mean CombineD?

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx line 666 at r2 (raw file):

    const isCockroachCloud = useContext(CockroachCloudContext);
    const insightsColumns = useMemo(
      () => makeInsightsColumns(isCockroachCloud, false, false),

the second false should not be hard coded, you should use hasAdminRole that already exists on this file

pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetails.tsx line 453 at r2 (raw file):

            );

            const insightsColumns = makeInsightsColumns(true, false, false);

don't hard code the values here. Get the real value for cockroach cloud and hasAdminRole

ericharmeling

can you should the network tab? I want to confirm the cache is working and it's not spamming refresh calls

Sure thing!

https://www.loom.com/share/89bab56b76a446fc9b2ca81775317e52

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

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

Previously, maryliag (Marylia Gutierrez) wrote…

did you mean CombineD?

I'm using the naming from the selector above this and below this (selectStatementInsightDetailsCombiner and selectTxnInsightDetailsCombiner).

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx line 666 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

the second false should not be hard coded, you should use hasAdminRole that already exists on this file

Done.

pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetails.tsx line 453 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

don't hard code the values here. Get the real value for cockroach cloud and hasAdminRole

Done.

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

Previously, maryliag (Marylia Gutierrez) wrote…

I don't see this used

Removed.

ericharmeling

FWIW, the changes in this PR do not work on CRDB versions without 06838a1

The SQL API call returns the following error:

code: "42703", message: "executing stmt 1: run-query-via-api: column \"cpu_sql_nanos\" does not exist", severity: "ERROR", …

But the page won't crash. There just won't be any insights on any fingerprint's details page.

There are also some cryptic jest failures that I need to investigate.

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

maryliag

Nice!

Once you get tests passing (you might need a rebase):

Reviewed 1 of 37 files at r1, 7 of 7 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @gtr)

xinhaoz · 2023-02-07T22:38:54Z

pkg/ui/workspaces/cluster-ui/src/api/txnInsightsApi.ts

+    start: req?.start,
+    end: req?.end,
+    execID: req?.txnExecutionID ? req.txnExecutionID : null,
+    fingerprintID: req?.txnFingerprintID ? req.txnFingerprintID : null,


nit: This can be simplified to req?.txFingerprintID. You don't need the ternary operator here, the ? will make the evaluated value null

xinhaoz · 2023-02-07T22:41:18Z

pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/transactionInsightDetails.tsx

@@ -75,7 +75,7 @@ export const TransactionInsightDetails: React.FC<

  useEffect(() => {
    const stmtsComplete =
-      stmts != null && stmts.length === txnDetails?.stmtExecutionIDs?.length;
+      stmts != null && stmts?.length === txnDetails?.stmtExecutionIDs?.length;


nit: You don't need to add the ? to the second part of the && here. If stmts is nullish the stmts != null && will ensure the part after && doesn't get read.

xinhaoz · 2023-02-07T22:44:04Z

pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/queriesCell.tsx

-    (transactionQueries.length === 1 &&
-      transactionQueries[0].length < textLimit)
+    !transactionQueries?.length ||
+    (transactionQueries?.length === 1 &&


Ditto for the addition of ? here. !transactionQueries?.length will ensure that the part after || means that transactionQueries has a non-zero length.

xinhaoz · 2023-02-07T22:45:55Z

pkg/ui/workspaces/cluster-ui/src/selectors/insightsCommon.selectors.ts

+  statementInsights: StmtInsightEvent[],
+  fingerprintID: string,
+): StmtInsightEvent[] | null => {
+  if (!statementInsights || statementInsights?.length < 1 || !fingerprintID) {


nit: can be condensed to (!statementInsights?.length || !fingerprintID)

xinhaoz · 2023-02-07T22:46:34Z

pkg/ui/workspaces/cluster-ui/src/selectors/insightsCommon.selectors.ts

+    return null;
+  }
+  const insightEvents: StmtInsightEvent[] = [];
+  statementInsights.forEach(insight => {


nit: Try replacing this with an aray.filter, and you can set the var above directly.

xinhaoz · 2023-02-07T22:49:26Z

pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetails.tsx

@@ -508,6 +558,31 @@ export class TransactionDetails extends React.Component<
                      </SummaryCard>
                    </Col>
                  </Row>
+                  {tableData != null && tableData?.length > 0 && (


nit: tableData?.length &&

xinhaoz · 2023-02-07T23:02:34Z

pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetails.tsx

+            );
+            const tableData: InsightRecommendation[] = [];
+            if (transactionInsights) {
+              const tableDataTypes: InsightType[] = [];


nit: if you're using this to track the insight types seen already, a set is prob preferred (although the array size will be small so it's probably not a big deal 🤷 )

ericharmeling

TFTR @xinhaoz ! I think I got to all of your comments.

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

pkg/ui/workspaces/cluster-ui/src/api/txnInsightsApi.ts line 516 at r3 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

nit: This can be simplified to req?.txFingerprintID. You don't need the ternary operator here, the ? will make the evaluated value null

Done.

pkg/ui/workspaces/cluster-ui/src/insights/workloadInsightDetails/transactionInsightDetails.tsx line 78 at r3 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

nit: You don't need to add the ? to the second part of the && here. If stmts is nullish the stmts != null && will ensure the part after && doesn't get read.

Done.

pkg/ui/workspaces/cluster-ui/src/insights/workloadInsights/util/queriesCell.tsx line 25 at r3 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Ditto for the addition of ? here. !transactionQueries?.length will ensure that the part after || means that transactionQueries has a non-zero length.

Done.

pkg/ui/workspaces/cluster-ui/src/selectors/insightsCommon.selectors.ts line 38 at r3 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

nit: can be condensed to (!statementInsights?.length || !fingerprintID)

Done.

pkg/ui/workspaces/cluster-ui/src/selectors/insightsCommon.selectors.ts line 42 at r3 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

nit: Try replacing this with an aray.filter, and you can set the var above directly.

Done.

pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetails.tsx line 467 at r3 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

nit: if you're using this to track the insight types seen already, a set is prob preferred (although the array size will be small so it's probably not a big deal 🤷 )

Done (here and in statementDetails).

pkg/ui/workspaces/cluster-ui/src/transactionDetails/transactionDetails.tsx line 561 at r3 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

nit: tableData?.length &&

Done.

I've addressed all requested changes.

xinhaoz · 2023-02-08T18:36:02Z

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx

    const req = getStatementDetailsRequestFromProps(this.props);
    this.props.refreshStatementDetails(req);
+    this.refreshStatementInsights(this.props.statementFingerprintID);


nit: if this param is just from the props, why not just put it directly in the function instead as a param?

xinhaoz · 2023-02-08T18:36:17Z

pkg/ui/workspaces/cluster-ui/src/insightsTable/insightsTable.tsx

@@ -271,6 +299,32 @@ function descriptionCell(
  }
 }

+function linkCell(insightRec: InsightRecommendation): React.ReactElement {
+  if (insightRec.execution.execType === InsightExecEnum.STATEMENT) {


nit: This might be a little clearer with a switch case statement

xinhaoz · 2023-02-08T18:44:37Z

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx

+    const isCockroachCloud = useContext(CockroachCloudContext);
+    const insightsColumns = useMemo(
+      () =>
+        makeInsightsColumns(isCockroachCloud, this.props.hasAdminRole, false),
+      [isCockroachCloud],
+    );


I don't think it's recommended to use hooks inside non-functional components. It can have some weird effects, and also I don't think conditional calls to hooks are allowed (not sure what hte error behaviour will be though, did you get any console warnings?). I believe what you're doing here means these hooks will attach to the parent component (in this case StatementDetails), and since we render something different when we don't have data we'll be conditionally calling these hooks. Maybe you can extract renderOverviewTabContent into its own component.

…etails This commit adds execution insights to the Statement Fingerprint and Transaction Fingerprint Details pages. Part of cockroachdb#83780. Release note (ui change): Added execution insights to the Statement Fingerprint Details and Transaction Fingerprint Details Pages.

ericharmeling

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

pkg/ui/workspaces/cluster-ui/src/insightsTable/insightsTable.tsx line 303 at r4 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

nit: This might be a little clearer with a switch case statement

Done.

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx line 272 at r4 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

nit: if this param is just from the props, why not just put it directly in the function instead as a param?

Done.

pkg/ui/workspaces/cluster-ui/src/statementDetails/statementDetails.tsx line 669 at r4 (raw file):

I don't think it's recommended to use hooks inside non-functional components.

That makes sense. I just removed the hook for now. IDK how much we save memoizing the cockroachcloud context.

also I don't think conditional calls to hooks are allowed (not sure what hte error behaviour will be though, did you get any console warnings?). I believe what you're doing here means these hooks will attach to the parent component (in this case StatementDetails), and since we render something different when we don't have data we'll be conditionally calling these hooks

I didn't get any console warnings (from what I'm reading, it's just bad practice), but that's good to know for future reference 😄 !

ericharmeling · 2023-02-09T18:41:49Z

Tests are passed and I've gotten to everyone's feedback. I'm going to go ahead and bors 😁.

ericharmeling · 2023-02-09T18:42:49Z

bors r+

craig · 2023-02-09T19:59:27Z

Build succeeded:

Bazel Essential CI (Cockroach)

cockroachdb#96440 introduced a bug to the Insights Table on the Schema Insights page. This commit fixes that bug. Release note: None Epic: None

97306: kvserver: add storage.keys.tombstone.count metric r=nicktrav a=jbowens Expose the number of tombstones in the storage engine as a timeseries metric. This provides some additional visibility into garbage collection and would help in diagnosing issues like cockroachlabs/support#2084. Epic: None Release note (ops change): Adds a new timeseries metric `storage.keys.tombstone.count` that shows the current count of point and range deletion tombstones across the storage engine. 97309: ui: fixed insights table bug on insights r=ericharmeling a=ericharmeling #96440 introduced a bug to the Insights Table on the Schema Insights page. This commit fixes that bug. Loom: https://www.loom.com/share/0f81bc55371f49418f7f0175bbc5dff4 Release note: None Epic: None Co-authored-by: Jackson Owens <[email protected]> Co-authored-by: Eric Harmeling <[email protected]>

ericharmeling requested a review from a team February 2, 2023 19:33

ericharmeling marked this pull request as ready for review February 2, 2023 19:33

ericharmeling requested a review from a team February 2, 2023 19:33

gtr previously requested changes Feb 2, 2023

View reviewed changes

ericharmeling force-pushed the insights-on-sql-activity branch from d2cdbd6 to 5a4eae3 Compare February 7, 2023 16:13

ericharmeling commented Feb 7, 2023

View reviewed changes

ericharmeling requested review from gtr and a team February 7, 2023 16:15

maryliag suggested changes Feb 7, 2023

View reviewed changes

ericharmeling force-pushed the insights-on-sql-activity branch 2 times, most recently from 041b32a to a216433 Compare February 7, 2023 20:10

ericharmeling commented Feb 7, 2023

View reviewed changes

ericharmeling force-pushed the insights-on-sql-activity branch from a216433 to 0b75cd9 Compare February 7, 2023 22:18

maryliag approved these changes Feb 7, 2023

View reviewed changes

xinhaoz reviewed Feb 7, 2023

View reviewed changes

ericharmeling force-pushed the insights-on-sql-activity branch from 0b75cd9 to 7716ab4 Compare February 8, 2023 18:13

ericharmeling commented Feb 8, 2023

View reviewed changes

ericharmeling requested a review from xinhaoz February 8, 2023 18:14

xinhaoz reviewed Feb 8, 2023

View reviewed changes

ericharmeling force-pushed the insights-on-sql-activity branch from 7716ab4 to 2ec730f Compare February 9, 2023 15:39

ericharmeling commented Feb 9, 2023

View reviewed changes

ericharmeling requested a review from xinhaoz February 9, 2023 16:47

craig bot merged commit 732c3a7 into cockroachdb:master Feb 9, 2023

cockroach-teamcity mentioned this pull request Feb 10, 2023

PR #96440 - ui: add execution insights to statement and transaction fingerprint d… cockroachdb/docs#16234

Closed

ericharmeling mentioned this pull request Feb 10, 2023

ui: add insight count to sql activity execution fingerprint overviews #91687

Closed

ericharmeling mentioned this pull request Feb 17, 2023

ui: fixed insights table bug on insights #97309

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: add execution insights to statement and transaction fingerprint details #96440

ui: add execution insights to statement and transaction fingerprint details #96440

ericharmeling commented Feb 2, 2023 •

edited

Loading

cockroach-teamcity commented Feb 2, 2023

gtr left a comment

ericharmeling left a comment

maryliag left a comment

ericharmeling left a comment

ericharmeling left a comment

maryliag left a comment

xinhaoz Feb 7, 2023 •

edited

Loading

xinhaoz Feb 7, 2023

xinhaoz Feb 7, 2023

xinhaoz Feb 7, 2023

xinhaoz Feb 7, 2023

xinhaoz Feb 7, 2023

xinhaoz Feb 7, 2023

ericharmeling left a comment

xinhaoz Feb 8, 2023

xinhaoz Feb 8, 2023

xinhaoz Feb 8, 2023

ericharmeling left a comment

ericharmeling commented Feb 9, 2023

ericharmeling commented Feb 9, 2023

craig bot commented Feb 9, 2023

ui: add execution insights to statement and transaction fingerprint details #96440

ui: add execution insights to statement and transaction fingerprint details #96440

Conversation

ericharmeling commented Feb 2, 2023 • edited Loading

cockroach-teamcity commented Feb 2, 2023

gtr left a comment

Choose a reason for hiding this comment

ericharmeling left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

ericharmeling left a comment

Choose a reason for hiding this comment

ericharmeling left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

xinhaoz Feb 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericharmeling left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericharmeling left a comment

Choose a reason for hiding this comment

ericharmeling commented Feb 9, 2023

ericharmeling commented Feb 9, 2023

craig bot commented Feb 9, 2023

ericharmeling commented Feb 2, 2023 •

edited

Loading

xinhaoz Feb 7, 2023 •

edited

Loading