getsentry/sentry-dotnet

Exceptions captured through `Extensions.Logging` marked as `handled = true`

bitsandfoxes opened this issue · 9 comments

An easy repro is Sentry.Samples.AspNetCore.Blazor.Wasm:

private static void Thrower() => throw null;

The sample throws null but the error gets captured through Sentry.Extensions.Logging which calls _hub.CaptureException which ends up in here getting marked as handled
else if (exception.StackTrace != null)
{
// The exception was thrown, but it was caught by the user, not an integration.
// Thus, we can mark it as handled.
mechanism.Handled = true;
}

This seems like a bit of an edge case - it's pretty unusual to throw null. Maybe we leave the logic for the SDK as it is and update the sample to throw an exception instead?

Edit: Turns out throw null works as a shortcut to throw a NullReferenceException. TIL.

It's not about the throw null but about our logging integration capturing an exception it receives from the logs and the processor assuming if exception has stacktrace then handled.

Since we're fairly sure that this is most likely relevant in a WASM context, figuring out how to properly set up global exception handling might make this problem go away on its own. MS Docs

Tricky. When an unhandled exception occurs in WASM... the framework logs these. The logging itself is taken care of by various source generated helper functions like this one for unhandled exceptions that occur during rendering.

Potentially, we could take advantage of the fact that the LogMessageAttribute (and the corresponding source generator that is using this marker attribute) is setting an EventId... in the case of unhandled rendering errors it's 100:

    private static partial class Log
    {
        [LoggerMessage(100, LogLevel.Critical, "Unhandled exception rendering component: {Message}", EventName = "ExceptionRenderingComponent")]
        public static partial void UnhandledExceptionRenderingComponent(ILogger logger, string message, Exception exception);
    }

There are various places where an error may occur though and rummaging around in the source code, it doesn't look like this pattern is used consistently to throw errors 😞 .

Exceptions for event handlers, for example, appear to be thrown from EventCallbackWorkItem.InvokeAsync but I'm not yet seeing how any exceptions thrown in those tasks result in messages being logged to the default logger.

Potential (partial) fix here: c6ccc52. It will identify the exception from our Sample project but I'm sure there are all kinds of other scenarios where the EventId.Name might be something else. Probably need to do a bit more testing, trying to generate errors from the various other places where an error may occur (linked above).

we could pass some scope to the logger that the sdk understands as setting the event as unhandled. like logError(...)
or set something in the scope instead?

Potential (partial) fix here: c6ccc52

those strings are guaranteed to be unhanlded/framework captured exceptions ?

feels fragile that way, but if we're not the ones calling logException i don't see another way than using the logger id

feels fragile that way, but if we're not the ones calling logException i don't see another way than using the logger id

Yeah LogException gets called by runtime (e.g. here)... we have no control over it.