diff --git a/MIGRATION.md b/MIGRATION.md index f2af8d53b6f5..61d564033e84 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -627,6 +627,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 | @@ -642,6 +643,8 @@ For the same reason, `useOperationNameForRootSpan` no longer renames the enclosi 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 f8c3fd086d1d..28cc6b61b743 100644 --- a/packages/core/src/tracing/spans/spanNames.ts +++ b/packages/core/src/tracing/spans/spanNames.ts @@ -76,6 +76,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 a16f8a744caf..9829b5d66e5d 100644 --- a/packages/nestjs/src/integrations/wrap-route.ts +++ b/packages/nestjs/src/integrations/wrap-route.ts @@ -1,7 +1,14 @@ import { HTTP_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 d683cef828a8..be2c33230587 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 b38893e0cec9..c6eb26102f20 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-utils.ts b/packages/server-utils/src/integrations/hapi-utils.ts index dd67a6f36e89..ece12ab518de 100644 --- a/packages/server-utils/src/integrations/hapi-utils.ts +++ b/packages/server-utils/src/integrations/hapi-utils.ts @@ -13,6 +13,7 @@ import { getActiveSpan, getClient, hasSpanStreamingEnabled, + REQUEST_HANDLER_SPAN_NAME_FALLBACK, ROUTER_SPAN_NAME_FALLBACK, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, startSpan, @@ -82,15 +83,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 836cb335314b..b4f9e0ecc72a 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'); }); });