ref(remix): Isolate Express instrumentation from server auto-instrumentation. (#9966)

Author: onurtemizkan

packages/remix/src/utils/instrumentServer.ts

@@ -28,6 +28,7 @@ import {
 } from './vendor/response';
 import type {
   AppData,
+  AppLoadContext,
   CreateRequestHandlerFunction,
   DataFunction,
   DataFunctionArgs,
@@ -46,9 +47,6 @@ import { normalizeRemixRequest } from './web-fetch';
 let FUTURE_FLAGS: FutureConfig | undefined;
 let IS_REMIX_V2: boolean | undefined;
 
-// Flag to track if the core request handler is instrumented.
-export let isRequestHandlerWrapped = false;
-
 const redirectStatusCodes = new Set([301, 302, 303, 307, 308]);
 function isRedirectResponse(response: Response): boolean {
   return redirectStatusCodes.has(response.status);
@@ -222,8 +220,13 @@ function makeWrappedDataFunction(
   id: string,
   name: 'action' | 'loader',
   remixVersion: number,
+  manuallyInstrumented: boolean,
 ): DataFunction {
   return async function (this: unknown, args: DataFunctionArgs): Promise<Response | AppData> {
+    if (args.context.__sentry_express_wrapped__ && !manuallyInstrumented) {
+      return origFn.call(this, args);
+    }
+
     let res: Response | AppData;
     const activeTransaction = getActiveTransaction();
     const currentScope = getCurrentScope();
@@ -265,15 +268,15 @@ function makeWrappedDataFunction(
 }
 
 const makeWrappedAction =
-  (id: string, remixVersion: number) =>
+  (id: string, remixVersion: number, manuallyInstrumented: boolean) =>
   (origAction: DataFunction): DataFunction => {
-    return makeWrappedDataFunction(origAction, id, 'action', remixVersion);
+    return makeWrappedDataFunction(origAction, id, 'action', remixVersion, manuallyInstrumented);
   };
 
 const makeWrappedLoader =
-  (id: string, remixVersion: number) =>
+  (id: string, remixVersion: number, manuallyInstrumented: boolean) =>
   (origLoader: DataFunction): DataFunction => {
-    return makeWrappedDataFunction(origLoader, id, 'loader', remixVersion);
+    return makeWrappedDataFunction(origLoader, id, 'loader', remixVersion, manuallyInstrumented);
   };
 
 function getTraceAndBaggage(): {
@@ -419,7 +422,13 @@ function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBui
   const routes = createRoutes(build.routes);
   const pkg = loadModule<ReactRouterDomPkg>('react-router-dom');
 
-  return async function (this: unknown, request: RemixRequest, loadContext?: unknown): Promise<Response> {
+  return async function (this: unknown, request: RemixRequest, loadContext?: AppLoadContext): Promise<Response> {
+    // This means that the request handler of the adapter (ex: express) is already wrapped.
+    // So we don't want to double wrap it.
+    if (loadContext?.__sentry_express_wrapped__) {
+      return origRequestHandler.call(this, request, loadContext);
+    }
+
     return runWithAsyncContext(async () => {
       const hub = getCurrentHub();
       const options = getClient()?.getOptions();
@@ -473,7 +482,7 @@ function wrapRequestHandler(origRequestHandler: RequestHandler, build: ServerBui
 /**
  * Instruments `remix` ServerBuild for performance tracing and error tracking.
  */
-export function instrumentBuild(build: ServerBuild): ServerBuild {
+export function instrumentBuild(build: ServerBuild, manuallyInstrumented: boolean = false): ServerBuild {
   const routes: ServerRouteManifest = {};
 
   const remixVersion = getRemixVersionFromBuild(build);
@@ -495,12 +504,12 @@ export function instrumentBuild(build: ServerBuild): ServerBuild {
 
     const routeAction = wrappedRoute.module.action as undefined | WrappedFunction;
     if (routeAction && !routeAction.__sentry_original__) {
-      fill(wrappedRoute.module, 'action', makeWrappedAction(id, remixVersion));
+      fill(wrappedRoute.module, 'action', makeWrappedAction(id, remixVersion, manuallyInstrumented));
     }
 
     const routeLoader = wrappedRoute.module.loader as undefined | WrappedFunction;
     if (routeLoader && !routeLoader.__sentry_original__) {
-      fill(wrappedRoute.module, 'loader', makeWrappedLoader(id, remixVersion));
+      fill(wrappedRoute.module, 'loader', makeWrappedLoader(id, remixVersion, manuallyInstrumented));
     }
 
     // Entry module should have a loader function to provide `sentry-trace` and `baggage`
@@ -523,13 +532,9 @@ export function instrumentBuild(build: ServerBuild): ServerBuild {
 function makeWrappedCreateRequestHandler(
   origCreateRequestHandler: CreateRequestHandlerFunction,
 ): CreateRequestHandlerFunction {
-  // To track if this wrapper has been applied, before other wrappers.
-  // Can't track `__sentry_original__` because it's not the same function as the potentially manually wrapped one.
-  isRequestHandlerWrapped = true;
-
   return function (this: unknown, build: ServerBuild, ...args: unknown[]): RequestHandler {
     FUTURE_FLAGS = getFutureFlagsServer(build);
-    const newBuild = instrumentBuild(build);
+    const newBuild = instrumentBuild(build, false);
     const requestHandler = origCreateRequestHandler.call(this, newBuild, ...args);
 
     return wrapRequestHandler(requestHandler, newBuild);

packages/remix/src/utils/serverAdapters/express.ts

@@ -1,24 +1,20 @@
-import { getClient, getCurrentHub, getCurrentScope, hasTracingEnabled } from '@sentry/core';
+import { getClient, getCurrentHub, getCurrentScope, hasTracingEnabled, runWithAsyncContext } from '@sentry/core';
 import { flush } from '@sentry/node';
 import type { Transaction } from '@sentry/types';
-import { extractRequestData, isString, logger } from '@sentry/utils';
+import { extractRequestData, fill, isString, logger } from '@sentry/utils';
 import { cwd } from 'process';
 
 import { DEBUG_BUILD } from '../debug-build';
-import {
-  createRoutes,
-  getTransactionName,
-  instrumentBuild,
-  isRequestHandlerWrapped,
-  startRequestHandlerTransaction,
-} from '../instrumentServer';
+import { createRoutes, getTransactionName, instrumentBuild, startRequestHandlerTransaction } from '../instrumentServer';
 import type {
+  AppLoadContext,
   ExpressCreateRequestHandler,
   ExpressCreateRequestHandlerOptions,
   ExpressNextFunction,
   ExpressRequest,
   ExpressRequestHandler,
   ExpressResponse,
+  GetLoadContextFunction,
   ReactRouterDomPkg,
   ServerBuild,
 } from '../vendor/types';
@@ -31,11 +27,6 @@ function wrapExpressRequestHandler(
 ): ExpressRequestHandler {
   const routes = createRoutes(build.routes);
 
-  // If the core request handler is already wrapped, don't wrap Express handler which uses it.
-  if (isRequestHandlerWrapped) {
-    return origRequestHandler;
-  }
-
   return async function (
     this: unknown,
     req: ExpressRequest,
@@ -54,33 +45,46 @@ function wrapExpressRequestHandler(
       }
     }
 
-    // eslint-disable-next-line @typescript-eslint/unbound-method
-    res.end = wrapEndMethod(res.end);
+    await runWithAsyncContext(async () => {
+      // eslint-disable-next-line @typescript-eslint/unbound-method
+      res.end = wrapEndMethod(res.end);
 
-    const request = extractRequestData(req);
-    const hub = getCurrentHub();
-    const options = getClient()?.getOptions();
-    const scope = getCurrentScope();
+      const request = extractRequestData(req);
+      const hub = getCurrentHub();
+      const options = getClient()?.getOptions();
+      const scope = getCurrentScope();
 
-    scope.setSDKProcessingMetadata({ request });
+      scope.setSDKProcessingMetadata({ request });
 
-    if (!options || !hasTracingEnabled(options) || !request.url || !request.method) {
-      return origRequestHandler.call(this, req, res, next);
-    }
+      if (!options || !hasTracingEnabled(options) || !request.url || !request.method) {
+        return origRequestHandler.call(this, req, res, next);
+      }
 
-    const url = new URL(request.url);
-    const [name, source] = getTransactionName(routes, url, pkg);
-    const transaction = startRequestHandlerTransaction(hub, name, source, {
-      headers: {
-        'sentry-trace': (req.headers && isString(req.headers['sentry-trace']) && req.headers['sentry-trace']) || '',
-        baggage: (req.headers && isString(req.headers.baggage) && req.headers.baggage) || '',
-      },
-      method: request.method,
+      const url = new URL(request.url);
+
+      const [name, source] = getTransactionName(routes, url, pkg);
+      const transaction = startRequestHandlerTransaction(hub, name, source, {
+        headers: {
+          'sentry-trace': (req.headers && isString(req.headers['sentry-trace']) && req.headers['sentry-trace']) || '',
+          baggage: (req.headers && isString(req.headers.baggage) && req.headers.baggage) || '',
+        },
+        method: request.method,
+      });
+      // save a link to the transaction on the response, so that even if there's an error (landing us outside of
+      // the domain), we can still finish it (albeit possibly missing some scope data)
+      (res as AugmentedExpressResponse).__sentryTransaction = transaction;
+      return origRequestHandler.call(this, req, res, next);
     });
-    // save a link to the transaction on the response, so that even if there's an error (landing us outside of
-    // the domain), we can still finish it (albeit possibly missing some scope data)
-    (res as AugmentedExpressResponse).__sentryTransaction = transaction;
-    return origRequestHandler.call(this, req, res, next);
+  };
+}
+
+function wrapGetLoadContext(origGetLoadContext: () => AppLoadContext): GetLoadContextFunction {
+  return function (this: unknown, req: ExpressRequest, res: ExpressResponse): AppLoadContext {
+    const loadContext = (origGetLoadContext.call(this, req, res) || {}) as AppLoadContext;
+
+    loadContext['__sentry_express_wrapped__'] = true;
+
+    return loadContext;
   };
 }
 
@@ -92,9 +96,18 @@ export function wrapExpressCreateRequestHandler(
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
 ): (options: any) => ExpressRequestHandler {
   // eslint-disable-next-line @typescript-eslint/no-explicit-any
-  return function (this: unknown, options: any): ExpressRequestHandler {
-    const newBuild = instrumentBuild((options as ExpressCreateRequestHandlerOptions).build);
-    const requestHandler = origCreateRequestHandler.call(this, { ...options, build: newBuild });
+  return function (this: unknown, options: ExpressCreateRequestHandlerOptions): ExpressRequestHandler {
+    if (!('getLoadContext' in options)) {
+      options['getLoadContext'] = () => ({});
+    }
+
+    fill(options, 'getLoadContext', wrapGetLoadContext);
+
+    const newBuild = instrumentBuild(options.build, true);
+    const requestHandler = origCreateRequestHandler.call(this, {
+      ...options,
+      build: newBuild,
+    });
 
     return wrapExpressRequestHandler(requestHandler, newBuild);
   };

packages/remix/src/utils/vendor/types.ts

@@ -64,7 +64,7 @@ export type RemixRequest = Request &
     agent: Agent | ((parsedURL: URL) => Agent) | undefined;
   };
 
-export type AppLoadContext = any;
+export type AppLoadContext = Record<string, unknown> & { __sentry_express_wrapped__?: boolean };
 export type AppData = any;
 export type RequestHandler = (request: RemixRequest, loadContext?: AppLoadContext) => Promise<Response>;
 export type CreateRequestHandlerFunction = (this: unknown, build: ServerBuild, ...args: any[]) => RequestHandler;
@@ -246,4 +246,4 @@ export interface ExpressCreateRequestHandlerOptions {
   mode?: string;
 }
 
-type GetLoadContextFunction = (req: any, res: any) => any;
+export type GetLoadContextFunction = (req: any, res: any) => AppLoadContext;

packages/remix/test/integration/test/server/loader.test.ts

@@ -170,7 +170,9 @@ describe.each(['builtin', 'express'])('Remix API Loaders with adapter = %s', ada
       const val = key[key.length - 1];
       expect(tags[key]).toEqual(val);
     });
-  });
+    // express tests tend to take slightly longer on node >= 20
+    // TODO: check why this is happening
+  }, 10000);
 
   it('continues transaction from sentry-trace header and baggage', async () => {
     const env = await RemixTestEnv.init(adapter);

packages/utils/src/requestdata.ts

@@ -200,7 +200,10 @@ export function extractRequestData(
   //   express: req.hostname in > 4 and req.host in < 4
   //   koa: req.host
   //   node, nextjs: req.headers.host
-  const host = req.hostname || req.host || headers.host || '<no host>';
+  // Express 4 mistakenly strips off port number from req.host / req.hostname so we can't rely on them
+  // See: https://github.com/expressjs/express/issues/3047#issuecomment-236653223
+  // Also: https://github.com/getsentry/sentry-javascript/issues/1917
+  const host = headers.host || req.hostname || req.host || '<no host>';
   // protocol:
   //   node, nextjs: <n/a>
   //   express, koa: req.protocol