ui: add error code to stmt and txn insights details pages#97138
ui: add error code to stmt and txn insights details pages#97138craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
maryliag
left a comment
There was a problem hiding this comment.
Please add screenshots/loom to the PR
Reviewable status:
complete! 0 of 0 LGTMs obtained
maryliag
left a comment
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1.
Reviewable status:
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
|
What is the reason for adding the |
eb10f83 to
0e54396
Compare
gtr
left a comment
There was a problem hiding this comment.
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:
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.
a2f6d47 to
2a292f5
Compare
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 |
|
|
||
| const cx = classNames.bind(styles); | ||
|
|
||
| function stmtStatusToBadge(status: StatementStatus) { |
There was a problem hiding this comment.
Isn't this more stmtStatusToString
| let insightType = "insight-type"; | ||
| if (value === "Failed Execution") { | ||
| insightType = "insight-type-failed"; | ||
| } | ||
| return <div className={cx(insightType)}>{value}</div>; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Done. The function looks way cleaner now!
83bd782 to
919e746
Compare
maryliag
left a comment
There was a problem hiding this comment.
@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:
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
left a comment
There was a problem hiding this comment.
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:
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. |
ad67d1a to
97a2a6e
Compare
maryliag
left a comment
There was a problem hiding this comment.
Great!
Reviewed 8 of 10 files at r9, 10 of 10 files at r10, all commit messages.
Reviewable status:
|
TFTR! |
|
Going to wait until the checks pass. |
|
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.
|
bors r+ |
|
Build succeeded: |
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.
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.