diff --git a/packages/remix/src/utils/instrumentServer.ts b/packages/remix/src/utils/instrumentServer.ts index 90f72296d2a1..62d820a30eeb 100644 --- a/packages/remix/src/utils/instrumentServer.ts +++ b/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 { + 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('react-router-dom'); - return async function (this: unknown, request: RemixRequest, loadContext?: unknown): Promise { + return async function (this: unknown, request: RemixRequest, loadContext?: AppLoadContext): Promise { + // 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); diff --git a/packages/remix/src/utils/serverAdapters/express.ts b/packages/remix/src/utils/serverAdapters/express.ts index ea765d35a353..fd0af832c2f5 100644 --- a/packages/remix/src/utils/serverAdapters/express.ts +++ b/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); }; diff --git a/packages/remix/src/utils/vendor/types.ts b/packages/remix/src/utils/vendor/types.ts index de1a20fcbff1..c3041dd4805e 100644 --- a/packages/remix/src/utils/vendor/types.ts +++ b/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 & { __sentry_express_wrapped__?: boolean }; export type AppData = any; export type RequestHandler = (request: RemixRequest, loadContext?: AppLoadContext) => Promise; 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; diff --git a/packages/remix/test/integration/test/server/loader.test.ts b/packages/remix/test/integration/test/server/loader.test.ts index 24d67422c3ca..a8a0d859e199 100644 --- a/packages/remix/test/integration/test/server/loader.test.ts +++ b/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); diff --git a/packages/utils/src/requestdata.ts b/packages/utils/src/requestdata.ts index cfb9473f2595..5860932994d4 100644 --- a/packages/utils/src/requestdata.ts +++ b/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 || ''; + // 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 || ''; // protocol: // node, nextjs: // express, koa: req.protocol