fixup! feat(core, node): portable Express integration

Author: isaacs

packages/core/src/integrations/express/index.ts

@@ -30,8 +30,6 @@
 
 import { debug } from '../../utils/debug-logger';
 import { captureException } from '../../exports';
-import { getIsolationScope } from '../../currentScopes';
-import { httpRequestToRequestData } from '../../utils/request';
 import { DEBUG_BUILD } from '../../debug-build';
 import type {
   ExpressApplication,
@@ -58,6 +56,7 @@ import {
 } from './utils';
 import { wrapMethod } from '../../utils/object';
 import { patchLayer } from './patch-layer';
+import { setSDKProcessingMetadata } from './set-sdk-processing-metadata';
 
 const getExpressExport = (express: ExpressModuleExport): ExpressExport =>
   hasDefaultProp(express) ? express.default : (express as ExpressExport);
@@ -166,11 +165,8 @@ export function expressErrorHandler(options?: ExpressHandlerOptions): ExpressErr
     res: ExpressResponse,
     next: (error: MiddlewareError) => void,
   ): void {
-    const normalizedRequest = httpRequestToRequestData(request);
-    // Ensure we use the express-enhanced request here, instead of the plain HTTP one
     // When an error happens, the `expressRequestHandler` middleware does not run, so we set it here too
-    getIsolationScope().setSDKProcessingMetadata({ normalizedRequest });
-
+    setSDKProcessingMetadata(request);
     const shouldHandleError = options?.shouldHandleError || defaultShouldHandleError;
 
     if (shouldHandleError(error)) {
@@ -223,10 +219,7 @@ export function setupExpressErrorHandler(
 
 function expressRequestHandler(): ExpressMiddleware {
   return function sentryRequestMiddleware(request: ExpressRequest, _res: ExpressResponse, next: () => void): void {
-    const normalizedRequest = httpRequestToRequestData(request);
-    // Ensure we use the express-enhanced request here, instead of the plain HTTP one
-    getIsolationScope().setSDKProcessingMetadata({ normalizedRequest });
-
+    setSDKProcessingMetadata(request);
     next();
   };
 }

packages/core/src/integrations/express/patch-layer.ts

@@ -26,12 +26,16 @@ import {
 import { getIsolationScope } from '../../currentScopes';
 import { getDefaultIsolationScope } from '../../defaultScopes';
 import { getOriginalFunction, markFunctionWrapped } from '../../utils/object';
+import { setSDKProcessingMetadata } from './set-sdk-processing-metadata';
 
 export type ExpressPatchLayerOptions = Pick<
   ExpressIntegrationOptions,
   'onRouteResolved' | 'ignoreLayers' | 'ignoreLayersType'
 >;
 
+// exported for testing
+export const kDidSetSDKMetadata = Symbol('sentry.DidSetSDKMetadata');
+
 export function patchLayer(options: ExpressPatchLayerOptions, maybeLayer?: ExpressLayer, layerPath?: string): void {
   if (!maybeLayer?.handle) {
     return;
@@ -52,11 +56,18 @@ export function patchLayer(options: ExpressPatchLayerOptions, maybeLayer?: Expre
 
   function layerHandlePatched(
     this: ExpressLayer,
-    req: ExpressRequest,
+    req: ExpressRequest & { [kDidSetSDKMetadata]?: boolean },
     res: ExpressResponse,
     //oxlint-disable-next-line no-explicit-any
     ...otherArgs: any[]
   ) {
+    // Set normalizedRequest here because expressRequestHandler middleware
+    // (registered via setupExpressErrorHandler) is added after routes and
+    // therefore never runs for successful requests — route handlers typically
+    // send a response without calling next(). It would be safe to set this
+    // multiple times, since the data is identical, but more performant not to.
+    setSDKProcessingMetadata(req);
+
     // Only create spans when there's an active parent span
     // Without a parent span, this request is being ignored, so skip it
     const parentSpan = getActiveSpan();

packages/core/src/integrations/express/set-sdk-processing-metadata.ts

@@ -0,0 +1,18 @@
+/**
+ * Abstract this out because we call it in multiple places, and it's cheaper to
+ * only do one time for any given request.
+ */
+
+import type { ExpressRequest } from './types';
+import { getIsolationScope } from '../../currentScopes';
+import { httpRequestToRequestData } from '../../utils/request';
+
+const didSetProcessingMetadata = new WeakMap<ExpressRequest, boolean>();
+
+export function setSDKProcessingMetadata(request: ExpressRequest) {
+  if (!didSetProcessingMetadata.get(request)) {
+    const normalizedRequest = httpRequestToRequestData(request);
+    getIsolationScope().setSDKProcessingMetadata({ normalizedRequest });
+    didSetProcessingMetadata.set(request, true);
+  }
+}

packages/core/test/lib/integrations/express/patch-layer.test.ts

@@ -10,6 +10,7 @@ import { type StartSpanOptions } from '../../../../src/types-hoist/startSpanOpti
 import { type Span } from '../../../../src/types-hoist/span';
 import { EventEmitter } from 'node:events';
 import { getOriginalFunction, markFunctionWrapped } from '../../../../src';
+import { setSDKProcessingMetadata } from '../../../../src/integrations/express/set-sdk-processing-metadata';
 
 let DEBUG_BUILD = false;
 beforeEach(() => (DEBUG_BUILD = true));
@@ -36,8 +37,11 @@ const notDefaultIsolationScope = {
   setTransactionName(name: string) {
     transactionNames.push(name);
   },
+  setSDKProcessingMetadata() {},
+};
+const defaultIsolationScope = {
+  setSDKProcessingMetadata() {},
 };
-const defaultIsolationScope = {};
 vi.mock('../../../../src/currentScopes', () => ({
   getIsolationScope() {
     return inDefaultIsolationScope ? defaultIsolationScope : notDefaultIsolationScope;

packages/core/test/lib/integrations/express/set-sdk-processing-metadata.test.ts

@@ -0,0 +1,43 @@
+import { vi, beforeEach, describe, it, expect } from 'vitest';
+import { setSDKProcessingMetadata } from '../../../../src/integrations/express/set-sdk-processing-metadata';
+
+const sdkProcessingMetadatas: unknown[] = [];
+beforeEach(() => (sdkProcessingMetadatas.length = 0));
+const isolationScope = {
+  setSDKProcessingMetadata(data: unknown) {
+    sdkProcessingMetadatas.push(data);
+  },
+};
+vi.mock('../../../../src/currentScopes', () => ({
+  getIsolationScope() {
+    return isolationScope;
+  },
+}));
+
+describe('setSDKProcessingMetadata', () => {
+  it('sets the normalized request data', () => {
+    const request = {
+      originalUrl: '/a/b/c',
+      route: '/a/:boo/:car',
+      method: 'POST',
+      headers: {
+        'Content-Type': 'application/x-www-form-urlencoded',
+      },
+    };
+    setSDKProcessingMetadata(request);
+    // call it again to cover no-op branch
+    setSDKProcessingMetadata(request);
+    expect(JSON.stringify(sdkProcessingMetadatas)).toBe(
+      JSON.stringify([
+        {
+          normalizedRequest: {
+            method: 'POST',
+            headers: {
+              'Content-Type': 'application/x-www-form-urlencoded',
+            },
+          },
+        },
+      ]),
+    );
+  });
+});