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); + }); });