From 9ff7d4871c0796b4caaa40157f8d632be00d6b5b Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 20 Oct 2026 15:24:11 -0700 Subject: [PATCH] ref(cloudflare): Use the shared request handler wrapper Moves Cloudflare's `wrapRequestHandler` onto `wrapServerRequestHandler`, which `Deno.serve` and `Bun.serve` use since the previous PRs. Cloudflare keeps only what is unique to it: client init, the `cf` and cloud resource contexts, the protocol from `cf.httpProtocol`, the `isStreamingResponse` option, `captureErrors`, and the flush through `waitUntil`, which skips the dispose for a 101 response. The helpers that only the old code used (`captureIncomingRequestBody`, `shouldCreateIncomingRequestSpan`, `getIncomingRequestSpanHooks` and `addRequest`) are gone. `wrapRequestHandler` must run on workers without the `nodejs_compat` flag, such as Shopify Oxygen, and every `@sentry/server-utils` entry loads `node:async_hooks`. So the wrapper moves to a new entry, `@sentry/server-utils/request-handler`, with no Node.js imports. Deno and Bun import it from there too. Cloudflare's module graph test allows this one entry, and a new test in server-utils walks the entry's module graph and fails on any Node.js import. The wrapper gets three optional fields: `isStreamingResponse`, `onDone` (called once when the request is done, with the response if there is one), and a `mechanism` that can be left out to not capture errors. For streamed responses, the shared stream monitor replaces Cloudflare's `TransformStream`. Workers with a compatibility date before 2022-11-30 cannot construct a `ReadableStream` without the `streams_enable_constructors` flag, so the wrapper then ends at handler return, as the `TransformStream` fallback did. The monitor now locks the source body only in `start()`, so the original response stays usable in that case. The wrapper builds the new response from the original one, which keeps `encodeBody: 'manual'`. Visible changes for Cloudflare: - Request spans set `client.address` from `CF-Connecting-IP` (or another forwarding header), as on the other runtimes. This follows `dataCollection.userInfo`, which is `true` by default. - Request spans get the response header attributes, and events get the `response` context. - An error from the source of a streamed response is captured (unless `captureErrors` is `false`), and the span gets an error status. - If the user removed `httpServerIntegration`, request bodies follow `dataCollection.httpBodies`. Before, the SDK attached no body. The minified `withSentry` bundle grows by about 2.4 KB (212,385 to 214,853 bytes), under the 211 KiB limit. Most of it is the request session and client address code, which Cloudflare now shares. Fixes #25577 Fixes JS-3975 Co-Authored-By: Claude Opus 5.5 (1M context) --- .size-limit.js | 1 + CHANGELOG.md | 19 ++ .../public-api/metrics/server-address/test.ts | 1 - .../suites/public-api/startSpan/test.ts | 10 + .../subpath-request-handler/test.ts | 13 +- packages/bun/src/integrations/bunserver.ts | 2 +- .../cloudflare/src/integrations/httpServer.ts | 160 +++--------- packages/cloudflare/src/scope-utils.ts | 8 - .../src/wrapRequestHandlerWithInit.ts | 230 +++--------------- packages/cloudflare/test/request.test.ts | 20 +- .../test/requestModuleGraph.test.ts | 7 +- .../deno/src/wrap-deno-request-handler.ts | 2 +- packages/server-utils/package.json | 5 + packages/server-utils/rollup.npm.config.mjs | 1 + packages/server-utils/src/exports.ts | 2 - .../server-utils/src/index.request-handler.ts | 4 + .../server-utils/src/utils/monitorStream.ts | 47 +++- .../src/utils/wrapServerRequestHandler.ts | 107 ++++++-- .../test/requestHandlerEntry.test.ts | 53 ++++ .../test/utils/monitorStream.test.ts | 58 +++++ .../utils/wrapServerRequestHandler.test.ts | 44 ++++ 21 files changed, 421 insertions(+), 373 deletions(-) create mode 100644 packages/server-utils/src/index.request-handler.ts create mode 100644 packages/server-utils/test/requestHandlerEntry.test.ts diff --git a/.size-limit.js b/.size-limit.js index b841667d9d89..075c3fb89a6c 100644 --- a/.size-limit.js +++ b/.size-limit.js @@ -467,6 +467,7 @@ module.exports = [ ignore: [...builtinModules, ...nodePrefixedBuiltinModules], gzip: false, brotli: false, + limit: '521 KiB', disablePlugins: ['@size-limit/webpack'], webpack: false, modifyEsbuildConfig: function (config) { diff --git a/CHANGELOG.md b/CHANGELOG.md index 12dd671c4c50..fd6a460eb818 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -106,6 +106,25 @@ Work in this release was contributed by @adityareddy-dev, @jpdefo, @Jxxunnn, @ph - The SDK no longer reads a streamed response ahead of the client. - Errors in `OPTIONS` and `HEAD` requests now carry the request data. +- **ref(bun): Use the shared request handler wrapper for `Bun.serve`** + + `Bun.serve` now uses the same request handler wrapper as `Deno.serve`. Bun spans now follow the Sentry span conventions for `http.server` spans, so some attributes change: + + - The matched route is in `http.route` instead of `url.template`. Routes without parameters now also set `http.route`, with the `route` source. + - The host is in `server.address` instead of `url.domain`. + - A request to `/` is named after the route (for example `GET /`), as on the other runtimes. + - Spans now also have `user_agent.original` and `http.request.body.size`. + + If you query Bun spans by `url.template` or `url.domain`, use `http.route` or `server.address` instead. Errors in `OPTIONS` and `HEAD` requests now carry the request data. On all three runtimes, the span now lists every `Set-Cookie` header of a response, not only the last one. + +- **ref(cloudflare): Use the shared request handler wrapper** + + Cloudflare Workers now use the same request handler wrapper as `Deno.serve` and `Bun.serve`. Changes for Cloudflare users: + + - Request spans now set `client.address` from the `CF-Connecting-IP` header (or another forwarding header), as on the other runtimes. This follows `dataCollection.userInfo`, which is `true` by default. To leave it out, set `dataCollection: { userInfo: false }`. + - Request spans now have the response header attributes, and events have the `response` context with the status code. + - When the source of a streamed response fails, the SDK now captures the error and marks the span as failed. With `captureErrors: false`, it does not capture it. + ## 11.8.0 ### Important Changes diff --git a/dev-packages/cloudflare-integration-tests/suites/public-api/metrics/server-address/test.ts b/dev-packages/cloudflare-integration-tests/suites/public-api/metrics/server-address/test.ts index 013bf552d772..12cf754e08b6 100644 --- a/dev-packages/cloudflare-integration-tests/suites/public-api/metrics/server-address/test.ts +++ b/dev-packages/cloudflare-integration-tests/suites/public-api/metrics/server-address/test.ts @@ -12,7 +12,6 @@ it('should add server.address attribute to metrics when serverName is set', asyn name: 'test.counter', type: 'counter', value: 1, - span_id: expect.any(String), timestamp: expect.any(Number), trace_id: expect.any(String), attributes: { diff --git a/dev-packages/cloudflare-integration-tests/suites/public-api/startSpan/test.ts b/dev-packages/cloudflare-integration-tests/suites/public-api/startSpan/test.ts index 0fa5d410368b..adf52419022f 100644 --- a/dev-packages/cloudflare-integration-tests/suites/public-api/startSpan/test.ts +++ b/dev-packages/cloudflare-integration-tests/suites/public-api/startSpan/test.ts @@ -1,5 +1,6 @@ import { SDK_VERSION, SEMANTIC_ATTRIBUTE_SENTRY_SAMPLE_RATE } from '@sentry/core'; import { + CLIENT_ADDRESS, SENTRY_SEGMENT_NAME_SOURCE, SENTRY_SDK_NAME, SENTRY_SDK_VERSION, @@ -205,6 +206,15 @@ it('sends a streamed span envelope with correct spans for a manually started spa type: 'string', value: 'localhost', }, + // From the `CF-Connecting-IP` header that `wrangler dev` sets. + [CLIENT_ADDRESS]: { + type: 'string', + value: '127.0.0.1', + }, + 'http.response.header.content-type': { + type: 'array', + value: ['text/plain;charset=UTF-8'], + }, 'url.full': { type: 'string', value: expect.stringMatching(/^http:\/\/localhost:.+$/), diff --git a/dev-packages/cloudflare-integration-tests/suites/tracing/opentelemetry-tracer/subpath-request-handler/test.ts b/dev-packages/cloudflare-integration-tests/suites/tracing/opentelemetry-tracer/subpath-request-handler/test.ts index 596ae6c6ef6d..27e35a07270f 100644 --- a/dev-packages/cloudflare-integration-tests/suites/tracing/opentelemetry-tracer/subpath-request-handler/test.ts +++ b/dev-packages/cloudflare-integration-tests/suites/tracing/opentelemetry-tracer/subpath-request-handler/test.ts @@ -1,16 +1,19 @@ -import type { Event } from '@sentry/core'; +import { HTTP_SERVER } from '@sentry/conventions/op'; import { expect, it } from 'vitest'; import { createRunner } from '../../../../runner'; +import { getSpanOp, getSpansFromEnvelope } from '../../../../spanUtils'; it('does not capture spans emitted through @opentelemetry/api inside wrapRequestHandler from the /request subpath', async ({ signal, }) => { const runner = createRunner(__dirname) .expect(envelope => { - const event = envelope[1]?.[0]?.[1] as Event; - expect(event.transaction).toBe('GET /'); - expect(event.contexts?.trace?.op).toBe('http.server'); - expect(event.spans).toEqual([]); + // Only the request's segment span. The OpenTelemetry span is not captured. + const spans = getSpansFromEnvelope(envelope); + expect(spans).toHaveLength(1); + expect(spans[0]?.is_segment).toBe(true); + expect(spans[0]?.name).toBe('GET /'); + expect(getSpanOp(spans[0]!)).toBe(HTTP_SERVER); }) .start(signal); diff --git a/packages/bun/src/integrations/bunserver.ts b/packages/bun/src/integrations/bunserver.ts index d34539325a30..25598243537b 100644 --- a/packages/bun/src/integrations/bunserver.ts +++ b/packages/bun/src/integrations/bunserver.ts @@ -2,7 +2,7 @@ import type { Client, Integration, IntegrationFn, MaxRequestBodySize, Span, Span import { debug, defineIntegration, getClient, withIsolationScope } from '@sentry/core'; import type { AssertTrue, CheckServerRequestOptions, ServerRequestOptions } from '@sentry/core/server'; import { URL_PATH_PARAMETER_KEY_BASE } from '@sentry/conventions/attributes'; -import { wrapServerRequestHandler } from '@sentry/server-utils'; +import { wrapServerRequestHandler } from '@sentry/server-utils/request-handler'; import type { Server, ServeOptions } from 'bun'; const INTEGRATION_NAME = 'BunServer' as const; diff --git a/packages/cloudflare/src/integrations/httpServer.ts b/packages/cloudflare/src/integrations/httpServer.ts index 1975f168819e..dbcc4bb839b0 100644 --- a/packages/cloudflare/src/integrations/httpServer.ts +++ b/packages/cloudflare/src/integrations/httpServer.ts @@ -1,15 +1,6 @@ -import type { Client, IntegrationFn, MaxRequestBodySize, Span } from '@sentry/core'; -import { - captureBodyFromWinterCGRequest, - defineIntegration, - getIsolationScope, - hasSpansEnabled, - parseStringToURLObject, - safeCallback, -} from '@sentry/core'; -import type { AssertTrue, CheckServerRequestOptions } from '@sentry/core/server'; -import { isStaticAssetRequest } from '@sentry/core/server'; -import { DEBUG_BUILD } from '../debug-build'; +import type { Client, Integration, IntegrationFn, MaxRequestBodySize, Span } from '@sentry/core'; +import { defineIntegration } from '@sentry/core'; +import type { AssertTrue, CheckServerRequestOptions, ServerRequestOptions } from '@sentry/core/server'; const INTEGRATION_NAME = 'HttpServer' as const; @@ -100,21 +91,45 @@ type _ServerRequestOptionsCheck = AssertTrue< > >; -interface HttpServerIntegrationInstance extends HttpServerIntegrationOptions { - name: string; - maxRequestBodySize: MaxRequestBodySize | undefined; +type SupportedServerRequestOptions = Omit< + ServerRequestOptions, + 'sessions' | 'sessionFlushingDelayMS' +>; + +// Every supported shared option is a required key, so a new one fails to compile below until the +// integration passes it on. +type HttpServerRequestOptions = { + [K in keyof Required]: SupportedServerRequestOptions[K]; +}; + +interface HttpServerIntegration extends Integration { + requestOptions: HttpServerRequestOptions; +} + +/** + * The incoming request options of the client's `httpServerIntegration`. Without the integration, + * the SDK attaches no request body, so users can turn body capture off by removing it. + */ +export function getHttpServerRequestOptions(client: Client): ServerRequestOptions { + return ( + client.getIntegrationByName(INTEGRATION_NAME)?.requestOptions ?? { + maxRequestBodySize: 'none', + } + ); } -const _httpServerIntegration = ((options: HttpServerIntegrationOptions = {}): HttpServerIntegrationInstance => { +const _httpServerIntegration = ((options: HttpServerIntegrationOptions = {}): HttpServerIntegration => { return { name: INTEGRATION_NAME, - maxRequestBodySize: options.maxRequestBodySize, - ignoreRequestBody: options.ignoreRequestBody, - ignoreIncomingRequests: options.ignoreIncomingRequests, - ignoreStaticAssets: options.ignoreStaticAssets ?? true, - spans: options.spans, - onSpanCreated: options.onSpanCreated, - onSpanEnd: options.onSpanEnd, + requestOptions: { + maxRequestBodySize: options.maxRequestBodySize, + ignoreRequestBody: options.ignoreRequestBody, + ignoreIncomingRequests: options.ignoreIncomingRequests, + ignoreStaticAssets: options.ignoreStaticAssets ?? true, + spans: options.spans, + onSpanCreated: options.onSpanCreated, + onSpanEnd: options.onSpanEnd, + }, }; }) satisfies IntegrationFn; @@ -137,102 +152,3 @@ const _httpServerIntegration = ((options: HttpServerIntegrationOptions = {}): Ht * ``` */ export const httpServerIntegration = defineIntegration(_httpServerIntegration); - -/** - * Capture the request body based on the HttpServer integration config. - * Called internally by `wrapRequestHandler`. - */ -export async function captureIncomingRequestBody(client: Client, request: Request): Promise { - const integration = client.getIntegrationByName(INTEGRATION_NAME); - - if (!integration) { - return; - } - - const configuredBodySize = integration.maxRequestBodySize; - const effectiveBodySize: MaxRequestBodySize = - configuredBodySize ?? - (client.getDataCollectionOptions().httpBodies.includes('incomingRequest') ? 'medium' : 'none'); - - if (effectiveBodySize === 'none') { - return; - } - - // Skip GET and HEAD requests - they don't have bodies - // Also skip OPTIONS, even if they may have a body, they might not give a lot of extra value - if (request.method === 'GET' || request.method === 'HEAD' || request.method === 'OPTIONS') { - return; - } - - const ignoreRequestBody = integration.ignoreRequestBody; - if ( - ignoreRequestBody && - // If the filter throws, skip the body so it cannot leak data the filter was meant to hide. - safeCallback( - DEBUG_BUILD ? 'Error in `ignoreRequestBody`, skipping the body:' : '', - () => ignoreRequestBody(request.url, request), - () => true, - ) - ) { - return; - } - - const isolationScope = getIsolationScope(); - await captureBodyFromWinterCGRequest(request, isolationScope, effectiveBodySize); -} - -/** - * Whether to create a span for the incoming request, based on the HttpServer integration config. - * Called internally by `wrapRequestHandler`. - */ -export function shouldCreateIncomingRequestSpan(client: Client | undefined, request: Request): boolean { - if (!client) { - return false; - } - - const integration = client.getIntegrationByName(INTEGRATION_NAME); - if (!(integration?.spans ?? hasSpansEnabled(client.getOptions()))) { - return false; - } - - const urlObject = parseStringToURLObject(request.url); - const urlPath = urlObject ? `${urlObject.pathname}${urlObject.search}` : '/'; - if (integration?.ignoreStaticAssets && request.method === 'GET' && isStaticAssetRequest(urlPath)) { - return false; - } - - const ignoreIncomingRequests = integration?.ignoreIncomingRequests; - return !( - ignoreIncomingRequests && - safeCallback( - DEBUG_BUILD ? 'Error in `ignoreIncomingRequests`, creating the span:' : '', - () => ignoreIncomingRequests(urlPath, request), - () => false, - ) - ); -} - -/** - * The `onSpanCreated` and `onSpanEnd` hooks of the HttpServer integration, guarded so that a hook - * that throws does not fail the request. Called internally by `wrapRequestHandler`. - */ -export function getIncomingRequestSpanHooks(client: Client | undefined): { - onSpanCreated: (span: Span, request: Request) => void; - onSpanEnd: (span: Span, request: Request, response: Response) => void; -} { - const integration = client?.getIntegrationByName(INTEGRATION_NAME); - return { - onSpanCreated: (span, request) => - safeCallback( - DEBUG_BUILD ? 'Error in `onSpanCreated`:' : '', - () => integration?.onSpanCreated?.(span, request), - () => undefined, - ), - onSpanEnd: (span, request, response) => - safeCallback( - DEBUG_BUILD ? 'Error in `onSpanEnd`:' : '', - () => integration?.onSpanEnd?.(span, request, response), - () => undefined, - ), - }; -} diff --git a/packages/cloudflare/src/scope-utils.ts b/packages/cloudflare/src/scope-utils.ts index 7c46e8af1001..d464d1389ee3 100644 --- a/packages/cloudflare/src/scope-utils.ts +++ b/packages/cloudflare/src/scope-utils.ts @@ -1,6 +1,5 @@ import type { CfProperties, IncomingRequestCfProperties } from '@cloudflare/workers-types'; import type { Scope } from '@sentry/core'; -import { winterCGRequestToRequestData } from '@sentry/core'; /** * Set cloud resource context on scope. @@ -19,10 +18,3 @@ export function addCultureContext(scope: Scope, cf: IncomingRequestCfProperties timezone: cf.timezone, }); } - -/** - * Set request data on scope - */ -export function addRequest(scope: Scope, request: Request): void { - scope.setSDKProcessingMetadata({ normalizedRequest: winterCGRequestToRequestData(request) }); -} diff --git a/packages/cloudflare/src/wrapRequestHandlerWithInit.ts b/packages/cloudflare/src/wrapRequestHandlerWithInit.ts index e852ad1cf661..954cf8d7bd3f 100644 --- a/packages/cloudflare/src/wrapRequestHandlerWithInit.ts +++ b/packages/cloudflare/src/wrapRequestHandlerWithInit.ts @@ -1,37 +1,15 @@ import type { CfProperties, IncomingRequestCfProperties } from '@cloudflare/workers-types'; -import { - NETWORK_PROTOCOL_NAME, - NETWORK_PROTOCOL_VERSION, - SENTRY_OP, - SENTRY_SEGMENT_NAME_SOURCE, -} from '@sentry/conventions/attributes'; -import { HTTP_SERVER } from '@sentry/conventions/op'; -import type { Span } from '@sentry/core'; -import { - captureException, - continueTrace, - getHttpSpanDetailsFromUrlObject, - hasSpanStreamingEnabled, - httpHeadersToSpanAttributes, - HTTP_SPAN_NAME_FALLBACK, - parseStringToURLObject, - safeCallback, - setHttpStatus, - startSpanManual, - winterCGHeadersToDict, -} from '@sentry/core'; -import { classifyResponseStreaming } from '@sentry/core/server'; +import { NETWORK_PROTOCOL_NAME, NETWORK_PROTOCOL_VERSION } from '@sentry/conventions/attributes'; +import type { SpanAttributes } from '@sentry/core'; +import { safeCallback } from '@sentry/core'; +import { wrapServerRequestHandler } from '@sentry/server-utils/request-handler'; import { DEBUG_BUILD } from './debug-build'; -import { - captureIncomingRequestBody, - getIncomingRequestSpanHooks, - shouldCreateIncomingRequestSpan, -} from './integrations/httpServer'; +import { getHttpServerRequestOptions } from './integrations/httpServer'; import { flushDeferredChannelEvents } from './orchestrion-deferred-channels'; import type { CloudflareClient, CloudflareOptions } from './client'; import type { ExecutionContextCompat } from './executionContext'; import { flushAndDispose, getOriginalWaitUntil } from './flush'; -import { addCloudResourceContext, addCultureContext, addRequest } from './scope-utils'; +import { addCloudResourceContext, addCultureContext } from './scope-utils'; import { getInvocationState, getInvocationWaitUntil } from './utils/invocationContext'; import { withInvocationIsolationScope } from './utils/invocationScope'; @@ -85,7 +63,6 @@ export function wrapRequestHandlerWithInit( : context ? getOriginalWaitUntil(context).bind(context) : undefined; - const errorMechanismType = getRequestErrorMechanismType(context); const client = initSdk({ ...options, ctx: context }); isolationScope.setClient(client); @@ -95,43 +72,8 @@ export function wrapRequestHandlerWithInit( // request, where workerd allows publish, and init() above has registered subscribers. flushDeferredChannelEvents(); - const urlObject = parseStringToURLObject(request.url); - const [rawName, attributes] = getHttpSpanDetailsFromUrlObject( - urlObject, - 'server', - 'auto.http.cloudflare', - request, - undefined, - client, - ); - // With span streaming, span names have to be low cardinality, so we can't fall back to the URL. - // A `route` source means the name already is (e.g. the `/` path), so it is kept as-is. - const name = - attributes[SENTRY_SEGMENT_NAME_SOURCE] === 'route' || !client || !hasSpanStreamingEnabled(client) - ? rawName - : request.method?.toUpperCase() || HTTP_SPAN_NAME_FALLBACK; - - const contentLength = request.headers.get('content-length'); - if (contentLength) { - attributes['http.request.body.size'] = parseInt(contentLength, 10); - } - - const userAgentHeader = request.headers.get('user-agent'); - if (userAgentHeader) { - attributes['user_agent.original'] = userAgentHeader; - } - - if (client) { - Object.assign( - attributes, - httpHeadersToSpanAttributes(winterCGHeadersToDict(request.headers), client.getDataCollectionOptions()), - ); - } - - attributes[SENTRY_OP] = HTTP_SERVER; - addCloudResourceContext(isolationScope); - addRequest(isolationScope, request); + const attributes: SpanAttributes = {}; if (request.cf) { addCultureContext(isolationScope, request.cf); @@ -142,143 +84,47 @@ export function wrapRequestHandlerWithInit( } } - // Do not capture spans for OPTIONS and HEAD requests - if (request.method === 'OPTIONS' || request.method === 'HEAD') { + if (!client) { try { return await handler(); - } catch (e) { - if (captureErrors) { - captureException(e, { mechanism: { handled: false, type: errorMechanismType } }); - } - throw e; } finally { waitUntil?.(flushAndDispose(client)); } } - if (client) { - await captureIncomingRequestBody(client, request); - } - - const { onSpanCreated, onSpanEnd } = getIncomingRequestSpanHooks(client); - - // Runs the handler. Ends the span (if any) and flushes when the response is done. - const respond = async (span: Span | undefined): Promise => { - let res: Response; - - try { - res = await handler(); - if (span) { - setHttpStatus(span, res.status); - } - - // After the handler runs, the span name might have been updated by nested instrumentation - // (e.g., Remix parameterizing routes). The span should already have the correct name - // from that instrumentation, so we don't need to do anything here. - } catch (e) { - span?.end(); - if (captureErrors) { - captureException(e, { mechanism: { handled: false, type: errorMechanismType } }); - } - waitUntil?.(flushAndDispose(client)); - throw e; - } - - let ended = false; - const endSpan = (): void => { - if (ended) return; - - ended = true; - if (span) { - onSpanEnd(span, request, res); - span.end(); - } - }; - - const userClassification = safeCallback( - DEBUG_BUILD ? 'Error in `isStreamingResponse`, using the default classification:' : '', - () => options.isStreamingResponse?.(res, request), - () => undefined, - ); - // An async callback returns a promise, which would otherwise count as `true`. - const isStreaming = - typeof userClassification === 'boolean' ? userClassification : classifyResponseStreaming(res).isStreaming; - - if (isStreaming && res.body) { - try { - let transformerUsed = false; - - const endSpanOnce = (): void => { - if (ended) return; - - endSpan(); + // Note: The span will not have a duration unless I/O happens in the handler. This is + // because of how the cloudflare workers runtime works. + // See: https://developers.cloudflare.com/workers/runtime-apis/performance/ + return wrapServerRequestHandler( + { + request, + client, + isolationScope, + options: getHttpServerRequestOptions(client), + origin: 'auto.http.cloudflare', + mechanism: captureErrors ? { handled: false, type: getRequestErrorMechanismType(context) } : undefined, + attributes, + isStreamingResponse: response => { + const classification = safeCallback( + DEBUG_BUILD ? 'Error in `isStreamingResponse`, using the default classification:' : '', + () => options.isStreamingResponse?.(response, request), + () => undefined, + ); + // An async callback returns a promise, which would otherwise count as `true`. + return typeof classification === 'boolean' ? classification : undefined; + }, + onDone: response => { + // Don't dispose for protocol upgrades (101 Switching Protocols) - the connection stays alive. + // This includes WebSocket upgrades where webSocketMessage/webSocketClose handlers + // will still be called and may need the client to capture errors. + if (response?.status === 101) { + waitUntil?.(client.flush(2000)); + } else { waitUntil?.(flushAndDispose(client)); - }; - - // Workers with a compatibility date before 2022-11-30 and without the - // `transformstream_enable_standard_constructor` flag (for example under older mini-oxygen - // releases) ignore the transformer, so `flush` and `cancel` would never end the span. Only a - // used transformer runs `start`. - const transform = new TransformStream({ - start() { - transformerUsed = true; - }, - flush() { - // Source stream completed normally. - endSpanOnce(); - }, - cancel() { - // Client disconnected (or downstream cancelled). The `cancel` - // is being called while the response is still considered - // active, so this is a safe place to end the span. - endSpanOnce(); - }, - }); - - if (transformerUsed) { - // Passing the original response as the init keeps `encodeBody: 'manual'`, which a - // Response does not expose as a property. Without it, workerd compresses a - // pre-compressed body a second time. - return new Response(res.body.pipeThrough(transform), res); } - } catch { - // Falls back to ending the span at handler return. - } - } - - // Non-streaming response - end span immediately and return original - endSpan(); - - // Don't dispose for protocol upgrades (101 Switching Protocols) - the connection stays alive. - // This includes WebSocket upgrades where webSocketMessage/webSocketClose handlers - // will still be called and may need the client to capture errors. - if (res.status === 101) { - waitUntil?.(client?.flush(2000)); - } else { - waitUntil?.(flushAndDispose(client)); - } - return res; - }; - - const createSpan = shouldCreateIncomingRequestSpan(client, request); - - return continueTrace( - { sentryTrace: request.headers.get('sentry-trace') || '', baggage: request.headers.get('baggage') }, - () => { - if (!createSpan) { - return respond(undefined); - } - - // Note: This span will not have a duration unless I/O happens in the handler. This is - // because of how the cloudflare workers runtime works. - // See: https://developers.cloudflare.com/workers/runtime-apis/performance/ - - // Use startSpanManual to control when span ends (needed for streaming responses) - return startSpanManual({ name, attributes }, span => { - onSpanCreated(span, request); - return respond(span); - }); + }, }, + () => handler(), ); }, wrapperOptions.context); } diff --git a/packages/cloudflare/test/request.test.ts b/packages/cloudflare/test/request.test.ts index 972180c62a8c..d05ad7420d44 100644 --- a/packages/cloudflare/test/request.test.ts +++ b/packages/cloudflare/test/request.test.ts @@ -677,6 +677,7 @@ describe('withSentry', () => { 'http.response.status_code': 200, 'http.request.body.size': 10, 'http.request.header.content-length': ['10'], + 'http.response.header.content-type': ['text/plain;charset=UTF-8'], }, op: 'http.server', origin: 'auto.http.cloudflare', @@ -1297,13 +1298,16 @@ describe('flushAndDispose', () => { }, ); - test('tears down at handler return when TransformStream ignores the transformer', async () => { - // workerd without `transformstream_enable_standard_constructor` returns an identity stream. + test('tears down at handler return when the ReadableStream constructor is not available', async () => { + const response = new Response('
shell
', { + headers: { 'content-type': 'text/x-component;charset=utf-8' }, + }); + // workerd before the 2022-11-30 compatibility date, without `streams_enable_constructors`. vi.stubGlobal( - 'TransformStream', - class extends TransformStream { + 'ReadableStream', + class { constructor() { - super(); + throw new TypeError('To use the new ReadableStream() constructor, enable streams_enable_constructors.'); } }, ); @@ -1316,16 +1320,14 @@ describe('flushAndDispose', () => { const flushSpy = vi.spyOn(SentryCore.Client.prototype, 'flush').mockResolvedValue(true); - const response = new Response('
shell
', { - headers: { 'content-type': 'text/x-component;charset=utf-8' }, - }); - const result = await wrapRequestHandler( { options, request: new Request('https://example.com'), context }, () => response, ); expect(result).toBe(response); + expect(result.bodyUsed).toBe(false); + expect(result.body?.locked).toBe(false); expect(waitUntil).toHaveBeenCalledTimes(1); flushSpy.mockRestore(); diff --git a/packages/cloudflare/test/requestModuleGraph.test.ts b/packages/cloudflare/test/requestModuleGraph.test.ts index 2100f4a47311..81295812f78c 100644 --- a/packages/cloudflare/test/requestModuleGraph.test.ts +++ b/packages/cloudflare/test/requestModuleGraph.test.ts @@ -69,7 +69,12 @@ describe('module graph of `wrapRequestHandler`', () => { it('contains no package that depends on Node.js APIs', () => { // `@sentry/server-utils` subscribes to `node:diagnostics_channel` and `@sentry/node` needs Node.js // throughout. Both are only allowed in `sdk.ts`, `index.ts` and `vite/` (see `.oxlintrc.json`). - expect(externals.filter(specifier => /^@sentry\/(node|server-utils)(\/|$)/.test(specifier))).toEqual([]); + // The `request-handler` entry of `@sentry/server-utils` has no Node.js imports. + const nodePackages = externals.filter( + specifier => + /^@sentry\/(node|server-utils)(\/|$)/.test(specifier) && specifier !== '@sentry/server-utils/request-handler', + ); + expect(nodePackages).toEqual([]); }); it('does not reach `async.ts`, the only shipped module importing `node:async_hooks`', () => { diff --git a/packages/deno/src/wrap-deno-request-handler.ts b/packages/deno/src/wrap-deno-request-handler.ts index 4d24405d58e4..e050c5523a2d 100644 --- a/packages/deno/src/wrap-deno-request-handler.ts +++ b/packages/deno/src/wrap-deno-request-handler.ts @@ -1,7 +1,7 @@ import type { Client } from '@sentry/core'; import { debug, getClient, withIsolationScope } from '@sentry/core'; import type { ServerRequestOptions } from '@sentry/core/server'; -import { wrapServerRequestHandler } from '@sentry/server-utils'; +import { wrapServerRequestHandler } from '@sentry/server-utils/request-handler'; export type RequestHandlerWrapperOptions = { request: Request; diff --git a/packages/server-utils/package.json b/packages/server-utils/package.json index 6ce9dc818ba7..a2a15bbc1fe5 100644 --- a/packages/server-utils/package.json +++ b/packages/server-utils/package.json @@ -27,6 +27,11 @@ "import": "./build/esm/index.no-diagnostic-channels.js", "require": "./build/cjs/index.no-diagnostic-channels.js" }, + "./request-handler": { + "types": "./build/types/index.request-handler.d.ts", + "import": "./build/esm/index.request-handler.js", + "require": "./build/cjs/index.request-handler.js" + }, "./orchestrion/config": { "types": "./build/types/orchestrion/config/index.d.ts", "import": "./build/esm/orchestrion/config/index.js", diff --git a/packages/server-utils/rollup.npm.config.mjs b/packages/server-utils/rollup.npm.config.mjs index ab0a51d3ef21..74f172ec5215 100644 --- a/packages/server-utils/rollup.npm.config.mjs +++ b/packages/server-utils/rollup.npm.config.mjs @@ -100,6 +100,7 @@ export default [ entrypoints: [ 'src/index.ts', 'src/index.no-diagnostic-channels.ts', + 'src/index.request-handler.ts', 'src/orchestrion/config/index.ts', 'src/orchestrion/bundler/vite.ts', 'src/orchestrion/bundler/rollup.ts', diff --git a/packages/server-utils/src/exports.ts b/packages/server-utils/src/exports.ts index 799ed7c69d18..15381f95dc9b 100644 --- a/packages/server-utils/src/exports.ts +++ b/packages/server-utils/src/exports.ts @@ -1,7 +1,5 @@ // Shared exports not using diagnostics channels export { setHttpServerSpanRouteAttribute } from './utils/setHttpServerSpanRouteAttribute'; -export { wrapServerRequestHandler } from './utils/wrapServerRequestHandler'; -export type { ServerRequestHandlerConfig } from './utils/wrapServerRequestHandler'; export { setAsyncLocalStorageAsyncContextStrategy } from './async-context'; export { openTelemetryIntegration, getOtlpTracesEndpoint } from './opentelemetry'; export * from './ai'; diff --git a/packages/server-utils/src/index.request-handler.ts b/packages/server-utils/src/index.request-handler.ts new file mode 100644 index 000000000000..73e0902cc088 --- /dev/null +++ b/packages/server-utils/src/index.request-handler.ts @@ -0,0 +1,4 @@ +// The web `Request` handler wrapper, on its own entry with no Node.js imports, so Cloudflare can use +// it on workers without the `nodejs_compat` compatibility flag. +export { wrapServerRequestHandler } from './utils/wrapServerRequestHandler'; +export type { ServerRequestHandlerConfig } from './utils/wrapServerRequestHandler'; diff --git a/packages/server-utils/src/utils/monitorStream.ts b/packages/server-utils/src/utils/monitorStream.ts index d19466a6a035..1ef9b423ecea 100644 --- a/packages/server-utils/src/utils/monitorStream.ts +++ b/packages/server-utils/src/utils/monitorStream.ts @@ -1,33 +1,66 @@ -/** Monitor the source without reading ahead or treating cancellation as a source failure. */ +import { debug } from '@sentry/core'; +import { DEBUG_BUILD } from '../debug-build'; + +/** + * Monitor the source without reading ahead or treating cancellation as a source failure. + * + * `onDone` runs once, while the returned stream is still open: before it closes, before it errors, + * or when the consumer cancels it. A runtime can end the request when the response body closes + * (Cloudflare Workers then reject `waitUntil`), so work that `onDone` starts must begin first. + */ export function monitorStream( stream: ReadableStream, onDone: () => void, onError: (error: unknown) => void, ): ReadableStream { - const reader = stream.getReader(); - // A cancelled reader closes normally; only a source failure should be captured as an error. - void reader.closed.then(onDone, error => { - onError(error); - onDone(); - }); + let reader: ReadableStreamDefaultReader; + let finished = false; + const finish = (error?: { error: unknown }): void => { + if (finished) { + return; + } + finished = true; + // A callback that throws must not fail the response body. + try { + if (error) { + onError(error.error); + } + onDone(); + } catch (e) { + DEBUG_BUILD && debug.error('Error when a monitored stream finished:', e); + } + }; return new ReadableStream( { + // Locks the source only once the stream exists, so a constructor that throws leaves it usable. + start() { + reader = stream.getReader(); + // A source can fail while no one reads from it. A cancelled reader closes normally; only a + // source failure should be captured as an error. + void reader.closed.then( + () => finish(), + error => finish({ error }), + ); + }, async pull(controller) { try { const { done, value } = await reader.read(); if (done) { + finish(); controller.close(); reader.releaseLock(); } else { controller.enqueue(value); } } catch (error) { + finish({ error }); controller.error(error); reader.releaseLock(); } }, async cancel(reason) { + finish(); try { await reader.cancel(reason); } finally { diff --git a/packages/server-utils/src/utils/wrapServerRequestHandler.ts b/packages/server-utils/src/utils/wrapServerRequestHandler.ts index bbe269c45262..f6ee67f05612 100644 --- a/packages/server-utils/src/utils/wrapServerRequestHandler.ts +++ b/packages/server-utils/src/utils/wrapServerRequestHandler.ts @@ -8,11 +8,12 @@ import { USER_AGENT_ORIGINAL, } from '@sentry/conventions/attributes'; import { HTTP_SERVER } from '@sentry/conventions/op'; -import type { Client, Mechanism, Scope, Span, SpanAttributes } from '@sentry/core'; +import type { Client, Mechanism, Scope, SpanAttributes } from '@sentry/core'; import { captureBodyFromWinterCGRequest, captureException, continueTrace, + getCurrentScope, getHttpSpanDetailsFromUrlObject, hasSpansEnabled, hasSpanStreamingEnabled, @@ -25,6 +26,8 @@ import { startSpanManual, winterCGHeadersToDict, winterCGRequestToRequestData, + withIsolationScope, + withScope, } from '@sentry/core'; import type { ServerRequestOptions } from '@sentry/core/server'; import { @@ -45,14 +48,21 @@ export interface ServerRequestHandlerConfig { options: ServerRequestOptions | undefined; /** Origin of the span, e.g. `auto.http.deno`. */ origin: string; - /** Mechanism of errors that the handler or the response stream throws. */ - mechanism: Mechanism; + /** Mechanism of errors that the handler or the response stream throws. Omit to not capture them. */ + mechanism?: Mechanism; /** The address and port of the socket, if the runtime knows them. */ getSocketAddress?: () => { address: string; port?: number } | undefined; /** The matched route of the server framework, e.g. `/users/:id`. Sets `http.route` and the span name. */ routeName?: string; /** Span attributes that only the runtime knows, e.g. the route parameters. */ attributes?: SpanAttributes; + /** Whether the response streams. Return `undefined` to use the default classification. */ + isStreamingResponse?: (response: Response) => boolean | undefined; + /** + * Called once when the request is done: when the response is returned, when a streamed body + * finishes, or when the handler throws (then without a response). + */ + onDone?: (response: Response | undefined) => void; } /** @@ -72,6 +82,11 @@ export async function wrapServerRequestHandler( ): Promise { const { request, client, isolationScope, options, mechanism } = config; const method = request.method.toUpperCase(); + const captureError = (error: unknown): void => { + if (mechanism) { + captureException(error, { mechanism }); + } + }; const endSession = options?.sessions ? startRequestSession(client, { @@ -79,6 +94,14 @@ export async function wrapServerRequestHandler( sessionFlushingDelayMS: options.sessionFlushingDelayMS, }) : undefined; + const finish = (response: Response | undefined): void => { + endSession?.(); + config.onDone?.(response); + }; + const isStreamingResponse = (response: Response): boolean => { + const classification = config.isStreamingResponse?.(response); + return typeof classification === 'boolean' ? classification : classifyResponseStreaming(response).isStreaming; + }; const socketAddress = config.getSocketAddress?.(); isolationScope.setSDKProcessingMetadata({ @@ -87,13 +110,15 @@ export async function wrapServerRequestHandler( }); if (method === 'OPTIONS' || method === 'HEAD') { + let response: Response | undefined; try { - return await handler(); + response = await handler(); + return response; } catch (e) { - captureException(e, { mechanism }); + captureError(e); throw e; } finally { - endSession?.(); + finish(response); } } @@ -132,12 +157,18 @@ export async function wrapServerRequestHandler( try { response = await handler(); } catch (e) { - captureException(e, { mechanism }); - endSession?.(); + captureError(e); + finish(undefined); throw e; } isolationScope.setContext('response', { status_code: response.status }); - return onResponseDone(response, mechanism, () => endSession?.()); + const inRequestScope = bindToRequestScope(isolationScope); + return onResponseDone( + response, + isStreamingResponse(response), + inRequestScope(() => finish(response)), + inRequestScope(captureError), + ); } const [rawName, attributes] = getHttpSpanDetailsFromUrlObject( @@ -202,8 +233,8 @@ export async function wrapServerRequestHandler( } catch (e) { span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); span.end(); - captureException(e, { mechanism }); - endSession?.(); + captureError(e); + finish(undefined); throw e; } @@ -213,10 +244,11 @@ export async function wrapServerRequestHandler( httpHeadersToSpanAttributes(responseHeadersToDict(response.headers), dataCollection, 'response'), ); + const inRequestScope = bindToRequestScope(isolationScope); return onResponseDone( response, - mechanism, - () => { + isStreamingResponse(response), + inRequestScope(() => { if (onSpanEnd) { safeCallback( DEBUG_BUILD ? 'Error in `onSpanEnd`:' : '', @@ -225,15 +257,35 @@ export async function wrapServerRequestHandler( ); } span.end(); - endSession?.(); - }, - span, + finish(response); + }), + inRequestScope((error: unknown) => { + span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); + captureError(error); + }), ); }); }, ); } +/** + * Returns a function that binds a callback to the request's isolation scope and the current scope. + * + * A streamed body finishes after the handler returns. Without `AsyncLocalStorage` (Cloudflare + * Workers without `nodejs_compat`), its callbacks then run outside the request's scopes, where + * `getClient()` finds no client: the segment span would be sent as a transaction, and a captured + * error would not update the request session. + */ +function bindToRequestScope( + isolationScope: Scope, +): (fn: (...args: A) => void) => (...args: A) => void { + const scope = getCurrentScope(); + return fn => + (...args) => + withIsolationScope(isolationScope, () => withScope(scope, () => fn(...args))); +} + function shouldCreateSpan( client: Client, options: ServerRequestOptions | undefined, @@ -261,14 +313,21 @@ function shouldCreateSpan( * Calls `onDone` when the response is done: at once, or when a streamed body finishes. * Returns the response to send. */ -function onResponseDone(response: Response, mechanism: Mechanism, onDone: () => void, span?: Span): Response { - if (response.body && !response.body.locked && classifyResponseStreaming(response).isStreaming) { - const body = monitorStream(response.body, onDone, error => { - span?.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); - captureException(error, { mechanism }); - }); - // Passing the original response as the init keeps its status, status text and headers. - return new Response(body, response); +function onResponseDone( + response: Response, + isStreaming: boolean, + onDone: () => void, + onStreamError: (error: unknown) => void, +): Response { + if (isStreaming && response.body && !response.body.locked) { + try { + // Passing the original response as the init keeps its status, status text and headers, and + // on Cloudflare also `encodeBody: 'manual'`, so workerd does not compress a compressed body again. + return new Response(monitorStream(response.body, onDone, onStreamError), response); + } catch { + // Workers with a compatibility date before 2022-11-30 and without the + // `streams_enable_constructors` flag cannot construct a `ReadableStream`, so end at once. + } } onDone(); return response; diff --git a/packages/server-utils/test/requestHandlerEntry.test.ts b/packages/server-utils/test/requestHandlerEntry.test.ts new file mode 100644 index 000000000000..9df030fa3420 --- /dev/null +++ b/packages/server-utils/test/requestHandlerEntry.test.ts @@ -0,0 +1,53 @@ +import { builtinModules } from 'node:module'; +import { existsSync, readFileSync } from 'node:fs'; +import { dirname, join, resolve } from 'node:path'; +import { describe, expect, it } from 'vitest'; + +/** + * Cloudflare imports the `request-handler` entry on workers without the `nodejs_compat` compatibility + * flag, where any Node.js import fails at startup. Assert on the module graph itself rather than on + * any bundler's output. + */ +const ENTRY_POINT = resolve(__dirname, '../src/index.request-handler.ts'); +const IMPORT_SPECIFIER_REGEX = /\bfrom\s*['"]([^'"]+)['"]|\bimport\s*['"]([^'"]+)['"]/g; + +function collectExternals(entryPoint: string): string[] { + const modules = new Set(); + const externals = new Set(); + const queue = [entryPoint]; + + while (queue.length) { + const importer = queue.pop() as string; + if (modules.has(importer)) { + continue; + } + modules.add(importer); + + for (const match of readFileSync(importer, 'utf8').matchAll(IMPORT_SPECIFIER_REGEX)) { + const specifier = match[1] ?? (match[2] as string); + if (specifier.startsWith('.')) { + const withoutExtension = join(dirname(importer), specifier); + const resolved = [`${withoutExtension}.ts`, join(withoutExtension, 'index.ts')].find(existsSync); + if (!resolved) { + throw new Error(`Could not resolve '${specifier}' imported from '${importer}'`); + } + queue.push(resolved); + } else { + externals.add(specifier); + } + } + } + + return [...externals].sort(); +} + +describe('module graph of the `request-handler` entry', () => { + it('contains no Node.js built-in module and no package that needs Node.js', () => { + const externals = collectExternals(ENTRY_POINT); + + expect(externals.filter(specifier => specifier.startsWith('node:') || builtinModules.includes(specifier))).toEqual( + [], + ); + expect(externals.filter(specifier => /^@sentry\/(node|server-utils)(\/|$)/.test(specifier))).toEqual([]); + }); +}); diff --git a/packages/server-utils/test/utils/monitorStream.test.ts b/packages/server-utils/test/utils/monitorStream.test.ts index eea047f4cee8..327b2f84154d 100644 --- a/packages/server-utils/test/utils/monitorStream.test.ts +++ b/packages/server-utils/test/utils/monitorStream.test.ts @@ -98,4 +98,62 @@ describe('monitorStream', () => { process.off('unhandledRejection', unhandled); } }); + + it('calls onDone before the stream closes', async () => { + const calls: string[] = []; + const source = new ReadableStream({ + start(controller) { + controller.enqueue(encode('a')); + controller.close(); + }, + }); + const reader = monitorStream( + source, + () => calls.push('onDone'), + () => calls.push('onError'), + ).getReader(); + void reader.closed.then(() => calls.push('closed')); + + await reader.read(); + await reader.read(); + await new Promise(resolve => setTimeout(resolve, 0)); + + expect(calls).toEqual(['onDone', 'closed']); + }); + + it('calls onDone in the same task as the cancel', async () => { + const onDone = vi.fn(); + const source = new ReadableStream({ + start(controller) { + controller.enqueue(encode('a')); + }, + }); + const reader = monitorStream(source, onDone, vi.fn()).getReader(); + await reader.read(); + + const cancelled = reader.cancel('client disconnected'); + expect(onDone).toHaveBeenCalledTimes(1); + + await cancelled; + await new Promise(resolve => setTimeout(resolve, 0)); + expect(onDone).toHaveBeenCalledTimes(1); + }); + + it('still delivers the body when onDone throws', async () => { + const source = new ReadableStream({ + start(controller) { + controller.enqueue(encode('ok')); + controller.close(); + }, + }); + const body = monitorStream( + source, + () => { + throw new Error('waitUntil failed'); + }, + vi.fn(), + ); + + expect(await new Response(body).text()).toBe('ok'); + }); }); diff --git a/packages/server-utils/test/utils/wrapServerRequestHandler.test.ts b/packages/server-utils/test/utils/wrapServerRequestHandler.test.ts index e2a26d0b5ba4..2fc986ef0b34 100644 --- a/packages/server-utils/test/utils/wrapServerRequestHandler.test.ts +++ b/packages/server-utils/test/utils/wrapServerRequestHandler.test.ts @@ -192,4 +192,48 @@ describe('wrapServerRequestHandler', () => { const setCookie = transactions[0]?.contexts?.trace?.data?.['http.response.header.set-cookie'] as string[]; expect(setCookie.map(cookie => cookie.split('=')[0])).toEqual(['first', 'second']); }); + + it('calls onDone once with the response, or without one when the handler throws', async () => { + const done: (number | undefined)[] = []; + const { serve } = setup({}, { onDone: response => done.push(response?.status) }); + + const response = await serve(new Request('https://example.com/ok'), () => new Response('ok', { status: 201 })); + await response.text(); + await expect( + serve(new Request('https://example.com/throws'), () => { + throw new Error('handler failed'); + }), + ).rejects.toThrow('handler failed'); + await serve(new Request('https://example.com/cors', { method: 'OPTIONS' }), () => new Response(null)); + + expect(done).toEqual([201, undefined, 200]); + }); + + it('does not capture errors without a mechanism', async () => { + const { events, serve } = setup({}, { mechanism: undefined }); + + await expect( + serve(new Request('https://example.com/throws'), () => { + throw new Error('handler failed'); + }), + ).rejects.toThrow('handler failed'); + + expect(events).toHaveLength(0); + }); + + it('calls onDone at once when isStreamingResponse returns false', async () => { + const onDone = vi.fn(); + const { serve } = setup({}, { onDone, isStreamingResponse: () => false }); + const source = new ReadableStream({ + start() { + // never closes + }, + }); + const original = new Response(source, { headers: { 'content-type': 'text/event-stream' } }); + + const response = await serve(new Request('https://example.com/events'), () => original); + + expect(response).toBe(original); + expect(onDone).toHaveBeenCalledTimes(1); + }); });