-
-
Notifications
You must be signed in to change notification settings - Fork 55
fix: Suppress and log known exceptions during native initialization #1898
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
public static void ReinstallBackend() => SentryNativeBridge.ReinstallBackend(); | ||
// We're calling this in `BeforeSceneLoad` instead of `SubsystemRegistration` as it's too soon and the | ||
// SignalHandler would still get overwritten. | ||
[RuntimeInitializeOnLoadMethod(RuntimeInitializeLoadType.BeforeSceneLoad)] |
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.
We could have had this in here all along? :)
I dont' recall why we left on SentryInitialization
originally, do you recall?
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 intention was to have everything annotated RuntimeInitializeOnLoadMethod
in the SentryInitialization.cs
so users could overwrite/remove it. But it should have belonged in SentryNative
all along.
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 actually though the flag was "UseNativeBackend" which we'll flip to false if native support was disabled, but also, on the catch block for the MissingEntryPoint.
And we can check that flag before doing any native calls. Or we don't need this because no native call is done anyway because we don't add the scope observers?
So the only thing left is making sure the BeforeScene thing doesn't run, hence the boolean you used in this PR? If that's the case there's no need to write LogDebug( "something failed")
because nothing failed, just no backend was selected.
And no need for flipping any flag on the catch block either if no where else after this we'll need to know if it worked or not. (We could disable sessions if we're afraid of reporting 100% crash free rate, so the user will at least see a drop in sessions if it's a widespread error)?
Co-authored-by: Bruno Garcia <[email protected]>
Co-authored-by: Bruno Garcia <[email protected]>
Co-authored-by: Bruno Garcia <[email protected]>
Exactly, if the init failed we don't add the observers and we won't reinstall the backend.
Yes. Fair point. Updated the logs! Thanks! |
The SDK will no longer attempt to reinstall the backend without native support enabled and without successful initialization.
It'll also capture
EntryPointNotFoundException
during reinstallation.