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

ui: add details to generic error message #105550

Merged
merged 1 commit into from
Jun 30, 2023
Merged

ui: add details to generic error message #105550

merged 1 commit into from
Jun 30, 2023

Conversation

j82w
Copy link
Contributor

@j82w j82w commented Jun 26, 2023

When a user encounters an error they only get a generic error message. Users need to go to the dev console and try to find and share the actual error message. If the issue doesn't reproduce, it's not possible to root cause the problem.

This commit adds the current date, error message, and the current URL to the error component so when a user shares a screen shot we have the necessary info without the need for them to go to the dev console.

This commit also refactors the check for the timeout logic into the error component which removes the duplicate logic.

Screenshot 2023-06-28 at 1 15 51 PM

Fixes: #105549

Release note (ui change): The generic error message now includes details about the actual error, and other context to make it easier to root cause.

@j82w j82w requested a review from a team June 26, 2023 15:15
@j82w j82w requested a review from a team as a code owner June 26, 2023 15:15
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@zachlite zachlite left a comment

Choose a reason for hiding this comment

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

Thank you for doing this!

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


pkg/ui/workspaces/cluster-ui/src/sqlActivity/sqlActivity.module.scss line 26 at r1 (raw file):

  flex-direction: row;
  font-size:xx-small;
  color:$colors--neutral-4;

Can you use secondary-text-color here? I think the current light gray on red is too low-contrast.

$secondary-text-color: $colors--neutral-5;

@j82w
Copy link
Contributor Author

j82w commented Jun 26, 2023

pkg/ui/workspaces/cluster-ui/src/sqlActivity/sqlActivity.module.scss line 26 at r1 (raw file):

Previously, zachlite wrote…

Can you use secondary-text-color here? I think the current light gray on red is too low-contrast.

$secondary-text-color: $colors--neutral-5;

@dongniwang what do you recommend? I did the low contract by design so they focus on the main error message and hopefully ignore the details. The current color is still readable for us, and not distracting to users.

@j82w j82w added backport-22.2.x Flags PRs that need to be backported to 22.2. backport-23.1.x Flags PRs that need to be backported to 23.1 labels Jun 26, 2023
@dongniwang
Copy link

Hey @j82w. I understand we'd like to make the message less distracting to users. And I do agreed with @zachlite the gray on red is low-contrast and not accessible.

We do have a set of Alerts in the Design System with alert messages. I'd recommend updating the message in a more legible color. Also, would it be possible to move the link into its own line? See attached screenshot. Let me know if you have any questions!
image

@j82w
Copy link
Contributor Author

j82w commented Jun 27, 2023

@dongniwang the message in your design looks like something that is meant for users. The only thing relevant for users is the first line. The new newline that starts with the date is only meant for CRDB employees to help root cause bugs in the ui console. We don't want users to even notice the new information being displayed as it may confuse them. Do you still think we should go with the proposed designed?

@dongniwang
Copy link

Just met with @j82w to discuss this, capturing our conversation and decisions here as well:

  1. For right now, @j82w will keep the existing action button location (next to message title) and update text from gray to body text color.
  2. Future improvement consideration 1) consider improving the existing error code system to include meaningful error code and message for better troubleshooting experience both internally and externally 2) consider adding a functionality such as "submit a ticket" in the error message to help user get support more easily. "Submit a ticket" should capture a screenshot of the page, include error message and timestamp of the console, and then zip everything as attachment and create a support ticket.
  3. @dongniwang to explore design option that's similar to the 404 page design.

When a user encounters an error they only get a generic error message.
Users need to go to the dev console and try to find and share the
actual error message. If the issue doesn't reproduce, it's not possible
to root cause the problem.

This commit adds the current date, error message, and the current URL
to the error component so when a user shares a screen shot we have the
necessary info without the need for them to go to the dev console.

Fixes: #105549

Release note (ui change): The generic error message now includes
details about the actual error, and other context to make it easier
to root cause.
Copy link
Contributor

@zachlite zachlite left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @dongniwang)

@j82w
Copy link
Contributor Author

j82w commented Jun 30, 2023

bors r+

@craig craig bot merged commit b77fca8 into cockroachdb:master Jun 30, 2023
7 checks passed
@craig
Copy link
Contributor

craig bot commented Jun 30, 2023

Build succeeded:

@blathers-crl
Copy link

blathers-crl bot commented Jun 30, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 5fcb13a to blathers/backport-release-22.2-105550: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.


error creating merge commit from 5fcb13a to blathers/backport-release-23.1-105550: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

abarganier added a commit to abarganier/cockroach that referenced this pull request Feb 5, 2024
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.
abarganier added a commit to abarganier/cockroach that referenced this pull request Feb 5, 2024
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.
abarganier added a commit to abarganier/cockroach that referenced this pull request Feb 5, 2024
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.
craig bot pushed a commit that referenced this pull request Feb 6, 2024
118782: pkg/ui: add request status code to error messaging r=koorosh a=abarganier

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

<img width="949" alt="Screenshot 2024-02-05 at 5 04 56 PM" src="https://github.com/cockroachdb/cockroach/assets/8194877/a6c05096-cda7-44aa-bf23-3ddcd2b68a17">





118815: changefeedccl: fix bug with avro encoding and zero-scale decimal cols r=jayshrivastava,rharding6373 a=andyyang890

This patch fixes a bug where creating a changefeed that targeted
tables with a `DECIMAL(n)` column (i.e. zero-scale `DECIMAL` column),
`format='avro'`, and `diff` would cause a panic.

The cause of this panic was the fact that the third-party `goavro`
library we use expected the JSON encoding of the schema to have
a numeric `scale` field for decimal types, but we omitted this
field whenever it was zero (using `omitempty`), which led to a
runtime type assertion failure. We've updated the field to a pointer
type in our type definition so that we can distinguish between an
unset value and a zero value.

Fixes #118647

Release note (enterprise change): Fixed a bug where creating a
changefeed that targeted tables with a `DECIMAL(n)` column
(i.e. zero-scale `DECIMAL` column), `format='avro'`, and `diff`
would cause a panic.

Co-authored-by: Alex Barganier <abarganier@cockroachlabs.com>
Co-authored-by: Andy Yang <yang@cockroachlabs.com>
wenyihu6 pushed a commit to wenyihu6/cockroach that referenced this pull request Feb 21, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-22.2.x Flags PRs that need to be backported to 22.2. backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui: add more info to error message
4 participants