feat(node): Split up http integration into composable parts (#17524)

This is a first step to de-compose the node/node-core `httpIntegration`
into multiple composable parts:

* `httpServerIntegration` - handles request isolation, sessions, trace
continuation, so core Sentry functionality
* `httpServerSpansIntegration` - emits `http.server` spans for incoming
requests

The `httpIntegration` sets these up under the hood, and for now it is
not really recommended for users to use these stand-alone (though it is
possible if users opt-out of the `httpIntegration`). The reason is to
remain backwards compatible with users using/customizing the
`httpIntegration`. We can revisit this in a later major.

These new integrations have a much slimmer API surface, and also allows
us to avoid having to prefix all the options etc. with what they are
about (e.g. `incomingXXX` or `outgoingXXX`). It also means you can
actually tree-shake certain features (span creation) out, in theory.

Outgoing request handling remains the same for the time being, once we
decoupled this from the otel http instrumentation we can do something
similar there.

The biggest challenge was how to make it possible to de-compose this
without having to monkey patch the http server twice. I opted to allow
to add callbacks to the `httpServerIntegration` which it will call on
any request. So the `httpServerSpansIntegration` can register a callback
for itself and plug into this with little overhead.

Author: mydea

packages/astro/src/index.server.ts

@@ -66,6 +66,8 @@ export {
   hapiIntegration,
   honoIntegration,
   httpIntegration,
+  httpServerIntegration,
+  httpServerSpansIntegration,
   // eslint-disable-next-line deprecation/deprecation
   inboundFiltersIntegration,
   eventFiltersIntegration,

packages/astro/src/server/sdk.ts

@@ -1,5 +1,5 @@
 import { applySdkMetadata } from '@sentry/core';
-import type { NodeClient, NodeOptions } from '@sentry/node';
+import type { Event, NodeClient, NodeOptions } from '@sentry/node';
 import { init as initNodeSdk } from '@sentry/node';
 
 /**
@@ -13,5 +13,28 @@ export function init(options: NodeOptions): NodeClient | undefined {
 
   applySdkMetadata(opts, 'astro', ['astro', 'node']);
 
-  return initNodeSdk(opts);
+  const client = initNodeSdk(opts);
+
+  client?.addEventProcessor(
+    Object.assign(
+      (event: Event) => {
+        // For http.server spans that did not go though the astro middleware,
+        // we want to drop them
+        // this is the case with http.server spans of prerendered pages
+        // we do not care about those, as they are effectively static
+        if (
+          event.type === 'transaction' &&
+          event.contexts?.trace?.op === 'http.server' &&
+          event.contexts?.trace?.origin === 'auto.http.otel.http'
+        ) {
+          return null;
+        }
+
+        return event;
+      },
+      { id: 'AstroHttpEventProcessor' },
+    ),
+  );
+
+  return client;
 }

packages/aws-serverless/src/index.ts

@@ -50,6 +50,8 @@ export {
   disableAnrDetectionForCallback,
   consoleIntegration,
   httpIntegration,
+  httpServerIntegration,
+  httpServerSpansIntegration,
   nativeNodeFetchIntegration,
   onUncaughtExceptionIntegration,
   onUnhandledRejectionIntegration,

packages/bun/src/index.ts

@@ -70,6 +70,8 @@ export {
   disableAnrDetectionForCallback,
   consoleIntegration,
   httpIntegration,
+  httpServerIntegration,
+  httpServerSpansIntegration,
   nativeNodeFetchIntegration,
   onUncaughtExceptionIntegration,
   onUnhandledRejectionIntegration,

packages/core/src/client.ts

@@ -26,6 +26,7 @@ import type { Integration } from './types-hoist/integration';
 import type { Log } from './types-hoist/log';
 import type { ClientOptions } from './types-hoist/options';
 import type { ParameterizedString } from './types-hoist/parameterize';
+import type { RequestEventData } from './types-hoist/request';
 import type { SdkMetadata } from './types-hoist/sdkmetadata';
 import type { Session, SessionAggregates } from './types-hoist/session';
 import type { SeverityLevel } from './types-hoist/severity';
@@ -687,6 +688,17 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
    */
   public on(hook: 'flushLogs', callback: () => void): () => void;
 
+  /**
+   * A hook that is called when a http server request is started.
+   * This hook is called after request isolation, but before the request is processed.
+   *
+   * @returns {() => void} A function that, when executed, removes the registered callback.
+   */
+  public on(
+    hook: 'httpServerRequest',
+    callback: (request: unknown, response: unknown, normalizedRequest: RequestEventData) => void,
+  ): () => void;
+
   /**
    * Register a hook on this client.
    */
@@ -875,6 +887,17 @@ export abstract class Client<O extends ClientOptions = ClientOptions> {
    */
   public emit(hook: 'flushLogs'): void;
 
+  /**
+   * Emit a hook event for client when a http server request is started.
+   * This hook is called after request isolation, but before the request is processed.
+   */
+  public emit(
+    hook: 'httpServerRequest',
+    request: unknown,
+    response: unknown,
+    normalizedRequest: RequestEventData,
+  ): void;
+
   /**
    * Emit a hook that was previously registered via `on()`.
    */

packages/core/test/lib/utils/promisebuffer.test.ts

@@ -149,10 +149,12 @@ describe('PromiseBuffer', () => {
       expect(p5).toHaveBeenCalled();
 
       expect(buffer.$.length).toEqual(5);
-      const result = await buffer.drain(8);
+      const result = await buffer.drain(6);
       expect(result).toEqual(false);
-      // p5 is still in the buffer
-      expect(buffer.$.length).toEqual(1);
+      // p5 & p4 are still in the buffer
+      // Leaving some wiggle room, possibly one or two items are still in the buffer
+      // to avoid flakiness
+      expect(buffer.$.length).toBeGreaterThanOrEqual(1);
 
       // Now drain final item
       const result2 = await buffer.drain();

packages/google-cloud-serverless/src/index.ts

@@ -50,6 +50,8 @@ export {
   disableAnrDetectionForCallback,
   consoleIntegration,
   httpIntegration,
+  httpServerIntegration,
+  httpServerSpansIntegration,
   nativeNodeFetchIntegration,
   onUncaughtExceptionIntegration,
   onUnhandledRejectionIntegration,

packages/node-core/src/index.ts

@@ -1,6 +1,9 @@
 import * as logger from './logs/exports';
 
 export { httpIntegration } from './integrations/http';
+export { httpServerSpansIntegration } from './integrations/http/httpServerSpansIntegration';
+export { httpServerIntegration } from './integrations/http/httpServerIntegration';
+
 export {
   SentryHttpInstrumentation,
   type SentryHttpInstrumentationOptions,

packages/node-core/src/integrations/http/SentryHttpInstrumentation.ts

@@ -2,16 +2,15 @@ import type { ChannelListener } from 'node:diagnostics_channel';
 import { subscribe, unsubscribe } from 'node:diagnostics_channel';
 import type * as http from 'node:http';
 import type * as https from 'node:https';
-import type { Span } from '@opentelemetry/api';
 import { context } from '@opentelemetry/api';
 import { isTracingSuppressed } from '@opentelemetry/core';
 import type { InstrumentationConfig } from '@opentelemetry/instrumentation';
 import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation';
+import type { Span } from '@sentry/core';
 import { debug, LRUMap, SDK_VERSION } from '@sentry/core';
 import { DEBUG_BUILD } from '../../debug-build';
 import { getRequestUrl } from '../../utils/getRequestUrl';
 import { INSTRUMENTATION_NAME } from './constants';
-import { instrumentServer } from './incoming-requests';
 import {
   addRequestBreadcrumb,
   addTracePropagationHeadersToOutgoingRequest,
@@ -23,31 +22,12 @@ type Https = typeof https;
 
 export type SentryHttpInstrumentationOptions = InstrumentationConfig & {
   /**
-   * Whether breadcrumbs should be recorded for requests.
+   * Whether breadcrumbs should be recorded for outgoing requests.
    *
    * @default `true`
    */
   breadcrumbs?: boolean;
 
-  /**
-   * Whether to create spans for requests or not.
-   * As of now, creates spans for incoming requests, but not outgoing requests.
-   *
-   * @default `true`
-   */
-  spans?: boolean;
-
-  /**
-   * Whether to extract the trace ID from the `sentry-trace` header for incoming requests.
-   * By default this is done by the HttpInstrumentation, but if that is not added (e.g. because tracing is disabled, ...)
-   * then this instrumentation can take over.
-   *
-   * @deprecated This is always true and the option will be removed in the future.
-   *
-   * @default `true`
-   */
-  extractIncomingTraceFromHeader?: boolean;
-
   /**
    * Whether to propagate Sentry trace headers in outgoing requests.
    * By default this is done by the HttpInstrumentation, but if that is not added (e.g. because tracing is disabled)
@@ -57,20 +37,6 @@ export type SentryHttpInstrumentationOptions = InstrumentationConfig & {
    */
   propagateTraceInOutgoingRequests?: boolean;
 
-  /**
-   * Whether to automatically ignore common static asset requests like favicon.ico, robots.txt, etc.
-   * This helps reduce noise in your transactions.
-   *
-   * @default `true`
-   */
-  ignoreStaticAssets?: boolean;
-
-  /**
-   * If true, do not generate spans for incoming requests at all.
-   * This is used by Remix to avoid generating spans for incoming requests, as it generates its own spans.
-   */
-  disableIncomingRequestSpans?: boolean;
-
   /**
    * Do not capture breadcrumbs for outgoing HTTP requests to URLs where the given callback returns `true`.
    * For the scope of this instrumentation, this callback only controls breadcrumb creation.
@@ -82,55 +48,51 @@ export type SentryHttpInstrumentationOptions = InstrumentationConfig & {
    */
   ignoreOutgoingRequests?: (url: string, request: http.RequestOptions) => boolean;
 
+  // All options below do not do anything anymore in this instrumentation, and will be removed in the future.
+  // They are only kept here for backwards compatibility - the respective functionality is now handled by the httpServerIntegration/httpServerSpansIntegration.
+
   /**
-   * Do not capture spans for incoming HTTP requests to URLs where the given callback returns `true`.
-   *
-   * @param urlPath Contains the URL path and query string (if any) of the incoming request.
-   * @param request Contains the {@type IncomingMessage} object of the incoming request.
+   * @deprecated This no longer does anything.
    */
-  ignoreSpansForIncomingRequests?: (urlPath: string, request: http.IncomingMessage) => boolean;
+  spans?: boolean;
 
   /**
-   * Do not capture the request body for incoming HTTP requests to URLs where the given callback returns `true`.
-   * 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.
+   * @depreacted This no longer does anything.
    */
-  ignoreIncomingRequestBody?: (url: string, request: http.RequestOptions) => boolean;
+  extractIncomingTraceFromHeader?: boolean;
 
   /**
-   * A hook that can be used to mutate the span for incoming requests.
-   * This is triggered after the span is created, but before it is recorded.
+   * @deprecated This no longer does anything.
    */
-  incomingRequestSpanHook?: (span: Span, request: http.IncomingMessage, response: http.ServerResponse) => void;
+  ignoreStaticAssets?: boolean;
 
   /**
-   * Controls the maximum size of incoming HTTP request bodies attached to events.
-   *
-   * Available options:
-   * - 'none': No request bodies will be attached
-   * - 'small': Request bodies up to 1,000 bytes will be attached
-   * - 'medium': Request bodies up to 10,000 bytes will be attached (default)
-   * - 'always': Request bodies will always be attached
-   *
-   * Note that even with 'always' setting, bodies exceeding 1MB will never be attached
-   * for performance and security reasons.
-   *
-   * @default 'medium'
+   * @deprecated This no longer does anything.
+   */
+  disableIncomingRequestSpans?: boolean;
+
+  /**
+   * @deprecated This no longer does anything.
+   */
+  ignoreSpansForIncomingRequests?: (urlPath: string, request: http.IncomingMessage) => boolean;
+
+  /**
+   * @deprecated This no longer does anything.
+   */
+  ignoreIncomingRequestBody?: (url: string, request: http.RequestOptions) => boolean;
+
+  /**
+   * @deprecated This no longer does anything.
    */
   maxIncomingRequestBodySize?: 'none' | 'small' | 'medium' | 'always';
 
   /**
-   * Whether the integration should create [Sessions](https://docs.sentry.io/product/releases/health/#sessions) for incoming requests to track the health and crash-free rate of your releases in Sentry.
-   * Read more about Release Health: https://docs.sentry.io/product/releases/health/
-   *
-   * Defaults to `true`.
+   * @deprecated This no longer does anything.
    */
   trackIncomingRequestsAsSessions?: boolean;
 
   /**
-   * @deprecated This is deprecated in favor of `incomingRequestSpanHook`.
+   * @deprecated This no longer does anything.
    */
   instrumentation?: {
     requestHook?: (span: Span, req: http.ClientRequest | http.IncomingMessage) => void;
@@ -143,9 +105,7 @@ export type SentryHttpInstrumentationOptions = InstrumentationConfig & {
   };
 
   /**
-   * Number of milliseconds until sessions tracked with `trackIncomingRequestsAsSessions` will be flushed as a session aggregate.
-   *
-   * Defaults to `60000` (60s).
+   * @deprecated This no longer does anything.
    */
   sessionFlushingDelayMS?: number;
 };
@@ -180,24 +140,6 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns
     // but we only want to register them once, whichever is loaded first
     let hasRegisteredHandlers = false;
 
-    const spansEnabled = this.getConfig().spans ?? true;
-
-    const onHttpServerRequestStart = ((_data: unknown) => {
-      const data = _data as { server: http.Server };
-      instrumentServer(data.server, {
-        // eslint-disable-next-line deprecation/deprecation
-        instrumentation: this.getConfig().instrumentation,
-        ignoreIncomingRequestBody: this.getConfig().ignoreIncomingRequestBody,
-        ignoreSpansForIncomingRequests: this.getConfig().ignoreSpansForIncomingRequests,
-        incomingRequestSpanHook: this.getConfig().incomingRequestSpanHook,
-        maxIncomingRequestBodySize: this.getConfig().maxIncomingRequestBodySize,
-        trackIncomingRequestsAsSessions: this.getConfig().trackIncomingRequestsAsSessions,
-        sessionFlushingDelayMS: this.getConfig().sessionFlushingDelayMS ?? 60_000,
-        ignoreStaticAssets: this.getConfig().ignoreStaticAssets,
-        spans: spansEnabled && !this.getConfig().disableIncomingRequestSpans,
-      });
-    }) satisfies ChannelListener;
-
     const onHttpClientResponseFinish = ((_data: unknown) => {
       const data = _data as { request: http.ClientRequest; response: http.IncomingMessage };
       this._onOutgoingRequestFinish(data.request, data.response);
@@ -220,7 +162,6 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns
 
       hasRegisteredHandlers = true;
 
-      subscribe('http.server.request.start', onHttpServerRequestStart);
       subscribe('http.client.response.finish', onHttpClientResponseFinish);
 
       // When an error happens, we still want to have a breadcrumb
@@ -238,7 +179,6 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns
     };
 
     const unwrap = (): void => {
-      unsubscribe('http.server.request.start', onHttpServerRequestStart);
       unsubscribe('http.client.response.finish', onHttpClientResponseFinish);
       unsubscribe('http.client.request.error', onHttpClientRequestError);
       unsubscribe('http.client.request.created', onHttpClientRequestCreated);