getsentry / sentry-dotnet

Sentry SDK for .NET
https://docs.sentry.io/platforms/dotnet
MIT License
582 stars 207 forks source link

Make SentryEvent.HasTerminalException() public #2877

Open espenrl opened 10 months ago

espenrl commented 10 months ago

Problem Statement

The scenario is regarding a MAUI app where common exceptions regarding network timeouts are of little interest to collect. Unless they are terminal which is a scenario that effectively kills the app. Then even a network timeout exception is suddenly of high interest to collect.

Solution Brainstorm

Make SentryEvent.HasTerminalException() public so the API can be used to make decisions about which exceptions to ignore.

bitsandfoxes commented 10 months ago

Hey @espenrl! What stops you from filtering based on the exception in the BeforeSend callback?

espenrl commented 10 months ago

Hi @bitsandfoxes What stops me is that there isn't anything indicating if the event is terminal or not. Terminal being a Sentry specific term - for MAUI it would be an unhandled exception that crashes the app.

I saw in the source code that there is an API to check if an event is terminal or not, though it's marked internal.

https://github.com/getsentry/sentry-dotnet/blob/65a7bba00a73db6c619aa5d4a44ca4da2092b738/src/Sentry/SentryEvent.cs#L184-L198

bruno-garcia commented 10 months ago

I'd rename that to IsUnhandled then, with a big explanation that this is from Sentry's PoV, by default events are handled:true, and we explicitly set to false when we capture things in one of our hooks like UnhandledExceptionHandler, Middleware on ASPNET Core, etc

Alternatively this could be put in Hint instead of adding something to the Event itself?

bitsandfoxes commented 10 months ago

Adding it to the hint is a great idea. The thing is under-utilized as it is! It would look like this:

options.SetBeforeSend((@event, hint) =>
{
    if (hint.Items.ContainsKey("HasTerminalException"))
    {
        // This event has a terminal exception
    }

    return @event;
});

The key is up for discussion tho. The only downside to this I can see right now is that it's basically undocumented in code.

bruno-garcia commented 10 months ago

I'd hide all of that on an extension method to Hint (or a method on it) so the user doesn't need all that boilerplate:

options.SetBeforeSend((@event, hint) =>
{
    if (hint.IsUnhandled()))
    {
        // This event has an unhandled exception
    }

    return @event;
});

It technically could be an extension method IsFromUnhandledException (here not an actual method I'd say) to SentryEvent since it's just:

this.SentryException.Values.Any(e => e.Handled == false), right? The information doesn't live outside of the event (for it to need to be in the hint).

I guess we started with IsFromTerminalException and I turned this into IsFromUnhandledException but my comment above works on both, perhaps we can add both extension method?