feat(node): Allow to pass instrumentation config to httpIntegration (#12761)

mydea · web-flow · commit a2dcb2869616 · 2024-07-05T12:41:56.000+02:00
Today, it is not (easily) possible to use custom config for the OTEL HttpInstrumentation. We depend on our own integration for various things, so overwriting this will lead to lots of problems. With this PR, you can now pass some config through directly, in an "allowlisted" way, + there is an escape hatch to add arbitrary other config: ```js Sentry.init({ integrations: [ Sentry.httpIntegration({ instrumentation: { // these three are "vetted" requestHook: (span, req) => span.setAttribute('custom', req.method), responseHook: (span, res) => span.setAttribute('custom', res.method), applyCustomAttributesOnSpan: (span, req, res) => span.setAttribute('custom', req.method), // escape hatch: Can add arbitrary other config that is passed through _experimentalConfig: { serverName: 'xxx' } }) ] }); ``` Closes #12672
diff --git a/dev-packages/node-integration-tests/suites/express/tracing/test.ts b/dev-packages/node-integration-tests/suites/express/tracing/test.ts
@@ -1,6 +1,6 @@
 import { cleanupChildProcesses, createRunner } from '../../../utils/runner';
 
-describe('express tracing experimental', () => {
+describe('express tracing', () => {
   afterAll(() => {
     cleanupChildProcesses();
   });
diff --git a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-experimental.js b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server-experimental.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',
+  // disable attaching headers to /test/* endpoints
+  tracePropagationTargets: [/^(?!.*test).*$/],
+  tracesSampleRate: 1.0,
+  transport: loggingTransport,
+
+  integrations: [
+    Sentry.httpIntegration({
+      instrumentation: {
+        _experimentalConfig: {
+          serverName: 'sentry-test-server-name',
+        },
+      },
+    }),
+  ],
+});
+
+// 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' });
+});
+
+Sentry.setupExpressErrorHandler(app);
+
+startExpressServerAndSendPortToRunner(app);
diff --git a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server.js b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/server.js
@@ -0,0 +1,58 @@
+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',
+  // disable attaching headers to /test/* endpoints
+  tracePropagationTargets: [/^(?!.*test).*$/],
+  tracesSampleRate: 1.0,
+  transport: loggingTransport,
+
+  integrations: [
+    Sentry.httpIntegration({
+      instrumentation: {
+        requestHook: (span, req) => {
+          span.setAttribute('attr1', 'yes');
+          Sentry.setExtra('requestHookCalled', {
+            url: req.url,
+            method: req.method,
+          });
+        },
+        responseHook: (span, res) => {
+          span.setAttribute('attr2', 'yes');
+          Sentry.setExtra('responseHookCalled', {
+            url: res.req.url,
+            method: res.req.method,
+          });
+        },
+        applyCustomAttributesOnSpan: (span, req, res) => {
+          span.setAttribute('attr3', 'yes');
+          Sentry.setExtra('applyCustomAttributesOnSpanCalled', {
+            reqUrl: req.url,
+            reqMethod: req.method,
+            resUrl: res.req.url,
+            resMethod: res.req.method,
+          });
+        },
+      },
+    }),
+  ],
+});
+
+// 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' });
+});
+
+Sentry.setupExpressErrorHandler(app);
+
+startExpressServerAndSendPortToRunner(app);
diff --git a/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts b/dev-packages/node-integration-tests/suites/tracing/httpIntegration/test.ts
@@ -0,0 +1,80 @@
+import { cleanupChildProcesses, createRunner } from '../../../utils/runner';
+
+describe('httpIntegration', () => {
+  afterAll(() => {
+    cleanupChildProcesses();
+  });
+
+  test('allows to pass instrumentation options to integration', done => {
+    // response shape seems different on Node 14, so we skip this there
+    const nodeMajorVersion = Number(process.versions.node.split('.')[0]);
+    if (nodeMajorVersion <= 14) {
+      done();
+      return;
+    }
+
+    createRunner(__dirname, 'server.js')
+      .ignore('session', 'sessions')
+      .expect({
+        transaction: {
+          contexts: {
+            trace: {
+              span_id: expect.any(String),
+              trace_id: expect.any(String),
+              data: {
+                url: expect.stringMatching(/\/test$/),
+                'http.response.status_code': 200,
+                attr1: 'yes',
+                attr2: 'yes',
+                attr3: 'yes',
+              },
+              op: 'http.server',
+              status: 'ok',
+            },
+          },
+          extra: {
+            requestHookCalled: {
+              url: expect.stringMatching(/\/test$/),
+              method: 'GET',
+            },
+            responseHookCalled: {
+              url: expect.stringMatching(/\/test$/),
+              method: 'GET',
+            },
+            applyCustomAttributesOnSpanCalled: {
+              reqUrl: expect.stringMatching(/\/test$/),
+              reqMethod: 'GET',
+              resUrl: expect.stringMatching(/\/test$/),
+              resMethod: 'GET',
+            },
+          },
+        },
+      })
+      .start(done)
+      .makeRequest('get', '/test');
+  });
+
+  test('allows to pass experimental config through to integration', done => {
+    createRunner(__dirname, 'server-experimental.js')
+      .ignore('session', 'sessions')
+      .expect({
+        transaction: {
+          contexts: {
+            trace: {
+              span_id: expect.any(String),
+              trace_id: expect.any(String),
+              data: {
+                url: expect.stringMatching(/\/test$/),
+                'http.response.status_code': 200,
+                'http.server_name': 'sentry-test-server-name',
+              },
+              op: 'http.server',
+              status: 'ok',
+            },
+          },
+        },
+      })
+      .start(done)
+      .makeRequest('get', '/test');
+  });
+});
diff --git a/packages/nextjs/src/server/httpIntegration.ts b/packages/nextjs/src/server/httpIntegration.ts
@@ -22,19 +22,7 @@ class CustomNextjsHttpIntegration extends HttpInstrumentation {
   }
 }
 
