feat(node): Update scope transactionName in http and express instrumentations (#11434)

Concrete changes:
1. `addRequestDataToEvent` no longer applies its transaction name to
_non-transaction_ events
2. Http instrumentation sets the isolation scope's `transactionName` in
the Otel `requestHook` to the unparameterized request URL path (w/o
query params or fragments).
3. Express instrumentation sets the isolation scope's `transactionName`
in the Otel `spanName` hook to a parameterized route, once a request
handler span (e.g. `app.get(...)`) is created.
4. As a consequence of the above, non-transaction events now get
assigned `event.transaction` correctly from the scopes.

Author: Lms24

dev-packages/node-integration-tests/suites/express/tracing/withError/server.js

@@ -0,0 +1,29 @@
+const { loggingTransport } = require('@sentry-internal/node-integration-tests');
+const Sentry = require('@sentry/node');
+
+Sentry.init({
+  dsn: 'https://[email protected]/1337',
+  release: '1.0',
+  // disable attaching headers to /test/* endpoints
+  tracePropagationTargets: [/^(?!.*test).*$/],
+  tracesSampleRate: 1.0,
+  transport: loggingTransport,
+});
+
+// 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/:id1/:id2', (_req, res) => {
+  Sentry.captureMessage(new Error('error_1'));
+  res.send('Success');
+});
+
+Sentry.setupExpressErrorHandler(app);
+
+startExpressServerAndSendPortToRunner(app);

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

@@ -0,0 +1,28 @@
+import { cleanupChildProcesses, createRunner } from '../../../../utils/runner';
+
+describe('express tracing experimental', () => {
+  afterAll(() => {
+    cleanupChildProcesses();
+  });
+
+  describe('CJS', () => {
+    test('should apply the scope transactionName to error events', done => {
+      createRunner(__dirname, 'server.js')
+        .ignore('session', 'sessions', 'transaction')
+        .expect({
+          event: {
+            exception: {
+              values: [
+                {
+                  value: 'error_1',
+                },
+              ],
+            },
+            transaction: 'GET /test/:id1/:id2',
+          },
+        })
+        .start(done)
+        .makeRequest('get', '/test/123/abc?q=1');
+    });
+  });
+});

packages/node/src/integrations/http.ts

@@ -4,10 +4,11 @@ import { SpanKind } from '@opentelemetry/api';
 import { registerInstrumentations } from '@opentelemetry/instrumentation';
 import { HttpInstrumentation } from '@opentelemetry/instrumentation-http';
 
-import { addBreadcrumb, defineIntegration, getIsolationScope, isSentryRequestUrl } from '@sentry/core';
+import { addBreadcrumb, defineIntegration, getIsolationScope, isSentryRequestUrl, spanToJSON } from '@sentry/core';
 import { _INTERNAL, getClient, getSpanKind } from '@sentry/opentelemetry';
 import type { IntegrationFn } from '@sentry/types';
 
+import { stripUrlQueryAndFragment } from '@sentry/utils';
 import type { NodeClient } from '../sdk/client';
 import { setIsolationScope } from '../sdk/scope';
 import type { HTTPModuleRequestIncomingMessage } from '../transports/http-module';
