-
Notifications
You must be signed in to change notification settings - Fork 254
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
Improve logging and available information when error reports are not sent #515
Conversation
In the following circumstances, the notify callback wasn't called: - report not sent due to releaseStage/notifyReleaseStages config - report not sent due to beforeSend callback This meant that the configured onUncaughtException and onUnhandledRejection callbacks (in Node) were never called, and so unhandled errors were effectively silently swallowed. Although intentionally not reported, it's still useful to know about so this change ensures the callbacks run. The types were updated to remove the no-longer present notion that the the return value of notify() indicated whether the report was sent or not. Fixes #449
This log can be preceded by "[bugsnag] Report not sent due to releaseStage/notifyReleaseStages configuration" so it's misleading to say that it was reported. Removing the verb completely makes it correct and retains enough meaning.
errorMessage: string, | ||
stacktrace?: any[], | ||
handledState?: IHandledState, | ||
originalError?: any, |
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.
Is there some docs or usage/tests to go along with this change?
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.
I figured I would write the docs once the implementation was ratified. I don't have a good reason for not adding tests for that aspect of this PR though so I will add some.
Is there any chance this could be added to the JS docs? I couldn't find it in there =), thanks for the originalError addition! It has helped us a lot with grouping errors etc. |
This PR contains a few small improvements for the following situations:
releaseStage
is not in the list ofnotifyReleaseStages
, therefore reports are not sendbeforeSend
callback prevents a report from being sentThe problem
As described in #449, Bugsnag shouldn't silently consume the errors if it's not going to send them anywhere, they should be logged out.
Unhandled errors
In both of these cases, when the error is unhandled the following log message would be written and the
notify(err, opts, cb)
callback would not run:By ensuring that the
notify(err, opts, cb)
callback is called, in Node this means the defautltonUncaughtException
(andonUnhandledRejection
) now correctly get called so the error is printed and the process exits (using the example from #449):Unit tests were added to ensure the behaviour was correct for various permutations.
Handled errors
The situation for handled errors is a little different. We can't assume that a log is desirable, but we can provide the means for users to log it themselves.
I added a property
originalError
to theReport
class which contains whatever the raw thing was that the report was created from. This also resolves #420, meaning that users who want to do something such ascan do so with the following:
Tests
Added unit tests to ensure the
client.notify(err, opts, cb)
callback was always called. The end-to-end tests aren't set up to make assertions about the the process's output, so I manually tested that the expected content was seen whenreleaseStage
was not innotifyReleaseStages
and when abeforeSend
prevented a report from sending.Bundle size discussion time
Since this makes changes to
@bugsnag/core
it stands to affect browser bundle size. Here's the change:Net increase of
0.01kB
. I think this is 🆗.