-
Notifications
You must be signed in to change notification settings - Fork 556
feat(integrations): Add unraisable exception integration #4733
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
Conversation
Adds an uncaught exception integration, enabled by default. The integration forwards the exception to Sentry only if the exception value and stacktrace are set.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4733 +/- ##
==========================================
- Coverage 84.92% 84.89% -0.03%
==========================================
Files 156 157 +1
Lines 16115 16136 +21
Branches 2741 2744 +3
==========================================
+ Hits 13685 13698 +13
- Misses 1643 1651 +8
Partials 787 787
|
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.
The integration looks good 💪🏻 Left two comments, please have a look
sentry_sdk/integrations/__init__.py
Outdated
if sys.version_info >= (3, 8): | ||
_DEFAULT_INTEGRATIONS.append( | ||
"sentry_sdk.integrations.unraisablehook.UnraisablehookIntegration" | ||
) | ||
|
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'd not make this default just yet, let's give it some time to soak. We can turn it on by default in 3.0 on the potel-base
branch -- could you prepare a PR for that?
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.
yes sounds good! #4749
TEST_PARAMETERS = [ | ||
("", "HttpTransport"), | ||
('_experiments={"transport_http2": True}', "Http2Transport"), | ||
] |
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.
Why do we need to test this both with and without HTTP/2? Is the behavior of the integration any different?
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.
No, this is copied over from test_excepthook
. I guess the test is more of an integration test as it checks that the unraisable exception goes all the way to the transport.
I'm happy to change this. Would you just use Http2Transport
now that will be the default?
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.
Didn't realize we're doing the same in the excepthook tests. I guess fine to keep as is then.
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.
🚀
Closes #374
Adds an uncaught exception integration, enabled by default. The integration forwards the exception to Sentry only if the exception value and stacktrace are set.
The implementation and tests are adapted from
excepthook
. Setssys.unraisablehook
instead ofsys.excepthook
, and changes relevant signatures and types. Compared with theexcepthook
integration, there is noalways_run
parameter. As far as I can tell the option was added to avoid excessive errors in interactive shells, which does not apply to unraisable errors.Unlike in the
excepthook
tests, theunraisablehook
test does not check for the presence of the local variable in the pre-context. The local variable from theexcepthook
test is not captured in an analogous test with an unraisable exception because an (unraisable) exception in__del__
has a stack frame with only one level.A good source about unraisable exceptions: https://vstinner.github.io/sys-unraisablehook-python38.html