fix: exceptions thrown from BeforeBreadcrumb, log filters and BeforeScreenshotCapture no longer reach the application - #5606
jamescrosswell wants to merge 5 commits into
Conversation
…creenshotCapture no longer reach the application Completes the callback-isolation work item from #5535 that #5545 started for TracesSampler. Each callback now runs inside a recovery boundary that logs an error naming the callback and applies the spec's fallback: - BeforeBreadcrumb (Scope.AddBreadcrumb, Android JNI, Cocoa) drops the breadcrumb - ILogEntryFilter.Filter treats a failing filter as "did not filter" - SetBeforeScreenshotCapture skips the screenshot and keeps the event Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5606 +/- ##
==========================================
+ Coverage 74.85% 74.92% +0.07%
==========================================
Files 515 515
Lines 18962 18966 +4
Branches 3694 3696 +2
==========================================
+ Hits 14194 14211 +17
+ Misses 3891 3876 -15
- Partials 877 879 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…ndroid device tests Addresses review feedback on #5606: distinguish native callback failures from managed ones in the diagnostic log. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ric-oliv
left a comment
There was a problem hiding this comment.
Looks good! Just one comment about the naming.
…ng it through Review feedback on #5606. Failing open meant a filter written to exclude entries would, once broken, start sending them. Before this PR the exception reached the app and nothing was sent, so dropping keeps that outcome while removing the crash, and matches the filter rule applied to BeforeSend in #5610. Also aligns the internal screenshot callback names with the public SetBeforeScreenshotCapture, and fixes a doc sample naming a method that no longer exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ShouldCaptureEvent and ShouldAddBreadcrumb each ran the whole filter chain, so a user filter was invoked twice for every entry that cleared both thresholds — and, since the previous commit, logged its failure twice. The two predicates now test only their level threshold, and the shared guard runs once in Log. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| EventId eventId, | ||
| Exception? exception) |
There was a problem hiding this comment.
Bug: The use of LINQ.Any() for log entry filtering causes short-circuiting when a filter throws an exception, preventing subsequent filters from running, contrary to the PR description.
Severity: LOW
Suggested Fix
To ensure all filters are evaluated even if one throws, replace the _options.Filters.Any(...) implementation with a foreach loop that iterates through all filters. Inside the loop, call IsFiltered for each filter. Track whether any filter returns true. This change would align the code's behavior with the PR description's claim that all filters get to decide.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location: src/Sentry.Extensions.Logging/SentryLogger.cs#L179-L180
Potential issue: The implementation of log entry filtering uses `LINQ.Any()`. When a
filter throws an exception, it is caught and the internal `IsFiltered` method returns
`true`. This causes `Any()` to short-circuit and immediately decide to filter the log
entry, preventing any subsequent filters in the collection from being evaluated. This
behavior contradicts the pull request's description, which claims that "the remaining
filters still get to decide." While the code's behavior to drop the entry on filter
failure is intentional, the short-circuiting prevents other filters from running, which
might be unexpected based on the documentation.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
Half right, and the useful half: the PR description was stale, not the code. I have rewritten it.
That sentence ("the remaining filters still get to decide") was written when a throwing filter returned false — "did not filter" — where continuing the chain genuinely mattered. After r4081841005 a throwing filter returns true, and the sentence should have gone with it.
Not changing the code, though. IsFiltered answers one question — "is this entry filtered out?" — and once any filter says yes, the entry is dropped no matter what the others would have said. Running them anyway cannot change the outcome; it only invokes more user code, which is the opposite of what the commit right above this one was for (filters used to run twice per entry; they now run once). Any() here is also exactly as short-circuiting as the All(f => !f.Filter(...)) it replaced, which stopped at the first filter returning true.
The tests do not assert the stale claim either — the one that did (LogCritical_FilterThrows_OtherFiltersStillApply) was replaced in the same commit that changed the semantics.
Finishes work item 1 ("isolate the callbacks that can reach the host app") of #5535. #5545 already covered
TracesSampleron all three paths; this covers the remaining five boxes.Each callback now runs inside a recovery boundary that logs an error naming the callback and applies the fallback the Callback Error Isolation spec asks for. Nothing changes for anyone whose callbacks don't throw, and there's no public API change.
BeforeBreadcrumbScope.AddBreadcrumbBeforeBreadcrumbPlatforms/Android/Callbacks/BeforeBreadcrumbCallbacknull— drop the breadcrumbBeforeBreadcrumbPlatforms/Cocoa/SentrySdknull— drop the breadcrumbILogEntryFilter.FilterSentry.Extensions.Logging/SentryLoggerSetBeforeScreenshotCaptureSentry.Maui/Internal/SentryMauiScreenshotProcessorNotes for review:
Scope.AddBreadcrumbwas only accidentally isolated before:HubExtensions.AddBreadcrumbhappens to route throughHub.ConfigureScope's catch-all, butScope.AddBreadcrumbis public and a direct call was unprotected — and the catch-all's"Failure to ConfigureScope"doesn't tell anyone their breadcrumb callback is broken.ILogEntryFilterdrops the entry, rather than letting it through as "did not filter". That was review feedback, and the argument for it is that before this PR the exception reached the app and nothing was sent — so failing open would have changed the data behaviour on top of fixing the crash, in the direction that leaks. A filter written to exclude entries would, once broken, start sending them. Dropping keeps the old outcome and only removes the crash. It also matches the spec rationale for filters and what fix: an exception in BeforeSend or BeforeSendTransaction now drops the item instead of sending it #5610 does forBeforeSend. (Conform to the callback error isolation spec (wrap all user callbacks) #5535 proposed "treat as did not filter";ILogEntryFilterisn't in the spec matrix, so that line was a judgement call rather than anything normative, and it was the wrong one.)ShouldCaptureEventandShouldAddBreadcrumbeach ran the whole chain, so user filter code was invoked twice for any entry clearing both thresholds — and, once failures are logged, logged twice. Their shared tail is now a single early return inLog, leaving the two predicates to test only their own level threshold. Never zero:IsEnabledalready guarantees at least one threshold is met, so anything reaching that line evaluated the chain at least once before.SetBeforeScreenshotCapture(BeforeCaptureInternal→BeforeScreenshotCaptureInternal), and a doc sample calling a non-existentSetBeforeCaptureis fixed. The public parameter is still namedbeforeCapture— parameter names are in the API approval snapshots and renaming breaks named arguments, so that's left alone.SetBeforeScreenshotCapturepreviously unwound to theHub.CaptureEventcatch-all and dropped the whole event, losing the error the user was trying to report.BeforeBreadcrumbhas no report in the spec matrix, andILogEntryFilter/SetBeforeScreenshotCapturearen't in it. Work items 2 (fix: telemetry dropped by user callbacks and by validation is now counted in client reports #5607) and 3 (fix: an exception in BeforeSend or BeforeSendTransaction now drops the item instead of sending it #5610) of Conform to the callback error isolation spec (wrap all user callbacks) #5535 cover the reports and theBeforeSendalignment, so this doesn't close the issue.BeforeBreadcrumbCallbackTests, run on API 34 and 36 across net9.0/net10.0). The Cocoa bridge isn't: itsBeforeBreadcrumbis a lambda assigned inline inInitSentryCocoaSdkwith no seam to invoke without initialising the native SDK. Extracting it would make it testable the same way — worth doing, but as its own change.🤖 Generated with Claude Code