feat(bun): Instrument outgoing fetch requests - #22869
Conversation
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.
225988a to
98d00aa
Compare
| 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
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.
| // `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.
98d00aa to
23639a5
Compare
|
I refactored the skipNativeFetch check out into a separate change/PR (this is overall not ideal IMHO). |
size-limit report 📦
|
3ba0b22 to
272b05c
Compare
272b05c to
8732c1e
Compare
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>
Co-authored-by: isaacs <i@izs.me>
6638540 to
386c63d
Compare

Adds outgoing
fetchinstrumentation to@sentry/bunvia a newfetchIntegration.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.Preserving Bun's non-standard
fetchpropertiesCore's fetch instrumentation now wraps the global
fetchin aProxyrather than a plain wrapper function. Bun hangs non-standard APIs off thefetchglobal (most notablyfetch.preconnect), and a plain wrapper closure would drop those own properties, breaking code that relies on them. TheProxytransparently forwards property access (andtoString()) to the native implementation, sofetch.preconnectand friends keep working after instrumentation is installed.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.