From 37816769b12705fd0563e56ce45e66e7e0c0ec46 Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 25 Aug 2026 17:31:18 -0700 Subject: [PATCH 1/2] 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 | 19 +- packages/core/src/tracing/spans/spanNames.ts | 4 +- .../integrations/express/patch-layer.test.ts | 73 +++++++ packages/elysia/src/withElysia.ts | 50 ++++- packages/elysia/test/withElysia.test.ts | 127 +++++++++++- .../nestjs/src/integrations/wrap-route.ts | 21 +- .../orchestrion-subscriber.test.ts | 23 ++- .../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, 617 insertions(+), 30 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 8e1990930207..8ec5a2552bb6 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -912,6 +912,7 @@ The following span names were adjusted: | `http.server` | The request method and route, or the raw URL path if the SDK couldn't resolve one (`GET /users/123`) | `GET /users/:id` when a route is known, otherwise just the request method (`GET`) | | `http.client`, `http.client.stream` | The request method and sanitized URL (`GET https://api.example.com/users/123`) | The request method and the domain (`GET api.example.com`), or just the method if there is no domain (`GET`) | | `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`) | | `gen_ai.chat`, `gen_ai.embeddings`, `gen_ai.generate_content` | `{operation} {model}`, or `{operation} unknown` if the model is missing (`chat unknown`) | `{operation} {model}`, or `{operation}` if the model is missing (`chat`) | | `gen_ai.invoke_agent` | The LangChain chain name, prefixed with `chain` rather than the operation (`chain format_prompt`) | `{operation} {name}`, where the name is the span's `gen_ai.agent.name`, `gen_ai.pipeline.name` or `gen_ai.function_id`, in that order (`invoke_agent format_prompt`), or `{operation}` if the span carries none | @@ -950,6 +951,8 @@ Because the URL path is gone from `http.client` names, `graphqlClientIntegration 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. + Messaging span names now read ` ` in every integration. The amqplib, kafkajs and NestJS BullMQ integrations used their own word order or verb, so their names change: `my-queue process` became `process my-queue`, amqplib's `publish` became `send`, and the kafkajs batch span's `poll` became `receive`. Cloudflare Queues and the kafkajs producer already matched the conventions, so their names are the same in both trace lifecycles. The operation name an integration reports upstream stays on `messaging.operation.name`. AWS SQS `SendMessage`, `SendMessageBatch` and `ReceiveMessage`, and SNS `Publish`, are messaging spans (e.g. `queue.publish`) rather than `rpc` ones now. Every other command on those clients, such as `DeleteMessage`, stays `rpc`. Their names follow the messaging conventions too, so the operation comes first (`my-queue receive` becomes `receive my-queue`, `my-topic send` becomes `send my-topic`). A streamed SNS `Publish` to a platform endpoint is named `send`, because the endpoint ARN it used to carry ends in a per-device id (`endpoint/GCM/myapp/ send`). The full ARN remains on `messaging.destination.name`. 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 d30aed69a967..7c23c2e11654 100644 --- a/packages/core/src/integrations/express/patch-layer.ts +++ b/packages/core/src/integrations/express/patch-layer.ts @@ -43,7 +43,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'; @@ -184,10 +184,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); - - const spanName = isStreamedRouterSpan ? actualMatchedRoute || ROUTER_SPAN_NAME_FALLBACK : name; + // 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 + : 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 6d3862a9856b..f2460a75d9c3 100644 --- a/packages/core/src/tracing/spans/spanNames.ts +++ b/packages/core/src/tracing/spans/spanNames.ts @@ -77,6 +77,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 63b42ac3ec7b..926ac08040b5 100644 --- a/packages/core/test/lib/integrations/express/patch-layer.test.ts +++ b/packages/core/test/lib/integrations/express/patch-layer.test.ts @@ -649,6 +649,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 3032f5af06ad..48ba27b60ee8 100644 --- a/packages/elysia/src/withElysia.ts +++ b/packages/elysia/src/withElysia.ts @@ -1,4 +1,11 @@ -import { HTTP_ROUTE, SENTRY_OP, SENTRY_SEGMENT_NAME_SOURCE, URL_FULL, URL_PATH } from '@sentry/conventions/attributes'; +import { + CODE_FUNCTION_NAME, + HTTP_ROUTE, + SENTRY_OP, + SENTRY_SEGMENT_NAME_SOURCE, + URL_FULL, + URL_PATH, +} from '@sentry/conventions/attributes'; import { HANDLER, HTTP_SERVER, MIDDLEWARE } from '@sentry/conventions/op'; import type { Span } from '@sentry/core'; import { @@ -9,6 +16,8 @@ import { getIsolationScope, getRootSpan, getTraceData, + hasSpanStreamingEnabled, + REQUEST_HANDLER_SPAN_NAME_FALLBACK, SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, setHttpStatus, @@ -18,11 +27,18 @@ import { winterCGRequestToRequestData, withIsolationScope, filterCollectedUrl, - hasSpanStreamingEnabled, HTTP_SPAN_NAME_FALLBACK, } 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; } @@ -109,20 +125,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, }, }); @@ -132,11 +166,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 } : {}), }, }); @@ -285,7 +323,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 212643ed1dd0..10971ab25441 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, HANDLER } 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'; @@ -61,7 +68,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 fef86fbfedd4..0a75619770ad 100644 --- a/packages/nestjs/test/integrations/orchestrion-subscriber.test.ts +++ b/packages/nestjs/test/integrations/orchestrion-subscriber.test.ts @@ -269,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({ @@ -280,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('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 ba3fc8d7c977..5dd568b2efe5 100644 --- a/packages/server-utils/src/integrations/express/instrumentation.ts +++ b/packages/server-utils/src/integrations/express/instrumentation.ts @@ -10,6 +10,7 @@ import { getDefaultIsolationScope, getIsolationScope, hasSpanStreamingEnabled, + REQUEST_HANDLER_SPAN_NAME_FALLBACK, ROUTER_SPAN_NAME_FALLBACK, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, startInactiveSpan, @@ -301,11 +302,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 860e99ac9a15..387d10a10a3a 100644 --- a/packages/server-utils/src/integrations/fastify/instrumentation.ts +++ b/packages/server-utils/src/integrations/fastify/instrumentation.ts @@ -26,7 +26,10 @@ import { HANDLER, MIDDLEWARE } from '@sentry/conventions/op'; import type { Span } from '@sentry/core'; import { isObjectLike, + getClient, getIsolationScope, + hasSpanStreamingEnabled, + REQUEST_HANDLER_SPAN_NAME_FALLBACK, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, SPAN_STATUS_ERROR, startInactiveSpan, @@ -178,8 +181,17 @@ function onRequest(this: any, request: any, _reply: any, hookDone: () => void): 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; @@ -328,9 +340,18 @@ function handlerWrapper(handler: AnyFn, hookName: string, spanAttributes: Record const hookType = spanAttributes[ATTRIBUTE_FASTIFY_TYPE]; const op = hookType === HOOK_TYPE_INSTANCE ? MIDDLEWARE : hookType === HOOK_TYPE_HANDLER ? HANDLER : undefined; + 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 attributeHookName = spanAttributes[ATTRIBUTE_HOOK_NAME]; - const name = op && typeof attributeHookName === 'string' ? attributeHookName : `${hookName} - ${handlerName}`; + const name = isStreamedRequestHandlerSpan + ? spanAttributes[HTTP_ROUTE] || REQUEST_HANDLER_SPAN_NAME_FALLBACK + : op && typeof attributeHookName === 'string' + ? attributeHookName + : `${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 6f418327d613..21c27d91eec5 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'); }); }); From b0193e4a3d90c072f205de28818d4a923e0e461e Mon Sep 17 00:00:00 2001 From: isaacs Date: Tue, 1 Sep 2026 08:06:20 -0700 Subject: [PATCH 2/2] fixup! feat(core): emit low cardinality request handler span names --- MIGRATION.md | 2 +- .../suites/express/tracing/instrument.mjs | 2 +- .../suites/express/tracing/test.ts | 31 +++ .../tracing/fastify-streamed/instrument.mjs | 10 - .../tracing/fastify-streamed/scenario.mjs | 28 --- .../suites/tracing/fastify-streamed/test.ts | 36 ---- .../suites/tracing/fastify/instrument.mjs | 2 +- .../suites/tracing/fastify/test.ts | 26 +++ .../suites/tracing/hapi/instrument.mjs | 2 +- .../suites/tracing/hapi/test.ts | 24 +++ packages/elysia/src/withElysia.ts | 7 +- packages/elysia/test/withElysia.test.ts | 17 +- .../express/instrumentation.test.ts | 186 ------------------ 13 files changed, 96 insertions(+), 277 deletions(-) delete mode 100644 dev-packages/node-integration-tests/suites/tracing/fastify-streamed/instrument.mjs delete mode 100644 dev-packages/node-integration-tests/suites/tracing/fastify-streamed/scenario.mjs delete mode 100644 dev-packages/node-integration-tests/suites/tracing/fastify-streamed/test.ts delete mode 100644 packages/server-utils/test/integrations/express/instrumentation.test.ts diff --git a/MIGRATION.md b/MIGRATION.md index 8ec5a2552bb6..44f29afe32c4 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -951,7 +951,7 @@ Because the URL path is gone from `http.client` names, `graphqlClientIntegration 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. +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 its handler spans. Elysia request handler spans also carry `http.route` now. Both attributes are set in both trace lifecycles. Messaging span names now read ` ` in every integration. The amqplib, kafkajs and NestJS BullMQ integrations used their own word order or verb, so their names change: `my-queue process` became `process my-queue`, amqplib's `publish` became `send`, and the kafkajs batch span's `poll` became `receive`. Cloudflare Queues and the kafkajs producer already matched the conventions, so their names are the same in both trace lifecycles. The operation name an integration reports upstream stays on `messaging.operation.name`. diff --git a/dev-packages/node-integration-tests/suites/express/tracing/instrument.mjs b/dev-packages/node-integration-tests/suites/express/tracing/instrument.mjs index c727f3046e61..2a81e8aef848 100644 --- a/dev-packages/node-integration-tests/suites/express/tracing/instrument.mjs +++ b/dev-packages/node-integration-tests/suites/express/tracing/instrument.mjs @@ -2,7 +2,7 @@ import * as Sentry from '@sentry/node'; import { loggingTransport } from '@sentry-internal/node-integration-tests'; Sentry.init({ - traceLifecycle: 'static', + traceLifecycle: process.env.STREAMED === 'true' ? 'stream' : 'static', dsn: 'https://public@dsn.ingest.sentry.io/1337', release: '1.0', // disable attaching headers to /test/* endpoints diff --git a/dev-packages/node-integration-tests/suites/express/tracing/test.ts b/dev-packages/node-integration-tests/suites/express/tracing/test.ts index 765a88c8dcea..8f86f80bbd04 100644 --- a/dev-packages/node-integration-tests/suites/express/tracing/test.ts +++ b/dev-packages/node-integration-tests/suites/express/tracing/test.ts @@ -51,6 +51,37 @@ describe('express tracing', () => { await runner.completed(); }); + test('names router and request handler spans after their route when span streaming is enabled', async () => { + const runner = createRunner() + .withEnv({ STREAMED: 'true' }) + .expect({ + span: container => { + const spanFor = (type: string): (typeof container.items)[number] | undefined => + container.items.find(item => item.attributes['express.type']?.value === type); + + const handlerSpan = spanFor('request_handler'); + expect(handlerSpan?.name).toBe('/test/router/user/:id'); + // The name has to stay in step with the attribute it comes from. + expect(handlerSpan?.attributes['http.route']?.value).toBe('/test/router/user/:id'); + expect(handlerSpan?.attributes['sentry.op']?.value).toBe('handler'); + + const routerSpan = spanFor('router'); + expect(routerSpan?.name).toBe('/test/router/user'); + + // Spans of other layer types keep their names. + expect(container.items.find(item => item.name === 'corsMiddleware')?.attributes['express.type']).toEqual({ + type: 'string', + value: 'middleware', + }); + }, + }) + .start(); + + await runner.makeRequest('get', '/test/router/user/123'); + + await runner.completed(); + }); + test('should set a correct transaction name for routes specified in RegEx', async () => { const runner = createRunner() .expect({ 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 deleted file mode 100644 index 53b9511a21f0..000000000000 --- a/dev-packages/node-integration-tests/suites/tracing/fastify-streamed/instrument.mjs +++ /dev/null @@ -1,10 +0,0 @@ -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 deleted file mode 100644 index 08e8fb2faad2..000000000000 --- a/dev-packages/node-integration-tests/suites/tracing/fastify-streamed/scenario.mjs +++ /dev/null @@ -1,28 +0,0 @@ -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 deleted file mode 100644 index 44b34a94a936..000000000000 --- a/dev-packages/node-integration-tests/suites/tracing/fastify-streamed/test.ts +++ /dev/null @@ -1,36 +0,0 @@ -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/dev-packages/node-integration-tests/suites/tracing/fastify/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/fastify/instrument.mjs index 170ad6f6a702..22bf57f14364 100644 --- a/dev-packages/node-integration-tests/suites/tracing/fastify/instrument.mjs +++ b/dev-packages/node-integration-tests/suites/tracing/fastify/instrument.mjs @@ -2,7 +2,7 @@ import * as Sentry from '@sentry/node'; import { loggingTransport } from '@sentry-internal/node-integration-tests'; Sentry.init({ - traceLifecycle: 'static', + traceLifecycle: process.env.STREAMED === 'true' ? 'stream' : 'static', dsn: 'https://public@dsn.ingest.sentry.io/1337', release: '1.0', tracesSampleRate: 1.0, diff --git a/dev-packages/node-integration-tests/suites/tracing/fastify/test.ts b/dev-packages/node-integration-tests/suites/tracing/fastify/test.ts index 863f8de184c1..847aad7dc50c 100644 --- a/dev-packages/node-integration-tests/suites/tracing/fastify/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/fastify/test.ts @@ -57,6 +57,32 @@ describe('fastify v5 auto-instrumentation', () => { await runner.completed(); }); + test('names request handler spans after their route when span streaming is enabled', async () => { + const runner = createRunner() + .withEnv({ STREAMED: 'true' }) + .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'); + // The name has to stay in step with the attribute it comes from. + expect(span.attributes['http.route']?.value).toBe('/test-transaction'); + } + + // Spans of other ops keep their names. + expect(container.items.find(item => item.name === 'preHandler - routePreHandler')).toBeDefined(); + }, + }) + .start(); + + await runner.makeRequest('get', '/test-transaction'); + + await runner.completed(); + }); + test('captures errors thrown in route handlers', async () => { const runner = createRunner() .ignore('transaction') diff --git a/dev-packages/node-integration-tests/suites/tracing/hapi/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/hapi/instrument.mjs index 170ad6f6a702..22bf57f14364 100644 --- a/dev-packages/node-integration-tests/suites/tracing/hapi/instrument.mjs +++ b/dev-packages/node-integration-tests/suites/tracing/hapi/instrument.mjs @@ -2,7 +2,7 @@ import * as Sentry from '@sentry/node'; import { loggingTransport } from '@sentry-internal/node-integration-tests'; Sentry.init({ - traceLifecycle: 'static', + traceLifecycle: process.env.STREAMED === 'true' ? 'stream' : 'static', dsn: 'https://public@dsn.ingest.sentry.io/1337', release: '1.0', tracesSampleRate: 1.0, diff --git a/dev-packages/node-integration-tests/suites/tracing/hapi/test.ts b/dev-packages/node-integration-tests/suites/tracing/hapi/test.ts index 057abb980951..e6d0f3369c3a 100644 --- a/dev-packages/node-integration-tests/suites/tracing/hapi/test.ts +++ b/dev-packages/node-integration-tests/suites/tracing/hapi/test.ts @@ -82,6 +82,30 @@ describe('hapi auto-instrumentation', () => { await runner.completed(); }); + test('names request handler spans after their route when span streaming is enabled', async () => { + const runner = createRunner() + .withEnv({ STREAMED: 'true' }) + .expect({ + span: container => { + const handlerSpan = container.items.find(item => item.attributes['sentry.op']?.value === 'handler'); + + // The route alone, without the `GET ` prefix the static name carries. + expect(handlerSpan?.name).toBe('/plugin-route'); + // The name has to stay in step with the attribute it comes from. + expect(handlerSpan?.attributes['http.route']?.value).toBe('/plugin-route'); + expect(handlerSpan?.attributes['hapi.type']?.value).toBe('plugin'); + + // Spans of other ops keep their names. + expect(container.items.find(item => item.name === 'ext - onPreResponse')).toBeDefined(); + }, + }) + .start(); + + await runner.makeRequest('get', '/plugin-route'); + + await runner.completed(); + }); + test('should handle returned plain errors in routes.', async () => { const runner = createRunner() .expect({ diff --git a/packages/elysia/src/withElysia.ts b/packages/elysia/src/withElysia.ts index 48ba27b60ee8..c68c192e3fa4 100644 --- a/packages/elysia/src/withElysia.ts +++ b/packages/elysia/src/withElysia.ts @@ -172,9 +172,10 @@ function instrumentLifecyclePhase( [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 } : {}), + // Streamed request handler spans are named after the route, so the + // handler name has no other place to go. Anonymous handlers have no + // name to record. + ...(isRequestHandlerSpan && child.name ? { [CODE_FUNCTION_NAME]: child.name } : {}), }, }); diff --git a/packages/elysia/test/withElysia.test.ts b/packages/elysia/test/withElysia.test.ts index 77bfd4fe7c3c..960372751b49 100644 --- a/packages/elysia/test/withElysia.test.ts +++ b/packages/elysia/test/withElysia.test.ts @@ -284,12 +284,18 @@ describe('withElysia', () => { expect(startedSpans[0]?.attributes).not.toHaveProperty('http.route'); }); - it('records the handler name on the child span it renamed', () => { + it('records the handler name on the handler child span 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[1]?.attributes).toMatchObject({ 'code.function.name': 'getUser' }); + expect(startedSpans[3]?.attributes).toMatchObject({ 'code.function.name': 'getUser' }); }); it('records no handler name for an anonymous handler', () => { @@ -299,15 +305,6 @@ describe('withElysia', () => { 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', () => { diff --git a/packages/server-utils/test/integrations/express/instrumentation.test.ts b/packages/server-utils/test/integrations/express/instrumentation.test.ts deleted file mode 100644 index 20751d7382dc..000000000000 --- a/packages/server-utils/test/integrations/express/instrumentation.test.ts +++ /dev/null @@ -1,186 +0,0 @@ -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'); - }); -});