search

getsentry / sentry-dotnet

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

Add managed attachments to native events #3545

Open TimBurik opened 3 months ago

TimBurik commented 3 months ago

Package

Sentry

.NET Flavor

.NET

.NET Version

8.0.303

OS

Android

SDK Version

4.10.1

Self-Hosted Sentry Version

No response

Steps to Reproduce

  1. Create an Android app and install Sentry NuGet package;
  2. Configure Sentry so that reported events should have attachments (both logcat and custom attachments are affected by the issue): 
    SentrySdk.Init(options =>
{
options.Dsn = "...";
options.Android.LogCatIntegration = LogCatIntegrationType.All;
});
SentrySdk.ConfigureScope(scope =>
{
scope.AddAttachment("path/to/existing/file.txt", AttachmentType.Default, MediaTypeNames.Text.Plain);
});
  3. Generate crashes using SentrySdk.CauseCrash() with different parameters

Expected Result

All reported events should have logcat.log attachment with logcat logs and custom file.txt file attachment.

Actual Result

This issue might be related to #3461

Reproduction sample: SampleProject.zip

TimBurik commented 3 months ago

The issue is also reproduced when ApplicationNotResponding is reported, no files are attached despite both logcat and custom attachments are configured.

jamescrosswell commented 3 months ago

Crash generated with parameter CrashType.Native is reported with mechanism signalhandler and contains no attachments;

I think this is similar to:

In both cases, Native crash reporting essentially skips all the .NET SDK processing.

jamescrosswell commented 3 months ago

Crash generated with parameter CrashType.JavaBackgroundThread is reported twice: one with mechanism UncaughtExceptionHandler and contains no attachments, and another with mechanism AppDomain.UnhandledException and contains both attachments;

I suspect the UncaughtExceptionHandler is coming from Java and the AppDomain.UnhandledException is coming from .NET.

@bitsandfoxes do you know what the expected behaviour is here?

jamescrosswell commented 3 months ago

Possibly relates to this (so maybe something that hasn't yet been implemented): https://github.com/getsentry/sentry-dotnet/blob/5fab09efd4204fd07d95c818df79f7aa71d61e88/src/Sentry/Platforms/Android/Extensions/AttachmentExtensions.cs#L5-L22

https://github.com/getsentry/sentry-dotnet/blob/5fab09efd4204fd07d95c818df79f7aa71d61e88/src/Sentry/Platforms/Android/Extensions/HintExtensions.cs#L13-L16

bitsandfoxes commented 2 months ago

Once the exception has been reported by Java, is there any way the .NET SDK could be aware of this and suppress further reporting? Or conversely, is there any way for the Java SDK to know this exception would be reported by the .NET SDK and so could safely ignore it?

I might have gotten that wrong but I was under the impression that the .NET SDK would be responsible for managed code and the Java SDK would provide native support. Having the same exception reported via the UncaughtExceptionHandler in Java and the AppDomain.UnhandledException is not ideal. But the reporting SDK is part of the event details.

jamescrosswell commented 2 months ago

I was under the impression that the .NET SDK would be responsible for managed code and the Java SDK would provide native support.

That's true, but you can add things to the scope (including attachments) that you want/expect to be sent with both Managed and Native exceptions. This hasn't been implemented for Attachments on Android yet.

bricefriha commented 5 days ago

Just as an update:

Crash generated with parameter CrashType.JavaBackgroundThread is reported twice: one with mechanism UncaughtExceptionHandler and contains no attachments, and another with mechanism AppDomain.UnhandledException and contains both attachments;

This is being addressed by PR #3756

Crash generated with parameter CrashType.Native is reported with mechanism signalhandler and contains no attachments;

This is going to be tackled separately on another PR, as these two issues are not related directly