fix(node): Ensure fetch/http breadcrumbs are created correctly (#12137)

This PR ensures that (node) fetch and http requests generate breadcrumbs
correctly, even if tracing is disabled.

It also adds tests for this, and ensures we pass the request/response to
the hint correctly.

Fixes #12132

dev-packages/node-integration-tests/suites/tracing/requests/fetch-breadcrumbs/scenario.ts

@@ -0,0 +1,33 @@
+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: [],
+  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;
+  },
+});
+
+async function run(): Promise<void> {
+  Sentry.addBreadcrumb({ message: 'manual breadcrumb' });
+
+  // Since fetch is lazy loaded, we need to wait a bit until it's fully instrumented
+  await new Promise(resolve => setTimeout(resolve, 100));
+  await fetch(`${process.env.SERVER_URL}/api/v0`).then(res => res.text());
+  await fetch(`${process.env.SERVER_URL}/api/v1`).then(res => res.text());
+  await fetch(`${process.env.SERVER_URL}/api/v2`).then(res => res.text());
+  await fetch(`${process.env.SERVER_URL}/api/v3`).then(res => res.text());
+
+  Sentry.captureException(new Error('foo'));
+}
+
+// eslint-disable-next-line @typescript-eslint/no-floating-promises
+run();

dev-packages/node-integration-tests/suites/tracing/requests/fetch-breadcrumbs/test.ts

@@ -0,0 +1,79 @@
+import { conditionalTest } from '../../../../utils';
+import { createRunner } from '../../../../utils/runner';
+import { createTestServer } from '../../../../utils/server';
+
+conditionalTest({ min: 18 })('outgoing fetch', () => {
+  test('outgoing fetch requests create breadcrumbs', done => {
+    createTestServer(done)
+      .start()
+      .then(SERVER_URL => {
+        createRunner(__dirname, 'scenario.ts')
+          .withEnv({ SERVER_URL })
+          .ensureNoErrorOutput()
+          .ignore('session', 'sessions')
+          .expect({
+            event: {
+              breadcrumbs: [
+                {
+                  message: 'manual breadcrumb',
+                  timestamp: expect.any(Number),
+                },
+                {
+                  category: 'http',
+                  data: {
+                    'http.method': 'GET',
+                    url: `${SERVER_URL}/api/v0`,
+                    status_code: 404,
+                    ADDED_PATH: '/api/v0',
+                  },
+                  timestamp: expect.any(Number),
+                  type: 'http',
+                },
+                {
+                  category: 'http',
+                  data: {
+                    'http.method': 'GET',
+                    url: `${SERVER_URL}/api/v1`,
+                    status_code: 404,
+                    ADDED_PATH: '/api/v1',
+                  },
+                  timestamp: expect.any(Number),
+                  type: 'http',
+                },
+                {
+                  category: 'http',
+                  data: {
+                    'http.method': 'GET',
+                    url: `${SERVER_URL}/api/v2`,
+                    status_code: 404,
+                    ADDED_PATH: '/api/v2',
+                  },
+                  timestamp: expect.any(Number),
+                  type: 'http',
+                },
+                {
+                  category: 'http',
+                  data: {
+                    'http.method': 'GET',
+                    url: `${SERVER_URL}/api/v3`,
+                    status_code: 404,
+                    ADDED_PATH: '/api/v3',
+                  },
+                  timestamp: expect.any(Number),
+                  type: 'http',
+                },
+              ],
+              exception: {
+                values: [
+                  {
+                    type: 'Error',
+                    value: 'foo',
+                  },
+                ],
+              },
+            },
+          })
+          .start(done);
+      });
+  });
+});

dev-packages/node-integration-tests/suites/tracing/requests/http-breadcrumbs/scenario.ts

