Repository navigation
fix(android): drop native events when BeforeSend conversion fails - #5868
osamashtayat wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5868 +/- ##
==========================================
+ Coverage 76.57% 76.67% +0.10%
==========================================
Files 528 528
Lines 19624 19524 -100
Branches 3873 3840 -33
==========================================
- Hits 15028 14971 -57
+ Misses 3711 3669 -42
+ Partials 885 884 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for the PR @osamashtayat ❤️
Hm... that's a fair question. One option would be to change: - return null;
+ return options.Native.EnableBeforeSend && options.BeforeSendInternal != null ? null : e;So you'd only return null (dropping the event) if the SDK user had set That seems like a hack/workaround though - inside this handler we're trying to guess/infer whether the code is being executed to handle a segv signal or not and adapt the behaviour of the code accordingly. Probably a cleaner solution would be to have the sigsegv errors trigger a different code path to the custom beforesend handlers that SDK users wire up. Java’s SentryOptions has a single BeforeSend slot, so the two jobs need to be chained... so something like this in JavaSdk.SentryOptions.IBeforeSendCallback? beforeSend = null;
if (options.Native.EnableBeforeSend && options.BeforeSendInternal is not null)
{
beforeSend = new BeforeSendCallback(BeforeSendWrapper(options), options, o);
}
if (options.Android.SuppressSegfaults)
{
beforeSend = new SegfaultSuppressionCallback(beforeSend);
}
o.BeforeSend = beforeSend; |
|
To fill in the gap above: internal class SegfaultSuppressionCallback : JavaObject, JavaSdk.SentryOptions.IBeforeSendCallback
{
private readonly JavaSdk.SentryOptions.IBeforeSendCallback? _next;
public SegfaultSuppressionCallback(JavaSdk.SentryOptions.IBeforeSendCallback? next) => _next = next;
public JavaSdk.SentryEvent? Execute(JavaSdk.SentryEvent e, JavaSdk.Hint h) =>
e.Exceptions?.FirstOrDefault() is { Type: "SIGSEGV", Value: "Segfault" }
? null
: _next is null ? e : _next.Execute(e, h);
}With that in place, (A Java |
Co-Authored-By: Codex <noreply@openai.com>
|
Thanks for the review @jamescrosswell. I addressed both comments in Both callbacks share Java’s I ran the Android emulator suite: 2,933 passed, 0 failed, 27 skipped. All 12 callback-related cases passed, including the conversion regressions and the new tests for callback ordering and preserving a filter’s edits or drop decision. |
Description
Problem
On Android, when native
BeforeSendis enabled, a Java/NDK event that cannot be converted to or from a managedSentryEventis currently sent unfiltered.BeforeSendCallback.Executeconverts the original Java event to a managed event, runs the user'sBeforeSend, then converts the result back to Java. If either conversion throws, the exception handler logs the error and returnse— the original Java event. That means:BeforeSendnever runs and the raw event is sent;Users enable
Native.EnableBeforeSendspecifically to redact native events, so sending the raw event when filtering cannot run defeats the purpose.Root cause
BeforeSendCallback.ExecutewrapsToSentryEvent,ToHint, the user callback, andToJavaSentryEventin onetryblock. Itscatchreturns the original Java event:Returning
ebypasses whatever the managed filter did, or would have done.Fix
Return
nullon conversion failure instead of returning the original event:The Java SDK treats a
nullBeforeSendresult as a drop and records abefore_sendclient report. This is consistent withBeforeSendWrapper, which already returnsnullwhen the user's callback throws (#5610), and with the other Android callback bridges.Successful conversions, explicit callback drops, and SIGSEGV suppression are unchanged. There is no public API change and no new dependency.
Tests
Added
BeforeSendCallbackTestswith three Android device tests:ServerName;ServerName.Verified on an API 34 arm64 emulator using the repository's device-test script:
Also green:
Sentry.Testson .NET 8, 9, and 10 (0 failures)Sentry.Testsnet10.0-android36.0 build (0 warnings, 0 errors)dotnet formatandgit diff --checkThe full macOS solution build could not complete on the local machine because of unrelated host tooling issues (Intel
protocon Apple Silicon, sample DSN placeholders, and the Cocoa-from-source step). Android CI should be the authoritative device check.Maintainer decision
Android.SuppressSegfaultsalone also installs this bridge. With this change, an event whose conversion fails is dropped even if it might not have been a SIGSEGV.I recommend dropping it because the SDK cannot evaluate the configured suppression rule. Please confirm this is the desired policy.
Issues
Closes #5764