@@ -81,19 +82,35 @@ const _httpIntegration = ((options: HttpOptions = {}) => {
           requireParentforOutgoingSpans: true,
           requireParentforIncomingSpans: false,
           requestHook: (span, req) => {
-            _updateSpan(span);
+            addOriginToSpan(span, 'auto.http.otel.http');
+
+            if (getSpanKind(span) !== SpanKind.SERVER) {
+              return;
+            }
 
             // Update the isolation scope, isolate this request
-            if (getSpanKind(span) === SpanKind.SERVER) {
-              const isolationScope = getIsolationScope().clone();
-              isolationScope.setSDKProcessingMetadata({ request: req });
-
-              const client = getClient<NodeClient>();
-              if (client && client.getOptions().autoSessionTracking) {
-                isolationScope.setRequestSession({ status: 'ok' });
-              }
-              setIsolationScope(isolationScope);
+            const isolationScope = getIsolationScope().clone();
+            isolationScope.setSDKProcessingMetadata({ request: req });
+
+            const client = getClient<NodeClient>();
+            if (client && client.getOptions().autoSessionTracking) {
+              isolationScope.setRequestSession({ status: 'ok' });
             }
+            setIsolationScope(isolationScope);
+
+            // attempt to update the scope's `transactionName` based on the request URL
+            // Ideally, framework instrumentations coming after the HttpInstrumentation
+            // update the transactionName once we get a parameterized route.
+            const attributes = spanToJSON(span).data;
+            if (!attributes) {
+              return;
+            }
+
+            const httpMethod = String(attributes['http.method']).toUpperCase() || 'GET';
+            const httpTarget = stripUrlQueryAndFragment(String(attributes['http.target'])) || '/';
+            const bestEffortTransactionName = `${httpMethod} ${httpTarget}`;
+
+            isolationScope.setTransactionName(bestEffortTransactionName);
           },
           responseHook: (span, res) => {
             if (_breadcrumbs) {
@@ -123,11 +140,6 @@ const _httpIntegration = ((options: HttpOptions = {}) => {
  */
 export const httpIntegration = defineIntegration(_httpIntegration);
 
-/** Update the span with data we need. */
-function _updateSpan(span: Span): void {
-  addOriginToSpan(span, 'auto.http.otel.http');
-}
-
 /** Add a breadcrumb for outgoing requests. */
 function _addRequestBreadcrumb(span: Span, response: HTTPModuleRequestIncomingMessage | ServerResponse): void {
   if (getSpanKind(span) !== SpanKind.CLIENT) {

packages/node/src/integrations/tracing/express.ts

@@ -18,6 +18,15 @@ const _expressIntegration = (() => {
             requestHook(span) {
               addOriginToSpan(span, 'auto.http.otel.express');
             },
+            spanNameHook(info, defaultName) {
+              if (info.layerType === 'request_handler') {
+                // type cast b/c Otel unfortunately types info.request as any :(
+                const req = info.request as { method?: string };
+                const method = req.method ? req.method.toUpperCase() : 'GET';
+                getIsolationScope().setTransactionName(`${method} ${info.route}`);
+              }
+              return defaultName;
+            },
           }),
         ],
       });

packages/utils/src/requestdata.ts

@@ -298,7 +298,7 @@ export function addRequestDataToEvent(
     }
   }
 
-  if (include.transaction && !event.transaction) {
+  if (include.transaction && !event.transaction && event.type === 'transaction') {
     // TODO do we even need this anymore?
     // TODO make this work for nextjs
     event.transaction = extractTransaction(req, include.transaction);

packages/utils/test/requestdata.test.ts

@@ -149,52 +149,63 @@ describe('addRequestDataToEvent', () => {
   });
 
   describe('transaction property', () => {
-    test('extracts method and full route path by default`', () => {
-      const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq);
+    describe('for transaction events', () => {
+      beforeEach(() => {
+        mockEvent.type = 'transaction';
+      });
 
-      expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/:parameterName');
-    });
+      test('extracts method and full route path by default`', () => {
+        const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq);
 
-    test('extracts method and full path by default when mountpoint is `/`', () => {
-      mockReq.originalUrl = mockReq.originalUrl.replace('/routerMountpath', '');
-      mockReq.baseUrl = '';
+        expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/:parameterName');
+      });
 
-      const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq);
+      test('extracts method and full path by default when mountpoint is `/`', () => {
+        mockReq.originalUrl = mockReq.originalUrl.replace('/routerMountpath', '');
+        mockReq.baseUrl = '';
 
-      // `subpath/` is the full path here, because there's no router mount path
-      expect(parsedRequest.transaction).toEqual('POST /subpath/:parameterName');
-    });
+        const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq);
 
-    test('fallback to method and `originalUrl` if route is missing', () => {
-      delete mockReq.route;
+        // `subpath/` is the full path here, because there's no router mount path
+        expect(parsedRequest.transaction).toEqual('POST /subpath/:parameterName');
+      });
 
-      const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq);
+      test('fallback to method and `originalUrl` if route is missing', () => {
+        delete mockReq.route;
 
-      expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/specificValue');
-    });
+        const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq);
 
-    test('can extract path only instead if configured', () => {
-      const optionsWithPathTransaction = {
-        include: {
-          transaction: 'path',
-        },
-      } as const;
+        expect(parsedRequest.transaction).toEqual('POST /routerMountPath/subpath/specificValue');
+      });
 
-      const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithPathTransaction);
+      test('can extract path only instead if configured', () => {
+        const optionsWithPathTransaction = {
+          include: {
+            transaction: 'path',
+          },
+        } as const;
 
-      expect(parsedRequest.transaction).toEqual('/routerMountPath/subpath/:parameterName');
-    });
+        const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithPathTransaction);
 
-    test('can extract handler name instead if configured', () => {
-      const optionsWithHandlerTransaction = {
-        include: {
-          transaction: 'handler',
-        },
-      } as const;
+        expect(parsedRequest.transaction).toEqual('/routerMountPath/subpath/:parameterName');
+      });
 
-      const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithHandlerTransaction);
+      test('can extract handler name instead if configured', () => {
+        const optionsWithHandlerTransaction = {
+          include: {
+            transaction: 'handler',
+          },
+        } as const;
+
+        const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq, optionsWithHandlerTransaction);
+
+        expect(parsedRequest.transaction).toEqual('parameterNameRouteHandler');
+      });
+    });
+    it('transaction is not applied to non-transaction events', () => {
+      const parsedRequest: Event = addRequestDataToEvent(mockEvent, mockReq);
 
-      expect(parsedRequest.transaction).toEqual('parameterNameRouteHandler');
+      expect(parsedRequest.transaction).toBeUndefined();
     });
   });
 });