diff --git a/.cursor/BUGBOT.md b/.cursor/BUGBOT.md index 8212b1b8489e..c0db9f483eb2 100644 --- a/.cursor/BUGBOT.md +++ b/.cursor/BUGBOT.md @@ -78,6 +78,7 @@ Keep reviews high-signal. Prefer actionable, high-confidence findings over specu - What to do instead: Use an existing reference to the scope or client. For example, this is possible in most `Integration` hooks. - Flag hard-coded attribute names and span ops (e.g. `'sentry.origin'`, `'sentry.op'`, `'http.request.method'`, `'db.system.name'`) when `@sentry/conventions` exports a constant for them. Use the constant from `@sentry/conventions/attributes` (e.g. `SENTRY_ORIGIN`, `SENTRY_OP`) or `@sentry/conventions/op` instead. Do not flag values that have no constant in `@sentry/conventions`. - Flag unnecessary `span.setAttribute(s)` calls: If data is already available at span start, it must be set via the `attributes` option of `startSpan`, `startSpanManual`, `startInactiveSpan` or `startIdleSpan` calls. This ensures that as much context as possible is available when `tracesSampler` or `ignoreSpans` SDK options are applied. If a `span.setAttribute(s)` call happens at a later time than right after span start and the attribute value can only be computed at that time, do not flag it. +- Flag a new option on an integration for incoming HTTP requests that is not in `ServerRequestOptions` (`packages/core/src/integrations/http/server-request-options.ts`), when it applies to more than one runtime. These integrations are Node `httpIntegration`, Deno `denoHttpIntegration` and `denoServeIntegration`, Bun `bunHttpServerIntegration` and `bunServerIntegration`, and Cloudflare `httpServerIntegration`. Add the option to `ServerRequestOptions`, then support it in each runtime or list it as unsupported in that runtime's `CheckServerRequestOptions` check. Also flag a runtime option that matches a shared option in purpose but has a different name or meaning. ### Code quality diff --git a/packages/bun/src/integrations/bunHttpServer.ts b/packages/bun/src/integrations/bunHttpServer.ts index f21d47e93f67..bb423dc5773d 100644 --- a/packages/bun/src/integrations/bunHttpServer.ts +++ b/packages/bun/src/integrations/bunHttpServer.ts @@ -3,7 +3,12 @@ import * as http from 'node:http'; import * as https from 'node:https'; import type { IntegrationFn, Span } from '@sentry/core'; import { defineIntegration } from '@sentry/core'; -import type { HttpIncomingMessage, HttpServerResponse } from '@sentry/core/server'; +import type { + AssertTrue, + CheckServerRequestOptions, + HttpIncomingMessage, + HttpServerResponse, +} from '@sentry/core/server'; import { getHttpServerSubscriptions, HTTP_ON_SERVER_REQUEST } from '@sentry/core/server'; const INTEGRATION_NAME = 'BunHttpServer' as const; @@ -37,8 +42,12 @@ interface BunHttpServerOptions { /** * Do not capture the request body for incoming HTTP requests to URLs where the given callback returns `true`. + * + * The `request` parameter is the incoming `node:http` `IncomingMessage`. The `RequestOptions` part of the type is + * deprecated and will be removed in v12. Its fields that `IncomingMessage` does not have (such as `path` or + * `hostname`) are always `undefined`. */ - ignoreRequestBody?: (url: string, request: http.RequestOptions) => boolean; + ignoreRequestBody?: (url: string, request: http.RequestOptions & HttpIncomingMessage) => boolean; /** * Controls the maximum size of incoming HTTP request bodies attached to events. @@ -73,6 +82,13 @@ interface BunHttpServerOptions { onSpanEnd?: (span: Span, request: HttpIncomingMessage, response: HttpServerResponse) => void; } +// Checks these options against the shared `ServerRequestOptions`. A new +// shared field fails to compile here, until this integration supports +// it or lists it as unsupported. +type _ServerRequestOptionsCheck = AssertTrue< + CheckServerRequestOptions +>; + let hasPatched = false; const _bunHttpServerIntegration = ((options: BunHttpServerOptions = {}) => { diff --git a/packages/bun/src/integrations/bunserver.ts b/packages/bun/src/integrations/bunserver.ts index 1bd340703aec..46ff0fc30e25 100644 --- a/packages/bun/src/integrations/bunserver.ts +++ b/packages/bun/src/integrations/bunserver.ts @@ -1,3 +1,4 @@ +/* eslint-disable max-lines */ import type { Integration, IntegrationFn, MaxRequestBodySize, SpanAttributes } from '@sentry/core'; import { captureBodyFromWinterCGRequest, @@ -20,6 +21,7 @@ import { filterCollectedUrl, filterCollectedUrlQuery, } from '@sentry/core'; +import type { AssertTrue, CheckServerRequestOptions } from '@sentry/core/server'; import { classifyResponseStreaming, getClientIPAddress } from '@sentry/core/server'; import type { Server, ServeOptions } from 'bun'; import { @@ -55,6 +57,26 @@ export type BunServerIntegrationOptions = { maxRequestBodySize?: MaxRequestBodySize; }; +// Checks these options against the shared `ServerRequestOptions`. A new +// shared field fails to compile here, until this integration supports +// it or lists it as unsupported. +type _ServerRequestOptionsCheck = AssertTrue< + CheckServerRequestOptions< + BunServerIntegrationOptions, + Request, + Response, + // Not supported yet. + | 'spans' + | 'sessions' + | 'sessionFlushingDelayMS' + | 'ignoreRequestBody' + | 'ignoreIncomingRequests' + | 'ignoreStaticAssets' + | 'onSpanCreated' + | 'onSpanEnd' + > +>; + const _bunServerIntegration = ((options: BunServerIntegrationOptions = {}) => { return { name: INTEGRATION_NAME, diff --git a/packages/cloudflare/src/integrations/httpServer.ts b/packages/cloudflare/src/integrations/httpServer.ts index 97e960f916d2..7fa3cd7ce30b 100644 --- a/packages/cloudflare/src/integrations/httpServer.ts +++ b/packages/cloudflare/src/integrations/httpServer.ts @@ -1,5 +1,6 @@ import type { Client, IntegrationFn, MaxRequestBodySize } from '@sentry/core'; import { captureBodyFromWinterCGRequest, defineIntegration, getIsolationScope } from '@sentry/core'; +import type { AssertTrue, CheckServerRequestOptions } from '@sentry/core/server'; const INTEGRATION_NAME = 'HttpServer' as const; @@ -40,6 +41,27 @@ export interface HttpServerIntegrationOptions { ignoreRequestBody?: (url: string, request: Request) => boolean; } +// Checks these options against the shared `ServerRequestOptions`. A new +// shared field fails to compile here, until this integration supports +// it or lists it as unsupported. +type _ServerRequestOptionsCheck = AssertTrue< + CheckServerRequestOptions< + HttpServerIntegrationOptions, + Request, + Response, + // Not supported yet. + | 'spans' + | 'ignoreIncomingRequests' + | 'ignoreStaticAssets' + | 'onSpanCreated' + | 'onSpanEnd' + // Not supported. A worker has no long-lived process to run the timer + // that flushes session aggregates. + | 'sessions' + | 'sessionFlushingDelayMS' + > +>; + interface HttpServerIntegrationInstance { name: string; maxRequestBodySize: MaxRequestBodySize | undefined; diff --git a/packages/core/src/integrations/http/server-request-options.ts b/packages/core/src/integrations/http/server-request-options.ts new file mode 100644 index 000000000000..7125978ad603 --- /dev/null +++ b/packages/core/src/integrations/http/server-request-options.ts @@ -0,0 +1,128 @@ +import type { Span } from '../../types/span'; +import type { MaxRequestBodySize } from '../../utils/request'; + +/** + * Options that every integration for incoming HTTP requests should + * support, in each runtime. + * + * `TRequest` and `TResponse` are the request and response types that the + * runtime passes to callbacks: `HttpIncomingMessage` and + * `HttpServerResponse` for `node:http`, or the web `Request` and + * `Response` for `Deno.serve`, `Bun.serve` and Cloudflare. + * + * Runtime options types do not extend this type. Each one declares its + * own fields, with docs for that runtime, and uses + * {@link CheckServerRequestOptions} to check them against this type. + * Then a new field here fails to compile in each runtime, until the + * runtime supports it or lists it as unsupported. + */ +export interface ServerRequestOptions { + /** + * Whether to create spans for incoming requests. + * Defaults to the client's tracing configuration. + */ + spans?: boolean; + + /** + * Whether to create [Sessions](https://docs.sentry.io/product/releases/health/#sessions) for incoming + * requests, to track the health and crash-free rate of releases. + * + * @default `true` + */ + sessions?: boolean; + + /** + * Number of milliseconds until sessions are flushed as a session aggregate. + * + * @default `60000` (60s) + */ + sessionFlushingDelayMS?: number; + + /** + * Controls the maximum size of incoming request bodies attached to events. + * + * @default 'medium' + */ + maxRequestBodySize?: MaxRequestBodySize; + + /** + * Do not capture the request body for incoming requests where the given callback returns `true`. + * + * `url` is the full URL of the request, including the query string. + */ + ignoreRequestBody?: (url: string, request: TRequest) => boolean; + + /** + * Do not create spans for incoming requests where the given callback returns `true`. + * + * `urlPath` is the path of the request, including the query string. + */ + ignoreIncomingRequests?: (urlPath: string, request: TRequest) => boolean; + + /** + * Whether to ignore common static asset requests (favicon.ico, robots.txt, etc.) + * when creating spans. + * + * @default `true` + */ + ignoreStaticAssets?: boolean; + + /** + * A hook to change the span for an incoming request, after the span is created. + */ + onSpanCreated?: (span: Span, request: TRequest) => void; + + /** + * A hook to change the span for an incoming request one last time, before it ends. + */ + onSpanEnd?: (span: Span, request: TRequest, response: TResponse) => void; +} + +/** + * `true` when `TOptions` matches {@link ServerRequestOptions}, else a + * string that names the problem. Use it with a type that accepts only + * `true`, so a mismatch fails to compile: + * + * ```ts + * type _Check = AssertTrue>; + * ``` + * + * `TUnsupported` lists the fields that the runtime does not support. + * + * The check fails when: + * - `TOptions` does not declare a field, and `TUnsupported` does not + * list it. + * - `TUnsupported` lists a field that `TOptions` declares. + * - A callback written for the shared type does not fit the field in + * `TOptions`. + */ +export type CheckServerRequestOptions< + TOptions, + TRequest, + TResponse, + TUnsupported extends keyof ServerRequestOptions = never, +> = [MissingFields] extends [never] + ? [Extract] extends [never] + ? SupportedFields< + ServerRequestOptions, + TRequest, + TResponse, + TUnsupported + > extends SupportedFields + ? true + : 'A field type does not match ServerRequestOptions' + : `Listed as unsupported, but declared: ${Extract & string}` + : `Missing field: ${MissingFields & string}`; + +type MissingFields = Exclude< + keyof ServerRequestOptions, + TUnsupported | keyof TOptions +>; + +type SupportedFields = Pick< + T, + Extract, TUnsupported>> +>; + +/** Accepts only `true`. Use it with {@link CheckServerRequestOptions}. */ +export type AssertTrue = T; diff --git a/packages/core/src/server.ts b/packages/core/src/server.ts index c6d78b00f52d..626858543c77 100644 --- a/packages/core/src/server.ts +++ b/packages/core/src/server.ts @@ -43,6 +43,11 @@ export type { HttpServerResponse, HttpModuleExport, } from './integrations/http/types'; +export type { + AssertTrue, + CheckServerRequestOptions, + ServerRequestOptions, +} from './integrations/http/server-request-options'; export { createFetchIntegration } from './integrations/fetch'; export type { FetchIntegrationOptions } from './integrations/fetch'; export { classifyResponseStreaming } from './utils/responseStreaming'; diff --git a/packages/deno/src/integrations/deno-serve.ts b/packages/deno/src/integrations/deno-serve.ts index 9d20e9084848..8c8a4038e5b5 100644 --- a/packages/deno/src/integrations/deno-serve.ts +++ b/packages/deno/src/integrations/deno-serve.ts @@ -1,5 +1,6 @@ import type { Event, IntegrationFn, MaxRequestBodySize, Span } from '@sentry/core'; import { debug, defineIntegration } from '@sentry/core'; +import type { AssertTrue, CheckServerRequestOptions } from '@sentry/core/server'; import { DEFAULT_IGNORE_STATUS_CODES, processHttpServerTransactionEvent } from '@sentry/core/server'; import type { RequestHandlerWrapperOptions } from '../wrap-deno-request-handler'; import { wrapDenoRequestHandler } from '../wrap-deno-request-handler'; @@ -99,6 +100,11 @@ export type DenoServeIntegrationOptions = { ignoreStatusCodes?: (number | [number, number])[]; }; +// Checks these options against the shared `ServerRequestOptions`. A new +// shared field fails to compile here, until this integration supports +// it or lists it as unsupported. +type _ServerRequestOptionsCheck = AssertTrue>; + export type ServeParams = // [(Request) => Response] | [Deno.ServeHandler] diff --git a/packages/deno/src/integrations/http.ts b/packages/deno/src/integrations/http.ts index 34dc1ff4e5f4..ea6a4fbae6c0 100644 --- a/packages/deno/src/integrations/http.ts +++ b/packages/deno/src/integrations/http.ts @@ -3,7 +3,13 @@ import { errorMonitor } from 'node:events'; import type { RequestOptions } from 'node:http'; import type { Event, Integration, IntegrationFn, Span } from '@sentry/core'; import { defineIntegration } from '@sentry/core'; -import type { HttpClientRequest, HttpIncomingMessage, HttpServerResponse } from '@sentry/core/server'; +import type { + AssertTrue, + CheckServerRequestOptions, + HttpClientRequest, + HttpIncomingMessage, + HttpServerResponse, +} from '@sentry/core/server'; import { DEFAULT_IGNORE_STATUS_CODES, getHttpClientSubscriptions, @@ -151,6 +157,13 @@ export interface DenoHttpIntegrationOptions { onSpanEnd?: (span: Span, request: HttpIncomingMessage, response: HttpServerResponse) => void; } +// Checks these options against the shared `ServerRequestOptions`. A new +// shared field fails to compile here, until this integration supports +// it or lists it as unsupported. +type _ServerRequestOptionsCheck = AssertTrue< + CheckServerRequestOptions +>; + const _denoHttpIntegration = ((options: DenoHttpIntegrationOptions = {}) => { const breadcrumbs = options.breadcrumbs ?? true; const tracePropagation = options.tracePropagation ?? true; diff --git a/packages/node/src/integrations/http/httpServerIntegration.ts b/packages/node/src/integrations/http/httpServerIntegration.ts index 6322cd0169bd..b3c0c939d971 100644 --- a/packages/node/src/integrations/http/httpServerIntegration.ts +++ b/packages/node/src/integrations/http/httpServerIntegration.ts @@ -45,9 +45,11 @@ export interface HttpServerIntegrationOptions { * This can be useful for long running requests where the body is not needed and we want to avoid capturing it. * * @param url Contains the entire URL, including query string (if any), protocol, host, etc. of the incoming request. - * @param request Contains the {@type RequestOptions} object used to make the incoming request. + * @param request The incoming request, a `node:http` `IncomingMessage`. Use `request.url`, `request.method`, + * `request.headers`, etc. The `RequestOptions` part of the type is deprecated and will be removed in v12. Its + * fields that `IncomingMessage` does not have (such as `path` or `hostname`) are always `undefined`. */ - ignoreRequestBody?: (url: string, request: RequestOptions) => boolean; + ignoreRequestBody?: (url: string, request: RequestOptions & HttpIncomingMessage) => boolean; /** * Controls the maximum size of incoming HTTP request bodies attached to events. @@ -84,13 +86,7 @@ const _httpServerIntegration = ((options: HttpServerIntegrationOptions = {}) => // core subscription helper. Explicitly opt out so the helper does not // double-create spans when the client has tracing enabled. spans: false as const, - // Cast: core uses HttpIncomingMessage; node consumers pass - // RequestOptions-typed callbacks. - // The two are structurally compatible for the fields the callback reads - // (url, method, headers). - ignoreRequestBody: options.ignoreRequestBody as - | ((url: string, request: HttpIncomingMessage) => boolean) - | undefined, + ignoreRequestBody: options.ignoreRequestBody, /** * Hook called by core's `instrumentServer` to wrap the upstream diff --git a/packages/node/src/integrations/http/index.ts b/packages/node/src/integrations/http/index.ts index f0cfc28cdc87..7431a6a34537 100644 --- a/packages/node/src/integrations/http/index.ts +++ b/packages/node/src/integrations/http/index.ts @@ -8,6 +8,12 @@ import { hasSpanStreamingEnabled, stripDataUrlContent, } from '@sentry/core'; +import type { + AssertTrue, + CheckServerRequestOptions, + HttpIncomingMessage, + HttpServerResponse, +} from '@sentry/core/server'; import { getRequestUrlFromClientRequest } from '@sentry/core/server'; import type { NodeClient } from '../../sdk/client'; import type { HttpServerIntegrationOptions } from './httpServerIntegration'; @@ -83,6 +89,19 @@ interface HttpOptions extends HttpServerIntegrationOptions, HttpServerSpansInteg outgoingRequestApplyCustomAttributes?: OutgoingHttpRequestInstrumentationOptions['outgoingRequestApplyCustomAttributes']; } +// Checks these options against the shared `ServerRequestOptions`. A new +// shared field fails to compile here, until this integration supports +// it or lists it as unsupported. +type _ServerRequestOptionsCheck = AssertTrue< + CheckServerRequestOptions< + HttpOptions, + HttpIncomingMessage, + HttpServerResponse, + // Not supported yet. + 'onSpanEnd' + > +>; + /** * The http integration instruments Node's internal http and https modules. * It creates breadcrumbs and spans for outgoing HTTP requests which will be attached to the currently active span.