ref(node): Refactor http instrumentation away from OTEL

dev-packages/node-integration-tests/suites/tracing/http-client-spans/http-otel-double-instrumentation/test.ts

@@ -31,10 +31,10 @@ describe('http OTel double instrumentation', () => {
  // - On Node >=22.12 Sentry's SentryHttpInstrumentation additionally
  // subscribes to the http.client.request.created diagnostic channel.
  // The channel fires inside OTel's already-patched http.request,
- // triggering Sentry to create a *second* http.client span as a
- //  child of the first.
- // - On Node <22.12 both instrumentations monkey-patch http.request,
- // so both wrappers fire and each creates its own span.
+ // triggering Sentry to create a *second* http.client span.
+ // - On Node <22.12 Sentry instead patches
+ //  `ClientRequest.prototype._storeHeader`, which also fires per
+ // request, so Sentry creates its own span there.
  //
  // MITIGATION: pass `spans: false` to httpIntegration() so Sentry
  // defers all outgoing span creation to OTel's HttpInstrumentation
@@ -43,17 +43,25 @@ describe('http OTel double instrumentation', () => {
  // See the 'mitigation' scenario alongside this test.
  expect(httpClientSpans).toHaveLength(2);
 
- // The outer span comes from OTel HttpInstrumentation (no Sentry
- // origin). The inner span is the one Sentry's own handler creates;
- // it is a *child* of the outer span with origin 'auto.http.client'.
+ // One span comes from OTel HttpInstrumentation (no Sentry origin),
+ // the other is the one Sentry's own handler creates (origin
+ // 'auto.http.client').
  const sentrySpan = httpClientSpans.find(s => s.data?.['sentry.origin'] === 'auto.http.client');
  const otelSpan = httpClientSpans.find(s => s.data?.['sentry.origin'] !== 'auto.http.client');
 
  expect(sentrySpan).toBeDefined();
  expect(otelSpan).toBeDefined();
 
- // the sentry-created span is nested inside the otel-created span.
- expect(sentrySpan!.parent_span_id).toBe(otelSpan!.span_id);
+ // How the two spans nest depends on *when* Sentry creates its span
+ // relative to OTel's patched `http.request`:
+ // - On Node >=22.12 the `http.client.request.created` channel fires
+ // inside OTel's patched `http.request`, so Sentry's span nests as
+ // a child of the OTel span.
+ // - On Node <22.12 Sentry's `_storeHeader` patch fires at
+ // `request.end()`, outside OTel's request context, so Sentry's
+ // span is a sibling of the OTel span under the transaction.
+ const transactionSpanId = txn.contexts?.trace?.span_id;
+ expect([otelSpan!.span_id, transactionSpanId]).toContain(sentrySpan!.parent_span_id);
  },
  })
  .start();

dev-packages/node-integration-tests/suites/tracing/requests/http-maxed-out-sockets/instrument.mjs

@@ -0,0 +1,9 @@
+import * as Sentry from '@sentry/node';
+import { loggingTransport } from '@sentry-internal/node-integration-tests';
+
+Sentry.init({
+ dsn: 'https://public@dsn.ingest.sentry.io/1337',
+ release: '1.0',
+ tracesSampleRate: 1.0,
+ transport: loggingTransport,
+});

dev-packages/node-integration-tests/suites/tracing/requests/http-maxed-out-sockets/scenario.mjs

@@ -0,0 +1,30 @@
+import * as Sentry from '@sentry/node';
+import * as http from 'http';
+
+// An agent that only allows a single concurrent socket. The second request is
+// queued behind the first, so its socket is not assigned until after the first
+// request finishes and its headers have already been serialized via
+// `_storeHeader`. Trace-propagation headers must still be injected in this case.
+const agent = new http.Agent({ maxSockets: 1, keepAlive: false });
+
+Sentry.startSpan({ name: 'test_span' }, async () => {
+ await Promise.all([
+ makeHttpRequest(`${process.env.SERVER_URL}/api/request-1`),
+ makeHttpRequest(`${process.env.SERVER_URL}/api/request-2`),
+ ]);
+});
+
+function makeHttpRequest(url) {
+ return new Promise(resolve => {
+ http
+ .request(url, { agent, headers: { connection: 'close' } }, httpRes => {
+ httpRes.on('data', () => {
+ // we don't care about data
+ });
+ httpRes.on('end', () => {
+ resolve();
+ });
+ })
+ .end();
+ });
+}

dev-packages/node-integration-tests/suites/tracing/requests/http-maxed-out-sockets/test.ts

@@ -0,0 +1,33 @@
+import { createTestServer } from '@sentry-internal/test-utils';
+import { describe, expect } from 'vitest';
+import { createEsmAndCjsTests } from '../../../../utils/runner';
+
+describe('outgoing http with maxed-out agent sockets', () => {
+ createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => {
+ test('injects trace headers into requests queued behind a busy socket', async () => {
+ expect.assertions(5);
+
+ const [SERVER_URL, closeTestServer] = await createTestServer()
+ .get('/api/request-1', headers => {
+ expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f\d]{32})-([a-f\d]{16})-1$/));
+ expect(headers['baggage']).toEqual(expect.any(String));
+ })
+ .get('/api/request-2', headers => {
+ expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f\d]{32})-([a-f\d]{16})-1$/));
+ expect(headers['baggage']).toEqual(expect.any(String));
+ })
+ .start();
+
+ await createRunner()
+ .withEnv({ SERVER_URL })
+ .expect({
+ transaction: {
+ // we're not too concerned with the actual transaction here since this is tested elsewhere
+ },
+ })
+ .start()
+ .completed();
+ closeTestServer();
+ });
+ });
+});

