-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(node): report errorMiddleware errors as unhandled #8048
fix(node): report errorMiddleware errors as unhandled #8048
Conversation
@@ -304,6 +305,11 @@ export function errorHandler(options?: { | |||
} | |||
} | |||
|
|||
_scope.addEventProcessor(event => { | |||
addExceptionMechanism(event, { type: 'middleware', handled: false }); |
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.
Idk what the type here should be. I was going to write "express", but this middleware can be used by many different libraries, unlike the sveltkit and angular integrations.
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 think middleware is fine for this, makes sense to me!
@AbhiPrasad is it possible to get a decision on merging this? |
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.
went on vacation so this dropped off my radar! Thanks for your patience, lgtm!
This pull request has gone three weeks without activity. In another week, I will close it. But! If you comment or otherwise update it, I will reset the clock, and if you label it "A weed is but an unloved flower." ― Ella Wheeler Wilcox 🥀 |
@AbhiPrasad it would be cool if this could be merged in to main |
@cdignam-segment mind rebasing the branch? Then I'll merge in so we can get it out with the next release. |
I found it confusing that unhandled errors caught by the express.js middleware were being reported as handled This change updates the behavior to match Sentry-Python's Django integration: https://github.com/getsentry/sentry-python/blob/81afcea403c0ac148d631164de29ed80d6a64840/sentry_sdk/integrations/asgi.py#L58
ef053f8
to
2c184c9
Compare
@AbhiPrasad Great! I've rebased my commits |
I found it confusing that unhandled errors caught by the express.js middleware were being reported as handled
This change updates the behavior to match Sentry-Python's Django integration: https://github.com/getsentry/sentry-python/blob/81afcea403c0ac148d631164de29ed80d6a64840/sentry_sdk/integrations/asgi.py#L58
I could use help testing this
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint
) & (yarn test
).