-
Notifications
You must be signed in to change notification settings - Fork 6
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
Do not log network errors to sentry #3083
Conversation
store.dispatch(notify({ message: err.message, kind: "warn" })); | ||
Sentry.captureException(err, { fingerprint: ["network_error"] }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is meant to fingerprint the error in order to aggregate them. The error is still being thrown below this line.
An unhandled promise rejection is typically treated as an unhandled error by Sentry so this doesn't resolve what the PR aims to resolve.
All this change would do is de-aggregate them and make more noise
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@agalin920 I see. I assume removing the throw error below this line would be enough? We're already dispatching a notification in the UI when this happens anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I believe so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
c29952a
to
f9cc467
Compare
Resolves #3074