sentry-dotnet icon indicating copy to clipboard operation
sentry-dotnet copied to clipboard

Add managed attachments to native events

Open TimBurik opened this issue 1 year ago • 6 comments

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);
});
  1. 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

  • Crash generated with parameter CrashType.Native is reported with mechanism signalhandler and contains no attachments;
  • 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;
  • All other crash types (CrashType.Managed, CrashType.ManagedBackgroundThread, CrashType.Java) are reported with mechanism AppDomain.UnhandledException and contain both attachments.

This issue might be related to #3461

Reproduction sample: SampleProject.zip

TimBurik avatar Aug 20 '24 13:08 TimBurik

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

TimBurik avatar Aug 20 '24 15:08 TimBurik

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

I think this is similar to:

  • https://github.com/getsentry/sentry-dotnet/issues/3549

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

jamescrosswell avatar Aug 23 '24 02:08 jamescrosswell

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?

  • Should the Java SDK be syncing it's scope via a scope action callback and so should it also contain the attachments?
  • 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?

jamescrosswell avatar Aug 25 '24 22:08 jamescrosswell

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

jamescrosswell avatar Aug 25 '24 23:08 jamescrosswell

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.

bitsandfoxes avatar Sep 04 '24 11:09 bitsandfoxes

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.

jamescrosswell avatar Sep 04 '24 22:09 jamescrosswell

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

bricefriha avatar Nov 21 '24 15:11 bricefriha

@jamescrosswell this PR was closed, we need another approach?

  • https://github.com/getsentry/sentry-dotnet/pull/3756

bruno-garcia avatar Dec 09 '24 23:12 bruno-garcia

@jamescrosswell this PR was closed, we need another approach?

Yes we do. That PR simply discarded all Java exceptions... we need to identify only those that are duplicates of managed .NET exceptions.

jamescrosswell avatar Dec 10 '24 06:12 jamescrosswell

@jamescrosswell are there any updates on the issue?

TimBurik avatar Jan 29 '25 10:01 TimBurik

@jamescrosswell are there any updates on the issue?

Apologies for the wait @TimBurik - it's near the top of the backlog... we have some extra folks coming on board to help maintain the repo, so we should be able to get to stuff like this soon.

jamescrosswell avatar Jan 29 '25 21:01 jamescrosswell

Is this issue covering attaching logcat logs into unhandled SIGABRT crashes like this one?

libc abort

split_config.arm64_v8a.apk xamarin::android::Helpers::abort_application
split_config.arm64_v8a.apk xamarin::android::internal::MonodroidRuntime::mono_log_handler
split_config.arm64_v8a.apk mono_debugger_agent_unhandled_exception
split_config.arm64_v8a.apk mono_debugger_agent_unhandled_exception
split_config.arm64_v8a.apk mono_gc_register_bridge_callbacks
split_config.arm64_v8a.apk mono_gc_register_bridge_callbacks
split_config.arm64_v8a.apk mono_gc_wbarrier_generic_store_internal
split_config.arm64_v8a.apk mono_gc_make_root_descr_all_refs
split_config.arm64_v8a.apk mono_gc_make_root_descr_all_refs
split_config.arm64_v8a.apk mono_gc_wbarrier_generic_store_internal
split_config.arm64_v8a.apk mono_sgen_mono_ilgen_init
split_config.arm64_v8a.apk mono_gc_deregister_root
split_config.arm64_v8a.apk mono_array_new_full_checked
split_config.arm64_v8a.apk mono_stack_mark_record_size
split_config.arm64_v8a.apk mono_object_clone
split_config.arm64_v8a.apk mono_lookup_icall_symbol

albyrock87 avatar Feb 21 '25 17:02 albyrock87

@albyrock87 not specifically no. Sentry should be capturing logcat attachments now however: https://github.com/getsentry/sentry-dotnet/blob/1ecab829c0c7d6533ba0cbd24dac9ac1e7caf6ba/test/Sentry.Maui.Tests/SentryMauiLogcatsTests.cs#L64

And it should be possible to preview these in Sentry now too.

jamescrosswell avatar Feb 23 '25 22:02 jamescrosswell

@jamescrosswell we can see logcat logs when it's a dotnet crash, but when it's native, or at least part of the mono layer like above, the logcat logs are missing.

albyrock87 avatar Feb 23 '25 23:02 albyrock87

Ah, actually, that looks like the first scenario that Tim described in the description for this issue (SIGABRT would be a native crash - not sure how much we'll be able to capture for a SIGABRT, given that the process is being terminated, but we can experiment and try).

jamescrosswell avatar Feb 24 '25 02:02 jamescrosswell

@jamescrosswell Is there any timeline on this fix? I am also not seeing attachments getting logged for iOS native crashes. Is this being tracked in this issue?

ramit-2343113 avatar Mar 03 '25 18:03 ramit-2343113

@jamescrosswell Is there any timeline on this fix? I am also not seeing attachments getting logged for iOS native crashes. Is this being tracked in this issue?

@ramit-2343113 this issue is specifically related to Android. If you're having problems with attachments in iOS then you should open a new issue for that.

jamescrosswell avatar Mar 03 '25 22:03 jamescrosswell

Hey @jamescrosswell what's the latest on this. I saw some mentioned Issues but is this also fixed?

Angelodaniel avatar Sep 02 '25 12:09 Angelodaniel

@Angelodaniel the upcoming net10.0 release is keeping us pretty busy at the moment.

It seems like quite a few folks want managed attachments for native events though.

Can I ask what the use case is and why this is important? That might help us prioritise this once the net10.0 stuff is done.

jamescrosswell avatar Sep 02 '25 22:09 jamescrosswell

@jamescrosswell I can describe the flow in our case, if it might help:

  • We are storing user's session log and other useful session context in a persistent storage on device. Each session has a unique identifier, so we are able to find information even for a couple of sessions back;
  • When using Sentry, we are passing this unique Session Id into the root scope as an "Extra", so it is propagated into the native layers and always present in the reported event;
  • Then, when crash happens, we are using the OnBeforeSend callback to post-process the captured event - specifically, extract Session Id from the event, find the session in persistent storage and attach session context to the report.
  • As you can see this is actually the same flow for both native and managed crash events, that's why it is surprising that the behavior is different. We know for sure that we are able to find and attach context for the session with the native crash, it's just get.. discarded.

I believe the root of the issue is related to how Java hints are converted to managed hint to be passed to OnBeforeSend: https://github.com/getsentry/sentry-dotnet/blob/89fe0878972189e05d3e699457ec0f357624e53f/src/Sentry/Platforms/Android/Callbacks/BeforeSendCallback.cs#L22-L39 https://github.com/getsentry/sentry-dotnet/blob/89fe0878972189e05d3e699457ec0f357624e53f/src/Sentry/Platforms/Android/Extensions/HintExtensions.cs#L5-L19

TimBurik avatar Sep 03 '25 07:09 TimBurik

OK thanks @TimBurik, that makes sense. And you're right, this hasn't yet been implemented in the hint extensions and the attachment extensions.

@Angelodaniel are you doing something similar or would this solve a different problem for you?

jamescrosswell avatar Sep 03 '25 08:09 jamescrosswell