-interface HttpOptions {
-  /**
-   * Whether breadcrumbs should be recorded for requests.
-   * Defaults to true
-   */
-  breadcrumbs?: boolean;
-
-  /**
-   * 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.
-   */
-  ignoreOutgoingRequests?: (url: string) => boolean;
-}
+type HttpOptions = Parameters<typeof originalHttpIntegration>[0];
 
 /**
  * The http integration instruments Node's internal http and https modules.
diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts
@@ -43,6 +43,25 @@ interface HttpOptions {
    */
   ignoreIncomingRequests?: (url: string) => boolean;
 
+  /**
+   * Additional instrumentation options that are passed to the underlying HttpInstrumentation.
+   */
+  instrumentation?: {
+    requestHook?: (span: Span, req: ClientRequest | HTTPModuleRequestIncomingMessage) => void;
+    responseHook?: (span: Span, response: HTTPModuleRequestIncomingMessage | ServerResponse) => void;
+    applyCustomAttributesOnSpan?: (
+      span: Span,
+      request: ClientRequest | HTTPModuleRequestIncomingMessage,
+      response: HTTPModuleRequestIncomingMessage | ServerResponse,
+    ) => void;
+
+    /**
+     * You can pass any configuration through to the underlying instrumention.
+     * Note that there are no semver guarantees for this!
+     */
+    _experimentalConfig?: ConstructorParameters<typeof HttpInstrumentation>[0];
+  };
+
   /** Allows to pass a custom version of HttpInstrumentation. We use this for Next.js. */
   _instrumentation?: typeof HttpInstrumentation;
 }
@@ -63,6 +82,7 @@ export const instrumentHttp = Object.assign(
     const _InstrumentationClass = _httpOptions._instrumentation || HttpInstrumentation;
 
     _httpInstrumentation = new _InstrumentationClass({
+      ..._httpOptions.instrumentation?._experimentalConfig,
       ignoreOutgoingRequestHook: request => {
         const url = getRequestUrl(request);
 
@@ -107,6 +127,7 @@ export const instrumentHttp = Object.assign(
         // both, incoming requests and "client" requests made within the app trigger the requestHook
         // we only want to isolate and further annotate incoming requests (IncomingMessage)
         if (_isClientRequest(req)) {
+          _httpOptions.instrumentation?.requestHook?.(span, req);
           return;
         }
 
@@ -134,24 +155,30 @@ export const instrumentHttp = Object.assign(
         const bestEffortTransactionName = `${httpMethod} ${httpTarget}`;
 
         isolationScope.setTransactionName(bestEffortTransactionName);
+
+        _httpOptions.instrumentation?.requestHook?.(span, req);
       },
-      responseHook: () => {
+      responseHook: (span, res) => {
         const client = getClient<NodeClient>();
         if (client && client.getOptions().autoSessionTracking) {
           setImmediate(() => {
             client['_captureRequestSession']();
           });
         }
+
+        _httpOptions.instrumentation?.responseHook?.(span, res);
       },
       applyCustomAttributesOnSpan: (
-        _span: Span,
+        span: Span,
         request: ClientRequest | HTTPModuleRequestIncomingMessage,
         response: HTTPModuleRequestIncomingMessage | ServerResponse,
       ) => {
         const _breadcrumbs = typeof _httpOptions.breadcrumbs === 'undefined' ? true : _httpOptions.breadcrumbs;
         if (_breadcrumbs) {
           _addRequestBreadcrumb(request, response);
         }
+
+        _httpOptions.instrumentation?.applyCustomAttributesOnSpan?.(span, request, response);
       },
     });
 
diff --git a/packages/remix/src/utils/integrations/http.ts b/packages/remix/src/utils/integrations/http.ts
@@ -16,19 +16,7 @@ class RemixHttpIntegration extends HttpInstrumentation {
   }
 }
 
-interface HttpOptions {
-  /**
-   * Whether breadcrumbs should be recorded for requests.
-   * Defaults to true
-   */
-  breadcrumbs?: boolean;
-
-  /**
-   * 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.
-   */
-  ignoreOutgoingRequests?: (url: string) => boolean;
-}
+type HttpOptions = Parameters<typeof originalHttpIntegration>[0];
 
 /**
  * The http integration instruments Node's internal http and https modules.
