getsentry / getsentry/sentry-javascript

Consolidate the global-`fetch` integrations into `@sentry/server-utils`

Closed
#24,344 2 comments 0 reactions 1 assignee Claimed by @isaacs View on GitHub
javascript
Dominant language
TypeScript
Stars
8.7k
Forks
1.8k
Avg merge
1d 17h
Merged PRs (30d)
523

Description

Follow-up to #24121 (deno fetch instrumentation). That PR lands a fourth near-identical copy of the same integration. This issue tracks folding all four into one shared implementation and fixing the drift they have already accumulated.

## Problem

Four packages carry the same ~90 lines around `instrumentFetchRequest`:

* `packages/bun/src/integrations/fetch.ts`
* `packages/cloudflare/src/integrations/fetch.ts`
* `packages/vercel-edge/src/integrations/wintercg-fetch.ts`
* `packages/deno/src/integrations/fetch.ts` (new in #24121)

They are identical apart from the table below. Every fix needs four edits, which is why the drift exists.

| | bun | cloudflare | vercel-edge | deno |
| -- | -- | -- | -- | -- |
| integration name | `Fetch` | `Fetch` | `WinterCGFetch` | `Fetch` |
| export | `fetchIntegration` | `fetchIntegration` | `winterCGFetchIntegration` | `fetchIntegration` |
| `spanOrigin` | `auto.http.fetch` | `auto.http.fetch` | `auto.http.wintercg_fetch` | `auto.http.fetch` |
| `propagateTraceparent` | yes | yes | **no** | yes |
| options type exported | no | yes (`Options`) | yes (`Options`) | no |
| options type name | `FetchOptions` | `Options` | `Options` | `FetchOptions` |
| `breadcrumbs` option | yes | yes | yes | no (see below) |
| `breadcrumbs` field | optional | required + `Partial<>` | required + `Partial<>` | n/a |
| unit tests | **none** | yes | yes | yes (deno) |

The `propagateTraceparent` row is a live bug: vercel-edge never sends the `traceparent` header, because that one copy was not updated.

Two more defects are shared by all four:

1. `setupOnce` **pins the first instance's options.** `setupOnce` runs once per integration name per process (`packages/core/src/integration.ts:117`), and the handler closes over the first instance's `shouldCreateSpanForRequest`, LRU caches, and `spans` map. A later `init()` reaches only `setup()`, which flips a boolean in `HAS_CLIENT_MAP`. Its options are dropped. Verified on deno: in a fresh process `fetchIntegration({ shouldCreateSpanForRequest: () => false })` works; after any earlier `init()` the callback is never called and the span is created anyway.
2. **No way to turn propagation off without turning spans off.** `shouldCreateSpanForRequest` suppresses the span but still injects headers. Node solves this with a `tracePropagation` boolean (`packages/node/src/integrations/node-fetch/types.ts:112-119`); none of the four has one. On deno this also means `denoHttpIntegration({ tracePropagation: false })` mutes `node:http` and silently leaves `fetch` propagating.

## Where it goes

`packages/server-utils/src/`, exported from `src/exports.ts`, so it is reachable from both `@sentry/server-utils` and `@sentry/server-utils/no-diagnostic-channels`.

Reasons:

* All four packages already depend on `@sentry/server-utils`.
* `src/exports.ts` is the surface with no `node:` builtins. cloudflare (workerd) and vercel-edge import only through `/no-diagnostic-channels`, so the shared code must live there, not in `src/index.ts`.
* `@sentry/core` is the wrong home: `instrumentFetchRequest` and `addFetchInstrumentationHandler` already live there, and this is the server-runtime integration wrapper around them.

Suggested path: `packages/server-utils/src/integrations/fetch.ts`.

## Shared API

```ts
createFetchIntegration({
name, // 'Fetch' | 'WinterCGFetch'
spanOrigin, // 'auto.http.fetch' | 'auto.http.wintercg_fetch'
breadcrumbs, // default for the option; deno passes false
});
```

Returns an `IntegrationFn` whose options are:

```ts
interface FetchIntegrationOptions {
breadcrumbs?: boolean; // default true
shouldCreateSpanForRequest?: (url: string) => boolean;
tracePropagation?: boolean; // default true, new (see defect 2)
}
```

The factory keeps the existing internals: the `isSentryRequestUrl` guard, the two `LRUMap` caches, the `spans` record, `propagateTraceparent` read from client options, and the breadcrumb builder.

Fix defect 1 while moving it: replace `WeakMap` with `WeakMap` populated in `setup(client)`, and have the single `setupOnce` handler read the current client's config from that map. Options then follow the client instead of the first `init()`.

## Per-package changes

`packages/server-utils`

* Add `src/integrations/fetch.ts` with the factory and the breadcrumb builder (lift from `packages/bun/src/integrations/fetch.ts`, which has the most complete copy).
* Export `createFetchIntegration` and `FetchIntegrationOptions` from `src/exports.ts`.
* Add the unit tests (below).

`packages/bun`

* Replace `src/integrations/fetch.ts` with a call to the factory.
* Export the options type from `src/index.ts` (currently missing).

`packages/cloudflare`

* Replace `src/integrations/fetch.ts` with a call to the factory.
* Rename the exported `Options` to `FetchIntegrationOptions` (keep the old name as a deprecated alias) and drop the `Partial` / required-field awkwardness.
* Drop the dead `client?.getOptions() || {}` optional chaining; the guard above it already narrows `client`.
* Retitle `test/integrations/fetch.test.ts`, whose describe block still says "WinterCGFetch instrumentation" although the integration is named `Fetch`.

`packages/vercel-edge`

* Replace `src/integrations/wintercg-fetch.ts` with a call to the factory.
* **Decide** whether it keeps `WinterCGFetch` / `auto.http.wintercg_fetch` or converges on `Fetch` / `auto.http.fetch`. Converging is a breaking change: the name is public API, and `dev-packages/e2e-tests/test-applications/nextjs-pages-dir/tests/middleware.test.ts:80` asserts the origin. Suggest keeping both as-is for now and passing them to the factory, then converging in the next major.
* `propagateTraceparent` starts working here as a side effect. Call it out in the changelog; it is a behavior change, not just a refactor.

`packages/deno`

* Replace `src/integrations/fetch.ts` with `createFetchIntegration({ name: 'Fetch', spanOrigin: 'auto.http.fetch', breadcrumbs: false })`.
* Export the options type from `src/index.ts`.

## Other integrations to fold or tidy

`packages/deno/src/integrations/breadcrumbs.ts`**.** Deno is the only server runtime that records fetch breadcrumbs in a separate integration; the other three do it inside the fetch integration. Two consequences worth resolving in the same pass:

* `breadcrumbs.ts:150` decides "is this a Sentry request?" with `url.match(/sentry_key/) && method === 'POST'`, while `fetch.ts:66` uses `isSentryRequestUrl`. One package, two rules. Move `breadcrumbs.ts` onto `isSentryRequestUrl`.
* Once the fetch integration can record breadcrumbs (via the shared `breadcrumbs` option), the fetch half of `breadcrumbs.ts` could be deleted and deno could pass `breadcrumbs: true` like the others. That aligns all four runtimes but changes which integration a user disables to stop fetch breadcrumbs (`breadcrumbsIntegration({ fetch: false })` today, `fetchIntegration({ breadcrumbs: false })` after). **Decide** whether that rename is worth doing now or at the next major. If it waits, keep `breadcrumbs: false` on deno and leave `breadcrumbs.ts` alone apart from the `isSentryRequestUrl` fix.

**Deno option-surface mismatch.** `denoHttpIntegration` offers `tracePropagation`, `ignoreOutgoingRequests`, `spans`, and `breadcrumbs`; the fetch path will offer `tracePropagation`, `breadcrumbs`, and `shouldCreateSpanForRequest`. The new `tracePropagation` option closes the worst of the gap. Until the names converge, both docstrings should say the option covers one client path only, so nobody reads `denoHttpIntegration({ tracePropagation: false })` as covering `fetch`.

## Not in scope

**Browser.** `packages/browser/src/tracing/request.ts` cannot fold in. It is not an integration: `instrumentOutgoingRequests(client, options)` is called by `browserTracingIntegration`, shares one `spans` map with the XHR path, reads `tracePropagationTargets` from *integration* options through its own `shouldAttachHeaders` rather than from client options through `shouldPropagateTraceForUrl`, passes `urlBase: WINDOW.location.origin`, sets `URL_FULL` and `SERVER_ADDRESS` after the fact, and adds Resource Timing data. It also has no `setupOnce` / `HAS_CLIENT_MAP` lifecycle, and `@sentry/browser` does not depend on `@sentry/server-utils`. The common part is already factored out as `instrumentFetchRequest` in core.

**Node.** `nativeNodeFetchIntegration` instruments undici through diagnostics channels, not the global `fetch` function. Different mechanism, no overlap.

`packages/browser/src/integrations/httpclient.ts` **and** `fetchStreamPerformance.ts`**.** Browser-only, unrelated.

## Tests

One shared suite in `packages/server-utils/test/integrations/fetch.test.ts` covering: span creation, `shouldCreateSpanForRequest`, header propagation against `tracePropagationTargets`, the new `tracePropagation: false`, `propagateTraceparent`, the `isSentryRequestUrl` skip, breadcrumbs on and off, the error path, and per-client options after a second `init()` (defect 1).

Per-package tests then shrink to "the integration is in the default set and is wired with the right name and origin". Bun gains its first fetch test this way. The deno test added in #24121 also lacks a case for the default `traceLifecycle: 'stream'`; add it here or there.

## Acceptance

* One implementation. No `instrumentFetchRequest` call sites outside `@sentry/server-utils` and `@sentry/browser`.
* vercel-edge sends `traceparent`.
* Integration options survive a second `init()`.
* `tracePropagation: false` stops header injection while spans stay on.
* Options types exported from all four packages.
* No behavior change for bun, cloudflare, or deno beyond the two new options.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.