fix(node): Ensure httpIntegration propagates traces (#15233)

mydea · Lms24 · web-flow · commit 3e31bdc405d2 · 2025-01-31T11:06:31.000+01:00
Related to #15231, I noticed that we today would not propagate traces in outgoing http requests if: 1. The user configures `httpIntegration({ spans: false })` 2. ..._or_ the user has a custom OTEL setup 3. _and_ the user does not add their own `HttpInstrumentation` Admittedly and edge case. More importantly, though, by actually adding distributed tracing information here, we are unblocked from potentially stopping to ship the http-instrumentation to users that do not need spans (and/or have a custom otel setup). --------- Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
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/core/src/utils-hoist/baggage.ts b/packages/core/src/utils-hoist/baggage.ts
@@ -130,7 +130,7 @@ function baggageHeaderToObject(baggageHeader: string): Record<string, string> {
  * @returns a baggage header string, or `undefined` if the object didn't have any values, since an empty baggage header
  * is not spec compliant.
  */
-function objectToBaggageHeader(object: Record<string, string>): string | undefined {
+export function objectToBaggageHeader(object: Record<string, string>): string | undefined {
   if (Object.keys(object).length === 0) {
     // An empty baggage header is not spec compliant: We return undefined.
     return undefined;
diff --git a/packages/core/src/utils-hoist/index.ts b/packages/core/src/utils-hoist/index.ts
@@ -128,6 +128,7 @@ export {
   baggageHeaderToDynamicSamplingContext,
   dynamicSamplingContextToSentryBaggageHeader,
   parseBaggageHeader,
+  objectToBaggageHeader,
 } from './baggage';
 
 export { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from './url';
diff --git a/packages/node/src/integrations/http/SentryHttpInstrumentation.ts b/packages/node/src/integrations/http/SentryHttpInstrumentation.ts
@@ -1,33 +1,44 @@
-/* eslint-disable max-lines */
 import type * as http from 'node:http';
 import type { IncomingMessage, RequestOptions } from 'node:http';
 import type * as https from 'node:https';
 import type { EventEmitter } from 'node:stream';
+/* eslint-disable max-lines */
 import { VERSION } from '@opentelemetry/core';
 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,
+  objectToBaggageHeader,
+  parseBaggageHeader,
   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 +91,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 +222,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 +470,44 @@ 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;
+
+  const { 'sentry-trace': sentryTrace, baggage } = addedHeaders;
+
+  // We do not want to overwrite existing header here, if it was already set
+  if (sentryTrace && !headers['sentry-trace']) {
+    headers['sentry-trace'] = sentryTrace;
+  }
+
+  // For baggage, we make sure to merge this into a possibly existing header
+  if (baggage) {
+    headers['baggage'] = mergeBaggageHeaders(headers['baggage'], baggage);
+  }
+}
+
 /**
  * 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 +582,49 @@ 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}`;
+  }
+}
+
+function mergeBaggageHeaders(
+  existing: string | string[] | number | undefined,
+  baggage: string,
+): string | string[] | number | undefined {
+  if (!existing) {
+    return baggage;
+  }
+
+  const existingBaggageEntries = parseBaggageHeader(existing);
+  const newBaggageEntries = parseBaggageHeader(baggage);
+
+  if (!newBaggageEntries) {
+    return existing;
+  }
+
+  // Existing entries take precedence, ensuring order remains stable for minimal changes
+  const mergedBaggageEntries = { ...existingBaggageEntries };
+  Object.entries(newBaggageEntries).forEach(([key, value]) => {
+    if (!mergedBaggageEntries[key]) {
+      mergedBaggageEntries[key] = value;
+    }
+  });
+
+  return objectToBaggageHeader(mergedBaggageEntries);
+}
