feat(browser): error filters and linked error causes - #1146
Conversation
Errors a customer's page throws are not all theirs to fix: browser extensions inject scripts, and ResizeObserver reports benign loop notices. They had no way to drop those, and every one became an issue. - errors.ignore / denyUrls / allowUrls / beforeCapture, checked before an error span exists, on both the global handlers and captureException. URL lists match the top frame's script URL, parsed from V8 and SpiderMonkey/JavaScriptCore stacks (window.onerror's filename when there is no stack). - Extension frames and ResizeObserver loop notices are dropped by default; errors.defaultFilters: false keeps them. - error.cause chains and AggregateError members (up to five) are appended to exception.stacktrace as "Caused by:" blocks after the error's own frames. Fingerprints hash the top three frames, so only errors with fewer than three frames of their own can move to a new issue. The error object is passed through untouched when nothing is linked, and `code` is kept otherwise, so exception.type never changes. - Eager budget 41 -> 42 kB, first-party 16 -> 17 kB for the capture-path code.
Maple reviewConfidence 3/5 · needs attention Adds a configurable
FindingsWarning · F1 ·
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (13)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe browser SDK adds configurable error filters for captured errors and formats linked causes in exception stacks. Capture handlers apply the filters before recording errors. Configuration, initialization, tests, documentation, and size budgets are updated to cover these changes. ChangesBrowser error capture
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant CaptureEntryPoints
participant recordException
participant shouldCapture
participant recordFailure
participant exceptionOf
participant OpenTelemetry
CaptureEntryPoints->>recordException: Pass source, error, and optional filename
recordException->>shouldCapture: Check normalized error and capture context
shouldCapture-->>recordException: Return capture decision
recordException->>recordFailure: Record accepted error
recordFailure->>exceptionOf: Build exception with cause stack
exceptionOf-->>recordFailure: Return exception representation
recordFailure->>OpenTelemetry: Record exception
Suggested reviewers: Merge Risk: ⚪ Minimal · up to Error filtering and linked-cause reporting appear consistent with the documented behavior. No actionable merge-blocking defect was established; merge after normal checks pass. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new filters remain within the application’s existing error-reporting boundary. Linked-error formatting introduces a bounded diagnostic failure risk, and it expands the information included in reports. No privilege escalation or cross-tenant exposure was established. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 12 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![maple-review-bot[bot]](https://avatars.githubusercontent.com/in/4108169?s=60&v=4){kind=small}
…arries
An Error wrapped around a non-Error value (a string rejection,
captureException("...")) has this SDK's frames, so denyUrls/allowUrls
matched Maple's bundle and window.onerror's filename was never used. Only a
thrown Error's own stack is matched now; window.onerror passes its filename
as the frame when no Error was thrown.
Maple reviewConfidence 4/5 · likely safe to merge Adds client-side error filters (
FindingsNote · F2 · A
|
A g/y RegExp keeps lastIndex between test() calls, so ignore/denyUrls/ allowUrls with /chunk/g dropped only every other matching error. Reset lastIndex before each test.
Maple reviewConfidence 4/5 · likely safe to merge Adds
Fixed since the last review
What was checked
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (!stack) return [] | ||
| const urls: string[] = [] | ||
| for (const line of stack.split("\n")) { | ||
| const url = FRAME_URL.exec(line)?.[1] |
2 participants
Part 2 of the browser SDK stack. Based on #1145.
Errors a customer's page throws are not all theirs to fix: browser extensions inject scripts, and
ResizeObserverreports benign loop notices. Until now there was no way to drop them, and each one became an issue.What changes
Filters (
errorsoption), checked before an error span exists, on both the global handlers andcaptureException:ignore: strings / RegExps matched againstName: message.denyUrls/allowUrls: matched against the top frame's script URL, parsed from V8 and SpiderMonkey/JavaScriptCore stacks (window.onerror'sfilenamewhen there is no stack). Errors with no frames are kept byallowUrls.beforeCapture(error, { source, originalError }): returnfalseto drop. A hook that throws keeps the error.chrome-extension://,moz-extension://,safari-web-extension://, ...) and theResizeObserver loopnotices are dropped.errors.defaultFilters: falsekeeps them.Filtering happens at capture time, not in a span processor, because a
SpanProcessorcannot drop a span, and a dropped error should cost nothing.Linked errors
error.causechains andAggregateErrormembers (up to five, cycle-safe) are appended toexception.stacktraceasCaused by:blocks after the error's own frames, the way OTel Java records aThrowable.error_events_mv), so only errors with fewer than three frames of their own can move to a new issue.recordExceptionuntouched when nothing is linked, andcodeis kept otherwise (OTel prefers it overnameforexception.type), soexception.typenever changes.Eager budget 41 → 42 kB and first-party 16 → 17 kB: this code runs on the capture path.
Testing
codepreserved).captureException, an extension-thrown uncaught error, and the cause chain landing inexception.stacktrace.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Summary by CodeRabbit
ResizeObserver loopnotices are filtered by default; default filters can be disabled.causeandAggregateErrorentries, up to five linked errors.