-
-
Notifications
You must be signed in to change notification settings - Fork 452
Allow multiple UncaughtExceptionHandlerIntegrations to be active #4462
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
base: main
Are you sure you want to change the base?
Conversation
Performance metrics 🚀
|
final UncaughtExceptionHandlerIntegration currentHandlerIntegration = | ||
(UncaughtExceptionHandlerIntegration) currentHandler; | ||
if (currentHandlerIntegration.scopes != null | ||
&& scopes.getGlobalScope() == currentHandlerIntegration.scopes.getGlobalScope()) { |
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.
hm, any specific reason we limit this to globalScope
only? Shouldn't we compare scopes == currentHandlerIntegration.scopes
given that the integration works on the scopes
level?
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.
My thinking was to allow exactly one UncaughtExceptionHandlerIntegration
to be registered per Sentry instance. For that I used the globalScope
as it is never forked.
With the new close
logic, however, I think we could also do what you suggested and use scopes
instead. But I'd have to test how this behaves
Do you see any pros/cons for one over the other?
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.
Not much pros for now, but if we change how Scopes behave under-the-hood this may break in theory. And also "per Sentry instance" probably implies Scopes
rather than globalScope
. I ran the test locally after changing this condition to comparing scopes
and it still passed. So, up to you :)
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.
Hmm, I get your point regarding the inner workings of Scopes
. I basically tried to be pretty conservative here as to not cause a regression of #3266.
In theory, with your suggestions, one can register multiple UncaughtExceptionHandlerIntegration
by passing a forked Scopes
instance to the register
method. Then again, if all UncaughtExceptionHandlerIntegration
instances are passed into the integration list of SentryOptions
as per the documentation, they should still clean up nicely.
val integration = UncaughtExceptionHandlerIntegration()
integration.register(scopes, options)
val forkedScopes = scopes.forkedScopes("test")
val integration2 = UncaughtExceptionHandlerIntegration()
integration2.register(forkedScopes, options)
This will register two UncaughtExceptionHandlerIntegration
instances, because by forking the Scopes
we get a new Scopes
instance.
If the integrations are added to the Sentry options:
options.addIntegration(integration)
options.addIntegration(integration2)
Then closing either the original or forked scopes will close both UncaughtExceptionHandlerIntegration
instances.
I'm fine with both approaches. I think the question becomes whether we want to allow that behaviour. WDYT?
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.
one question, but LGTM otherwise, nice job!
📜 Description
Reworks registration and removal of
UncaughtExceptionHandlerIntegration
to be able to add oneUncaughtExceptionHandlerIntegration
perscopes.globalScope
.Functionality is now consistent with the documentation about using Sentry in an SDK here.
💡 Motivation and Context
Fixes #4429
💚 How did you test it?
📝 Checklist
sendDefaultPII
is enabled.🔮 Next steps