Open
Conversation
📝 WalkthroughWalkthroughThis PR implements low-value network error sampling and detection for Sentry error instrumentation. It adds URL extraction from browser network errors and breadcrumbs, first-party API classification logic, a sampling decision system to drop or tag noisy errors, and breadcrumb enrichment with first-party metadata. Core changes span error classification, URL sanitization, and beforeSend hook integration. ChangesLow-Value Network Error Sampling & Breadcrumb Enrichment
Sequence DiagramsequenceDiagram
participant Browser as Browser/App
participant Error as Error Handler
participant Sentry as Sentry SDK
participant BeforeSend as beforeSend Hook
participant Breadcrumbs as Breadcrumb System
participant Decision as Sampling Decision
participant Transport as Sentry Transport
Browser->>Error: Network error occurs (fetch fails)
Error->>Breadcrumbs: HTTP breadcrumb recorded (status 0)
Error->>Sentry: captureException(error)
Sentry->>BeforeSend: beforeSend(event)
BeforeSend->>Error: Classify as app-wrapped or raw browser error
alt Raw Browser Network Error
Error->>Breadcrumbs: Extract latest failed fetch breadcrumb URL
Error->>BeforeSend: Rewrite exception message with breadcrumb URL
else App-Wrapped Error
Error->>BeforeSend: Preserve original message
end
BeforeSend->>Decision: getLowValueNetworkErrorDecision(event)
Decision->>Decision: Extract URL candidates from breadcrumbs
Decision->>Decision: Detect first-party vs third-party targets
Decision->>Decision: Apply stable-hash sampling (10% rate)
alt Sampled Out (Drop)
Decision-->>BeforeSend: "drop"
BeforeSend->>Sentry: return null
Sentry-->>Transport: Event discarded
else Sampled In (Keep)
Decision-->>BeforeSend: "keep_sampled"
BeforeSend->>Error: Tag event with network metadata
BeforeSend->>Sentry: return event (tagged)
Sentry->>Transport: Send tagged event
else Not Applicable (Keep)
Decision-->>BeforeSend: "not_applicable"
BeforeSend->>Sentry: return event
Sentry->>Transport: Send untagged event
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b2cf5a1a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+445
to
+446
| if (latestBreadcrumb?.statusCode !== 0) { | ||
| return false; |
There was a problem hiding this comment.
The low-value network filter currently inspects only the single most recent HTTP breadcrumb with any status, and immediately returns when that status is not 0. In events where a failed first-party request (status_code: 0) is followed by a later successful request before capture, this path returns "not_applicable" and the noisy browser transport error is never sampled/dropped. This undermines the new noise-reduction behavior for realistic multi-request pages; the check should walk back to the most recent failed transport breadcrumb (or otherwise consider matching failed breadcrumbs), not bail on the first non-zero status.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@instrumentation-client.ts`:
- Around line 299-303: The current raw-browser check (isRawBrowserNetworkError
using error instanceof TypeError && isNetworkError(error.message)) is too
permissive and can catch unrelated TypeErrors; tighten it so only genuine
network-related TypeErrors are matched before retagging/fingerprinting. Update
the isRawBrowserNetworkError condition (or refine isNetworkError) to require a
stricter match on error.message (e.g., explicit tokens like "NetworkError",
"Failed to fetch", or a regex with word boundaries like /\bnetwork\b/i) and/or
check error.name equals "NetworkError" where available; keep the existing
isAppWrappedApiNetworkError logic intact so you only retag events when either
isAppWrappedApiNetworkError(error.message) is true or the tightened
isRawBrowserNetworkError is true. Ensure references to
isAppWrappedApiNetworkError, isNetworkError, isRawBrowserNetworkError, and the
error instanceof TypeError check are updated accordingly.
- Around line 139-173: The helper getLatestHttpBreadcrumbUrl currently only
treats explicit status_code === 0 as a failed transport; update
getLatestHttpBreadcrumbUrl (or getLatestNetworkBreadcrumbUrl) to apply the same
failed-transport heuristic used by getLowValueNetworkErrorDecision: if an
HTTP/fetch breadcrumb has no explicit status code but has breadcrumb.level ===
"error" (and is identified as a fetch-type breadcrumb by isHttpBreadcrumb or
equivalent), treat it as status 0 for matching so a later successful breadcrumb
cannot overwrite the failed-transport URL; ensure you reference and reuse the
same predicate logic from getLowValueNetworkErrorDecision to keep behavior
consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1efa7d1e-6312-4aec-a37d-d56b564f0f76
📒 Files selected for processing (6)
__tests__/instrumentation-client.test.ts__tests__/utils/sentry-client-filters.test.ts__tests__/utils/sentry-sanitizer.test.tsinstrumentation-client.tsutils/sentry-client-filters.tsutils/sentry-sanitizer.ts
Comment on lines
+139
to
+173
| function getLatestHttpBreadcrumbUrl( | ||
| event: Sentry.Event, | ||
| statusCode?: number | ||
| ): unknown { | ||
| const breadcrumbs = event.breadcrumbs; | ||
| if (!Array.isArray(breadcrumbs)) { | ||
| return undefined; | ||
| } | ||
|
|
||
| for (let index = breadcrumbs.length - 1; index >= 0; index -= 1) { | ||
| const breadcrumb = breadcrumbs[index]; | ||
| if (!breadcrumb || !isHttpBreadcrumb(breadcrumb)) { | ||
| continue; | ||
| } | ||
|
|
||
| if ( | ||
| statusCode !== undefined && | ||
| getBreadcrumbStatusCode(breadcrumb) !== statusCode | ||
| ) { | ||
| continue; | ||
| } | ||
|
|
||
| const url = breadcrumb.data?.["url"]; | ||
| if (url) { | ||
| return url; | ||
| } | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| function getLatestNetworkBreadcrumbUrl(event: Sentry.Event): unknown { | ||
| return ( | ||
| getLatestHttpBreadcrumbUrl(event, 0) ?? getLatestHttpBreadcrumbUrl(event) | ||
| ); |
There was a problem hiding this comment.
Use the same failed-transport breadcrumb heuristic here as in the sampler.
This helper only treats explicit status_code === 0 as a failed transport. getLowValueNetworkErrorDecision() also treats fetch breadcrumbs with level: "error" and no status as status 0, so a later successful HTTP breadcrumb can rewrite the message with the wrong URL here and make the drop check miss the real failed target.
Proposed fix
function getBreadcrumbStatusCode(breadcrumb: Breadcrumb): number | null {
const data = breadcrumb.data;
return (
getNumericValue(data?.["status_code"]) ??
getNumericValue(data?.["http.response.status_code"])
);
}
+function getBreadcrumbTransportStatusCode(breadcrumb: Breadcrumb): number | null {
+ const statusCode = getBreadcrumbStatusCode(breadcrumb);
+ if (statusCode !== null) {
+ return statusCode;
+ }
+
+ if (breadcrumb.category === "fetch" && breadcrumb.level === "error") {
+ return 0;
+ }
+
+ return null;
+}
+
function getLatestHttpBreadcrumbUrl(
event: Sentry.Event,
statusCode?: number
): unknown {
const breadcrumbs = event.breadcrumbs;
@@
for (let index = breadcrumbs.length - 1; index >= 0; index -= 1) {
const breadcrumb = breadcrumbs[index];
if (!breadcrumb || !isHttpBreadcrumb(breadcrumb)) {
continue;
}
- if (
- statusCode !== undefined &&
- getBreadcrumbStatusCode(breadcrumb) !== statusCode
- ) {
+ const breadcrumbStatus =
+ statusCode === 0
+ ? getBreadcrumbTransportStatusCode(breadcrumb)
+ : getBreadcrumbStatusCode(breadcrumb);
+ if (statusCode !== undefined && breadcrumbStatus !== statusCode) {
continue;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@instrumentation-client.ts` around lines 139 - 173, The helper
getLatestHttpBreadcrumbUrl currently only treats explicit status_code === 0 as a
failed transport; update getLatestHttpBreadcrumbUrl (or
getLatestNetworkBreadcrumbUrl) to apply the same failed-transport heuristic used
by getLowValueNetworkErrorDecision: if an HTTP/fetch breadcrumb has no explicit
status code but has breadcrumb.level === "error" (and is identified as a
fetch-type breadcrumb by isHttpBreadcrumb or equivalent), treat it as status 0
for matching so a later successful breadcrumb cannot overwrite the
failed-transport URL; ensure you reference and reuse the same predicate logic
from getLowValueNetworkErrorDecision to keep behavior consistent.
Comment on lines
+299
to
+303
| const isAppWrappedError = isAppWrappedApiNetworkError(error.message); | ||
| const isRawBrowserNetworkError = | ||
| error instanceof TypeError && isNetworkError(error.message); | ||
|
|
||
| if (!isAppWrappedError && !isRawBrowserNetworkError) { |
There was a problem hiding this comment.
Tighten raw browser network detection before retagging the event.
isNetworkError() still matches any message containing the bare word network, so this branch will rewrite, fingerprint, and potentially sample out unrelated TypeErrors whose text just happens to mention that word.
Proposed fix
- const isRawBrowserNetworkError =
- error instanceof TypeError && isNetworkError(error.message);
+ const normalizedMessage = error.message.toLowerCase();
+ const isRawBrowserNetworkError =
+ error instanceof TypeError &&
+ (
+ normalizedMessage.includes("failed to fetch") ||
+ normalizedMessage.includes("load failed") ||
+ normalizedMessage.includes("networkerror") ||
+ normalizedMessage.includes("network error") ||
+ normalizedMessage.includes("network request failed")
+ );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@instrumentation-client.ts` around lines 299 - 303, The current raw-browser
check (isRawBrowserNetworkError using error instanceof TypeError &&
isNetworkError(error.message)) is too permissive and can catch unrelated
TypeErrors; tighten it so only genuine network-related TypeErrors are matched
before retagging/fingerprinting. Update the isRawBrowserNetworkError condition
(or refine isNetworkError) to require a stricter match on error.message (e.g.,
explicit tokens like "NetworkError", "Failed to fetch", or a regex with word
boundaries like /\bnetwork\b/i) and/or check error.name equals "NetworkError"
where available; keep the existing isAppWrappedApiNetworkError logic intact so
you only retag events when either isAppWrappedApiNetworkError(error.message) is
true or the tightened isRawBrowserNetworkError is true. Ensure references to
isAppWrappedApiNetworkError, isNetworkError, isRawBrowserNetworkError, and the
error instanceof TypeError check are updated accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests