ui: add error code to stmt and txn insights details pages #97138

gtr · 2023-02-14T19:12:01Z

Part of: #87785.

Previously, the stmt and txn insights details pages did not show any
further information for failed executions. This commit adds an "error
code" column to the insights table for a failed execution in the stmt
and txn insights details pages. Additionally, a "status" column was
added to the stmt and txn workload insights tables which is either
"Completed" or "Failed".

Future work involves adding the error message string in addition to the
error code but it needs to be redacted first. Additionally, the txn
status is missing the implementation of a "Cancelled" status.

Note to reviewers: only consider the second commit, as the first is
required to get the txn status.

Loom demo.

Release note (ui change): Adds error code column to the insights table
for a failed execution in the stmt and txn insights details page. Adds
status column to the stmt and txn workload insights tables.

cockroach-teamcity · 2023-02-14T19:12:18Z

This change is

maryliag

Please add screenshots/loom to the PR

Reviewable status: complete! 0 of 0 LGTMs obtained

maryliag

Reviewed 9 of 9 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @gtr)

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

  errorCode?: string;
  status?: string;
  executionCount?: number;

I don't see this being used anywhere

j82w · 2023-02-14T21:15:45Z

What is the reason for adding the status column when there is already the insights column that shows that it failed?

gtr

Please add screenshots/loom to the PR

Done.

What is the reason for adding the status column when there is already the insights column that shows that it failed?

Good question. I based it off the figma mock-up, which I think makes it more clear when a statement was "Completed" or "Failed" as opposed to looking at the insights column and seeing any insight other than "Failed Execution" and assuming that the execution was in fact completed. Open to other opinions though, cc: @kevin-v-ngo @dongniwang.

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

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

Previously, maryliag (Marylia Gutierrez) wrote…

I don't see this being used anywhere

Good catch, removed.

dongniwang · 2023-02-16T15:50:19Z

What is the reason for adding the status column when there is already the insights column that shows that it failed?

The Status column was proposed based on user feedback from design review - It was mentioned that "failed execution" is more of a status rather than an insight. I'd suggest to keep a status column to clearly indicate whether an execution was completed or failed. In my opinion, a failed execution should be perceived as an urgent issue users should pay attention to immediately when troubleshooting. Ideally, we should keep the status column and move the "failed execution" out of the insights. But moving it out of insights has an impact on the design of the page, so let's evaluate!

cc: @kevin-v-ngo

xinhaoz · 2023-02-16T16:27:41Z

...spaces/cluster-ui/src/insights/workloadInsights/statementInsights/statementInsightsTable.tsx


 const cx = classNames.bind(styles);

+function stmtStatusToBadge(status: StatementStatus) {


Isn't this more stmtStatusToString

Good call, done.

xinhaoz · 2023-02-16T16:36:26Z

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

+  let insightType = "insight-type";
+  if (value === "Failed Execution") {
+    insightType = "insight-type-failed";
+  }
+  return <div className={cx(insightType)}>{value}</div>;


nit, replace wtih:

const className = value === "FailedExecution" ? "insight-type-failed" : "insight-type";

Can you do me a favour and refactor this function 🙏 :

capitalize this function also, since it's returning a component

Change the param to take the InsightTypes enum instead of just a string, do the enum to string conversion in this function instead of the call site

Done. The function looks way cleaner now!

maryliag

@dongniwang should the status column exist on both statement and transaction? Right now seems to be only on statements

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

dongniwang · 2023-03-06T15:57:48Z

@dongniwang should the status column exist on both statement and transaction? Right now seems to be only on statements

Do we have a concept of fail statement execution or we always roll up to transaction execution? If we have a fail for statement, I'd expect to see status exist for both.

maryliag

We do have status on statement level, which is what I mentioned is being displayed in this PR. My questions what on the transaction level, because if any of the statements inside a transaction have failed, should we show also a column for failed on the transaction?

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

gtr · 2023-03-08T20:12:39Z

We do have status on statement level, which is what I mentioned is being displayed in this PR. My questions what on the transaction level, because if any of the statements inside a transaction have failed, should we show also a column for failed on the transaction?

I updated the PR to also include a "status" column to the transactions workload insights table. That backend change can be found on #98217 and I've included that commit here too.

I also updated the demo video to show both the transaction and the statement workload insights tables as well as a transaction with multiple statements.

maryliag

Great!

Reviewed 8 of 10 files at r9, 10 of 10 files at r10, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @xinhaoz)

gtr · 2023-03-13T20:09:16Z

TFTR!
bors r+

gtr · 2023-03-13T20:10:07Z

Going to wait until the checks pass.
bors r-

craig · 2023-03-13T20:10:10Z

Canceled.

