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

New Messaging for Tool Form Modal Errors #19562

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

hujambo-dunia
Copy link
Contributor

@hujambo-dunia hujambo-dunia commented Feb 6, 2025

Your error has been logged in Sentry to improve your experience.
Greenshot 2025-02-06 12 29 07

Added clarity for errors before Job ID or Dataset ID creation:

  • Componentized for: default (ErrorPlugin) or third-party (Sentry)
  • Extensible for future user error-tracking (Event-ID) information

Reported by @hexylena in issue
#17560

How to test the changes?

(Select all options that apply)

  • I've included appropriate automated tests.
  • This is a refactoring of components with existing test coverage.
  • Instructions for manual testing are as follows:
    1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

  • I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

Added clarity for errors before Job ID or Dataset ID creation:
* Componentized for: default (ErrorPlugin) or third-party (Sentry)
* Extensible for future user error-tracking (Event-ID) information

Reported by @hexylena in issue
galaxyproject#17560
@github-actions github-actions bot added this to the 25.0 milestone Feb 6, 2025
Copy link
Contributor

@davelopez davelopez left a comment

Choose a reason for hiding this comment

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

Remember you can run make client-format to fix all the formatting issues.

const errorReportingAvailable = computed(() =>{
const Galaxy = getGalaxyInstance();
if (!!Galaxy.Sentry.isInitialized) {
return "Your error has been logged in Sentry to improve your experience.";
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't explicitly mention Sentry, users may not know what it is. Maybe something like:

Suggested change
return "Your error has been logged in Sentry to improve your experience.";
return "The error has been logged in our issue tracker system, and our team is investigating to fix it as soon as possible.";

Or something similar :)

@mvdbeek
Copy link
Member

mvdbeek commented Feb 7, 2025

Users don't know what Sentry is, and this is also not logged to Sentry at all, as it is not a bug but a response to a user action that isn't possible. All the actionable information is in the message, but it could be explained better. My suggestion

That message could be improved (this would absolutely be a worthwhile project, the API response is structured so you could mount a nice little component that explains this further),

was to create a component that can expand on the error message by explaining what auto means in this case.
I would also remove the API transcript in that case or hide it under an expandable section, it's not useful to users when what they need to know is already in the message.

The link with Sentry is for when there is actually a bug (a.k.a unhandled exception), for that you'd have to augment the backend response with the backend's sentry clients last_event_id() id (https://github.com/getsentry/sentry-python/blob/bc72f78eea76a77bfd4b445a0424767223d76787/sentry_sdk/scope.py#L326-L340). This would like require a custom FastAPI middleware / exception handler.

const errorReportingAvailable = computed(() =>{
const Galaxy = getGalaxyInstance();
if (!!Galaxy.Sentry.isInitialized) {
Copy link
Member

Choose a reason for hiding this comment

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

This just means we have a sentry client that is able to submit client errors, but that doesn't mean entirely expected things like this error are captured in sentry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants