feat(bun): Instrument outgoing fetch requests - #22869
Conversation
|
|
||
| function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNativeFetchCheck: boolean = false): void { | ||
| if (skipNativeFetchCheck && !supportsNativeFetch()) { | ||
| if (!skipNativeFetchCheck && !supportsNativeFetch()) { |
There was a problem hiding this comment.
this check was just incorrect before I think, we only checked this for cloudflare which was the opposite of what we wanted?
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 225988a. Configure here.
| 'http.request.header.host': expect.any(String), | ||
| 'http.request.header.user_agent': expect.stringContaining('Bun'), | ||
| 'http.request.header.baggage': expect.any(String), | ||
| 'http.request.header.sentry_trace': expect.any(String), |
There was a problem hiding this comment.
Flaky header assertions in Bunserver tests
Medium Severity
These Bunserver unit tests now require http.request.header.baggage and http.request.header.sentry_trace on plain fetch calls, but they never install fetchIntegration. Those headers only appear if another file’s init() has already patched global fetch in the shared Bun test process. That violates the flaky-test guidance in the PR review rules: the assertions are order-dependent and fail under isolation or a different file order. Flagged because it was mentioned in this rules file.
Additional Locations (2)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 225988a. Configure here.
There was a problem hiding this comment.
Yeah, I think this is valid. Order/timing dependent assertions.
Probably best to just remove all 6 of the lines referenced? I'm not sure they're doing much beyond being a test-flake landmine.
- packages/bun/test/integrations/bunserver.test.ts#L107-L109
- packages/bun/test/integrations/bunserver.test.ts#L201-L203
If we wanna test these specific keys, we can set them explicitly on the outgoing request in the test (eg, fetch(url, { headers: { baggage: '...', 'sentry-trace': '...' } })) as the existing test at packages/bun/test/integrations/bunserver.test.ts already does. But propagation would be better in the new dev-packages/bun-integration-tests/suites/fetch/ suite, where it is already being covered.
Add a `fetchIntegration` to `@sentry/bun` that patches the global `fetch` to create `http.client` spans, record breadcrumbs, and inject trace-propagation headers. Bun implements `fetch` natively rather than through undici, so the Node SDK's `nativeNodeFetchIntegration` (which subscribes to undici's `diagnostics_channel` events) never fires. The new integration mirrors the Cloudflare approach and replaces `nativeNodeFetchIntegration` in the Bun default integrations. Also fix an inverted guard in core's `instrumentFetch`: `skipNativeFetchCheck` did the opposite of its name — passing `true` performed the browser-only native check, while the default skipped it. Only Cloudflare/Bun passed `true`, so they were the only callers that ran the check. The guard now reads `!skipNativeFetchCheck && !supportsNativeFetch()` so the parameter matches its name. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
225988a to
98d00aa
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNativeFetchCheck: boolean = false): void { | ||
| if (skipNativeFetchCheck && !supportsNativeFetch()) { | ||
| if (!skipNativeFetchCheck && !supportsNativeFetch()) { | ||
| return; |
There was a problem hiding this comment.
Bug: The updated guard condition in addFetchInstrumentationHandler incorrectly prevents fetch instrumentation in browser environments where fetch is polyfilled, such as in browser extensions or test frameworks.
Severity: MEDIUM
Suggested Fix
Add an explicit supportsNativeFetch() guard within integrations like breadcrumbsIntegration before they call addFetchInstrumentationHandler. Alternatively, revise the guard logic in addFetchInstrumentationHandler to accommodate both the intended fix for Bun/Cloudflare and the browser polyfill scenario without causing a regression.
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: packages/core/src/instrument/fetch.ts#L49-L51
Potential issue: The change to the guard condition in `addFetchInstrumentationHandler`
from `if (skipNativeFetchCheck && !supportsNativeFetch())` to `if (!skipNativeFetchCheck
&& !supportsNativeFetch())` introduces a regression. In browser environments with a
polyfilled or wrapped `fetch` implementation, `supportsNativeFetch()` returns `false`.
Integrations like `breadcrumbsIntegration` call `addFetchInstrumentationHandler` with
`skipNativeFetchCheck` as `false` (the default). The new condition `if (!false &&
!false)` evaluates to `true`, causing the function to exit early and silently disable
fetch instrumentation. This results in a loss of fetch-related breadcrumbs and tracing
spans in these specific environments.
Did we get this right? 👍 / 👎 to inform future reviews.
There was a problem hiding this comment.
The bot's context is wrong ("browser environments"), but the issue is real; just for deno rather than browsers.
I went looking for cases where this function is being called, because the guard here is indeed really confusing. One case where it's being called is in the deno breadcrumb handling logic.
This test shows that it passes with the old guard, and not with the new one, so this is definitely a regression: https://gist.github.com/isaacs/1fdf09ad8f6c327e8041bfdd7b035baf
Deno previously didn't have any unit or integration tests for breadcrumbs, which I assume is why this didn't get caught more helpfully.
I'm not sure what the best fix is, but it seems like we have two options:
supportsNativeFetch() passes there. In that case, I think it'd be good to add a comment explaining why it has the double-check, or even rename the variables here, because it's super confusing. (It seems like "don't skip the native check" would mean you ought to do the native check, not the opposite? This is very weird.)
packages/deno/src/integrations/breadcrumbs.ts line 50 becomes: addFetchInstrumentationHandler(_getFetchBreadcrumbHandler(client), true). This is a bit riskier, since it's also called in packages/vercel-edge/src/integrations/wintercg-fetch.ts and a few other places, and we'd need to make sure that we don't miss any.
I recommend
There was a problem hiding this comment.
Oh! actually addFetchInstrumentationHandler which calls maybeInstrument, so the first global caller will win. So, it's tricky to guarantee that the flag will be set properly.
You found a sticky knot! Yeah, I recommend leaving this as-is for this PR, and cleaning up carefully in a followup issue.
| shouldCreateSpanForRequest?: (url: string) => boolean; | ||
| } | ||
|
|
||
| const _fetchIntegration = ((options: FetchOptions = {}) => { |
There was a problem hiding this comment.
l: Seems like it is an almost 1:1 copy from the one from Cloudflare. Would be nice if this could get moved to server-utils and named somehoe nativeFetchIntegration or so? Then we can reuse it 😍
isaacs
left a comment
isaacs
left a comment
There was a problem hiding this comment.
The only change that I'd gate on here is the issue with the confusing native fetch detection flags. I recommend leaving that aside in this PR, and addressing later in a cleanup that can be more careful about making that less footgunny.
| function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNativeFetchCheck: boolean = false): void { | ||
| if (skipNativeFetchCheck && !supportsNativeFetch()) { | ||
| if (!skipNativeFetchCheck && !supportsNativeFetch()) { | ||
| return; |
There was a problem hiding this comment.
The bot's context is wrong ("browser environments"), but the issue is real; just for deno rather than browsers.
I went looking for cases where this function is being called, because the guard here is indeed really confusing. One case where it's being called is in the deno breadcrumb handling logic.
This test shows that it passes with the old guard, and not with the new one, so this is definitely a regression: https://gist.github.com/isaacs/1fdf09ad8f6c327e8041bfdd7b035baf
Deno previously didn't have any unit or integration tests for breadcrumbs, which I assume is why this didn't get caught more helpfully.
I'm not sure what the best fix is, but it seems like we have two options:
supportsNativeFetch() passes there. In that case, I think it'd be good to add a comment explaining why it has the double-check, or even rename the variables here, because it's super confusing. (It seems like "don't skip the native check" would mean you ought to do the native check, not the opposite? This is very weird.)
packages/deno/src/integrations/breadcrumbs.ts line 50 becomes: addFetchInstrumentationHandler(_getFetchBreadcrumbHandler(client), true). This is a bit riskier, since it's also called in packages/vercel-edge/src/integrations/wintercg-fetch.ts and a few other places, and we'd need to make sure that we don't miss any.
I recommend
| 'http.request.header.host': expect.any(String), | ||
| 'http.request.header.user_agent': expect.stringContaining('Bun'), | ||
| 'http.request.header.baggage': expect.any(String), | ||
| 'http.request.header.sentry_trace': expect.any(String), |
There was a problem hiding this comment.
Yeah, I think this is valid. Order/timing dependent assertions.
Probably best to just remove all 6 of the lines referenced? I'm not sure they're doing much beyond being a test-flake landmine.
- packages/bun/test/integrations/bunserver.test.ts#L107-L109
- packages/bun/test/integrations/bunserver.test.ts#L201-L203
If we wanna test these specific keys, we can set them explicitly on the outgoing request in the test (eg, fetch(url, { headers: { baggage: '...', 'sentry-trace': '...' } })) as the existing test at packages/bun/test/integrations/bunserver.test.ts already does. But propagation would be better in the new dev-packages/bun-integration-tests/suites/fetch/ suite, where it is already being covered.
| function instrumentFetch(onFetchResolved?: (response: Response) => void, skipNativeFetchCheck: boolean = false): void { | ||
| if (skipNativeFetchCheck && !supportsNativeFetch()) { | ||
| if (!skipNativeFetchCheck && !supportsNativeFetch()) { | ||
| return; |
There was a problem hiding this comment.
Oh! actually addFetchInstrumentationHandler which calls maybeInstrument, so the first global caller will win. So, it's tricky to guarantee that the flag will be set properly.
You found a sticky knot! Yeah, I recommend leaving this as-is for this PR, and cleaning up carefully in a followup issue.
| httpIntegration, | ||
| httpServerIntegration, | ||
| httpServerSpansIntegration, | ||
| nativeNodeFetchIntegration, |
There was a problem hiding this comment.
Is this still needed? Or can we drop the nativeNodeFetchIntegration (since the point is that we know it doesn't work)?
There was a problem hiding this comment.
yes, right, this should go away 👍
| const { propagateTraceparent } = client?.getOptions() || {}; | ||
| if (!client || !HAS_CLIENT_MAP.get(client)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Can simplify this a bit, and skip the work in cases where it'd be thrown away immediately.
| const { propagateTraceparent } = client?.getOptions() || {}; | |
| if (!client || !HAS_CLIENT_MAP.get(client)) { | |
| return; | |
| } | |
| if (!client || !HAS_CLIENT_MAP.get(client)) { | |
| return; | |
| } | |
| const { propagateTraceparent } = client.getOptions(); |
EDIT: ah, this is copied from cf, might be worth keeping them aligned and then abstracting them both out somewhere and making this fix? Idk, minor issue anyway.
| // `skipNativeFetchCheck: true` — the native-fetch check is browser-only (it probes for | ||
| // `[native code]` and falls back to an iframe DOM check). On Bun `fetch` may already be | ||
| // wrapped by the host (e.g. Next.js) and there is no DOM, so we always patch the global. | ||
| }, true); |
There was a problem hiding this comment.
This drops fetch.preconnect (a bun-specific non-standard addition).
Would probably be worth adding ownProps back on:
const globalFetch = GLOBAL_OBJ.fetch;
// ... instrument fetch call
for (const k of Object.getOwnPropertyNames(globalFetch)) {
Object.defineProperty(fetch, k, Object.getOwnPropertyDescriptor(globalFetch, k));
}Or something to that effect.
3 participants
Adds outgoing
fetchinstrumentation to@sentry/bunvia a newfetchIntegration, and fixes an inverted native-fetch guard in@sentry/core.Why the Bun SDK needed this
@sentry/bunshippednativeNodeFetchIntegration(from@sentry/node) in its defaults, but that integration works exclusively by subscribing to undici'sdiagnostics_channelevents (undici:request:create, etc.). Bun implementsfetchnatively (in Zig, with its own HTTP client) and never emits those channels, so the integration was registered but completely inert — nohttp.clientspans, no breadcrumbs, and no trace propagation on outgoing requests.Approach
The new
fetchIntegrationpatches the globalfetch(via core'saddFetchInstrumentationHandler+instrumentFetchRequest), mirroring the Cloudflare SDK, which has the same "no undici / no diagnostics_channel" constraint. It createshttp.clientspans (originauto.http.fetch), records fetch breadcrumbs, and injectssentry-trace/baggageheaders gated bytracePropagationTargets. It replacesnativeNodeFetchIntegrationin the Bun default integrations to avoid shipping a dead integration and any risk of double instrumentation.Because
@sentry/elysiabuilds on@sentry/bun's defaults, it inherits this automatically.Core guard fix
instrumentFetch(onFetchResolved, skipNativeFetchCheck)guarded withif (skipNativeFetchCheck && !supportsNativeFetch()). That is the inverse of what the parameter name implies: passingtrueperformed the (browser-only) native-fetch check, while the default (false) skipped it. Only Cloudflare and Bun passedtrue, so they were the sole callers actually running the check — and on BunsupportsNativeFetch()returnsfalse(it probes for[native code], which fails once a host like Next.js has wrappedfetch, then falls back to an iframe DOM check that can't run outside a browser), causing the patch to bail. The guard is now!skipNativeFetchCheck && !supportsNativeFetch()so the parameter matches its name. Existing browser/deno/vercel-edge callers are unaffected (their fetch reads as native /EdgeRuntimeshort-circuits); core + browser fetch unit tests pass.Tests
dev-packages/bun-integration-tests/suites/fetch/— assertshttp.clientspan creation, header propagation to allowed targets, no propagation to disallowed targets, and breadcrumb recording. Verified against a real Bun runtime.elysia-bune2e — un-fixme'd the two outgoing-fetch propagation tests, which now pass.nextjs-16-bune2e — un-skipped; asserts propagation through the Next.jsAppRouteRouteHandlers.runHandlerspan (see below).Known limitation: Next.js on Bun
For apps running
@sentry/nextjs(→@sentry/node) on the Bun runtime (not@sentry/bun), thefetchIntegrationcan be added manually, but outgoing-fetch propagation is owned by the active OTel context, whose active span during a route handler is Next'sexecuting api routespan — not our inactivehttp.clientspan. So the downstream request becomes a sibling of ourhttp.clientspan rather than its child. Thenextjs-16-buntest asserts this actual behavior. A proper fix (a Bun-runtime fetch path inside@sentry/nodethat participates in the active context) is out of scope here.