fix(node): Ensure correct URL is passed to ignoreIncomingRequests callback (#12929)

Fix an oversight in our Node `httpIntegration`. It looks like
we assumed that the `request` object being passed to
`ignoreIncomingRequestHook` and `ignoreOutgoingRequestHook` was of the
same type. However, it's not:
- `request` is of type `IncomingMessage` in `ignoreIncomingRequestHook`
- `request` is of type `RequestOptions` in `ignoreOutgoingRequestHook` 

fix the bug by simply taking the request.url property instead and adds integration tests to properly test the two options.

Author: Lms24

dev-packages/node-integration-tests/src/index.ts

@@ -20,13 +20,17 @@ export function loggingTransport(_options: BaseTransportOptions): Transport {
 
 /**
  * Starts an express server and sends the port to the runner
+ * @param app Express app
+ * @param port Port to start the app on. USE WITH CAUTION! By default a random port will be chosen.
+ * Setting this port to something specific is useful for local debugging but dangerous for
+ * CI/CD environments where port collisions can cause flakes!
  */
-export function startExpressServerAndSendPortToRunner(app: Express): void {
-  const server = app.listen(0, () => {
+export function startExpressServerAndSendPortToRunner(app: Express, port: number | undefined = undefined): void {
+  const server = app.listen(port || 0, () => {
     const address = server.address() as AddressInfo;
 
     // eslint-disable-next-line no-console
-    console.log(`{"port":${address.port}}`);
+    console.log(`{"port":${port || address.port}}`);
   });
 }
 

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

@@ -0,0 +1,38 @@
+const { loggingTransport } = require('@sentry-internal/node-integration-tests');
+const Sentry = require('@sentry/node');
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  tracesSampleRate: 1.0,
+  transport: loggingTransport,
+
+  integrations: [
+    Sentry.httpIntegration({
+      ignoreIncomingRequests: url => {
+        return url.includes('/liveness');
+      },
+    }),
+  ],
+});
+
+// express must be required after Sentry is initialized
+const express = require('express');
+const cors = require('cors');
+const { startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests');
+
+const app = express();
+
+app.use(cors());
+
+app.get('/test', (_req, res) => {
+  res.send({ response: 'response 1' });
+});
+
+app.get('/liveness', (_req, res) => {
+  res.send({ response: 'liveness' });
+});
+
+Sentry.setupExpressErrorHandler(app);
+
+startExpressServerAndSendPortToRunner(app);

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

@@ -0,0 +1,42 @@
+const { loggingTransport } = require('@sentry-internal/node-integration-tests');
+const Sentry = require('@sentry/node');
+const http = require('http');
+
+Sentry.init({
+  dsn: 'https://public@dsn.ingest.sentry.io/1337',
+  release: '1.0',
+  tracesSampleRate: 1.0,
+  transport: loggingTransport,
+
+  integrations: [
+    Sentry.httpIntegration({
+      ignoreOutgoingRequests: url => {
+        return url.includes('example.com');
+      },
+    }),
+  ],
+});
+
+// express must be required after Sentry is initialized
+const express = require('express');
+const cors = require('cors');
+const { startExpressServerAndSendPortToRunner } = require('@sentry-internal/node-integration-tests');
+
+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();
+});
+
+Sentry.setupExpressErrorHandler(app);
+
+startExpressServerAndSendPortToRunner(app);

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

@@ -75,4 +75,59 @@ describe('httpIntegration', () => {
       .start(done)
       .makeRequest('get', '/test');
   });
+
+  test("doesn't create a root span for incoming requests ignored via `ignoreIncomingRequests`", done => {
+    const runner = createRunner(__dirname, 'server-ignoreIncomingRequests.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',
+        },
+      })
+      .start(done);
+
+    runner.makeRequest('get', '/liveness'); // should be ignored
+    runner.makeRequest('get', '/test');
+  });
+
+  test("doesn't create child spans for outgoing requests ignored via `ignoreOutgoingRequests`", 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' }),
+          ],
+        },
+      })
+      .start(done);
+
+    runner.makeRequest('get', '/test');
+  });
 });

packages/node/src/integrations/http.ts

@@ -34,14 +34,20 @@ interface HttpOptions {
   /**
    * Do not capture spans or breadcrumbs for outgoing 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.
+   *
+   * The `url` param contains the entire URL, including query string (if any), protocol, host, etc. of the outgoing request.
+   * For example: `'https://someService.com/users/details?id=123'`
    */
   ignoreOutgoingRequests?: (url: string) => 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.
+   *
+   * The `urlPath` param consists of the URL path and query string (if any) of the incoming request.
+   * For example: `'/users/details?id=123'`
    */
-  ignoreIncomingRequests?: (url: string) => boolean;
+  ignoreIncomingRequests?: (urlPath: string) => boolean;
 
   /**
    * Additional instrumentation options that are passed to the underlying HttpInstrumentation.
@@ -103,7 +109,9 @@ export const instrumentHttp = Object.assign(
       },
 
       ignoreIncomingRequestHook: request => {
-        const url = getRequestUrl(request);
+        // request.url is the only property that holds any information about the url
+        // it only consists of the URL path and query string (if any)
+        const urlPath = request.url;
 
         const method = request.method?.toUpperCase();
         // We do not capture OPTIONS/HEAD requests as transactions
@@ -112,7 +120,7 @@ export const instrumentHttp = Object.assign(
         }
 
         const _ignoreIncomingRequests = _httpOptions.ignoreIncomingRequests;
-        if (_ignoreIncomingRequests && _ignoreIncomingRequests(url)) {
+        if (urlPath && _ignoreIncomingRequests && _ignoreIncomingRequests(urlPath)) {
           return true;
         }