diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/instrument.mjs b/dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/instrument.mjs index a43afcba814f..c16240141b6d 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/instrument.mjs +++ b/dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/instrument.mjs @@ -5,9 +5,4 @@ Sentry.init({ environment: 'qa', // dynamic sampling bias to keep transactions tracesSampleRate: 1.0, tunnel: `http://localhost:3031/`, // proxy server - integrations: function (integrations) { - return integrations.filter(integration => { - return integration.name !== 'ReactRouterServer'; - }); - }, }); diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/tests/performance/performance.server.test.ts b/dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/tests/performance/performance.server.test.ts index abca82a6d938..a53b85228f00 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/tests/performance/performance.server.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-router-7-framework-custom/tests/performance/performance.server.test.ts @@ -2,7 +2,7 @@ import { expect, test } from '@playwright/test'; import { waitForTransaction } from '@sentry-internal/test-utils'; import { APP_NAME } from '../constants'; -test.describe('servery - performance', () => { +test.describe('server - performance', () => { test('should send server transaction on pageload', async ({ page }) => { const txPromise = waitForTransaction(APP_NAME, async transactionEvent => { return transactionEvent.transaction === 'GET /performance'; @@ -19,11 +19,11 @@ test.describe('servery - performance', () => { trace_id: expect.any(String), data: { 'sentry.op': 'http.server', - 'sentry.origin': 'auto.http.otel.http', + 'sentry.origin': 'auto.http.react-router.request-handler', 'sentry.source': 'route', }, op: 'http.server', - origin: 'auto.http.otel.http', + origin: 'auto.http.react-router.request-handler', }, }, spans: expect.any(Array), @@ -70,11 +70,11 @@ test.describe('servery - performance', () => { trace_id: expect.any(String), data: { 'sentry.op': 'http.server', - 'sentry.origin': 'auto.http.otel.http', + 'sentry.origin': 'auto.http.react-router.request-handler', 'sentry.source': 'route', }, op: 'http.server', - origin: 'auto.http.otel.http', + origin: 'auto.http.react-router.request-handler', }, }, spans: expect.any(Array), @@ -107,21 +107,52 @@ test.describe('servery - performance', () => { test('should instrument wrapped server loader', async ({ page }) => { const txPromise = waitForTransaction(APP_NAME, async transactionEvent => { - console.log(110, transactionEvent.transaction); - return transactionEvent.transaction === 'GET /performance/server-loader'; + return transactionEvent.transaction === 'GET /performance/server-loader.data'; }); await page.goto(`/performance`); - await page.waitForTimeout(500); await page.getByRole('link', { name: 'Server Loader' }).click(); const transaction = await txPromise; - expect(transaction?.spans?.[transaction.spans?.length - 1]).toMatchObject({ + expect(transaction).toEqual( + expect.objectContaining({ + contexts: expect.objectContaining({ + trace: { + span_id: expect.any(String), + trace_id: expect.any(String), + op: 'http.server', + origin: 'auto.http.react-router.loader', + parent_span_id: expect.any(String), + status: 'ok', + data: expect.objectContaining({ + 'http.method': 'GET', + 'http.response.status_code': 200, + 'http.status_code': 200, + 'http.status_text': 'OK', + 'http.target': '/performance/server-loader.data', + 'http.url': 'http://localhost:3030/performance/server-loader.data', + 'sentry.op': 'http.server', + 'sentry.origin': 'auto.http.react-router.loader', + 'sentry.source': 'url', + url: 'http://localhost:3030/performance/server-loader.data', + }), + }, + }), + transaction: 'GET /performance/server-loader.data', + type: 'transaction', + transaction_info: { source: 'url' }, + platform: 'node', + }), + ); + // ensure we do not have a stray, bogus route attribute + expect(transaction.contexts?.trace?.data?.['http.route']).not.toBeDefined(); + + expect(transaction?.spans).toContainEqual({ span_id: expect.any(String), trace_id: expect.any(String), data: { - 'sentry.origin': 'auto.http.react-router', + 'sentry.origin': 'auto.http.react-router.loader', 'sentry.op': 'function.react-router.loader', }, description: 'Executing Server Loader', @@ -130,13 +161,13 @@ test.describe('servery - performance', () => { timestamp: expect.any(Number), status: 'ok', op: 'function.react-router.loader', - origin: 'auto.http.react-router', + origin: 'auto.http.react-router.loader', }); }); test('should instrument a wrapped server action', async ({ page }) => { const txPromise = waitForTransaction(APP_NAME, async transactionEvent => { - return transactionEvent.transaction === 'POST /performance/server-action'; + return transactionEvent.transaction === 'POST /performance/server-action.data'; }); await page.goto(`/performance/server-action`); @@ -144,11 +175,44 @@ test.describe('servery - performance', () => { const transaction = await txPromise; - expect(transaction?.spans?.[transaction.spans?.length - 1]).toMatchObject({ + expect(transaction).toEqual( + expect.objectContaining({ + contexts: expect.objectContaining({ + trace: { + span_id: expect.any(String), + trace_id: expect.any(String), + op: 'http.server', + origin: 'auto.http.react-router.action', + parent_span_id: expect.any(String), + status: 'ok', + data: expect.objectContaining({ + 'http.method': 'POST', + 'http.response.status_code': 200, + 'http.status_code': 200, + 'http.status_text': 'OK', + 'http.target': '/performance/server-action.data', + 'http.url': 'http://localhost:3030/performance/server-action.data', + 'sentry.op': 'http.server', + 'sentry.origin': 'auto.http.react-router.action', + 'sentry.source': 'url', + url: 'http://localhost:3030/performance/server-action.data', + }), + }, + }), + transaction: 'POST /performance/server-action.data', + type: 'transaction', + transaction_info: { source: 'url' }, + platform: 'node', + }), + ); + // ensure we do not have a stray, bogus route attribute + expect(transaction.contexts?.trace?.data?.['http.route']).not.toBeDefined(); + + expect(transaction?.spans).toContainEqual({ span_id: expect.any(String), trace_id: expect.any(String), data: { - 'sentry.origin': 'auto.http.react-router', + 'sentry.origin': 'auto.http.react-router.action', 'sentry.op': 'function.react-router.action', }, description: 'Executing Server Action', @@ -157,7 +221,7 @@ test.describe('servery - performance', () => { timestamp: expect.any(Number), status: 'ok', op: 'function.react-router.action', - origin: 'auto.http.react-router', + origin: 'auto.http.react-router.action', }); }); }); diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/instrument.mjs b/dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/instrument.mjs index 70768dd2a6b4..48e4b7b61ff3 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/instrument.mjs +++ b/dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/instrument.mjs @@ -1,9 +1,8 @@ import * as Sentry from '@sentry/react-router'; Sentry.init({ - // todo: grab from env dsn: 'https://username@domain/123', environment: 'qa', // dynamic sampling bias to keep transactions tracesSampleRate: 1.0, - tunnel: `http://localhost:3031/`, // proxy server + tunnel: `http://localhost:3031/`, // proxy server, }); diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/tests/performance/performance.server.test.ts b/dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/tests/performance/performance.server.test.ts index b747719b5ff2..dd55a2ed6625 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/tests/performance/performance.server.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-router-7-framework-node-20-18/tests/performance/performance.server.test.ts @@ -2,7 +2,7 @@ import { expect, test } from '@playwright/test'; import { waitForTransaction } from '@sentry-internal/test-utils'; import { APP_NAME } from '../constants'; -test.describe('servery - performance', () => { +test.describe('server - performance', () => { test('should send server transaction on pageload', async ({ page }) => { const txPromise = waitForTransaction(APP_NAME, async transactionEvent => { return transactionEvent.transaction === 'GET /performance'; @@ -19,11 +19,11 @@ test.describe('servery - performance', () => { trace_id: expect.any(String), data: { 'sentry.op': 'http.server', - 'sentry.origin': 'auto.http.otel.http', + 'sentry.origin': 'auto.http.react-router.request-handler', 'sentry.source': 'route', }, op: 'http.server', - origin: 'auto.http.otel.http', + origin: 'auto.http.react-router.request-handler', }, }, spans: expect.any(Array), @@ -70,11 +70,11 @@ test.describe('servery - performance', () => { trace_id: expect.any(String), data: { 'sentry.op': 'http.server', - 'sentry.origin': 'auto.http.otel.http', + 'sentry.origin': 'auto.http.react-router.request-handler', 'sentry.source': 'route', }, op: 'http.server', - origin: 'auto.http.otel.http', + origin: 'auto.http.react-router.request-handler', }, }, spans: expect.any(Array), @@ -110,26 +110,59 @@ test.describe('servery - performance', () => { return transactionEvent.transaction === 'GET /performance/server-loader.data'; }); - await page.goto(`/performance`); // initial ssr pageloads do not contain .data requests - await page.waitForTimeout(500); // quick breather before navigation + await page.goto('/performance'); // initial ssr pageloads do not contain .data requests await page.getByRole('link', { name: 'Server Loader' }).click(); // this will actually trigger a .data request const transaction = await txPromise; - expect(transaction?.spans?.[transaction.spans?.length - 1]).toMatchObject({ + expect(transaction).toEqual( + expect.objectContaining({ + contexts: expect.objectContaining({ + trace: { + span_id: expect.any(String), + trace_id: expect.any(String), + op: 'http.server', + origin: 'auto.http.react-router.server', + parent_span_id: expect.any(String), + status: 'ok', + data: expect.objectContaining({ + 'http.method': 'GET', + 'http.response.status_code': 200, + 'http.status_code': 200, + 'http.status_text': 'OK', + 'http.target': '/performance/server-loader.data', + 'http.url': 'http://localhost:3030/performance/server-loader.data', + 'sentry.op': 'http.server', + 'sentry.origin': 'auto.http.react-router.server', + 'sentry.source': 'url', + url: 'http://localhost:3030/performance/server-loader.data', + }), + }, + }), + transaction: 'GET /performance/server-loader.data', + type: 'transaction', + transaction_info: { source: 'url' }, + platform: 'node', + }), + ); + + // ensure we do not have a stray, bogus route attribute + expect(transaction.contexts?.trace?.data?.['http.route']).not.toBeDefined(); + + expect(transaction.spans).toContainEqual({ span_id: expect.any(String), trace_id: expect.any(String), data: { - 'sentry.origin': 'auto.http.react-router', 'sentry.op': 'function.react-router.loader', + 'sentry.origin': 'auto.http.react-router.server', }, description: 'Executing Server Loader', + op: 'function.react-router.loader', + origin: 'auto.http.react-router.server', parent_span_id: expect.any(String), start_timestamp: expect.any(Number), - timestamp: expect.any(Number), status: 'ok', - op: 'function.react-router.loader', - origin: 'auto.http.react-router', + timestamp: expect.any(Number), }); }); @@ -143,20 +176,53 @@ test.describe('servery - performance', () => { const transaction = await txPromise; - expect(transaction?.spans?.[transaction.spans?.length - 1]).toMatchObject({ + expect(transaction).toEqual( + expect.objectContaining({ + contexts: expect.objectContaining({ + trace: { + span_id: expect.any(String), + trace_id: expect.any(String), + op: 'http.server', + origin: 'auto.http.react-router.server', + parent_span_id: expect.any(String), + status: 'ok', + data: expect.objectContaining({ + 'http.method': 'POST', + 'http.response.status_code': 200, + 'http.status_code': 200, + 'http.status_text': 'OK', + 'http.target': '/performance/server-action.data', + 'http.url': 'http://localhost:3030/performance/server-action.data', + 'sentry.op': 'http.server', + 'sentry.origin': 'auto.http.react-router.server', + 'sentry.source': 'url', + url: 'http://localhost:3030/performance/server-action.data', + }), + }, + }), + transaction: 'POST /performance/server-action.data', + type: 'transaction', + transaction_info: { source: 'url' }, + platform: 'node', + }), + ); + // ensure we do not have a stray, bogus route attribute + expect(transaction.contexts?.trace?.data?.['http.route']).not.toBeDefined(); + + expect(transaction.spans).toContainEqual({ span_id: expect.any(String), trace_id: expect.any(String), data: { - 'sentry.origin': 'auto.http.react-router', 'sentry.op': 'function.react-router.action', + 'sentry.origin': 'auto.http.react-router.server', }, description: 'Executing Server Action', + op: 'function.react-router.action', + origin: 'auto.http.react-router.server', parent_span_id: expect.any(String), start_timestamp: expect.any(Number), - timestamp: expect.any(Number), status: 'ok', - op: 'function.react-router.action', - origin: 'auto.http.react-router', + timestamp: expect.any(Number), }); }); }); diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-framework/instrument.mjs b/dev-packages/e2e-tests/test-applications/react-router-7-framework/instrument.mjs index 70768dd2a6b4..c16240141b6d 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-framework/instrument.mjs +++ b/dev-packages/e2e-tests/test-applications/react-router-7-framework/instrument.mjs @@ -1,7 +1,6 @@ import * as Sentry from '@sentry/react-router'; Sentry.init({ - // todo: grab from env dsn: 'https://username@domain/123', environment: 'qa', // dynamic sampling bias to keep transactions tracesSampleRate: 1.0, diff --git a/dev-packages/e2e-tests/test-applications/react-router-7-framework/tests/performance/performance.server.test.ts b/dev-packages/e2e-tests/test-applications/react-router-7-framework/tests/performance/performance.server.test.ts index 6b80e1e95fd4..f3f5e26a6154 100644 --- a/dev-packages/e2e-tests/test-applications/react-router-7-framework/tests/performance/performance.server.test.ts +++ b/dev-packages/e2e-tests/test-applications/react-router-7-framework/tests/performance/performance.server.test.ts @@ -19,11 +19,11 @@ test.describe('server - performance', () => { trace_id: expect.any(String), data: { 'sentry.op': 'http.server', - 'sentry.origin': 'auto.http.otel.http', + 'sentry.origin': 'auto.http.react-router.request-handler', 'sentry.source': 'route', }, op: 'http.server', - origin: 'auto.http.otel.http', + origin: 'auto.http.react-router.request-handler', }, }, spans: expect.any(Array), @@ -70,11 +70,11 @@ test.describe('server - performance', () => { trace_id: expect.any(String), data: { 'sentry.op': 'http.server', - 'sentry.origin': 'auto.http.otel.http', + 'sentry.origin': 'auto.http.react-router.request-handler', 'sentry.source': 'route', }, op: 'http.server', - origin: 'auto.http.otel.http', + origin: 'auto.http.react-router.request-handler', }, }, spans: expect.any(Array), diff --git a/packages/react-router/src/server/instrumentation/reactRouter.ts b/packages/react-router/src/server/instrumentation/reactRouter.ts index 5bfc0b62e352..f369e22ce66e 100644 --- a/packages/react-router/src/server/instrumentation/reactRouter.ts +++ b/packages/react-router/src/server/instrumentation/reactRouter.ts @@ -1,5 +1,6 @@ import type { InstrumentationConfig } from '@opentelemetry/instrumentation'; import { InstrumentationBase, InstrumentationNodeModuleDefinition } from '@opentelemetry/instrumentation'; +import { SEMATTRS_HTTP_TARGET } from '@opentelemetry/semantic-conventions'; import { getActiveSpan, getRootSpan, @@ -8,11 +9,13 @@ import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, + spanToJSON, startSpan, + updateSpanName, } from '@sentry/core'; import type * as reactRouter from 'react-router'; import { DEBUG_BUILD } from '../../common/debug-build'; -import { getOpName, getSpanName, isDataRequest, SEMANTIC_ATTRIBUTE_SENTRY_OVERWRITE } from './util'; +import { getOpName, getSpanName, isDataRequest } from './util'; type ReactRouterModuleExports = typeof reactRouter; @@ -81,19 +84,23 @@ export class ReactRouterInstrumentation extends InstrumentationBase { return { name: INTEGRATION_NAME, setupOnce() { - instrumentReactRouterServer(); + if ( + (NODE_VERSION.major === 20 && NODE_VERSION.minor < 19) || // https://nodejs.org/en/blog/release/v20.19.0 + (NODE_VERSION.major === 22 && NODE_VERSION.minor < 12) // https://nodejs.org/en/blog/release/v22.12.0 + ) { + instrumentReactRouterServer(); + } + }, + processEvent(event) { + // Express generates bogus `*` routes for data loaders, which we want to remove here + // we cannot do this earlier because some OTEL instrumentation adds this at some unexpected point + if ( + event.type === 'transaction' && + event.contexts?.trace?.data && + event.contexts.trace.data[ATTR_HTTP_ROUTE] === '*' && + // This means the name has been adjusted before, but the http.route remains, so we need to remove it + event.transaction !== 'GET *' && + event.transaction !== 'POST *' + ) { + // eslint-disable-next-line @typescript-eslint/no-dynamic-delete + delete event.contexts.trace.data[ATTR_HTTP_ROUTE]; + } + + return event; }, }; }); diff --git a/packages/react-router/src/server/sdk.ts b/packages/react-router/src/server/sdk.ts index 07ea80e867ea..9d8a22862a11 100644 --- a/packages/react-router/src/server/sdk.ts +++ b/packages/react-router/src/server/sdk.ts @@ -1,10 +1,8 @@ -import { ATTR_HTTP_ROUTE } from '@opentelemetry/semantic-conventions'; -import type { EventProcessor, Integration } from '@sentry/core'; -import { applySdkMetadata, getGlobalScope, logger, setTag } from '@sentry/core'; +import type { Integration } from '@sentry/core'; +import { applySdkMetadata, logger, setTag } from '@sentry/core'; import type { NodeClient, NodeOptions } from '@sentry/node'; -import { getDefaultIntegrations as getNodeDefaultIntegrations, init as initNodeSdk, NODE_VERSION } from '@sentry/node'; +import { getDefaultIntegrations as getNodeDefaultIntegrations, init as initNodeSdk } from '@sentry/node'; import { DEBUG_BUILD } from '../common/debug-build'; -import { SEMANTIC_ATTRIBUTE_SENTRY_OVERWRITE } from './instrumentation/util'; import { lowQualityTransactionsFilterIntegration } from './integration/lowQualityTransactionsFilterIntegration'; import { reactRouterServerIntegration } from './integration/reactRouterServer'; @@ -13,16 +11,11 @@ import { reactRouterServerIntegration } from './integration/reactRouterServer'; * @param options The options for the SDK. */ export function getDefaultReactRouterServerIntegrations(options: NodeOptions): Integration[] { - const integrations = [...getNodeDefaultIntegrations(options), lowQualityTransactionsFilterIntegration(options)]; - - if ( - (NODE_VERSION.major === 20 && NODE_VERSION.minor < 19) || // https://nodejs.org/en/blog/release/v20.19.0 - (NODE_VERSION.major === 22 && NODE_VERSION.minor < 12) // https://nodejs.org/en/blog/release/v22.12.0 - ) { - integrations.push(reactRouterServerIntegration()); - } - - return integrations; + return [ + ...getNodeDefaultIntegrations(options), + lowQualityTransactionsFilterIntegration(options), + reactRouterServerIntegration(), + ]; } /** @@ -42,31 +35,6 @@ export function init(options: NodeOptions): NodeClient | undefined { setTag('runtime', 'node'); - // Overwrite the transaction name for instrumented data loaders because the trace data gets overwritten at a later point. - // We only update the tx in case SEMANTIC_ATTRIBUTE_SENTRY_OVERWRITE got set in our instrumentation before. - getGlobalScope().addEventProcessor( - Object.assign( - (event => { - const overwrite = event.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_SENTRY_OVERWRITE]; - if ( - event.type === 'transaction' && - (event.transaction === 'GET *' || event.transaction === 'POST *') && - event.contexts?.trace?.data?.[ATTR_HTTP_ROUTE] === '*' && - overwrite - ) { - event.transaction = overwrite; - event.contexts.trace.data[ATTR_HTTP_ROUTE] = 'url'; - } - - // always yeet this attribute into the void, as this should not reach the server - delete event.contexts?.trace?.data?.[SEMANTIC_ATTRIBUTE_SENTRY_OVERWRITE]; - - return event; - }) satisfies EventProcessor, - { id: 'ReactRouterTransactionEnhancer' }, - ), - ); - DEBUG_BUILD && logger.log('SDK successfully initialized'); return client; diff --git a/packages/react-router/src/server/wrapSentryHandleRequest.ts b/packages/react-router/src/server/wrapSentryHandleRequest.ts index 40a336a40fbd..df7d65109338 100644 --- a/packages/react-router/src/server/wrapSentryHandleRequest.ts +++ b/packages/react-router/src/server/wrapSentryHandleRequest.ts @@ -5,7 +5,7 @@ import { getActiveSpan, getRootSpan, getTraceMetaTags, - SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME, + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, } from '@sentry/core'; import type { AppLoadContext, EntryContext } from 'react-router'; @@ -37,26 +37,25 @@ export function wrapSentryHandleRequest(originalHandle: OriginalHandleRequest): const parameterizedPath = routerContext?.staticHandlerContext?.matches?.[routerContext.staticHandlerContext.matches.length - 1]?.route.path; - if (parameterizedPath) { - const activeSpan = getActiveSpan(); - if (activeSpan) { - const rootSpan = getRootSpan(activeSpan); - const routeName = `/${parameterizedPath}`; + const activeSpan = getActiveSpan(); + const rootSpan = activeSpan ? getRootSpan(activeSpan) : undefined; - // The express instrumentation writes on the rpcMetadata and that ends up stomping on the `http.route` attribute. - const rpcMetadata = getRPCMetadata(context.active()); + if (parameterizedPath && rootSpan) { + const routeName = `/${parameterizedPath}`; - if (rpcMetadata?.type === RPCType.HTTP) { - rpcMetadata.route = routeName; - } + // The express instrumentation writes on the rpcMetadata and that ends up stomping on the `http.route` attribute. + const rpcMetadata = getRPCMetadata(context.active()); - // The span exporter picks up the `http.route` (ATTR_HTTP_ROUTE) attribute to set the transaction name - rootSpan.setAttributes({ - [ATTR_HTTP_ROUTE]: routeName, - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - [SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME]: `${request.method} ${routeName}`, - }); + if (rpcMetadata?.type === RPCType.HTTP) { + rpcMetadata.route = routeName; } + + // The span exporter picks up the `http.route` (ATTR_HTTP_ROUTE) attribute to set the transaction name + rootSpan.setAttributes({ + [ATTR_HTTP_ROUTE]: routeName, + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router.request-handler', + }); } return originalHandle(request, responseStatusCode, responseHeaders, routerContext, loadContext); diff --git a/packages/react-router/src/server/wrapServerAction.ts b/packages/react-router/src/server/wrapServerAction.ts index 9da0e8d351f8..7dc8851e2171 100644 --- a/packages/react-router/src/server/wrapServerAction.ts +++ b/packages/react-router/src/server/wrapServerAction.ts @@ -1,16 +1,16 @@ +import { SEMATTRS_HTTP_TARGET } from '@opentelemetry/semantic-conventions'; import type { SpanAttributes } from '@sentry/core'; import { getActiveSpan, getRootSpan, - parseStringToURLObject, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, spanToJSON, startSpan, + updateSpanName, } from '@sentry/core'; import type { ActionFunctionArgs } from 'react-router'; -import { SEMANTIC_ATTRIBUTE_SENTRY_OVERWRITE } from './instrumentation/util'; type SpanOptions = { name?: string; @@ -42,13 +42,18 @@ export function wrapServerAction(options: SpanOptions = {}, actionFn: (args: const active = getActiveSpan(); if (active) { const root = getRootSpan(active); - // coming from auto.http.otel.http - if (spanToJSON(root).description === 'POST') { - const url = parseStringToURLObject(args.request.url); - if (url?.pathname) { + const spanData = spanToJSON(root); + if (spanData.origin === 'auto.http.otel.http') { + // eslint-disable-next-line deprecation/deprecation + const target = spanData.data[SEMATTRS_HTTP_TARGET]; + + if (target) { + // We cannot rely on the regular span name inferral here, as the express instrumentation sets `*` as the route + // So we force this to be a more sensible name here + updateSpanName(root, `${args.request.method} ${target}`); root.setAttributes({ [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - [SEMANTIC_ATTRIBUTE_SENTRY_OVERWRITE]: `${args.request.method} ${url.pathname}`, + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router.action', }); } } @@ -59,7 +64,7 @@ export function wrapServerAction(options: SpanOptions = {}, actionFn: (args: name, ...options, attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router.action', [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.react-router.action', ...options.attributes, }, diff --git a/packages/react-router/src/server/wrapServerLoader.ts b/packages/react-router/src/server/wrapServerLoader.ts index dda64a1a9204..3d32f0c9d159 100644 --- a/packages/react-router/src/server/wrapServerLoader.ts +++ b/packages/react-router/src/server/wrapServerLoader.ts @@ -1,16 +1,16 @@ +import { SEMATTRS_HTTP_TARGET } from '@opentelemetry/semantic-conventions'; import type { SpanAttributes } from '@sentry/core'; import { getActiveSpan, getRootSpan, - parseStringToURLObject, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, spanToJSON, startSpan, + updateSpanName, } from '@sentry/core'; import type { LoaderFunctionArgs } from 'react-router'; -import { SEMANTIC_ATTRIBUTE_SENTRY_OVERWRITE } from './instrumentation/util'; type SpanOptions = { name?: string; @@ -40,16 +40,21 @@ export function wrapServerLoader(options: SpanOptions = {}, loaderFn: (args: return async function (args: LoaderFunctionArgs) { const name = options.name || 'Executing Server Loader'; const active = getActiveSpan(); + if (active) { const root = getRootSpan(active); - // coming from auto.http.otel.http - if (spanToJSON(root).description === 'GET') { - const url = parseStringToURLObject(args.request.url); + const spanData = spanToJSON(root); + if (spanData.origin === 'auto.http.otel.http') { + // eslint-disable-next-line deprecation/deprecation + const target = spanData.data[SEMATTRS_HTTP_TARGET]; - if (url?.pathname) { + if (target) { + // We cannot rely on the regular span name inferral here, as the express instrumentation sets `*` as the route + // So we force this to be a more sensible name here + updateSpanName(root, `${args.request.method} ${target}`); root.setAttributes({ [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', - [SEMANTIC_ATTRIBUTE_SENTRY_OVERWRITE]: `${args.request.method} ${url.pathname}`, + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router.loader', }); } } @@ -59,7 +64,7 @@ export function wrapServerLoader(options: SpanOptions = {}, loaderFn: (args: name, ...options, attributes: { - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router.loader', [SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.react-router.loader', ...options.attributes, }, diff --git a/packages/react-router/test/server/instrumentation/reactRouterServer.test.ts b/packages/react-router/test/server/instrumentation/reactRouterServer.test.ts index ddcb856c68b9..d89fdf624294 100644 --- a/packages/react-router/test/server/instrumentation/reactRouterServer.test.ts +++ b/packages/react-router/test/server/instrumentation/reactRouterServer.test.ts @@ -1,4 +1,4 @@ -import type { Span } from '@sentry/core'; +import type { Span, SpanJSON } from '@sentry/core'; import * as SentryCore from '@sentry/core'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { ReactRouterInstrumentation } from '../../../src/server/instrumentation/reactRouter'; @@ -8,6 +8,8 @@ vi.mock('@sentry/core', async () => { return { getActiveSpan: vi.fn(), getRootSpan: vi.fn(), + spanToJSON: vi.fn(), + updateSpanName: vi.fn(), logger: { debug: vi.fn(), }, @@ -90,6 +92,7 @@ describe('ReactRouterInstrumentation', () => { vi.spyOn(Util, 'isDataRequest').mockReturnValue(true); vi.spyOn(SentryCore, 'getActiveSpan').mockReturnValue(mockSpan as Span); vi.spyOn(SentryCore, 'getRootSpan').mockReturnValue(mockSpan as Span); + vi.spyOn(SentryCore, 'spanToJSON').mockReturnValue({ data: {} } as SpanJSON); vi.spyOn(Util, 'getSpanName').mockImplementation((pathname, method) => `span:${pathname}:${method}`); vi.spyOn(SentryCore, 'startSpan').mockImplementation((_opts, fn) => fn(mockSpan as Span)); diff --git a/packages/react-router/test/server/integration/reactRouterServer.test.ts b/packages/react-router/test/server/integration/reactRouterServer.test.ts new file mode 100644 index 000000000000..a5eac42643e5 --- /dev/null +++ b/packages/react-router/test/server/integration/reactRouterServer.test.ts @@ -0,0 +1,65 @@ +import * as SentryNode from '@sentry/node'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { ReactRouterInstrumentation } from '../../../src/server/instrumentation/reactRouter'; +import { reactRouterServerIntegration } from '../../../src/server/integration/reactRouterServer'; + +vi.mock('../../../src/server/instrumentation/reactRouter', () => { + return { + ReactRouterInstrumentation: vi.fn(), + }; +}); + +vi.mock('@sentry/node', () => { + return { + generateInstrumentOnce: vi.fn((_name: string, callback: () => any) => { + return Object.assign(callback, { id: 'test' }); + }), + NODE_VERSION: { + major: 0, + minor: 0, + patch: 0, + }, + }; +}); + +describe('reactRouterServerIntegration', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('sets up ReactRouterInstrumentation for Node 20.18', () => { + vi.spyOn(SentryNode, 'NODE_VERSION', 'get').mockReturnValue({ major: 20, minor: 18, patch: 0 }); + + const integration = reactRouterServerIntegration(); + integration.setupOnce!(); + + expect(ReactRouterInstrumentation).toHaveBeenCalled(); + }); + + it('sets up ReactRouterInstrumentationfor Node.js 22.11', () => { + vi.spyOn(SentryNode, 'NODE_VERSION', 'get').mockReturnValue({ major: 22, minor: 11, patch: 0 }); + + const integration = reactRouterServerIntegration(); + integration.setupOnce!(); + + expect(ReactRouterInstrumentation).toHaveBeenCalled(); + }); + + it('does not set up ReactRouterInstrumentation for Node.js 20.19', () => { + vi.spyOn(SentryNode, 'NODE_VERSION', 'get').mockReturnValue({ major: 20, minor: 19, patch: 0 }); + + const integration = reactRouterServerIntegration(); + integration.setupOnce!(); + + expect(ReactRouterInstrumentation).not.toHaveBeenCalled(); + }); + + it('does not set up ReactRouterInstrumentation for Node.js 22.12', () => { + vi.spyOn(SentryNode, 'NODE_VERSION', 'get').mockReturnValue({ major: 22, minor: 12, patch: 0 }); + + const integration = reactRouterServerIntegration(); + integration.setupOnce!(); + + expect(ReactRouterInstrumentation).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/react-router/test/server/sdk.test.ts b/packages/react-router/test/server/sdk.test.ts index 861144e3f62b..6e1879f8e24b 100644 --- a/packages/react-router/test/server/sdk.test.ts +++ b/packages/react-router/test/server/sdk.test.ts @@ -72,27 +72,7 @@ describe('React Router server SDK', () => { expect(filterIntegration).toBeDefined(); }); - it('adds reactRouterServer integration for Node.js 20.18', () => { - vi.spyOn(SentryNode, 'NODE_VERSION', 'get').mockReturnValue({ major: 20, minor: 18, patch: 0 }); - - reactRouterInit({ - dsn: 'https://public@dsn.ingest.sentry.io/1337', - }); - - expect(nodeInit).toHaveBeenCalledTimes(1); - const initOptions = nodeInit.mock.calls[0]?.[0]; - const defaultIntegrations = initOptions?.defaultIntegrations as Integration[]; - - const reactRouterServerIntegration = defaultIntegrations.find( - integration => integration.name === 'ReactRouterServer', - ); - - expect(reactRouterServerIntegration).toBeDefined(); - }); - - it('adds reactRouterServer integration for Node.js 22.11', () => { - vi.spyOn(SentryNode, 'NODE_VERSION', 'get').mockReturnValue({ major: 22, minor: 11, patch: 0 }); - + it('adds reactRouterServer integration by default', () => { reactRouterInit({ dsn: 'https://public@dsn.ingest.sentry.io/1337', }); @@ -107,41 +87,5 @@ describe('React Router server SDK', () => { expect(reactRouterServerIntegration).toBeDefined(); }); - - it('does not add reactRouterServer integration for Node.js 20.19', () => { - vi.spyOn(SentryNode, 'NODE_VERSION', 'get').mockReturnValue({ major: 20, minor: 19, patch: 0 }); - - reactRouterInit({ - dsn: 'https://public@dsn.ingest.sentry.io/1337', - }); - - expect(nodeInit).toHaveBeenCalledTimes(1); - const initOptions = nodeInit.mock.calls[0]?.[0]; - const defaultIntegrations = initOptions?.defaultIntegrations as Integration[]; - - const reactRouterServerIntegration = defaultIntegrations.find( - integration => integration.name === 'ReactRouterServer', - ); - - expect(reactRouterServerIntegration).toBeUndefined(); - }); - - it('does not add reactRouterServer integration for Node.js 22.12', () => { - vi.spyOn(SentryNode, 'NODE_VERSION', 'get').mockReturnValue({ major: 22, minor: 12, patch: 0 }); - - reactRouterInit({ - dsn: 'https://public@dsn.ingest.sentry.io/1337', - }); - - expect(nodeInit).toHaveBeenCalledTimes(1); - const initOptions = nodeInit.mock.calls[0]?.[0]; - const defaultIntegrations = initOptions?.defaultIntegrations as Integration[]; - - const reactRouterServerIntegration = defaultIntegrations.find( - integration => integration.name === 'ReactRouterServer', - ); - - expect(reactRouterServerIntegration).toBeUndefined(); - }); }); }); diff --git a/packages/react-router/test/server/wrapSentryHandleRequest.test.ts b/packages/react-router/test/server/wrapSentryHandleRequest.test.ts index 61a92e7b6546..40dce7c83702 100644 --- a/packages/react-router/test/server/wrapSentryHandleRequest.test.ts +++ b/packages/react-router/test/server/wrapSentryHandleRequest.test.ts @@ -1,6 +1,12 @@ import { RPCType } from '@opentelemetry/core'; import { ATTR_HTTP_ROUTE } from '@opentelemetry/semantic-conventions'; -import { getActiveSpan, getRootSpan, getTraceMetaTags, SEMANTIC_ATTRIBUTE_SENTRY_SOURCE } from '@sentry/core'; +import { + getActiveSpan, + getRootSpan, + getTraceMetaTags, + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, +} from '@sentry/core'; import { PassThrough } from 'stream'; import { beforeEach, describe, expect, test, vi } from 'vitest'; import { getMetaTagTransformer, wrapSentryHandleRequest } from '../../src/server/wrapSentryHandleRequest'; @@ -12,7 +18,7 @@ vi.mock('@opentelemetry/core', () => ({ vi.mock('@sentry/core', () => ({ SEMANTIC_ATTRIBUTE_SENTRY_SOURCE: 'sentry.source', - SEMANTIC_ATTRIBUTE_SENTRY_CUSTOM_SPAN_NAME: 'sentry.custom-span-name', + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN: 'sentry.origin', getActiveSpan: vi.fn(), getRootSpan: vi.fn(), getTraceMetaTags: vi.fn(), @@ -49,7 +55,7 @@ describe('wrapSentryHandleRequest', () => { const originalHandler = vi.fn().mockResolvedValue('test'); const wrappedHandler = wrapSentryHandleRequest(originalHandler); - const mockActiveSpan = { setAttribute: vi.fn() }; + const mockActiveSpan = {}; const mockRootSpan = { setAttributes: vi.fn() }; const mockRpcMetadata = { type: RPCType.HTTP, route: '/some-path' }; @@ -66,17 +72,21 @@ describe('wrapSentryHandleRequest', () => { await wrappedHandler(new Request('https://nacho.queso'), 200, new Headers(), routerContext, {} as any); - expect(getActiveSpan).toHaveBeenCalled(); - expect(getRootSpan).toHaveBeenCalledWith(mockActiveSpan); expect(mockRootSpan.setAttributes).toHaveBeenCalledWith({ [ATTR_HTTP_ROUTE]: '/some-path', - 'sentry.custom-span-name': 'GET /some-path', [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router.request-handler', }); expect(mockRpcMetadata.route).toBe('/some-path'); }); test('should not set span attributes when parameterized path does not exist', async () => { + const mockActiveSpan = {}; + const mockRootSpan = { setAttributes: vi.fn() }; + + (getActiveSpan as unknown as ReturnType).mockReturnValue(mockActiveSpan); + (getRootSpan as unknown as ReturnType).mockReturnValue(mockRootSpan); + const originalHandler = vi.fn().mockResolvedValue('test'); const wrappedHandler = wrapSentryHandleRequest(originalHandler); @@ -88,15 +98,20 @@ describe('wrapSentryHandleRequest', () => { await wrappedHandler(new Request('https://guapo.chulo'), 200, new Headers(), routerContext, {} as any); - expect(getActiveSpan).not.toHaveBeenCalled(); + expect(mockRootSpan.setAttributes).not.toHaveBeenCalled(); }); test('should not set span attributes when active span does not exist', async () => { const originalHandler = vi.fn().mockResolvedValue('test'); const wrappedHandler = wrapSentryHandleRequest(originalHandler); + const mockRpcMetadata = { type: RPCType.HTTP, route: '/some-path' }; + (getActiveSpan as unknown as ReturnType).mockReturnValue(null); + const getRPCMetadata = vi.fn().mockReturnValue(mockRpcMetadata); + vi.mocked(vi.importActual('@opentelemetry/core')).getRPCMetadata = getRPCMetadata; + const routerContext = { staticHandlerContext: { matches: [{ route: { path: 'some-path' } }], @@ -105,8 +120,7 @@ describe('wrapSentryHandleRequest', () => { await wrappedHandler(new Request('https://tio.pepe'), 200, new Headers(), routerContext, {} as any); - expect(getActiveSpan).toHaveBeenCalled(); - expect(getRootSpan).not.toHaveBeenCalled(); + expect(getRPCMetadata).not.toHaveBeenCalled(); }); }); diff --git a/packages/react-router/test/server/wrapServerAction.test.ts b/packages/react-router/test/server/wrapServerAction.test.ts index 931e4c72b446..14933fe87e4f 100644 --- a/packages/react-router/test/server/wrapServerAction.test.ts +++ b/packages/react-router/test/server/wrapServerAction.test.ts @@ -20,7 +20,7 @@ describe('wrapServerAction', () => { { name: 'Executing Server Action', attributes: { - [core.SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router', + [core.SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router.action', [core.SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.react-router.action', }, }, @@ -48,7 +48,7 @@ describe('wrapServerAction', () => { { name: 'Custom Action', attributes: { - [core.SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router', + [core.SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router.action', [core.SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.react-router.action', 'sentry.custom': 'value', }, diff --git a/packages/react-router/test/server/wrapServerLoader.test.ts b/packages/react-router/test/server/wrapServerLoader.test.ts index 53fce752286b..67b7d512bcbe 100644 --- a/packages/react-router/test/server/wrapServerLoader.test.ts +++ b/packages/react-router/test/server/wrapServerLoader.test.ts @@ -20,7 +20,7 @@ describe('wrapServerLoader', () => { { name: 'Executing Server Loader', attributes: { - [core.SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router', + [core.SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router.loader', [core.SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.react-router.loader', }, }, @@ -48,7 +48,7 @@ describe('wrapServerLoader', () => { { name: 'Custom Loader', attributes: { - [core.SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router', + [core.SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.react-router.loader', [core.SEMANTIC_ATTRIBUTE_SENTRY_OP]: 'function.react-router.loader', 'sentry.custom': 'value', },