From d76f57e48cc7a38375337a6d86df1666efea6155 Mon Sep 17 00:00:00 2001 From: Charly Gomez Date: Fri, 23 May 2025 13:51:12 +0200 Subject: [PATCH 1/4] replace proxy with wrap --- .../src/server/instrumentation/reactRouter.ts | 110 +++++++++--------- 1 file changed, 53 insertions(+), 57 deletions(-) diff --git a/packages/react-router/src/server/instrumentation/reactRouter.ts b/packages/react-router/src/server/instrumentation/reactRouter.ts index 5bfc0b62e352..ed4984cd7431 100644 --- a/packages/react-router/src/server/instrumentation/reactRouter.ts +++ b/packages/react-router/src/server/instrumentation/reactRouter.ts @@ -1,5 +1,5 @@ import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; -import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation'; +import { InstrumentationBase, InstrumentationNodeModuleDefinition, isWrapped } from '@opentelemetry/instrumentation'; import { getActiveSpan, getRootSpan, @@ -37,11 +37,14 @@ export class ReactRouterInstrumentation extends InstrumentationBase { - return this._createPatchedModuleProxy(moduleExports); + if (isWrapped(moduleExports['createRequestHandler'])) { + this._unwrap(moduleExports, 'createRequestHandler'); + } + this._wrap(moduleExports, 'createRequestHandler', this._patchCreateRequestHandler()); + return moduleExports; }, - (_moduleExports: unknown) => { - // nothing to unwrap here - return _moduleExports; + (moduleExports: ReactRouterModuleExports) => { + this._unwrap(moduleExports, 'createRequestHandler'); }, ); @@ -49,63 +52,56 @@ export class ReactRouterInstrumentation extends InstrumentationBase any { + return function sentryWrappedCreateRequestHandler(this: unknown, ...args: unknown[]) { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore not sure why original isn't found here? + const originalRequestHandler = (original as typeof reactRouter.createRequestHandler).apply(this, args); + return async function sentryWrappedRequestHandler(request: Request, initialContext?: unknown) { + let url: URL; + try { + url = new URL(request.url); + } catch (error) { + return originalRequestHandler(request, initialContext); + } - // We currently just want to trace loaders and actions - if (!isDataRequest(url.pathname)) { - return originalRequestHandler(request, initialContext); - } + // We currently just want to trace loaders and actions + if (!isDataRequest(url.pathname)) { + return originalRequestHandler(request, initialContext); + } - const activeSpan = getActiveSpan(); - const rootSpan = activeSpan && getRootSpan(activeSpan); + const activeSpan = getActiveSpan(); + const rootSpan = activeSpan && getRootSpan(activeSpan); - if (!rootSpan) { - DEBUG_BUILD && logger.debug('No active root span found, skipping tracing for data request'); - return originalRequestHandler(request, initialContext); - } + if (!rootSpan) { + DEBUG_BUILD && logger.debug('No active root span found, skipping tracing for data request'); + return originalRequestHandler(request, initialContext); + } - // Set the source and overwrite attributes on the root span to ensure the transaction name - // is derived from the raw URL pathname rather than any parameterized route that may be set later - // TODO: try to set derived parameterized route from build here (args[0]) - rootSpan.setAttributes({ - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - [SEMANTIC_ATTRIBUTE_SENTRY_OVERWRITE]: `${request.method} ${url.pathname}`, - }); + // Set the source and overwrite attributes on the root span to ensure the transaction name + // is derived from the raw URL pathname rather than any parameterized route that may be set later + // TODO: try to set derived parameterized route from build here (args[0]) + rootSpan.setAttributes({ + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + [SEMANTIC_ATTRIBUTE_SENTRY_OVERWRITE]: `${request.method} ${url.pathname}`, + }); - return startSpan( - { - name: getSpanName(url.pathname, request.method), - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: getOpName(url.pathname, request.method), - }, - }, - () => { - return originalRequestHandler(request, initialContext); - }, - ); - }; - }; - } - return Reflect.get(target, prop, receiver); - }, - }); + return startSpan( + { + name: getSpanName(url.pathname, request.method), + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: getOpName(url.pathname, request.method), + }, + }, + () => { + return originalRequestHandler(request, initialContext); + }, + ); + }; + }; } } From fff626d360cca90c04613d21ad98ca0486188751 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 23 May 2025 14:20:31 +0200 Subject: [PATCH 2/4] maybe fix instr --- .../src/server/instrumentation/reactRouter.ts | 96 ++++++++++--------- 1 file changed, 49 insertions(+), 47 deletions(-) diff --git a/packages/react-router/src/server/instrumentation/reactRouter.ts b/packages/react-router/src/server/instrumentation/reactRouter.ts index ed4984cd7431..c1ab4f445a16 100644 --- a/packages/react-router/src/server/instrumentation/reactRouter.ts +++ b/packages/react-router/src/server/instrumentation/reactRouter.ts @@ -40,7 +40,7 @@ export class ReactRouterInstrumentation extends InstrumentationBase { @@ -50,58 +50,60 @@ export class ReactRouterInstrumentation extends InstrumentationBase any { - return function sentryWrappedCreateRequestHandler(this: unknown, ...args: unknown[]) { - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore not sure why original isn't found here? - const originalRequestHandler = (original as typeof reactRouter.createRequestHandler).apply(this, args); - return async function sentryWrappedRequestHandler(request: Request, initialContext?: unknown) { - let url: URL; - try { - url = new URL(request.url); - } catch (error) { - return originalRequestHandler(request, initialContext); - } +/** + * Returns a patched version of the createRequestHandler function that adds Sentry performance monitoring. + * This wraps the request handler to create spans for data loader and action requests. + */ +function _patchCreateRequestHandler( + original: typeof reactRouter.createRequestHandler, +): typeof reactRouter.createRequestHandler { + return function sentryWrappedCreateRequestHandler(this: unknown, ...args: unknown[]) { + // eslint-disable-next-line @typescript-eslint/ban-ts-comment + // @ts-ignore not sure why original isn't found here? + const originalRequestHandler = (original as typeof reactRouter.createRequestHandler).apply(this, args); + return async function sentryWrappedRequestHandler(request: Request, initialContext?: unknown) { + let url: URL; + try { + url = new URL(request.url); + } catch (error) { + return originalRequestHandler(request, initialContext); + } - // We currently just want to trace loaders and actions - if (!isDataRequest(url.pathname)) { - return originalRequestHandler(request, initialContext); - } + // We currently just want to trace loaders and actions + if (!isDataRequest(url.pathname)) { + return originalRequestHandler(request, initialContext); + } - const activeSpan = getActiveSpan(); - const rootSpan = activeSpan && getRootSpan(activeSpan); + const activeSpan = getActiveSpan(); + const rootSpan = activeSpan && getRootSpan(activeSpan); - if (!rootSpan) { - DEBUG_BUILD && logger.debug('No active root span found, skipping tracing for data request'); - return originalRequestHandler(request, initialContext); - } + if (!rootSpan) { + DEBUG_BUILD && logger.debug('No active root span found, skipping tracing for data request'); + return originalRequestHandler(request, initialContext); + } - // Set the source and overwrite attributes on the root span to ensure the transaction name - // is derived from the raw URL pathname rather than any parameterized route that may be set later - // TODO: try to set derived parameterized route from build here (args[0]) - rootSpan.setAttributes({ - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - [SEMANTIC_ATTRIBUTE_SENTRY_OVERWRITE]: `${request.method} ${url.pathname}`, - }); + // Set the source and overwrite attributes on the root span to ensure the transaction name + // is derived from the raw URL pathname rather than any parameterized route that may be set later + // TODO: try to set derived parameterized route from build here (args[0]) + rootSpan.setAttributes({ + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + [SEMANTIC_ATTRIBUTE_SENTRY_OVERWRITE]: `${request.method} ${url.pathname}`, + }); - return startSpan( - { - name: getSpanName(url.pathname, request.method), - attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router', - [SEMANTIC_ATTRIBUTE_SENTRY_OP]: getOpName(url.pathname, request.method), - }, + return startSpan( + { + name: getSpanName(url.pathname, request.method), + attributes: { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router', + [SEMANTIC_ATTRIBUTE_SENTRY_OP]: getOpName(url.pathname, request.method), }, - () => { - return originalRequestHandler(request, initialContext); - }, - ); - }; + }, + () => { + return originalRequestHandler(request, initialContext); + }, + ); }; - } + }; } From a2c2c721470e8384d82355be5890753b69dd8709 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 23 May 2025 14:20:58 +0200 Subject: [PATCH 3/4] cleanup --- packages/react-router/src/server/instrumentation/reactRouter.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/react-router/src/server/instrumentation/reactRouter.ts b/packages/react-router/src/server/instrumentation/reactRouter.ts index c1ab4f445a16..cd0001d25d05 100644 --- a/packages/react-router/src/server/instrumentation/reactRouter.ts +++ b/packages/react-router/src/server/instrumentation/reactRouter.ts @@ -60,8 +60,6 @@ function _patchCreateRequestHandler( original: typeof reactRouter.createRequestHandler, ): typeof reactRouter.createRequestHandler { return function sentryWrappedCreateRequestHandler(this: unknown, ...args: unknown[]) { - // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore not sure why original isn't found here? const originalRequestHandler = (original as typeof reactRouter.createRequestHandler).apply(this, args); return async function sentryWrappedRequestHandler(request: Request, initialContext?: unknown) { let url: URL; From db11fe88a651de7aca5eeda7d3234342d90326ff Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Fri, 23 May 2025 14:31:21 +0200 Subject: [PATCH 4/4] try another approach --- .../src/server/instrumentation/reactRouter.ts | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/packages/react-router/src/server/instrumentation/reactRouter.ts b/packages/react-router/src/server/instrumentation/reactRouter.ts index cd0001d25d05..13ee744ffa4f 100644 --- a/packages/react-router/src/server/instrumentation/reactRouter.ts +++ b/packages/react-router/src/server/instrumentation/reactRouter.ts @@ -37,14 +37,12 @@ export class ReactRouterInstrumentation extends InstrumentationBase { - if (isWrapped(moduleExports['createRequestHandler'])) { - this._unwrap(moduleExports, 'createRequestHandler'); - } - this._wrap(moduleExports, 'createRequestHandler', _patchCreateRequestHandler); - return moduleExports; - }, - (moduleExports: ReactRouterModuleExports) => { - this._unwrap(moduleExports, 'createRequestHandler'); + const { createRequestHandler, ...rest } = moduleExports; + + return { + ...rest, + createRequestHandler: _patchCreateRequestHandler(createRequestHandler), + }; }, );