Skip to content

Conform to the callback error isolation spec (wrap all user callbacks) #5535

Description

@jamescrosswell

Summary

The Hooks spec gained a Callback Error Isolation section (spec 1.1.0, candidate) in getsentry/sentry-docs#19189, following INC-2332, where an exception thrown from a user-provided traces_sampler took down an ingest path.

sentry-dotnet partially implements the spec and deviates from it in several places. This issue tracks bringing us into conformance.

The spec requires, for every user-provided callback:

  • invoke it inside a recovery boundary — a failure MUST NOT reach the host application;
  • emit an error-level internal log naming the callback;
  • MUST NOT re-throw, capture the failure as an event, or attach it to the item;
  • apply the per-callback fallback matrix (filters drop, samplers fall back as if unconfigured);
  • record the same client report a deliberate drop would produce.

Audited against main @ f2df15b.

Implementation / Status


Conformance table

Callback Spec behaviour on failure sentry-dotnet today
BeforeSend Drop event, report before_send/error Logs, adds a breadcrumb containing the exception message + stack trace, keeps and sends the event ❌
BeforeSendTransaction Drop transaction, report before_send/transaction + spans Same as above — breadcrumb, keeps and sends ❌
BeforeSendFeedback Drop feedback, report before_send/feedback Logs, drops, correct client report ✅
BeforeSendLog Drop log, report before_send/log_item Logs, drops, no client report ⚠️
BeforeSendMetric Drop metric, report before_send/trace_metric Logs, drops, no client report ⚠️
BeforeBreadcrumb Drop breadcrumb, no report Managed: dropped only by the Hub.ConfigureScope catch-all, which logs "Failure to ConfigureScope" rather than naming the callback. Native bridges (Android/Cocoa): unguarded ❌
TracesSampler Fall back as if unconfigured, report sample_rate if sampled out Unguarded on all three paths (managed, Android, Cocoa) ❌
Event processors Drop event, report event_processor + category Dropped by the Hub.CaptureEvent catch-all; no client report; log names the capture, not the processor ⚠️
BeforeSendCheckIn Drop check-in Not implemented (#4538) n/a
BeforeSendSpan Emit the span Not implemented n/a
ProfilesSampler Fall back as if unconfigured Not implemented (we only have ProfilesSampleRate) n/a
ErrorSampler Fall back to SampleRate Not implemented (spec says MAY) n/a

Two .NET-specific callbacks are not in the spec's matrix but are covered by "applies to every user-provided callback":

Callback Sensible fallback Today
ILogEntryFilter.Filter (Sentry.Extensions.Logging) Treat as "did not filter" Unguarded — throws into the application's own ILogger.Log call ❌
SetBeforeScreenshotCapture (Sentry.Maui) Skip the screenshot Unguarded; unwinds to the Hub.CaptureEvent catch-all and drops the whole event ❌

Work items

1. Isolate the callbacks that can reach the host app

The highest-value fix. No public API change, and no behaviour change for anyone whose callbacks don't throw.

2. Emit the missing client reports

The spec is explicit that "isolating a callback without reporting the loss is a conformance failure".

No new DiscardReason is needed — the spec rules out internal_sdk_error and says sampler failure is deliberately indistinguishable from ordinary sampling.

3. Align BeforeSend / BeforeSendTransaction with the matrix

This deviates twice over. The spec says a failure MUST NOT be attached to the item, and the matrix says drop — with the rationale spelled out: "filters drop because a callback that failed part-way may not have applied the redaction the user wrote it to apply."

That's the realistic case here, and it's a data-protection bug rather than a style preference: the commonest job for BeforeSend in .NET is PII scrubbing, so a callback that throws mid-scrub currently causes us to send both the partially-redacted event and the exception message and stack trace we stapled to it. A user's redaction failure turns into two kinds of unintended data landing in Sentry.

This is therefore being treated as a bug/security fix and can ship in a minor — it is not held back for 7.0.0, and it does not gate on the spec leaving candidate.

Notes for whoever picks this up:

  • Still user-visible for anyone whose BeforeSend throws today (they currently get a degraded event; they will now get none), so it needs a clear changelog line and is worth calling out in the release notes.
  • CaptureTransaction_BeforeSendTransactionThrows_ErrorToEventBreadcrumb in SentryClientTests.verify.cs pins the current behaviour and will need replacing.
  • The "defensive copy" SHOULD in the spec only applies to callbacks that keep the item, i.e. before_send_span, which we don't implement — nothing to do there.

Already conformant — please don't re-do these

  • ConfigureScope / ConfigureScopeAsync — guarded in Hub (this is the .NET issue cited in the Linear project).
  • BeforeSendFeedback — logs, drops, correct discard reason.
  • CrashedLastRun — guarded in GlobalSessionManager.
  • OnCrashedLastRun (Cocoa) and BeforeSend (Android bridge) — both wrapped.
  • ProcessOnBeforeSend (Cocoa native events) — has its own try/catch.
  • Scope.OnEvaluating — guarded.

And when BeforeSendCheckIn (#4538), BeforeSendSpan, ProfilesSampler or ErrorSampler land, they should be born inside a recovery boundary rather than retrofitted.


Refs:

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

.NETPull requests that update .net codeBugSomething isn't workingImprovementTraces

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions