fix(node): Ensure httpIntegration propagates traces

mydea · mydea · commit 1e2a35ede687 · 2025-01-30T14:06:29.000+01:00
We used to rely on the existence of another `httpIntegration`
diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/scenario.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/scenario.ts
@@ -0,0 +1,61 @@
+import { loggingTransport } from '@sentry-internal/node-integration-tests';
+import * as Sentry from '@sentry/node';
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  tracePropagationTargets: [/\/v0/, 'v1'],
+  integrations: [Sentry.httpIntegration({ spans: false })],
+  transport: loggingTransport,
+  // Ensure this gets a correct hint
+  beforeBreadcrumb(breadcrumb, hint) {
+    breadcrumb.data = breadcrumb.data || {};
+    const req = hint?.request as { path?: string };
+    breadcrumb.data.ADDED_PATH = req?.path;
+    return breadcrumb;
+  },
+});
+
+import * as http from 'http';
+
+async function run(): Promise<void> {
+  Sentry.addBreadcrumb({ message: 'manual breadcrumb' });
+
+  await makeHttpRequest(`${process.env.SERVER_URL}/api/v0`);
+  await makeHttpGet(`${process.env.SERVER_URL}/api/v1`);
+  await makeHttpRequest(`${process.env.SERVER_URL}/api/v2`);
+  await makeHttpRequest(`${process.env.SERVER_URL}/api/v3`);
+
+  Sentry.captureException(new Error('foo'));
+}
+
+// eslint-disable-next-line @typescript-eslint/no-floating-promises
+run();
+
+function makeHttpRequest(url: string): Promise<void> {
+  return new Promise<void>(resolve => {
+    http
+      .request(url, httpRes => {
+        httpRes.on('data', () => {
+          // we don't care about data
+        });
+        httpRes.on('end', () => {
+          resolve();
+        });
+      })
+      .end();
+  });
+}
+
+function makeHttpGet(url: string): Promise<void> {
+  return new Promise<void>(resolve => {
+    http.get(url, httpRes => {
+      httpRes.on('data', () => {
+        // we don't care about data
+      });
+      httpRes.on('end', () => {
+        resolve();
+      });
+    });
+  });
+}
diff --git a/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts b/dev-packages/node-integration-tests/suites/tracing/requests/http-no-tracing-no-spans/test.ts
@@ -0,0 +1,95 @@
+import { createRunner } from '../../../../utils/runner';
+import { createTestServer } from '../../../../utils/server';
+
+test('outgoing http requests are correctly instrumented with tracing & spans disabled', done => {
+  expect.assertions(11);
+
+  createTestServer(done)
+    .get('/api/v0', headers => {
+      expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/));
+      expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000');
+      expect(headers['baggage']).toEqual(expect.any(String));
+    })
+    .get('/api/v1', headers => {
+      expect(headers['sentry-trace']).toEqual(expect.stringMatching(/^([a-f0-9]{32})-([a-f0-9]{16})$/));
+      expect(headers['sentry-trace']).not.toEqual('00000000000000000000000000000000-0000000000000000');
+      expect(headers['baggage']).toEqual(expect.any(String));
+    })
+    .get('/api/v2', headers => {
+      expect(headers['baggage']).toBeUndefined();
+      expect(headers['sentry-trace']).toBeUndefined();
+    })
+    .get('/api/v3', headers => {
+      expect(headers['baggage']).toBeUndefined();
+      expect(headers['sentry-trace']).toBeUndefined();
+    })
+    .start()
+    .then(([SERVER_URL, closeTestServer]) => {
+      createRunner(__dirname, 'scenario.ts')
+        .withEnv({ SERVER_URL })
+        .ensureNoErrorOutput()
+        .expect({
+          event: {
+            exception: {
+              values: [
+                {
+                  type: 'Error',
+                  value: 'foo',
+                },
+              ],
+            },
+            breadcrumbs: [
+              {
+                message: 'manual breadcrumb',
+                timestamp: expect.any(Number),
+              },
+              {
+                category: 'http',
+                data: {
+                  'http.method': 'GET',
+                  url: `${SERVER_URL}/api/v0`,
+                  status_code: 200,
+                  ADDED_PATH: '/api/v0',
+                },
+                timestamp: expect.any(Number),
+                type: 'http',
+              },
+              {
+                category: 'http',
+                data: {
+                  'http.method': 'GET',
+                  url: `${SERVER_URL}/api/v1`,
+                  status_code: 200,
+                  ADDED_PATH: '/api/v1',
+                },
+                timestamp: expect.any(Number),
+                type: 'http',
+              },
+              {
+                category: 'http',
+                data: {
+                  'http.method': 'GET',
+                  url: `${SERVER_URL}/api/v2`,
+                  status_code: 200,
+                  ADDED_PATH: '/api/v2',
+                },
+                timestamp: expect.any(Number),
+                type: 'http',
+              },
+              {
+                category: 'http',
+                data: {
+                  'http.method': 'GET',
+                  url: `${SERVER_URL}/api/v3`,
+                  status_code: 200,
+                  ADDED_PATH: '/api/v3',
+                },
+                timestamp: expect.any(Number),
+                type: 'http',
+              },
+            ],
+          },
+        })
+        .start(closeTestServer);
+    });
+});
diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts
@@ -8,26 +8,35 @@ import type { InstrumentationConfig } from '@opentelemetry/instrumentation';
 import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation';
 import type { AggregationCounts, Client, RequestEventData, SanitizedRequestData, Scope } from '@sentry/core';
 import {
+  LRUMap,
   addBreadcrumb,
   generateSpanId,
   getBreadcrumbLogLevelFromHttpStatusCode,
   getClient,
   getIsolationScope,
   getSanitizedUrlString,
+  getTraceData,
   httpRequestToRequestData,
   logger,
   parseUrl,
   stripUrlQueryAndFragment,
   withIsolationScope,
   withScope,
 } from '@sentry/core';