Part of: cockroachdb#87785. Previously, the stmt and txn insights details pages did not show any further information for failed executions. This commit adds an "error code" column to the insights table for a failed execution in the stmt and txn insights details pages. Additionally, a "status" column was added to the stmt and txn workload insights tables which is either "Completed" or "Failed". Future work involves adding the error message string in addition to the error code but it needs to be redacted first. Additionally, the txn status is missing the implementation of a "Cancelled" status. Release note (ui change): Adds error code column to the insights table for a failed execution in the stmt and txn insights details page. Adds status column to the stmt and txn workload insights tables.

gtr · 2023-03-14T17:59:39Z

bors r+

craig · 2023-03-14T19:00:45Z

Build succeeded:

Bazel Essential CI (Cockroach)

gtr force-pushed the error-code-ui branch from 66233c9 to ab79e98 Compare February 14, 2023 19:13

gtr marked this pull request as ready for review February 14, 2023 19:13

gtr requested review from a team February 14, 2023 19:13

maryliag reviewed Feb 14, 2023

View reviewed changes

gtr force-pushed the error-code-ui branch 2 times, most recently from eb10f83 to 0e54396 Compare February 15, 2023 16:23

gtr commented Feb 15, 2023

View reviewed changes

gtr force-pushed the error-code-ui branch 2 times, most recently from a2f6d47 to 2a292f5 Compare February 15, 2023 21:36

xinhaoz reviewed Feb 16, 2023

View reviewed changes

gtr force-pushed the error-code-ui branch from 2a292f5 to 7d3263d Compare February 22, 2023 16:49

rafiss removed the request for review from a team February 28, 2023 15:59

gtr force-pushed the error-code-ui branch 4 times, most recently from 83bd782 to 919e746 Compare March 4, 2023 00:23

maryliag reviewed Mar 6, 2023

View reviewed changes

gtr force-pushed the error-code-ui branch from 919e746 to 9cd690b Compare March 8, 2023 20:09

gtr force-pushed the error-code-ui branch 2 times, most recently from ad67d1a to 97a2a6e Compare March 13, 2023 19:53

maryliag approved these changes Mar 13, 2023

View reviewed changes

gtr force-pushed the error-code-ui branch from 97a2a6e to fe25d50 Compare March 14, 2023 15:29

gtr force-pushed the error-code-ui branch from fe25d50 to 45a1b58 Compare March 14, 2023 16:58

craig bot merged commit 421d136 into cockroachdb:master Mar 14, 2023

gtr deleted the error-code-ui branch March 14, 2023 19:07

cockroach-teamcity mentioned this pull request Mar 15, 2023

PR #97138 - ui: add error code to stmt and txn insights details pages cockroachdb/docs#16497

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: add error code to stmt and txn insights details pages #97138

ui: add error code to stmt and txn insights details pages #97138

gtr commented Feb 14, 2023 •

edited

Loading

cockroach-teamcity commented Feb 14, 2023

maryliag left a comment

maryliag left a comment

j82w commented Feb 14, 2023

gtr left a comment

dongniwang commented Feb 16, 2023

xinhaoz Feb 16, 2023

gtr Feb 22, 2023

xinhaoz Feb 16, 2023

gtr Feb 22, 2023

maryliag left a comment

dongniwang commented Mar 6, 2023

maryliag left a comment

gtr commented Mar 8, 2023

maryliag left a comment

gtr commented Mar 13, 2023

gtr commented Mar 13, 2023

craig bot commented Mar 13, 2023

gtr commented Mar 14, 2023

craig bot commented Mar 14, 2023


		const cx = classNames.bind(styles);

		function stmtStatusToBadge(status: StatementStatus) {

ui: add error code to stmt and txn insights details pages #97138

ui: add error code to stmt and txn insights details pages #97138

Conversation

gtr commented Feb 14, 2023 • edited Loading

cockroach-teamcity commented Feb 14, 2023

maryliag left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

j82w commented Feb 14, 2023

gtr left a comment

Choose a reason for hiding this comment

dongniwang commented Feb 16, 2023

xinhaoz Feb 16, 2023

Choose a reason for hiding this comment

gtr Feb 22, 2023

Choose a reason for hiding this comment

xinhaoz Feb 16, 2023

Choose a reason for hiding this comment

gtr Feb 22, 2023

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

dongniwang commented Mar 6, 2023

maryliag left a comment

Choose a reason for hiding this comment

gtr commented Mar 8, 2023

maryliag left a comment

Choose a reason for hiding this comment

gtr commented Mar 13, 2023

gtr commented Mar 13, 2023

craig bot commented Mar 13, 2023

gtr commented Mar 14, 2023

craig bot commented Mar 14, 2023

gtr commented Feb 14, 2023 •

edited

Loading