Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pkg/ui: add request status code to error messaging #118782

Merged
merged 1 commit into from Feb 6, 2024

Conversation

abarganier
Copy link
Member

Fixes: #114582

We recently made some nice improvements to our error messaging in DB Console via the changes in #105550

However, these changes didn't include the HTTP response status code in the error message, which has created some guess work when interpreting error messages, especially when involving things like timeouts from the load balancer servicing the request.

This patch updates the error messaging to include the HTTP response code if the request being rendered is a RequestError. If it's not, the field is omitted.

Release note (ui change): The error messages surfaced in DB Console now include information about the HTTP response status code, if one is present.

Epic: CRDB-20791

Screenshot 2024-02-05 at 5 04 56 PM

@abarganier abarganier requested review from koorosh, a team and maryliag and removed request for a team February 5, 2024 21:06
@abarganier abarganier requested a review from a team as a code owner February 5, 2024 21:06
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@koorosh koorosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/ui/workspaces/cluster-ui/src/sqlActivity/errorComponent.tsx line 21 at r1 (raw file):

interface SQLActivityErrorProps {
  statsType: string;
  error: Error;

I suggest to extend error type to Error | RequestError as it is also a possible type.


pkg/ui/workspaces/cluster-ui/src/sqlActivity/errorComponent.tsx line 88 at r1 (raw file):

      : "";

  const respCode = (props?.error as RequestError)?.status;

nit. util package contains isRequestError function to check whether it is plain error or RequestError. It might be more verbose but avoids explicit casting of a type.

Fixes: cockroachdb#114582

We recently made some nice improvements to our error messaging in DB
Console via the changes in cockroachdb#105550

However, these changes didn't include the HTTP response status code in
the error message, which has created some guess work when interpreting
error messages, especially when involving things like timeouts from the
load balancer servicing the request.

This patch updates the error messaging to include the HTTP response
code if the request being rendered is a RequestError. If it's not, the
field is omitted.

Release note (ui change): The error messages surfaced in DB Console now include
information about the HTTP response status code, if one is present.
Copy link
Member Author

@abarganier abarganier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/ui/workspaces/cluster-ui/src/sqlActivity/errorComponent.tsx line 21 at r1 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

I suggest to extend error type to Error | RequestError as it is also a possible type.

Good thinking! Done.


pkg/ui/workspaces/cluster-ui/src/sqlActivity/errorComponent.tsx line 88 at r1 (raw file):

Previously, koorosh (Andrii Vorobiov) wrote…

nit. util package contains isRequestError function to check whether it is plain error or RequestError. It might be more verbose but avoids explicit casting of a type.

Thanks for the callout - switched to using isRequestError.

Let me know if there's a more idiomatic TypeScript way to do what I'm doing here 🙂

Copy link
Collaborator

@koorosh koorosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: 🔥

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

@abarganier
Copy link
Member Author

TFTR!

bors r=koorosh

@craig
Copy link
Contributor

craig bot commented Feb 6, 2024

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

@craig
Copy link
Contributor

craig bot commented Feb 6, 2024

Build succeeded:

@craig craig bot merged commit 17f1eac into cockroachdb:master Feb 6, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Blank error message field on error when clicking databases page
3 participants