From 667c4bdae56c6b728307f1c88f09ed48672ed893 Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 25 Aug 2026 17:31:18 -0700 Subject: [PATCH] feat(core): emit low cardinality request handler span names Name `handler` spans after the route they serve when span streaming is enabled, or `Request handler` if no route set. Static mode left as is. Drop Hapi method, as the template dictates. NestJS resolves no route when the span starts. The NestJS callback name stays on `nestjs.callback`. Elysia sets `context.route` when the request enters the compiled handler, which is before the `Handle` phase reports. Read it in the trace listener so streamed handler spans carry the route instead of the `Request handler` fallback. The fallback now applies only when the context has no route. Set `code.function.name` only on the child spans this renames, and only when the handler has a name. Static mode keeps the handler name in the span name, so the attribute adds nothing there, and an anonymous handler has no name to record. Register the Fastify test route from a plugin. Fastify installs the SDK's `onRoute` hook when it flushes its plugin list, which is after root-level routes are in place. A root-level route therefore produces no route handler span, and the test never reached that code path. Also: correct `REQUEST_HANDLER_SPAN_NAME_FALLBACK`: the conventions spell the fallback `Request handler`, and its `@see` link pointed at the resource section. closes #23533 Co-Authored-By: Claude Opus 5 (1M context) --- MIGRATION.md | 3 + .../tracing/fastify-streamed/instrument.mjs | 10 + .../tracing/fastify-streamed/scenario.mjs | 28 +++ .../suites/tracing/fastify-streamed/test.ts | 36 ++++ .../src/integrations/express/patch-layer.ts | 17 +- packages/core/src/tracing/spans/spanNames.ts | 4 +- .../integrations/express/patch-layer.test.ts | 73 +++++++ packages/elysia/src/withElysia.ts | 49 ++++- packages/elysia/test/withElysia.test.ts | 127 +++++++++++- .../nestjs/src/integrations/wrap-route.ts | 21 +- .../orchestrion-subscriber.test.ts | 26 ++- .../integrations/express/instrumentation.ts | 16 +- .../integrations/fastify/instrumentation.ts | 25 ++- .../src/integrations/hapi/hapi-utils.ts | 12 +- .../express/instrumentation.test.ts | 186 ++++++++++++++++++ .../test/integrations/hapi-utils.test.ts | 14 +- 16 files changed, 618 insertions(+), 29 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/tracing/fastify-streamed/instrument.mjs create mode 100644 dev-packages/node-integration-tests/suites/tracing/fastify-streamed/scenario.mjs create mode 100644 dev-packages/node-integration-tests/suites/tracing/fastify-streamed/test.ts create mode 100644 packages/server-utils/test/integrations/express/instrumentation.test.ts diff --git a/MIGRATION.md b/MIGRATION.md index dc2dd5da8632..eabd6ea6aaf0 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -794,6 +794,7 @@ The following span names were adjusted: | ------------------------------------------------------------------------ | --------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------ | | `pageload` | The parameterized route, or the raw URL path if the SDK couldn't resolve one (`/users/123`) | The parameterized route, or `Pageload` if the SDK has none | | `router` | Framework-specific, sometimes containing the raw URL (`/users/123`, `SvelteKit Route Change`) | The span's `http.route`, or `Router` if the SDK has none | +| `handler` | Framework-specific, often carrying the request method (`GET /users/:id`, `route-handler`, `getUser`) | The span's `http.route`, or `Request handler` if the SDK has none | | `graphql` | The graphql phase and, for operations, the operation name (`query GetUser`, `graphql.parse`, `graphql.resolve user.0.name`) | The operation type, or the processing type where there is none (`GraphQL query`, `GraphQL parse`, `GraphQL resolve`) | | `resource.*` | The resource URL, relative to the page origin for same-origin resources (`/assets/app.js`) | The resource domain (`cdn.example.com`), or `Resource` if the SDK has none | | `mcp.server` | The method and its target, including the resource URI (`resources/read file:///docs/api.md`) | The method alone for resource methods (`resources/read`). Tool and prompt names are unchanged (`tools/call get-weather`) | @@ -813,6 +814,8 @@ Resource URIs are unbounded, so they are no longer part of an `mcp.server` span Only the Express, Koa and Hapi integrations resolve a route template for `router` spans. Angular, Ember and SvelteKit have none when the span starts, so their router spans are named `Router`. +The Express, Fastify, Hapi and Elysia integrations resolve a route template for `handler` spans. NestJS has none when the span starts, so its request handler spans are named `Request handler`. The handler function name is no longer part of these span names. It stays on an attribute: `nestjs.callback` for NestJS, and `code.function.name` for Elysia, which now sets it on the handler spans it renames. Elysia request handler spans also carry `http.route` now, in both trace lifecycles. + Child spans of a service or root span carry its name in their `sentry.segment.name` attribute, so that changes with it. If you group or filter spans by segment name in dashboards or alerts, update those references. `ignoreSpans` is evaluated when a span **starts**, at which point a span might not yet have its final name. For example, an unresolved pageload span name is named `'Pageload'` and might receive its final, resolved route name later. diff --git a/dev-packages/node-integration-tests/suites/tracing/fastify-streamed/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/fastify-streamed/instrument.mjs new file mode 100644 index 000000000000..53b9511a21f0 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/fastify-streamed/instrument.mjs @@ -0,0 +1,10 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + transport: loggingTransport, + traceLifecycle: 'stream', +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/fastify-streamed/scenario.mjs b/dev-packages/node-integration-tests/suites/tracing/fastify-streamed/scenario.mjs new file mode 100644 index 000000000000..08e8fb2faad2 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/fastify-streamed/scenario.mjs @@ -0,0 +1,28 @@ +import { sendPortToRunner } from '@sentry-internal/node-integration-tests'; +import Fastify from 'fastify'; + +const app = Fastify(); + +// The routes go through `register` so that they reach the SDK's `onRoute` hook. +// That hook is installed when Fastify flushes its plugin list, which is after +// routes registered directly on the root instance are already in place. +app.register(async instance => { + instance.get( + '/test-transaction/:id', + { + preHandler: function routePreHandler(_request, _reply, done) { + done(); + }, + }, + async () => { + return {}; + }, + ); +}); + +const run = async () => { + await app.listen({ port: 0, host: 'localhost' }); + sendPortToRunner(app.server.address().port); +}; + +run(); diff --git a/dev-packages/node-integration-tests/suites/tracing/fastify-streamed/test.ts b/dev-packages/node-integration-tests/suites/tracing/fastify-streamed/test.ts new file mode 100644 index 000000000000..44b34a94a936 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/fastify-streamed/test.ts @@ -0,0 +1,36 @@ +import { afterAll, describe, expect } from 'vitest'; +import { cleanupChildProcesses, createEsmAndCjsTests } from '../../../utils/runner'; + +describe('fastify auto-instrumentation (streamed)', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { + test('names request handler spans after their route', async () => { + const runner = createRunner() + .expect({ + span: container => { + const handlerSpans = container.items.filter(item => item.attributes['sentry.op']?.value === 'handler'); + + // The request span and the route handler span. + expect(handlerSpans).toHaveLength(2); + for (const span of handlerSpans) { + expect(span.name).toBe('/test-transaction/:id'); + // The name has to stay in step with the attribute it comes from. + expect(span.attributes['http.route']?.value).toBe('/test-transaction/:id'); + } + + // Spans of other ops keep their names. + const hookSpan = container.items.find(item => item.name === 'preHandler - routePreHandler'); + expect(hookSpan).toBeDefined(); + }, + }) + .start(); + + await runner.makeRequest('get', '/test-transaction/123'); + + await runner.completed(); + }); + }); +}); diff --git a/packages/core/src/integrations/express/patch-layer.ts b/packages/core/src/integrations/express/patch-layer.ts index 5ccd63ace2fa..4847dd8f318b 100644 --- a/packages/core/src/integrations/express/patch-layer.ts +++ b/packages/core/src/integrations/express/patch-layer.ts @@ -33,7 +33,7 @@ import { DEBUG_BUILD } from '../../debug-build'; import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '../../semanticAttributes'; import { SPAN_STATUS_ERROR, withActiveSpan } from '../../tracing'; import { hasSpanStreamingEnabled } from '../../tracing/spans/hasSpanStreamingEnabled'; -import { ROUTER_SPAN_NAME_FALLBACK } from '../../tracing/spans/spanNames'; +import { REQUEST_HANDLER_SPAN_NAME_FALLBACK, ROUTER_SPAN_NAME_FALLBACK } from '../../tracing/spans/spanNames'; import { startSpanManual } from '../../tracing/trace'; import { debug } from '../../utils/debug-logger'; import type { SpanAttributes } from '../../types/span'; @@ -168,10 +168,19 @@ export function patchLayer( } const client = getClient(); - // With span streaming, span names have to be low cardinality, so router spans are named after their route. - const isStreamedRouterSpan = type === ExpressLayerType_ROUTER && !!client && hasSpanStreamingEnabled(client); + // With span streaming, span names have to be low cardinality, so router + // and request handler spans are named after their route. A route that did + // not validate against the request URL can describe a different request, + // so those spans take the static fallback instead. + const isStreamedSpan = !!client && hasSpanStreamingEnabled(client); + const isStreamedRouterSpan = isStreamedSpan && type === ExpressLayerType_ROUTER; + const isStreamedRequestHandlerSpan = isStreamedSpan && type === ExpressLayerType_REQUEST_HANDLER; - const spanName = isStreamedRouterSpan ? actualMatchedRoute || ROUTER_SPAN_NAME_FALLBACK : name; + const spanName = isStreamedRouterSpan + ? actualMatchedRoute || ROUTER_SPAN_NAME_FALLBACK + : isStreamedRequestHandlerSpan + ? actualMatchedRoute || REQUEST_HANDLER_SPAN_NAME_FALLBACK + : name; return startSpanManual({ name: spanName, attributes }, span => { let spanHasEnded = false; diff --git a/packages/core/src/tracing/spans/spanNames.ts b/packages/core/src/tracing/spans/spanNames.ts index 88eb200ae973..66b006fc7a83 100644 --- a/packages/core/src/tracing/spans/spanNames.ts +++ b/packages/core/src/tracing/spans/spanNames.ts @@ -70,6 +70,6 @@ export const ROUTER_SPAN_NAME_FALLBACK = 'Router'; /** * Fallback name for request handler spans when no better-suited span name is available. - * @see https://getsentry.github.io/sentry-conventions/names/#resource-resources + * @see https://getsentry.github.io/sentry-conventions/names/#web_server-request-handler */ -export const REQUEST_HANDLER_SPAN_NAME_FALLBACK = 'Request Handler'; +export const REQUEST_HANDLER_SPAN_NAME_FALLBACK = 'Request handler'; diff --git a/packages/core/test/lib/integrations/express/patch-layer.test.ts b/packages/core/test/lib/integrations/express/patch-layer.test.ts index de557a02dac8..cd56118fe869 100644 --- a/packages/core/test/lib/integrations/express/patch-layer.test.ts +++ b/packages/core/test/lib/integrations/express/patch-layer.test.ts @@ -542,6 +542,79 @@ describe('patchLayer', () => { ]); }); + it('names request handler spans after their route when span streaming is enabled', () => { + spanStreamingEnabled = true; + const options: ExpressPatchLayerOptions = {}; + const req = Object.assign(new EventEmitter(), { + originalUrl: '/a/b/c', + }) as unknown as ExpressRequest; + + const layer = { + name: 'handle', + handle: vi.fn(), + } as unknown as ExpressLayer; + + const res = Object.assign(new EventEmitter(), {}) as unknown as ExpressResponse; + + storeLayer(req, '/a'); + storeLayer(req, '/b'); + + patchLayer(() => options, layer, '/c'); + layer.handle(req, res); + + checkSpans([ + { + status: { code: 0, message: 'OK' }, + data: { + 'express.name': '/a/b/c', + 'express.type': 'request_handler', + 'http.route': '/a/b/c', + 'sentry.op': 'handler', + 'sentry.origin': 'auto.http.express', + }, + description: '/a/b/c', + }, + ]); + res.emit('finish'); + checkSpans([]); + }); + + it('falls back to a static request handler span name when the route is unknown', () => { + spanStreamingEnabled = true; + const options: ExpressPatchLayerOptions = {}; + const req = Object.assign(new EventEmitter(), { + originalUrl: '/abcdef', + }) as unknown as ExpressRequest; + + const layer = { + name: 'handle', + handle: vi.fn(), + } as unknown as ExpressLayer; + + const res = Object.assign(new EventEmitter(), {}) as unknown as ExpressResponse; + + storeLayer(req, '/a'); + storeLayer(req, '/b'); + + patchLayer(() => options, layer, '/c'); + layer.handle(req, res); + + checkSpans([ + { + status: { code: 0, message: 'OK' }, + data: { + 'express.name': '/a/b/c', + 'express.type': 'request_handler', + 'sentry.op': 'handler', + 'sentry.origin': 'auto.http.express', + }, + description: 'Request handler', + }, + ]); + res.emit('finish'); + checkSpans([]); + }); + it('handles case when route does not match url', () => { const onRouteResolved = vi.fn(); const options: ExpressPatchLayerOptions = { onRouteResolved }; diff --git a/packages/elysia/src/withElysia.ts b/packages/elysia/src/withElysia.ts index c58834cfa511..594ac1959cf6 100644 --- a/packages/elysia/src/withElysia.ts +++ b/packages/elysia/src/withElysia.ts @@ -1,13 +1,22 @@ -import { SENTRY_SEGMENT_NAME_SOURCE, HTTP_ROUTE, URL_FULL, URL_PATH } from '@sentry/conventions/attributes'; +import { + SENTRY_SEGMENT_NAME_SOURCE, + CODE_FUNCTION_NAME, + HTTP_ROUTE, + URL_FULL, + URL_PATH, +} from '@sentry/conventions/attributes'; import { MIDDLEWARE } from '@sentry/conventions/op'; import type { Span } from '@sentry/core'; import { captureException, continueTrace, getActiveSpan, + getClient, getIsolationScope, getRootSpan, getTraceData, + hasSpanStreamingEnabled, + REQUEST_HANDLER_SPAN_NAME_FALLBACK, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, setHttpStatus, @@ -20,6 +29,14 @@ import { } from '@sentry/core'; import type { AnyElysia, Elysia, ErrorContext, TraceHandler, TraceListener } from 'elysia'; +/** + * The part of Elysia's request context that the lifecycle spans read. Elysia types + * `.trace()`'s context as an index signature, which a required property would reject. + */ +interface LifecycleContext { + route?: string; +} + interface ElysiaHandlerOptions { shouldHandleError?: (context: ErrorContext) => boolean; } @@ -107,20 +124,38 @@ function defaultShouldHandleError(context: ErrorContext): boolean { * @param rootSpan - The root server span to parent lifecycle spans under. * Must be passed explicitly because Elysia's .trace() listener callbacks run * in a different async context where getActiveSpan() returns undefined. + * @param context - The request context. Read `route` off it inside the listener: + * Elysia assigns the route when the request enters the compiled handler, which + * is after `.trace()` hands out its listeners. */ -function instrumentLifecyclePhase(phaseName: string, listener: TraceListener, rootSpan: Span | undefined): void { +function instrumentLifecyclePhase( + phaseName: string, + listener: TraceListener, + rootSpan: Span | undefined, + context: LifecycleContext, +): void { const op = ELYSIA_LIFECYCLE_OP_MAP[phaseName]; if (!op) { return; } void listener(process => { + const client = getClient(); + const isRequestHandlerSpan = op === 'handler'; + // With span streaming, span names have to be low cardinality, so request handler + // spans are named after their route. + const isStreamedRequestHandlerSpan = isRequestHandlerSpan && !!client && hasSpanStreamingEnabled(client); + // The route describes the span in both trace lifecycles, and the other server + // integrations put it on their request handler spans too. + const routeAttribute = isRequestHandlerSpan && context.route ? { [HTTP_ROUTE]: context.route } : {}; + const phaseSpan = startInactiveSpan({ - name: phaseName, + name: isStreamedRequestHandlerSpan ? context.route || REQUEST_HANDLER_SPAN_NAME_FALLBACK : phaseName, parentSpan: rootSpan, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_OP]: op, [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ELYSIA_ORIGIN, + ...routeAttribute, }, }); @@ -130,11 +165,15 @@ function instrumentLifecyclePhase(phaseName: string, listener: TraceListener, ro void process.onEvent(child => { const handlerName = child.name || 'anonymous'; const childSpan = startInactiveSpan({ - name: handlerName, + name: isStreamedRequestHandlerSpan ? context.route || REQUEST_HANDLER_SPAN_NAME_FALLBACK : handlerName, parentSpan: phaseSpan, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_OP]: op, [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ELYSIA_ORIGIN, + ...routeAttribute, + // These spans are named after the route, so the handler name has no + // other place to go. Anonymous handlers have no name to record. + ...(isStreamedRequestHandlerSpan && child.name ? { [CODE_FUNCTION_NAME]: child.name } : {}), }, }); @@ -277,7 +316,7 @@ export function withElysia(app: T, options: ElysiaHandlerOp for (const [phaseName, listener] of phases) { if (listener) { - instrumentLifecyclePhase(phaseName, listener, rootSpan); + instrumentLifecyclePhase(phaseName, listener, rootSpan, lifecycle.context); } } }; diff --git a/packages/elysia/test/withElysia.test.ts b/packages/elysia/test/withElysia.test.ts index db42a65d3fe4..77bfd4fe7c3c 100644 --- a/packages/elysia/test/withElysia.test.ts +++ b/packages/elysia/test/withElysia.test.ts @@ -5,12 +5,16 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; // Capture handlers registered by withElysia let onAfterHandleHandler: (context: unknown) => void; let onErrorHandler: (context: unknown) => void; +let traceHandler: (lifecycle: unknown) => void; function createMockApp() { const app: Record = {}; app.use = vi.fn().mockReturnValue(app); app.wrap = vi.fn().mockReturnValue(app); - app.trace = vi.fn().mockReturnValue(app); + app.trace = vi.fn((_opts: unknown, handler: (lifecycle: unknown) => void) => { + traceHandler = handler; + return app; + }); app.onRequest = vi.fn(() => app); app.onAfterHandle = vi.fn((_opts: unknown, handler: (context: unknown) => void) => { onAfterHandleHandler = handler; @@ -30,9 +34,16 @@ const mockGetIsolationScope = vi.fn(() => ({ setSDKProcessingMetadata: vi.fn(), setTransactionName: vi.fn(), })); +let traceLifecycle: 'static' | 'stream' = 'stream'; const mockGetClient = vi.fn(() => ({ on: vi.fn(), + getOptions: () => ({ traceLifecycle }), })); +const startedSpans: { name: string; attributes?: Record }[] = []; +const mockStartInactiveSpan = vi.fn((options: { name: string; attributes?: Record }) => { + startedSpans.push({ name: options.name, attributes: options.attributes }); + return { end: vi.fn() }; +}); const mockRootSpan = { setAttribute: vi.fn(), setAttributes: vi.fn(), @@ -56,6 +67,8 @@ vi.mock('@sentry/core', async importActual => { getClient: () => mockGetClient(), getRootSpan: () => mockGetRootSpan(), getTraceData: () => mockGetTraceData(), + startInactiveSpan: (options: { name: string; attributes?: Record }) => + mockStartInactiveSpan(options), }; }); @@ -65,6 +78,8 @@ const { withElysia } = await import('../src/withElysia'); describe('withElysia', () => { beforeEach(() => { mockApp = createMockApp(); + startedSpans.length = 0; + traceLifecycle = 'stream'; }); afterEach(() => { @@ -185,6 +200,116 @@ describe('withElysia', () => { }); }); + describe('request handler span names', () => { + /** Drive the registered trace handler through a single `Handle` phase. */ + function runHandlePhase(handlerNames: string[], route = '/users/:id'): void { + traceHandler({ + context: { request: new Request('http://localhost/users/123'), route }, + onHandle: (callback: (process: unknown) => void) => { + callback({ + total: handlerNames.length, + onEvent: (onChild: (child: unknown) => void) => { + for (const name of handlerNames) { + onChild({ name, onStop: () => {} }); + } + }, + onStop: () => {}, + }); + }, + }); + } + + it('names the spans after the route when span streaming is enabled', () => { + // @ts-expect-error - mock app + withElysia(mockApp); + runHandlePhase(['getUser']); + + expect(startedSpans.map(span => span.name)).toEqual(['/users/:id', '/users/:id']); + }); + + it('uses the low cardinality fallback when the context carries no route', () => { + // @ts-expect-error - mock app + withElysia(mockApp); + runHandlePhase(['getUser'], ''); + + expect(startedSpans.map(span => span.name)).toEqual(['Request handler', 'Request handler']); + }); + + it('keeps the phase and handler names in static mode', () => { + traceLifecycle = 'static'; + // @ts-expect-error - mock app + withElysia(mockApp); + runHandlePhase(['getUser']); + + expect(startedSpans.map(span => span.name)).toEqual(['Handle', 'getUser']); + }); + + it('records the route on the handler spans in both trace lifecycles', () => { + // @ts-expect-error - mock app + withElysia(mockApp); + runHandlePhase(['getUser']); + + traceLifecycle = 'static'; + // @ts-expect-error - mock app + withElysia(createMockApp()); + runHandlePhase(['getUser']); + + expect(startedSpans).toHaveLength(4); + for (const span of startedSpans) { + expect(span.attributes).toMatchObject({ 'http.route': '/users/:id' }); + } + }); + + it('records no route when the context carries none', () => { + // @ts-expect-error - mock app + withElysia(mockApp); + runHandlePhase(['getUser'], ''); + + expect(startedSpans[0]?.attributes).not.toHaveProperty('http.route'); + expect(startedSpans[1]?.attributes).not.toHaveProperty('http.route'); + }); + + it('records no route on the spans of other lifecycle phases', () => { + // @ts-expect-error - mock app + withElysia(mockApp); + traceHandler({ + context: { request: new Request('http://localhost/users/123'), route: '/users/:id' }, + onRequest: (callback: (process: unknown) => void) => { + callback({ total: 0, onEvent: () => {}, onStop: () => {} }); + }, + }); + + expect(startedSpans).toHaveLength(1); + expect(startedSpans[0]?.attributes).toMatchObject({ 'sentry.op': 'middleware' }); + expect(startedSpans[0]?.attributes).not.toHaveProperty('http.route'); + }); + + it('records the handler name on the child span it renamed', () => { + // @ts-expect-error - mock app + withElysia(mockApp); + runHandlePhase(['getUser']); + + expect(startedSpans[1]?.attributes).toMatchObject({ 'code.function.name': 'getUser' }); + }); + + it('records no handler name for an anonymous handler', () => { + // @ts-expect-error - mock app + withElysia(mockApp); + runHandlePhase(['']); + + expect(startedSpans[1]?.attributes).not.toHaveProperty('code.function.name'); + }); + + it('records no handler name in static mode, where the span name still carries it', () => { + traceLifecycle = 'static'; + // @ts-expect-error - mock app + withElysia(mockApp); + runHandlePhase(['getUser']); + + expect(startedSpans[1]?.attributes).not.toHaveProperty('code.function.name'); + }); + }); + describe('custom shouldHandleError', () => { it('uses custom shouldHandleError when provided', () => { const customShouldHandle = vi.fn(() => false); diff --git a/packages/nestjs/src/integrations/wrap-route.ts b/packages/nestjs/src/integrations/wrap-route.ts index 906ba7172fc8..124cdc31b99d 100644 --- a/packages/nestjs/src/integrations/wrap-route.ts +++ b/packages/nestjs/src/integrations/wrap-route.ts @@ -1,7 +1,14 @@ import { HTTP_REQUEST_METHOD, HTTP_ROUTE, SENTRY_OP, URL_FULL } from '@sentry/conventions/attributes'; import { FUNCTION } from '@sentry/conventions/op'; import type { SpanAttributes } from '@sentry/core'; -import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, startSpan, filterCollectedUrl } from '@sentry/core'; +import { + getClient, + hasSpanStreamingEnabled, + REQUEST_HANDLER_SPAN_NAME_FALLBACK, + SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, + startSpan, + filterCollectedUrl, +} from '@sentry/core'; import type { AnyFn } from './helpers'; import { copyReflectMetadata, HTTP_ORIGIN, isWrapped, markWrapped } from './helpers'; import { AttributeNames, NestType } from './enums'; @@ -62,7 +69,17 @@ export function wrapRouteHandler(callback: AnyFn, moduleVersion?: string): AnyFn [AttributeNames.VERSION]: moduleVersion || undefined, }; const wrapped = function (this: unknown, ...args: unknown[]): unknown { - return startSpan({ name: spanName, attributes }, () => callback.apply(this, args)); + const client = getClient(); + // With span streaming, span names have to be low cardinality. This wrapper + // sees only the controller method, not the request, so it has no route to + // name the span after and takes the static fallback. The enclosing + // request-context span carries `http.route`, and the callback name stays on + // the `nestjs.callback` attribute. + const isStreamedSpan = !!client && hasSpanStreamingEnabled(client); + + return startSpan({ name: isStreamedSpan ? REQUEST_HANDLER_SPAN_NAME_FALLBACK : spanName, attributes }, () => + callback.apply(this, args), + ); }; if (callback.name) { Object.defineProperty(wrapped, 'name', { value: callback.name }); diff --git a/packages/nestjs/test/integrations/orchestrion-subscriber.test.ts b/packages/nestjs/test/integrations/orchestrion-subscriber.test.ts index 93d42ededb4a..ee519452ebb5 100644 --- a/packages/nestjs/test/integrations/orchestrion-subscriber.test.ts +++ b/packages/nestjs/test/integrations/orchestrion-subscriber.test.ts @@ -36,7 +36,7 @@ class TestClient extends Client { } } -function initTestClient(): void { +function initTestClient(options: { traceLifecycle?: 'static' | 'stream' } = {}): void { //@ts-expect-error - just a mock for the test, this is fine initAndBind(TestClient, { dsn: 'https://username@domain/123', @@ -45,6 +45,7 @@ function initTestClient(): void { stackParser: () => [], tracesSampleRate: 1, transport: () => createTransport({ recordDroppedEvent: () => undefined }, () => resolvedSyncPromise({})), + ...options, }); } @@ -268,7 +269,9 @@ describe('NestJS orchestrion subscriber: request_context / request_handler', () wrappedCallback.call(instance); expect(handlerSpanJson).toBeDefined(); - expect(handlerSpanJson!.name).toBe('getCats'); + // With span streaming, the span name is low cardinality and the callback name + // only stays on the `nestjs.callback` attribute. + expect(handlerSpanJson!.name).toBe('Request handler'); expect(handlerSpanJson!.attributes['sentry.op']).toBe('handler'); expect(handlerSpanJson!.attributes['sentry.origin']).toBe('auto.http.nestjs'); expect(handlerSpanJson!.attributes).toMatchObject({ @@ -279,6 +282,25 @@ describe('NestJS orchestrion subscriber: request_context / request_handler', () }); }); + it('names the request_handler span after the callback in static mode', () => { + installTestAsyncContextStrategy(); + initTestClient({ traceLifecycle: 'static' }); + subscribeToNestChannels(); + + class CatsController {} + const instance = new CatsController(); + let handlerSpanJson: ReturnType | undefined; + function getCats(): string { + handlerSpanJson = spanToJSON(getActiveSpan()!); + return 'cats'; + } + + const { wrappedCallback } = driveCreate(instance, getCats, '10.4.1', () => () => undefined); + wrappedCallback.call(instance); + + expect(handlerSpanJson!.name).toBe('getCats'); + }); + it('nests the request_handler span under the request_context span', () => { installTestAsyncContextStrategy(); initTestClient(); diff --git a/packages/server-utils/src/integrations/express/instrumentation.ts b/packages/server-utils/src/integrations/express/instrumentation.ts index bb1991a659e0..bc13bc4cf13b 100644 --- a/packages/server-utils/src/integrations/express/instrumentation.ts +++ b/packages/server-utils/src/integrations/express/instrumentation.ts @@ -9,6 +9,7 @@ import { getDefaultIsolationScope, getIsolationScope, hasSpanStreamingEnabled, + REQUEST_HANDLER_SPAN_NAME_FALLBACK, ROUTER_SPAN_NAME_FALLBACK, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, startInactiveSpan, @@ -228,11 +229,20 @@ function getSpanForLayer(data: HandleChannelContext, options: ExpressIntegration } const client = getClient(); - // With span streaming, span names have to be low cardinality, so router spans are named after their route. - const isStreamedRouterSpan = type === 'router' && !!client && hasSpanStreamingEnabled(client); + // With span streaming, span names have to be low cardinality, so router + // and request handler spans are named after their route. A route that did + // not validate against the request URL can describe a different request, + // so those spans take the static fallback instead. + const isStreamedSpan = !!client && hasSpanStreamingEnabled(client); + const isStreamedRouterSpan = isStreamedSpan && type === 'router'; + const isStreamedRequestHandlerSpan = isStreamedSpan && type === 'request_handler'; const span = startInactiveSpan({ - name: isStreamedRouterSpan ? matchedRoute || ROUTER_SPAN_NAME_FALLBACK : name, + name: isStreamedRouterSpan + ? matchedRoute || ROUTER_SPAN_NAME_FALLBACK + : isStreamedRequestHandlerSpan + ? matchedRoute || REQUEST_HANDLER_SPAN_NAME_FALLBACK + : name, attributes: { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN, [SENTRY_OP]: EXPRESS_TYPE_TO_SPAN_OP[type], diff --git a/packages/server-utils/src/integrations/fastify/instrumentation.ts b/packages/server-utils/src/integrations/fastify/instrumentation.ts index bf8f55ac76b8..87e0630e1102 100644 --- a/packages/server-utils/src/integrations/fastify/instrumentation.ts +++ b/packages/server-utils/src/integrations/fastify/instrumentation.ts @@ -27,7 +27,10 @@ import type { Span } from '@sentry/core'; import { isObjectLike, debug, + getClient, getIsolationScope, + hasSpanStreamingEnabled, + REQUEST_HANDLER_SPAN_NAME_FALLBACK, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SPAN_STATUS_ERROR, startInactiveSpan, @@ -196,8 +199,17 @@ function startRequestSpanHook(this: any, request: any, _reply: any, hookDone: () setHttpServerSpanRouteAttribute(route); } + const client = getClient(); + // With span streaming, span names have to be low cardinality, so request handler + // spans are named after their route alone, without the method prefix. + const isStreamedSpan = !!client && hasSpanStreamingEnabled(client); + const requestSpan = startInactiveSpan({ - name: route != null ? `${request.method} ${route}` : 'request', + name: isStreamedSpan + ? route || REQUEST_HANDLER_SPAN_NAME_FALLBACK + : route != null + ? `${request.method} ${route}` + : 'request', attributes, }); request[kRequestSpan] = requestSpan; @@ -347,7 +359,16 @@ function handlerWrapper(handler: AnyFn, hookName: string, spanAttributes: Record const op = hookType === HOOK_TYPE_INSTANCE ? HOOK_OP : hookType === HOOK_TYPE_HANDLER ? REQUEST_HANDLER_OP : undefined; - const name = op ? stripFastifyPrefix(spanAttributes[ATTRIBUTE_HOOK_NAME]) : `${hookName} - ${handlerName}`; + const client = getClient(); + // With span streaming, span names have to be low cardinality, so request handler + // spans are named after their route. + const isStreamedRequestHandlerSpan = hookType === HOOK_TYPE_HANDLER && !!client && hasSpanStreamingEnabled(client); + + const name = isStreamedRequestHandlerSpan + ? spanAttributes[HTTP_ROUTE] || REQUEST_HANDLER_SPAN_NAME_FALLBACK + : op + ? stripFastifyPrefix(spanAttributes[ATTRIBUTE_HOOK_NAME]) + : `${hookName} - ${handlerName}`; return startSpan( { diff --git a/packages/server-utils/src/integrations/hapi/hapi-utils.ts b/packages/server-utils/src/integrations/hapi/hapi-utils.ts index f4ea3a6d43fe..69f445738035 100644 --- a/packages/server-utils/src/integrations/hapi/hapi-utils.ts +++ b/packages/server-utils/src/integrations/hapi/hapi-utils.ts @@ -14,6 +14,7 @@ import { getClient, hasSpanStreamingEnabled, isObjectLike, + REQUEST_HANDLER_SPAN_NAME_FALLBACK, ROUTER_SPAN_NAME_FALLBACK, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, startSpan, @@ -122,15 +123,14 @@ export const getRouteMetadata = (route: ServerRoute, pluginName?: string): SpanM } const client = getClient(); - // With span streaming, span names have to be low cardinality, so router spans are named after their - // route alone, without the method prefix. - const isStreamedRouterSpan = !pluginName && !!client && hasSpanStreamingEnabled(client); + // With span streaming, span names have to be low cardinality, so router and request + // handler spans are named after their route alone, without the method prefix. + const isStreamedSpan = !!client && hasSpanStreamingEnabled(client); + const fallbackName = pluginName ? REQUEST_HANDLER_SPAN_NAME_FALLBACK : ROUTER_SPAN_NAME_FALLBACK; return { attributes, - name: isStreamedRouterSpan - ? route.path || ROUTER_SPAN_NAME_FALLBACK - : `${route.method.toUpperCase()} ${route.path}`, + name: isStreamedSpan ? route.path || fallbackName : `${route.method.toUpperCase()} ${route.path}`, }; }; diff --git a/packages/server-utils/test/integrations/express/instrumentation.test.ts b/packages/server-utils/test/integrations/express/instrumentation.test.ts new file mode 100644 index 000000000000..20751d7382dc --- /dev/null +++ b/packages/server-utils/test/integrations/express/instrumentation.test.ts @@ -0,0 +1,186 @@ +import { AsyncLocalStorage } from 'node:async_hooks'; +import { tracingChannel } from 'node:diagnostics_channel'; +import type { Scope, Span } from '@sentry/core'; +import { + _INTERNAL_setSpanForScope, + Client, + createTransport, + getActiveSpan, + getAsyncContextStrategy, + getDefaultCurrentScope, + getDefaultIsolationScope, + getMainCarrier, + initAndBind, + resolvedSyncPromise, + setAsyncContextStrategy, + spanToJSON, + startSpan, +} from '@sentry/core'; +import { afterAll, afterEach, beforeAll, describe, expect, it } from 'vitest'; +import { expressChannels } from '../../../src/orchestrion/config/express'; +import { instrumentExpress } from '../../../src/integrations/express/instrumentation'; + +interface TestStore { + scope: Scope; + isolationScope: Scope; +} + +class TestClient extends Client { + public eventFromException(): PromiseLike { + return resolvedSyncPromise({}); + } + public eventFromMessage(): PromiseLike { + return resolvedSyncPromise({}); + } +} + +function initTestClient(options: { traceLifecycle?: 'static' | 'stream' } = {}): void { + //@ts-expect-error - just a mock for the test, this is fine + initAndBind(TestClient, { + dsn: 'https://username@domain/123', + integrations: [], + sendClientReports: false, + stackParser: () => [], + tracesSampleRate: 1, + transport: () => createTransport({ recordDroppedEvent: () => undefined }, () => resolvedSyncPromise({})), + ...options, + }); +} + +function installTestAsyncContextStrategy(): void { + const asyncStorage = new AsyncLocalStorage(); + + function getScopes(): TestStore { + return ( + asyncStorage.getStore() || { + scope: getDefaultCurrentScope(), + isolationScope: getDefaultIsolationScope(), + } + ); + } + + setAsyncContextStrategy({ + withScope: callback => { + const scope = getScopes().scope.clone(); + const isolationScope = getScopes().isolationScope; + return asyncStorage.run({ scope, isolationScope }, () => callback(scope)); + }, + withSetScope: (scope, callback) => { + const isolationScope = getScopes().isolationScope; + return asyncStorage.run({ scope, isolationScope }, () => callback(scope)); + }, + withIsolationScope: callback => { + const scope = getScopes().scope; + const isolationScope = getScopes().isolationScope.clone(); + return asyncStorage.run({ scope, isolationScope }, () => callback(isolationScope)); + }, + withSetIsolationScope: (isolationScope, callback) => { + const scope = getScopes().scope; + return asyncStorage.run({ scope, isolationScope }, () => callback(isolationScope)); + }, + getCurrentScope: () => getScopes().scope, + getIsolationScope: () => getScopes().isolationScope, + getTracingChannelBinding: () => ({ + asyncLocalStorage: asyncStorage, + getStoreWithActiveSpan: (span: Span) => { + const scope = getScopes().scope.clone(); + const isolationScope = getScopes().isolationScope; + _INTERNAL_setSpanForScope(scope, span); + return { scope, isolationScope }; + }, + }), + }); +} + +/** A request whose response never finishes, so only `next()` ends the layer span. */ +function createRequest(originalUrl: string): unknown { + return { method: 'GET', originalUrl }; +} + +const RESPONSE = { + once: () => undefined, + removeListener: () => undefined, +}; + +/** + * Drive one route-dispatch layer through the register and handle channels, the + * way orchestrion's transform does, and return the span it opened. + */ +function handleRouteLayer(registeredPath: string, originalUrl: string): ReturnType | undefined { + // `bound dispatch` is the Express v4 route-dispatch layer, which maps to the + // `request_handler` layer type. + const layer = { name: 'bound dispatch', route: { path: registeredPath }, handle: { length: 3 } }; + + tracingChannel(expressChannels.EXPRESS_REGISTER).traceSync(() => undefined, { + self: { stack: [layer] }, + arguments: [registeredPath], + }); + + let json: ReturnType | undefined; + + startSpan({ name: 'GET /' }, () => { + tracingChannel(expressChannels.EXPRESS_HANDLE).traceSync( + () => { + const span = getActiveSpan(); + json = span ? spanToJSON(span) : undefined; + }, + { self: layer, arguments: [createRequest(originalUrl), RESPONSE, () => undefined] }, + ); + }); + + return json; +} + +describe('instrumentExpress request handler span names', () => { + // The subscriber captures the async-context strategy's ALS when it binds, and + // `instrumentExpress` only subscribes once per module instance, so both happen + // once for the file. Only the client varies per test. + beforeAll(() => { + installTestAsyncContextStrategy(); + instrumentExpress({}, tracingChannel); + }); + + afterAll(() => { + setAsyncContextStrategy(undefined); + }); + + afterEach(() => { + // Keep the strategy the subscriber bound to; wiping it would strand its ALS. + const acs = getAsyncContextStrategy(getMainCarrier()); + getMainCarrier().__SENTRY__ = undefined; + setAsyncContextStrategy(acs); + }); + + it('names the span after the matched route when span streaming is enabled', () => { + initTestClient(); + + const json = handleRouteLayer('/users/:id', '/users/123'); + + expect(json?.name).toBe('/users/:id'); + expect(json?.attributes).toMatchObject({ + 'sentry.op': 'handler', + 'sentry.origin': 'auto.http.express', + 'http.route': '/users/:id', + 'express.type': 'request_handler', + }); + }); + + it('falls back to a static span name when the route does not match the url', () => { + initTestClient(); + + const json = handleRouteLayer('/users', '/other'); + + expect(json?.name).toBe('Request handler'); + // The constructed route was not validated against the URL, so it is not the + // span's `http.route` either. + expect(json?.attributes['http.route']).toBeUndefined(); + }); + + it('keeps the constructed route as the span name in static mode', () => { + initTestClient({ traceLifecycle: 'static' }); + + const json = handleRouteLayer('/users', '/other'); + + expect(json?.name).toBe('/users'); + }); +}); diff --git a/packages/server-utils/test/integrations/hapi-utils.test.ts b/packages/server-utils/test/integrations/hapi-utils.test.ts index 9eec9f183c46..a0a7a43971fa 100644 --- a/packages/server-utils/test/integrations/hapi-utils.test.ts +++ b/packages/server-utils/test/integrations/hapi-utils.test.ts @@ -40,11 +40,21 @@ describe('getRouteMetadata', () => { expect(getRouteMetadata(route).name).toBe('/users/{id}'); }); - it('keeps the plugin span name when span streaming is enabled', () => { + it('drops the method from the plugin span name when span streaming is enabled', () => { const client = new TestClient(getDefaultTestClientOptions({ traceLifecycle: 'stream' })); setCurrentClient(client); - expect(getRouteMetadata(route, 'my-plugin').name).toBe('GET /users/{id}'); + expect(getRouteMetadata(route, 'my-plugin').name).toBe('/users/{id}'); + }); + + it('falls back to a static span name when the route has no path', () => { + const client = new TestClient(getDefaultTestClientOptions({ traceLifecycle: 'stream' })); + setCurrentClient(client); + + const pathlessRoute = { path: '', method: 'get' } as any; + + expect(getRouteMetadata(pathlessRoute).name).toBe('Router'); + expect(getRouteMetadata(pathlessRoute, 'my-plugin').name).toBe('Request handler'); }); });