+import { shouldPropagateTraceForUrl } from '@sentry/opentelemetry';
 import { DEBUG_BUILD } from '../../debug-build';
 import { getRequestUrl } from '../../utils/getRequestUrl';
 import { getRequestInfo } from './vendor/getRequestInfo';
 
 type Http = typeof http;
 type Https = typeof https;
 
+type RequestArgs =
+  // eslint-disable-next-line @typescript-eslint/ban-types
+  | [url: string | URL, options?: RequestOptions, callback?: Function]
+  // eslint-disable-next-line @typescript-eslint/ban-types
+  | [options: RequestOptions, callback?: Function];
+
 type SentryHttpInstrumentationOptions = InstrumentationConfig & {
   /**
    * Whether breadcrumbs should be recorded for requests.
@@ -80,8 +89,11 @@ const MAX_BODY_BYTE_LENGTH = 1024 * 1024;
  * https://github.com/open-telemetry/opentelemetry-js/blob/f8ab5592ddea5cba0a3b33bf8d74f27872c0367f/experimental/packages/opentelemetry-instrumentation-http/src/http.ts
  */
 export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpInstrumentationOptions> {
+  private _propagationDecisionMap: LRUMap<string, boolean>;
+
   public constructor(config: SentryHttpInstrumentationOptions = {}) {
     super('@sentry/instrumentation-http', VERSION, config);
+    this._propagationDecisionMap = new LRUMap<string, boolean>(100);
   }
 
   /** @inheritdoc */
@@ -208,22 +220,21 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns
       return function outgoingRequest(this: unknown, ...args: unknown[]): http.ClientRequest {
         instrumentation._diag.debug('http instrumentation for outgoing requests');
 
-        // Making a copy to avoid mutating the original args array
         // We need to access and reconstruct the request options object passed to `ignoreOutgoingRequests`
         // so that it matches what Otel instrumentation passes to `ignoreOutgoingRequestHook`.
         // @see https://github.com/open-telemetry/opentelemetry-js/blob/7293e69c1e55ca62e15d0724d22605e61bd58952/experimental/packages/opentelemetry-instrumentation-http/src/http.ts#L756-L789
-        const argsCopy = [...args];
-
-        const options = argsCopy.shift() as URL | http.RequestOptions | string;
+        const requestArgs = [...args] as RequestArgs;
+        const options = requestArgs[0];
+        const extraOptions = typeof requestArgs[1] === 'object' ? requestArgs[1] : undefined;
 
-        const extraOptions =
-          typeof argsCopy[0] === 'object' && (typeof options === 'string' || options instanceof URL)
-            ? (argsCopy.shift() as http.RequestOptions)
-            : undefined;
+        const { optionsParsed, origin, pathname } = getRequestInfo(instrumentation._diag, options, extraOptions);
+        const url = getAbsoluteUrl(origin, pathname);
 
-        const { optionsParsed } = getRequestInfo(instrumentation._diag, options, extraOptions);
+        addSentryHeadersToRequestOptions(url, optionsParsed, instrumentation._propagationDecisionMap);
 
-        const request = original.apply(this, args) as ReturnType<typeof http.request>;
+        const request = original.apply(this, [optionsParsed, ...requestArgs.slice(1)]) as ReturnType<
+          typeof http.request
+        >;
 
         request.prependListener('response', (response: http.IncomingMessage) => {
           const _breadcrumbs = instrumentation.getConfig().breadcrumbs;
@@ -457,6 +468,41 @@ function patchRequestToCaptureBody(req: IncomingMessage, isolationScope: Scope):
   }
 }
 
+/**
+ * Mutates the passed in `options` and adds `sentry-trace` / `baggage` headers, if they are not already set.
+ */
+function addSentryHeadersToRequestOptions(
+  url: string,
+  options: RequestOptions,
+  propagationDecisionMap: LRUMap<string, boolean>,
+): void {
+  // Manually add the trace headers, if it applies
+  // Note: We do not use `propagation.inject()` here, because our propagator relies on an active span
+  // Which we do not have in this case
+  const tracePropagationTargets = getClient()?.getOptions().tracePropagationTargets;
+  const addedHeaders = shouldPropagateTraceForUrl(url, tracePropagationTargets, propagationDecisionMap)
+    ? getTraceData()
+    : undefined;
+
+  if (!addedHeaders) {
+    return;
+  }
+
+  if (!options.headers) {
+    options.headers = {};
+  }
+  const headers = options.headers;
+
+  Object.entries(addedHeaders).forEach(([k, v]) => {
+    // We do not want to overwrite existing headers here
+    // If the core UndiciInstrumentation is registered, it will already have set the headers
+    // We do not want to add any then
+    if (!headers[k]) {
+      headers[k] = v;
+    }
+  });
+}
+
 /**
  * Starts a session and tracks it in the context of a given isolation scope.
  * When the passed response is finished, the session is put into a task and is
@@ -531,3 +577,23 @@ const clientToRequestSessionAggregatesMap = new Map<
   Client,
   { [timestampRoundedToSeconds: string]: { exited: number; crashed: number; errored: number } }
 >();
+
+function getAbsoluteUrl(origin: string, path: string = '/'): string {
+  try {
+    const url = new URL(path, origin);
+    return url.toString();
+  } catch {
+    // fallback: Construct it on our own
+    const url = `${origin}`;
+
+    if (url.endsWith('/') && path.startsWith('/')) {
+      return `${url}${path.slice(1)}`;
+    }
+
+    if (!url.endsWith('/') && !path.startsWith('/')) {
+      return `${url}/${path.slice(1)}`;
+    }
+
+    return `${url}${path}`;
+  }
+}
