getsentry / getsentry/sentry-dotnet
Conform to the callback error isolation spec (wrap all user callbacks)
- Dominant language
- C#
- Stars
- 770
- Forks
- 248
- Avg merge
- 2d 22h
- Merged PRs (30d)
- 51
Description
## Summary
The `Hooks` spec gained a **Callback Error Isolation** section (spec 1.1.0, candidate) in getsentry/sentry-docs#19189, following [INC-2332](https://linear.app/getsentry/project/wrap-all-sdk-user-callbacks-in-a-try-catch-6bfdbe9f7268), 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` @ f2df15bd.
---
## 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.
- [ ] `TracesSampler` — [`Internal/Hub.cs:208`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry/Internal/Hub.cs#L208). Invoked bare inside `StartTransaction`, so a throwing sampler propagates out of `SentrySdk.StartTransaction` into the caller. **The fallback the spec asks for is already there**: leaving `isSampled` null falls through to the branch that honours an inherited decision first (`context.IsSampled ?? …`) and only then the static `TracesSampleRate`, which is exactly *"fall back as if unconfigured, including any inherited decision that takes precedence over the static rate"*.
Where that actually reaches the host application — **not** ASP.NET Core, which is already isolated: [`SentryTracingMiddleware.TryStartTransaction`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry.AspNetCore/SentryTracingMiddleware.cs#L93) wraps the call, logs `"Failed to start transaction."` and returns `null`. (It drops the transaction rather than falling back, and the log names the caller rather than the callback, so it still deviates from the spec — but it does not reach the app.) The paths that do:
- ASP.NET **classic** — [`Sentry.AspNet/HttpContextExtensions.cs:120`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry.AspNet/HttpContextExtensions.cs#L120) is unguarded, and `StartSentryTransaction()` is called from the user's own `Application_BeginRequest`. That is a 500 on every request — the same shape as [RUBY-49](https://github.com/getsentry/sentry-ruby/issues/49) and the incident itself.
- OpenTelemetry — [`SentrySpanProcessor.OnStart`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry.OpenTelemetry/SentrySpanProcessor.cs#L169) is unguarded, so the exception surfaces inside the user's `ActivitySource.StartActivity`. Reachable through `UseOpenTelemetry()`, whose `disableSentryTracing` parameter defaults to `false`; the Exporter integration sets it `true`, after which `Hub.StartTransaction` returns before the sampler runs.
- Any direct `SentrySdk.StartTransaction` / `hub.StartTransaction` call in user code.
- [ ] `TracesSampler` — [`Platforms/Android/Callbacks/TracesSamplerCallback.cs:17`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry/Platforms/Android/Callbacks/TracesSamplerCallback.cs#L17). Called back from Java over JNI; an escaping managed exception surfaces as an app crash.
- [ ] `TracesSampler` — [`Platforms/Cocoa/SentrySdk.cs:83`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry/Platforms/Cocoa/SentrySdk.cs#L83). Same, invoked from Objective-C.
- [ ] `BeforeBreadcrumb` — [`Platforms/Android/Callbacks/BeforeBreadcrumbCallback.cs:21`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry/Platforms/Android/Callbacks/BeforeBreadcrumbCallback.cs#L21) and [`Platforms/Cocoa/SentrySdk.cs:65`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry/Platforms/Cocoa/SentrySdk.cs#L65). Note `BeforeSendCallback` right next door *is* wrapped, with a comment saying *"because this can go out to user code, we want to prevent external crashing"* — the pattern exists, it just wasn't applied to its neighbours.
- [ ] `ILogEntryFilter.Filter` — [`Sentry.Extensions.Logging/SentryLogger.cs:164,179`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry.Extensions.Logging/SentryLogger.cs#L164).
- [ ] `SetBeforeScreenshotCapture` — [`Sentry.Maui/Internal/SentryMauiScreenshotProcessor.cs:21`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry.Maui/Internal/SentryMauiScreenshotProcessor.cs#L21). Log and skip the screenshot rather than losing the error the user was trying to report.
- [ ] `BeforeBreadcrumb` — guard [`Scope.cs:338`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry/Scope.cs#L338) itself. Managed `BeforeBreadcrumb` is only *accidentally* isolated: `HubExtensions.AddBreadcrumb` happens to route through `Hub.ConfigureScope`, which catches everything. `Scope.AddBreadcrumb` is public, so calling it directly is unprotected — and the rescue logs `"Failure to ConfigureScope"`, which doesn't tell anyone their breadcrumb callback is broken (spec: the log **MUST** name the callback).
### 2. Emit the missing client reports
The spec is explicit that *"isolating a callback without reporting the loss is a conformance failure"*.
- [ ] `BeforeSendLog` — [`Internal/DefaultSentryStructuredLogger.cs:92`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry/Internal/DefaultSentryStructuredLogger.cs#L92). The catch is a bare `return`, and so is the ordinary `return null` path — a user dropping logs via `BeforeSendLog` currently produces no `before_send` discards at all. Same for the `configureLog` callback at [line 69](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry/Internal/DefaultSentryStructuredLogger.cs#L69). `DataCategory.LogItem` already exists.
- [ ] `BeforeSendMetric` — [`Internal/DefaultSentryMetricEmitter.cs:75`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry/Internal/DefaultSentryMetricEmitter.cs#L75). Identical; `DataCategory.TraceMetric` already exists.
- [ ] Event processors — a throw skips the `RecordDiscardedEvent(EventProcessor, …)` branch in [`Internal/SentryEventHelper.cs:23`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry/Internal/SentryEventHelper.cs#L23) and lands in the `Hub.CaptureEvent` catch-all at [`Hub.cs:687`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry/Internal/Hub.cs#L687), which records nothing. This is precisely the control-flow trap the Linear write-up calls out ("a catch that returns directly bypasses that branch and drops silently").
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
- [ ] [`Internal/SentryEventHelper.cs:51`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry/Internal/SentryEventHelper.cs#L51) and [`SentryClient.cs:259`](https://github.com/getsentry/sentry-dotnet/blob/main/src/Sentry/SentryClient.cs#L259) currently demystify the exception, add a `"BeforeSend callback failed."` breadcrumb carrying its message and stack trace, and **send the item anyway**.
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:
- Spec PR: getsentry/sentry-docs#19189
- Linear: https://linear.app/getsentry/project/wrap-all-sdk-user-callbacks-in-a-try-catch-6bfdbe9f7268
Contributor guide
Research direction
Start with the callback entry points listed in Internal/Hub.cs, Platforms/Android/Callbacks, Platforms/Cocoa/SentrySdk.cs, Sentry.Extensions.Logging/SentryLogger.cs, and Sentry.Maui/Internal/SentryMauiScreenshotProcessor.cs. Review SentryEventHelper.cs, DefaultSentryStructuredLogger.cs, DefaultSentryMetricEmitter.cs, and SentryClientTests.verify.cs for current discard and BeforeSend behavior. Done means every listed callback follows the conformance matrix, including named error logs and required client reports.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- android, csharp, ios
- Domain
- devtools
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100