-
-
Couldn't load subscription status.
- Fork 225
feat: Added SessionEndStatus.Unhandled
#4633
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: version6
Are you sure you want to change the base?
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## version6 #4633 +/- ##
===========================================
Coverage ? 72.97%
===========================================
Files ? 480
Lines ? 17378
Branches ? 3431
===========================================
Hits ? 12681
Misses ? 3840
Partials ? 857 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This is really annoying.. running because and all I want to do is to please Verify... |
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.
while talking through this a bigger problem has been uncovered, with restarting sessions on unhandled
test/Sentry.AspNetCore.TestUtils/Sentry.AspNetCore.TestUtils.csproj
Outdated
Show resolved
Hide resolved
|
@bitsandfoxes CI is all broken |
|
@sentry review |
Co-authored-by: James Crosswell <[email protected]>
Relates to #4632
Problem
The SDK should use the new
SessionEndStatus.Unhandledwhen capturing an unhandled exception that does not cause the application to crash.Unhandled exceptions typically lead to a terminal state of the application. The only exception (hehe) to this was the
UnobservedTaskExceptionIntegration. Another instance where this is highly relevant is Unity: Unhandled exceptions get "swallowed" by the engine and the game just continues.Context
Within the Sentry SDK for .NET the only non-terminal unhandled exception comes from the
UnobservedTaskExceptionIntegrationthat also gets special cased inSentryEvent.HasTerminalException()where we compare against the mechanism keysentry-dotnet/src/Sentry/SentryEvent.cs
Lines 188 to 191 in 04c932b
This is limiting as it does not allow for other integrations i.e. coming from the Unity SDK to add to this check.
Proposal
Option 1: Add list ofNonTerminalMechanismKeyon the optionsOne straight forward solution to this would be to have an
internal List<string>? NonTerminalMechanismKeys;onthe options and have integrations add to the list during their registration. We'd need to pass the options into the
HasTerminalExceptioncheck but they are available everywhere we'd need them.There are a couple of downsides to this:
internalorpublic. If we keep itinternalthe Unity SDK can add to it but no user written integration can.Option 2: Rework the Mechanismbool? Handledto an explicitenum ExceptionHandledStateThe
Terminalstate is only ever relevant in case of anUnhandled Exception. This makes the additional property somewhat redundant. An alternative solution would be to replace thebool? Handledwith an explicitExceptionHandledState. This would be quite the breaking change and we'd need to take special care during serialization.Option 3: Extend
Mechanismto have abool? TerminalpropertyThe mechanism on the exception already has things like
HandledandSynthetic. We can extend this by addingbool? Terminal. Integrations that capture unhandled non-terminal exceptions can set theTerminalstate tofalseand we can check for this on theSentryEvent. We keep it on the top-level ofMechanismbut we don't need to serialize it so it won't get sent to Sentry.This sounds to me the most pragmatic and minimal invasive change. The only downside I can see right now is that the property is only ever going to be useful when
Handledisfalse. But we can keep it nullable.