dev-packages/test-utils/src/server.ts

@@ -40,9 +40,13 @@ export function createBasicSentryServer(onEnvelope: (env: Envelope) => void): Pr
 
 type HeaderAssertCallback = (headers: Record<string, string | string[] | undefined>) => void;
 
+interface TestServer {
+ get: (path: string, callback: HeaderAssertCallback, result?: number) => TestServer;
+ start: () => Promise<[string, () => void]>;
+}
+
 /** Creates a test server that can be used to check headers */
-// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
-export function createTestServer() {
+export function createTestServer(): TestServer {
  const gets: Array<[string, HeaderAssertCallback, number]> = [];
  let error: unknown | undefined;
 

packages/core/src/integrations/http/client-patch.ts

@@ -37,63 +37,84 @@ import type { HttpExport, HttpModuleExport, HttpInstrumentationOptions, HttpClie
 import { getOriginalFunction, wrapMethod } from '../../utils/object';
 import { getHttpClientSubscriptions } from './client-subscriptions';
 
-function patchHttpRequest(httpModule: HttpExport, options: HttpInstrumentationOptions): void {
- // avoid double-wrap
- if (!getOriginalFunction(httpModule.request)) {
- const { [HTTP_ON_CLIENT_REQUEST]: onHttpClientRequestCreated } = getHttpClientSubscriptions({
- ...options,
- http: httpModule,
- });
+let onHttpClientRequestCreated: ReturnType<typeof getHttpClientSubscriptions>[typeof HTTP_ON_CLIENT_REQUEST];
 
- const originalRequest = httpModule.request;
- wrapMethod(httpModule, 'request', function patchedRequest(this: HttpExport, ...args: unknown[]) {
- const request = originalRequest.apply(this, args) as HttpClientRequest;
- onHttpClientRequestCreated({ request }, HTTP_ON_CLIENT_REQUEST);
- return request;
- });
+/**
+ * Patch `ClientRequest.prototype._storeHeader` so that every outgoing request
+ * is routed through our instrumentation.
+ *
+ * We deliberately patch the shared `ClientRequest` prototype rather than the
+ * module's `request`/`get` exports. Every outgoing request — no matter how the
+ * module was imported (`require('node:http')`, `import http from 'node:http'`,
+ * or `import * as http from 'node:http'`) — ultimately constructs a
+ * `ClientRequest` and serializes its headers through `_storeHeader` on this one
+ * prototype. ES module namespace bindings (`import * as http` / `import
+ * { request }`) are immutable snapshots that cannot be monkey-patched at all,
+ * but the prototype is a shared, mutable object, so patching it reaches those
+ * consumers too.
+ *
+ * `_storeHeader` is the method that renders the outgoing header block into
+ * `request._header`. We run *before* the original, which is the last moment a
+ * header can still be added via `setHeader` (afterwards `request._header` is
+ * set and any `setHeader` call throws `ERR_HTTP_HEADERS_SENT`). It runs
+ * synchronously in the caller's async context — during `request.end()` /
+ * `request.write()` — so spans are parented correctly, and is invoked exactly
+ * once per request whether the headers are being sent immediately or buffered
+ * while a socket is assigned.
+ *
+ * We intentionally do *not* hook `onSocket`: it does not fire until a socket is
+ * assigned to the request, which for an `Agent` with all sockets busy (e.g.
+ * `maxSockets: 1`) happens only *after* the request's headers have already been
+ * serialized — far too late to inject trace-propagation headers.
+ *
+ * `https` requests reuse `http`'s `ClientRequest`, so patching `http` covers
+ * both; the `https` module does not expose its own `ClientRequest` and is a
+ * no-op here.
+ */
+function patchClientRequest(httpModule: HttpExport, options: HttpInstrumentationOptions): void {
+ const proto = httpModule.ClientRequest?.prototype;
+
+ // Nothing to patch if the module doesn't expose `ClientRequest` (e.g. `https`)
+ if (typeof proto?._storeHeader !== 'function') {
+ return;
  }
-}
 
-// This simply ensures that http.get calls http.request, which we patched.
-// Call it from the object each time, to ensure that any subsequent patches
-// or other mutations are also respected.
-function patchHttpGet(httpModule: HttpExport) {
- if (!getOriginalFunction(httpModule.get)) {
- // match node's normalization to exactly 3 arguments.
- wrapMethod(httpModule, 'get', function patchedGet(this: HttpExport, input: unknown, options: unknown, cb: unknown) {
- // http.get is like http.request but automatically calls .end()
- const request = httpModule.request.call(this, input, options, cb) as HttpClientRequest;
- request.end();
- return request;
- });
+ const subscriptions = getHttpClientSubscriptions({
+ ...options,
+ http: httpModule,
+ });
+
+ onHttpClientRequestCreated = subscriptions[HTTP_ON_CLIENT_REQUEST];
+
+ // This means it was already wrapped, we just update onHttpClientRequestCreated and then stop
+ // future calls will pick up the new function
+ if (getOriginalFunction(proto._storeHeader)) {
+ return;
  }
+
+ const originalStoreHeader = proto._storeHeader;
+ wrapMethod(proto, '_storeHeader', function patchedStoreHeader(this: HttpClientRequest, ...args: unknown[]) {
+ // Never let instrumentation errors break the underlying request.
+ try {
+ onHttpClientRequestCreated({ request: this }, HTTP_ON_CLIENT_REQUEST);
+ } catch {
+ // ignore
+ }
+ return originalStoreHeader.apply(this, args);
+ });
 }
 
 function patchModule(httpModuleExport: HttpModuleExport, options: HttpInstrumentationOptions = {}): HttpModuleExport {
- const httpDefault = getDefaultExport(httpModuleExport);
- const httpModule = httpModuleExport as HttpExport;
- // if we have a default, patch that, and copy to the import container
- if (httpDefault !== httpModuleExport) {
- patchModule(httpDefault, options);
- // copy with defineProperty because these might be configured oddly
- for (const method of ['get', 'request']) {
- const desc = Object.getOwnPropertyDescriptor(httpDefault, method);
- /* v8 ignore start - will always be set at this point */
- if (desc) {
- Object.defineProperty(httpModule, method, desc);
- }
- /* v8 ignore stop */
- }
- return httpModule;
- }
- patchHttpRequest(httpModule, options);
- patchHttpGet(httpModule);
+ // Resolve to the underlying module in case we were handed an interop
+ // container (e.g. `{ default: http }`). Either the container or its default
+ // export carries the same `ClientRequest` class.
+ const httpModule = getDefaultExport(httpModuleExport);
+ patchClientRequest(httpModule, options);
  return httpModuleExport;
 }
 
 /**
- * Patch an `node:http` or `node:https` module-shaped export so that every
- * outgoing request is tracked by Sentry.
+ * Patch `node:http`. This also covers `node:https` as it reuses the same `ClientRequest` class.
  *
  * @example
  * ```javascript

packages/core/src/integrations/http/types.ts

@@ -102,12 +102,33 @@ export interface HttpIncomingMessage {
  removeListener(event: string | symbol, listener: (...args: unknown[]) => void): this;
 }
 
-/** Minimal interface for a Node.js http / https module export */
+/** Minimal interface for the Node.js `http.ClientRequest` constructor. */
+export interface HttpClientRequestConstructor {
+ prototype: {
+ // The method we actually patch: it renders the outgoing header block into
+ // `request._header`, is called exactly once per request right before the
+ // headers are serialized, and is the last point at which a header can still
+ // be added. `https` requests go through `http`'s `ClientRequest`, so this
+ // is the shared choke point for all outgoing requests. `_storeHeader` is
+ // not part of the public `@types/node` surface, so we also reference the
+ // (publicly typed) `onSocket` to keep the real `node:http` `ClientRequest`
+ // structurally assignable to this type.
+ //oxlint-disable-next-line typescript/no-explicit-any
+ _storeHeader?: (this: HttpClientRequest, ...args: any[]) => unknown;
+ //oxlint-disable-next-line typescript/no-explicit-any
+ onSocket?: (this: HttpClientRequest, ...args: any[]) => unknown;
+ };
+}
+
+/** Minimal interface for a Node.js http module export */
 export interface HttpExport {
  //oxlint-disable typescript/no-explicit-any
  request: (...args: any[]) => HttpClientRequest;
  //oxlint-disable typescript/no-explicit-any
  get: (...args: any[]) => HttpClientRequest;
+ // Only `http` exports this; `https` reuses `http`'s `ClientRequest`, so this
+ // is `undefined` on the `https` module.
+ ClientRequest?: HttpClientRequestConstructor;
  [key: string]: unknown;
 }