fix(node): Ensure ignoreOutgoingRequests of httpIntegration appli…

…es to breadcrumbs (#13970)

Fix a bug which caused the `httpIntegration`'s `ignoreOutgoingRequests` option to
not be applied to breadcrumb creation for outgoing requests. As a result,
despite spans being correctly filtered by the option,
breadcrumbs would still be created which contradicts the function'S
JSDoc and [our
docs](https://docs.sentry.io/platforms/javascript/guides/node/configuration/integrations/http/#ignoreoutgoingrequests).

This patch fixes the bug by:
- correctly passing in `ignoreOutgoingRequests` to
`SentryHttpIntegration` (same signature as `httpIntegration`)
- reconstructing the `url` and `request` parameter like in the
[OpenTelemetry
instrumentation](https://github.com/open-telemetry/opentelemetry-js/blob/7293e69c1e55ca62e15d0724d22605e61bd58952/experimental/packages/opentelemetry-instrumentation-http/src/http.ts#L756-L789)
- adding/adjusting a regression test so that we properly test agains
`ignoreOutgoingRequests` with both params. Now not just for filtered
spans but also for breadcrumbs.

dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-ignoreOutgoingRequests.js

@@ -11,10 +11,11 @@ Sentry.init({
   integrations: [
     Sentry.httpIntegration({
       ignoreOutgoingRequests: (url, request) => {
-        if (url.includes('example.com')) {
+        if (url === 'http://example.com/blockUrl') {
           return true;
         }
-        if (request.method === 'POST' && request.path === '/path') {
+
+        if (request.hostname === 'example.com' && request.path === '/blockRequest') {
           return true;
         }
         return false;
@@ -32,28 +33,37 @@ const app = express();
 
 app.use(cors());
 
-app.get('/test', (_req, response) => {
-  http
-    .request('http://example.com/', res => {
-      res.on('data', () => {});
-      res.on('end', () => {
-        response.send({ response: 'done' });
-      });
-    })
-    .end();
+app.get('/testUrl', (_req, response) => {
+  makeHttpRequest('http://example.com/blockUrl').then(() => {
+    makeHttpRequest('http://example.com/pass').then(() => {
+      response.send({ response: 'done' });
+    });
+  });
 });
 
-app.post('/testPath', (_req, response) => {
-  http
-    .request('http://example.com/path', res => {
-      res.on('data', () => {});
-      res.on('end', () => {
-        response.send({ response: 'done' });
-      });
-    })
-    .end();
+app.get('/testRequest', (_req, response) => {
+  makeHttpRequest('http://example.com/blockRequest').then(() => {
+    makeHttpRequest('http://example.com/pass').then(() => {
+      response.send({ response: 'done' });
+    });
+  });
 });
 
 Sentry.setupExpressErrorHandler(app);
 
 startExpressServerAndSendPortToRunner(app);
+
+function makeHttpRequest(url) {
+  return new Promise((resolve, reject) => {
+    http
+      .get(url, res => {
+        res.on('data', () => {});
+        res.on('end', () => {
+          resolve();
+        });
+      })
+      .on('error', error => {
+        reject(error);
+      });
+  });
+}

dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts

@@ -128,65 +128,45 @@ describe('httpIntegration', () => {
     });
   });
 
-  describe("doesn't create child spans for outgoing requests ignored via `ignoreOutgoingRequests`", () => {
+  describe("doesn't create child spans or breadcrumbs for outgoing requests ignored via `ignoreOutgoingRequests`", () => {
     test('via the url param', done => {
       const runner = createRunner(__dirname, 'server-ignoreOutgoingRequests.js')
         .expect({
-          transaction: {
-            contexts: {
-              trace: {
-                span_id: expect.any(String),
-                trace_id: expect.any(String),
-                data: {
-                  url: expect.stringMatching(/\/test$/),
-                  'http.response.status_code': 200,
-                },
-                op: 'http.server',
-                status: 'ok',
-              },
-            },
-            transaction: 'GET /test',
-            spans: [
-              expect.objectContaining({ op: 'middleware.express', description: 'query' }),
-              expect.objectContaining({ op: 'middleware.express', description: 'expressInit' }),
-              expect.objectContaining({ op: 'middleware.express', description: 'corsMiddleware' }),
-              expect.objectContaining({ op: 'request_handler.express', description: '/test' }),
-            ],
+          transaction: event => {
+            expect(event.transaction).toBe('GET /testUrl');
+
+            const requestSpans = event.spans?.filter(span => span.op === 'http.client');
+            expect(requestSpans).toHaveLength(1);
+            expect(requestSpans![0]?.description).toBe('GET http://example.com/pass');
+
+            const breadcrumbs = event.breadcrumbs?.filter(b => b.category === 'http');
+            expect(breadcrumbs).toHaveLength(1);
+            expect(breadcrumbs![0]?.data?.url).toEqual('http://example.com/pass');
           },
         })
         .start(done);
 
-      runner.makeRequest('get', '/test');
+      runner.makeRequest('get', '/testUrl');
     });
 
     test('via the request param', done => {
       const runner = createRunner(__dirname, 'server-ignoreOutgoingRequests.js')
         .expect({
-          transaction: {
-            contexts: {
-              trace: {
-                span_id: expect.any(String),
-                trace_id: expect.any(String),
-                data: {
-                  url: expect.stringMatching(/\/testPath$/),
-                  'http.response.status_code': 200,
-                },
-                op: 'http.server',
-                status: 'ok',
-              },
-            },
-            transaction: 'POST /testPath',
-            spans: [
-              expect.objectContaining({ op: 'middleware.express', description: 'query' }),
-              expect.objectContaining({ op: 'middleware.express', description: 'expressInit' }),
-              expect.objectContaining({ op: 'middleware.express', description: 'corsMiddleware' }),
-              expect.objectContaining({ op: 'request_handler.express', description: '/testPath' }),
-            ],
+          transaction: event => {
+            expect(event.transaction).toBe('GET /testRequest');
+
+            const requestSpans = event.spans?.filter(span => span.op === 'http.client');
+            expect(requestSpans).toHaveLength(1);
+            expect(requestSpans![0]?.description).toBe('GET http://example.com/pass');
+
+            const breadcrumbs = event.breadcrumbs?.filter(b => b.category === 'http');
+            expect(breadcrumbs).toHaveLength(1);
+            expect(breadcrumbs![0]?.data?.url).toEqual('http://example.com/pass');
           },
         })
         .start(done);
 
-      runner.makeRequest('post', '/testPath');
+      runner.makeRequest('get', '/testRequest');
     });
   });
 });

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

@@ -1,8 +1,10 @@
 import type * as http from 'node:http';
+import type { RequestOptions } from 'node:http';
 import type * as https from 'node:https';
 import { VERSION } from '@opentelemetry/core';
 import type { InstrumentationConfig } from '@opentelemetry/instrumentation';
 import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation';
+import { getRequestInfo } from '@opentelemetry/instrumentation-http';
 import { addBreadcrumb, getClient, getIsolationScope, withIsolationScope } from '@sentry/core';
 import type { SanitizedRequestData } from '@sentry/types';
 import {
@@ -12,12 +14,29 @@ import {
   stripUrlQueryAndFragment,
 } from '@sentry/utils';
 import type { NodeClient } from '../../sdk/client';
+import { getRequestUrl } from '../../utils/getRequestUrl';
 
 type Http = typeof http;
 type Https = typeof https;
 
 type SentryHttpInstrumentationOptions = InstrumentationConfig & {
+  /**
+   * Whether breadcrumbs should be recorded for requests.
+   *
+   * @default `true`
+   */
   breadcrumbs?: 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.
+   * The same option can be passed to the top-level httpIntegration where it controls both, breadcrumb and
+   * span creation.
+   *
+   * @param url Contains the entire URL, including query string (if any), protocol, host, etc. of the outgoing request.
+   * @param request Contains the {@type RequestOptions} object used to make the outgoing request.
+   */
+  ignoreOutgoingRequests?: (url: string, request: RequestOptions) => boolean;
 };
 
 /**
@@ -140,20 +159,42 @@ export class SentryHttpInstrumentation extends InstrumentationBase<SentryHttpIns
   private _getPatchOutgoingRequestFunction(): (
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     original: (...args: any[]) => http.ClientRequest,
-  ) => (...args: unknown[]) => http.ClientRequest {
+  ) => (options: URL | http.RequestOptions | string, ...args: unknown[]) => http.ClientRequest {
     // eslint-disable-next-line @typescript-eslint/no-this-alias
     const instrumentation = this;
 
     return (original: (...args: unknown[]) => http.ClientRequest): ((...args: unknown[]) => http.ClientRequest) => {
       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 extraOptions =
+          typeof argsCopy[0] === 'object' && (typeof options === 'string' || options instanceof URL)
+            ? (argsCopy.shift() as http.RequestOptions)
+            : undefined;
+
+        const { optionsParsed } = getRequestInfo(options, extraOptions);
+
         const request = original.apply(this, args) as ReturnType<typeof http.request>;
 
         request.prependListener('response', (response: http.IncomingMessage) => {
-          const breadcrumbs = instrumentation.getConfig().breadcrumbs;
-          const _breadcrumbs = typeof breadcrumbs === 'undefined' ? true : breadcrumbs;
-          if (_breadcrumbs) {
+          const _breadcrumbs = instrumentation.getConfig().breadcrumbs;
+          const breadCrumbsEnabled = typeof _breadcrumbs === 'undefined' ? true : _breadcrumbs;
+
+          const _ignoreOutgoingRequests = instrumentation.getConfig().ignoreOutgoingRequests;
+          const shouldCreateBreadcrumb =
+            typeof _ignoreOutgoingRequests === 'function'
+              ? !_ignoreOutgoingRequests(getRequestUrl(request), optionsParsed)
+              : true;
+
+          if (breadCrumbsEnabled && shouldCreateBreadcrumb) {
             addRequestBreadcrumb(request, response);
           }
         });

packages/node/src/integrations/http/index.ts

@@ -20,7 +20,7 @@ const INSTRUMENTATION_NAME = '@opentelemetry_sentry-patched/instrumentation-http
 
 interface HttpOptions {
   /**
-   * Whether breadcrumbs should be recorded for requests.
+   * Whether breadcrumbs should be recorded for outgoing requests.
    * Defaults to true
    */
   breadcrumbs?: boolean;
@@ -45,8 +45,8 @@ interface HttpOptions {
   ignoreOutgoingRequests?: (url: string, request: RequestOptions) => boolean;
 
   /**
-   * Do not capture spans or breadcrumbs for incoming HTTP requests to URLs where the given callback returns `true`.
-   * This controls both span & breadcrumb creation - spans will be non recording if tracing is disabled.
+   * Do not capture spans for incoming HTTP requests to URLs where the given callback returns `true`.
+   * Spans will be non recording if tracing is disabled.
    *
    * The `urlPath` param consists of the URL path and query string (if any) of the incoming request.
    * For example: `'/users/details?id=123'`
@@ -82,12 +82,15 @@ interface HttpOptions {
   };
 }
 
-export const instrumentSentryHttp = generateInstrumentOnce<{ breadcrumbs?: boolean }>(
-  `${INTEGRATION_NAME}.sentry`,
-  options => {
-    return new SentryHttpInstrumentation({ breadcrumbs: options?.breadcrumbs });
-  },
-);
+export const instrumentSentryHttp = generateInstrumentOnce<{
+  breadcrumbs?: HttpOptions['breadcrumbs'];
+  ignoreOutgoingRequests?: HttpOptions['ignoreOutgoingRequests'];
+}>(`${INTEGRATION_NAME}.sentry`, options => {
+  return new SentryHttpInstrumentation({
+    breadcrumbs: options?.breadcrumbs,
+    ignoreOutgoingRequests: options?.ignoreOutgoingRequests,
+  });
+});
 
 export const instrumentOtelHttp = generateInstrumentOnce<HttpInstrumentationConfig>(INTEGRATION_NAME, config => {
   const instrumentation = new HttpInstrumentation(config);