@@ -0,0 +1,48 @@
+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: [],
+  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 makeHttpRequest(`${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();
+  });
+}

dev-packages/node-integration-tests/suites/tracing/requests/http-breadcrumbs/test.ts

@@ -0,0 +1,76 @@
+import { createRunner } from '../../../../utils/runner';
+import { createTestServer } from '../../../../utils/server';
+
+test('outgoing http requests create breadcrumbs', done => {
+  createTestServer(done)
+    .start()
+    .then(SERVER_URL => {
+      createRunner(__dirname, 'scenario.ts')
+        .withEnv({ SERVER_URL })
+        .ensureNoErrorOutput()
+        .ignore('session', 'sessions')
+        .expect({
+          event: {
+            breadcrumbs: [
+              {
+                message: 'manual breadcrumb',
+                timestamp: expect.any(Number),
+              },
+              {
+                category: 'http',
+                data: {
+                  'http.method': 'GET',
+                  url: `${SERVER_URL}/api/v0`,
+                  status_code: 404,
+                  ADDED_PATH: '/api/v0',
+                },
+                timestamp: expect.any(Number),
+                type: 'http',
+              },
+              {
+                category: 'http',
+                data: {
+                  'http.method': 'GET',
+                  url: `${SERVER_URL}/api/v1`,
+                  status_code: 404,
+                  ADDED_PATH: '/api/v1',
+                },
+                timestamp: expect.any(Number),
+                type: 'http',
+              },
+              {
+                category: 'http',
+                data: {
+                  'http.method': 'GET',
+                  url: `${SERVER_URL}/api/v2`,
+                  status_code: 404,
+                  ADDED_PATH: '/api/v2',
+                },
+                timestamp: expect.any(Number),
+                type: 'http',
+              },
+              {
+                category: 'http',
+                data: {
+                  'http.method': 'GET',
+                  url: `${SERVER_URL}/api/v3`,
+                  status_code: 404,
+                  ADDED_PATH: '/api/v3',
+                },
+                timestamp: expect.any(Number),
+                type: 'http',
+              },
+            ],
+            exception: {
+              values: [
+                {
+                  type: 'Error',
+                  value: 'foo',
+                },
+              ],
+            },
+          },
+        })
+        .start(done);
+    });
+});

packages/node/src/integrations/http.ts

@@ -1,6 +1,5 @@
-import type { ClientRequest, IncomingMessage, ServerResponse } from 'node:http';
+import type { ClientRequest, ServerResponse } from 'node:http';
 import type { Span } from '@opentelemetry/api';
-import { SpanKind } from '@opentelemetry/api';
 import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
 import { addOpenTelemetryInstrumentation } from '@sentry/opentelemetry';
 
@@ -13,10 +12,10 @@ import {
   isSentryRequestUrl,
   setCapturedScopesOnSpan,
 } from '@sentry/core';
-import { getClient, getRequestSpanData, getSpanKind } from '@sentry/opentelemetry';
-import type { IntegrationFn } from '@sentry/types';
+import { getClient } from '@sentry/opentelemetry';
+import type { IntegrationFn, SanitizedRequestData } from '@sentry/types';
 
-import { stripUrlQueryAndFragment } from '@sentry/utils';
+import { getSanitizedUrlString, parseUrl, stripUrlQueryAndFragment } from '@sentry/utils';
 import type { NodeClient } from '../sdk/client';
 import { setIsolationScope } from '../sdk/scope';
 import type { HTTPModuleRequestIncomingMessage } from '../transports/http-module';
@@ -127,18 +126,23 @@ const _httpIntegration = ((options: HttpOptions = {}) => {
 
             isolationScope.setTransactionName(bestEffortTransactionName);
           },
-          responseHook: (span, res) => {
-            if (_breadcrumbs) {
-              _addRequestBreadcrumb(span, res);
-            }
-
+          responseHook: () => {
             const client = getClient<NodeClient>();
             if (client && client.getOptions().autoSessionTracking) {
               setImmediate(() => {
                 client['_captureRequestSession']();
               });
             }
           },
+          applyCustomAttributesOnSpan: (
+            _span: Span,
+            request: ClientRequest | HTTPModuleRequestIncomingMessage,
+            response: HTTPModuleRequestIncomingMessage | ServerResponse,
+          ) => {
+            if (_breadcrumbs) {
+              _addRequestBreadcrumb(request, response);
+            }
+          },
         }),
       );
     },
@@ -152,12 +156,16 @@ const _httpIntegration = ((options: HttpOptions = {}) => {
 export const httpIntegration = defineIntegration(_httpIntegration);
 
 /** Add a breadcrumb for outgoing requests. */
-function _addRequestBreadcrumb(span: Span, response: HTTPModuleRequestIncomingMessage | ServerResponse): void {
-  if (getSpanKind(span) !== SpanKind.CLIENT) {
+function _addRequestBreadcrumb(
+  request: ClientRequest | HTTPModuleRequestIncomingMessage,
+  response: HTTPModuleRequestIncomingMessage | ServerResponse,
+): void {
+  // Only generate breadcrumbs for outgoing requests
+  if (!_isClientRequest(request)) {
     return;
   }
 
-  const data = getRequestSpanData(span);
+  const data = getBreadcrumbData(request);
   addBreadcrumb(
     {
       category: 'http',
@@ -169,19 +177,42 @@ function _addRequestBreadcrumb(span: Span, response: HTTPModuleRequestIncomingMe
     },
     {
       event: 'response',
-      // TODO FN: Do we need access to `request` here?
-      // If we do, we'll have to use the `applyCustomAttributesOnSpan` hook instead,
-      // but this has worse context semantics than request/responseHook.
+      request,
       response,
     },
   );
 }
 
+function getBreadcrumbData(request: ClientRequest): Partial<SanitizedRequestData> {
+  try {
+    // `request.host` does not contain the port, but the host header does
+    const host = request.getHeader('host') || request.host;
+    const url = new URL(request.path, `${request.protocol}//${host}`);
+    const parsedUrl = parseUrl(url.toString());
+
+    const data: Partial<SanitizedRequestData> = {
+      url: getSanitizedUrlString(parsedUrl),
+      'http.method': request.method || 'GET',
+    };
+
+    if (parsedUrl.search) {
+      data['http.query'] = parsedUrl.search;
+    }
+    if (parsedUrl.hash) {
+      data['http.fragment'] = parsedUrl.hash;
+    }
+
+    return data;
+  } catch {
+    return {};
+  }
+}
+
 /**
  * Determines if @param req is a ClientRequest, meaning the request was created within the express app
  * and it's an outgoing request.
  * Checking for properties instead of using `instanceOf` to avoid importing the request classes.
  */
-function _isClientRequest(req: ClientRequest | IncomingMessage): req is ClientRequest {
+function _isClientRequest(req: ClientRequest | HTTPModuleRequestIncomingMessage): req is ClientRequest {
   return 'outputData' in req && 'outputSize' in req && !('client' in req) && !('statusCode' in req);
 }