fixup: review fixes

Author: isaacs

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

@@ -112,7 +112,9 @@ export const patchExpressModule = (options: ExpressIntegrationOptions) => {
       function useTrace(this: ExpressApplication, ...args: Parameters<typeof originalRouterUse>) {
         const route = originalRouterUse.apply(this, args);
         const layer = this.stack[this.stack.length - 1];
-        if (!layer) return route;
+        if (!layer) {
+          return route;
+        }
         patchLayer(options, layer, getLayerPath(args));
         return route;
       },

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

@@ -1,18 +1,18 @@
+import { DEBUG_BUILD } from '../../debug-build';
 import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '../../semanticAttributes';
 import { SPAN_STATUS_ERROR, startSpanManual, withActiveSpan } from '../../tracing';
+import { debug } from '../../utils/debug-logger';
 import type { SpanAttributes } from '../../types-hoist/span';
 import { getActiveSpan } from '../../utils/spanUtils';
 import { getStoredLayers, storeLayer } from './request-layer-store';
 import {
   type ExpressRequest,
   type ExpressResponse,
-  kLayerPatched,
   type ExpressIntegrationOptions,
   type ExpressLayer,
   ATTR_HTTP_ROUTE,
   ATTR_EXPRESS_TYPE,
   ATTR_EXPRESS_NAME,
-  type ExpressLayerType,
   ExpressLayerType_MIDDLEWARE,
   ExpressLayerType_ROUTER,
 } from './types';
@@ -21,27 +21,31 @@ import {
   getActualMatchedRoute,
   getConstructedRoute,
   getLayerMetadata,
-  getSpanName,
   isLayerIgnored,
 } from './utils';
+import { getIsolationScope } from '../../currentScopes';
+import { getDefaultIsolationScope } from '../../defaultScopes';
+import { getOriginalFunction, markFunctionWrapped } from '../../utils/object';
 
 export type ExpressPatchLayerOptions = Pick<
   ExpressIntegrationOptions,
   'onRouteResolved' | 'ignoreLayers' | 'ignoreLayersType'
 >;
 
 export function patchLayer(options: ExpressPatchLayerOptions, maybeLayer?: ExpressLayer, layerPath?: string): void {
-  if (!maybeLayer) {
+  if (!maybeLayer?.handle) {
     return;
   }
   const layer = maybeLayer;
 
+  const layerHandleOriginal = layer.handle;
+
   // avoid patching multiple times the same layer
-  if (layer[kLayerPatched] === true) return;
-  layer[kLayerPatched] = true;
+  if (getOriginalFunction(layerHandleOriginal)) {
+    return;
+  }
 
-  const originalHandle = layer.handle;
-  if (originalHandle.length === 4) {
+  if (layerHandleOriginal.length === 4) {
     // todo: instrument error handlers
     return;
   }
@@ -57,10 +61,12 @@ export function patchLayer(options: ExpressPatchLayerOptions, maybeLayer?: Expre
     // Without a parent span, this request is being ignored, so skip it
     const parentSpan = getActiveSpan();
     if (!parentSpan) {
-      return originalHandle.apply(this, [req, res, ...otherArgs]);
+      return layerHandleOriginal.apply(this, [req, res, ...otherArgs]);
     }
 
-    if (layerPath) storeLayer(req, layerPath);
+    if (layerPath) {
+      storeLayer(req, layerPath);
+    }
     const storedLayers = getStoredLayers(req);
     const isLayerPathStored = !!layerPath;
 
@@ -70,8 +76,8 @@ export function patchLayer(options: ExpressPatchLayerOptions, maybeLayer?: Expre
     options.onRouteResolved?.(actualMatchedRoute);
 
     const metadata = getLayerMetadata(constructedRoute, layer, layerPath);
-    const name = metadata.attributes[ATTR_EXPRESS_NAME] as string ?? metadata.name;
-    const type = metadata.attributes[ATTR_EXPRESS_TYPE] as ExpressLayerType;
+    const name = metadata.attributes[ATTR_EXPRESS_NAME];
+    const type = metadata.attributes[ATTR_EXPRESS_TYPE];
     const attributes: SpanAttributes = Object.assign(metadata.attributes, {
       [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.express',
       [SEMANTIC_ATTRIBUTE_SENTRY_OP]: `${type}.express`,
@@ -91,18 +97,21 @@ export function patchLayer(options: ExpressPatchLayerOptions, maybeLayer?: Expre
       if (isLayerPathStored && type === ExpressLayerType_MIDDLEWARE) {
         storedLayers.pop();
       }
-      return originalHandle.apply(this, [req, res, ...otherArgs]);
+      return layerHandleOriginal.apply(this, [req, res, ...otherArgs]);
     }
 
-    const spanName = getSpanName(
-      {
-        request: req,
-        layerType: type,
-        route: constructedRoute,
-      },
-      name,
-    );
-    return startSpanManual({ name: spanName, attributes }, span => {
+    const currentScope = getIsolationScope();
+    if (currentScope !== getDefaultIsolationScope()) {
+      if (type === 'request_handler') {
+        // type cast b/c Otel unfortunately types info.request as any :(
+        const method = req.method ? req.method.toUpperCase() : 'GET';
+        currentScope.setTransactionName(`${method} ${constructedRoute}`);
+      }
+    } else {
+      DEBUG_BUILD && debug.warn('Isolation scope is still default isolation scope - skipping setting transactionName');
+    }
+
+    return startSpanManual({ name, attributes }, span => {
       let spanHasEnded = false;
       // TODO: Fix router spans (getRouterPath does not work properly) to
       // have useful names before removing this branch
@@ -121,7 +130,9 @@ export function patchLayer(options: ExpressPatchLayerOptions, maybeLayer?: Expre
       // verify we have a callback
       for (let i = 0; i < otherArgs.length; i++) {
         const callback = otherArgs[i] as Function;
-        if (typeof callback !== 'function') continue;
+        if (typeof callback !== 'function') {
+          continue;
+        }
 
         //oxlint-disable-next-line no-explicit-any
         otherArgs[i] = function (...args: any[]) {
@@ -155,7 +166,7 @@ export function patchLayer(options: ExpressPatchLayerOptions, maybeLayer?: Expre
       }
 
       try {
-        return originalHandle.apply(this, [req, res, ...otherArgs]);
+        return layerHandleOriginal.apply(this, [req, res, ...otherArgs]);
       } catch (anyError) {
         const [_, message] = asErrorAndMessage(anyError);
         // intentionally do not record the exception here, because
@@ -186,21 +197,23 @@ export function patchLayer(options: ExpressPatchLayerOptions, maybeLayer?: Expre
   // ref: https://github.com/open-telemetry/opentelemetry-js-contrib/issues/1950
   // ref: https://github.com/open-telemetry/opentelemetry-js-contrib/issues/2271
   // oxlint-disable-next-line guard-for-in
-  for (const key in originalHandle as Function & Record<string, unknown>) {
+  for (const key in layerHandleOriginal as Function & Record<string, unknown>) {
     // skip standard function prototype fields that both have
     if (key in layerHandlePatched) {
       continue;
     }
     Object.defineProperty(layerHandlePatched, key, {
       get() {
-        return originalHandle[key];
+        return layerHandleOriginal[key];
       },
       set(value) {
-        originalHandle[key] = value;
+        layerHandleOriginal[key] = value;
       },
     });
   }
 
+  markFunctionWrapped(layerHandlePatched, layerHandleOriginal);
+
   Object.defineProperty(layer, 'handle', {
     enumerable: true,
     configurable: true,

packages/core/src/integrations/express/request-layer-store.ts

@@ -4,16 +4,18 @@ import type { ExpressRequest } from './types';
 const requestLayerStore = new WeakMap<ExpressRequest, string[]>();
 export const storeLayer = (req: ExpressRequest, layer: string) => {
   const store = requestLayerStore.get(req);
-  if (!store) requestLayerStore.set(req, [layer]);
-  else store.push(layer);
+  if (!store) {
+    requestLayerStore.set(req, [layer]);
+  } else {
+    store.push(layer);
+  }
 };
 
 export const getStoredLayers = (req: ExpressRequest) => {
-  const store = requestLayerStore.get(req);
-  if (store) return store;
-  else {
-    const store: string[] = [];
+  let store = requestLayerStore.get(req);
+  if (!store) {
+    store = [];
     requestLayerStore.set(req, store);
-    return store;
   }
+  return store;
 };

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

@@ -1,7 +1,6 @@
 import type { RequestEventData } from '../../types-hoist/request';
 import type { SpanAttributes } from '../../types-hoist/span';
 
-export const kLayerPatched: unique symbol = Symbol('express-layer-patched');
 export const ATTR_EXPRESS_NAME = 'express.name';
 export const ATTR_HTTP_ROUTE = 'http.route';
 export const ATTR_EXPRESS_TYPE = 'express.type';

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

@@ -1,7 +1,3 @@
-import { getIsolationScope } from '../../currentScopes';
-import { DEBUG_BUILD } from '../../debug-build';
-import { getDefaultIsolationScope } from '../../defaultScopes';
-import { debug } from '../../utils/debug-logger';
 import type { SpanAttributes } from '../../types-hoist/span';
 import { getStoredLayers } from './request-layer-store';
 import type {
@@ -10,7 +6,6 @@ import type {
   ExpressLayer,
   ExpressLayerType,
   ExpressRequest,
-  ExpressRequestInfo,
   IgnoreMatcher,
   LayerPathSegment,
   MiddlewareError,
@@ -27,17 +22,6 @@ import {
 } from './types';
 import { isMatchingPattern } from '../../utils/string';
 
-/**
- * Check whether the given obj match pattern
- * @param constant e.g URL of request
- * @param obj obj to inspect
- * @param pattern Match pattern
- */
-export const satisfiesPattern = (constant: string, pattern: IgnoreMatcher): boolean => {
-  // XXX replace calls to satisfiesPattern with this:
-  return isMatchingPattern(constant, pattern, true);
-};
-
 /**
  * Converts a user-provided error value into an error and error message pair
  *
@@ -66,7 +50,7 @@ export const getLayerMetadata = (
   layer: ExpressLayer,
   layerPath?: string,
 ): {
-  attributes: SpanAttributes;
+  attributes: SpanAttributes & { [ATTR_EXPRESS_NAME]: string; [ATTR_EXPRESS_TYPE]: ExpressLayerType };
   name: string;
 } => {
   if (layer.name === 'router') {
@@ -136,10 +120,12 @@ export const isLayerIgnored = (
   if (Array.isArray(config?.ignoreLayersType) && config?.ignoreLayersType?.includes(type)) {
     return true;
   }
-  if (!Array.isArray(config?.ignoreLayers)) return false;
+  if (!Array.isArray(config?.ignoreLayers)) {
+    return false;
+  }
   try {
     for (const pattern of config.ignoreLayers) {
-      if (satisfiesPattern(name, pattern)) {
+      if (isMatchingPattern(name, pattern, true)) {
         return true;
       }
     }
@@ -251,20 +237,6 @@ export const hasDefaultProp = (
   default: ExpressExport;
 } => !!express && typeof express === 'object' && 'default' in express && typeof express.default === 'function';
 
-export function getSpanName(info: ExpressRequestInfo<unknown>, defaultName: string): string {
-  if (getIsolationScope() === getDefaultIsolationScope()) {
-    DEBUG_BUILD && debug.warn('Isolation scope is still default isolation scope - skipping setting transactionName');
-    return 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;
-}
-
 function getStatusCodeFromResponse(error: MiddlewareError): number {
   const statusCode = error.status || error.statusCode || error.status_code || error.output?.statusCode;
   return statusCode ? parseInt(statusCode as string, 10) : 500;