diff --git a/dev-packages/node-integration-tests/suites/tracing/httpServerSpans-streamed-unrouted/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/httpServerSpans-streamed-unrouted/instrument.mjs new file mode 100644 index 000000000000..53b9511a21f0 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/httpServerSpans-streamed-unrouted/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/httpServerSpans-streamed-unrouted/server.mjs b/dev-packages/node-integration-tests/suites/tracing/httpServerSpans-streamed-unrouted/server.mjs new file mode 100644 index 000000000000..5a66afe5f1b7 --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/httpServerSpans-streamed-unrouted/server.mjs @@ -0,0 +1,12 @@ +import { sendPortToRunner } from '@sentry-internal/node-integration-tests'; +import http from 'http'; + +// A bare `node:http` server: no framework ever resolves a route, so the server span +// keeps whatever name it was given at span start. +const server = http.createServer((_request, response) => { + response.end('Hello Node.js Server!'); +}); + +server.listen(0, () => { + sendPortToRunner(server.address().port); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/httpServerSpans-streamed-unrouted/test.ts b/dev-packages/node-integration-tests/suites/tracing/httpServerSpans-streamed-unrouted/test.ts new file mode 100644 index 000000000000..d488bf9bcd5a --- /dev/null +++ b/dev-packages/node-integration-tests/suites/tracing/httpServerSpans-streamed-unrouted/test.ts @@ -0,0 +1,35 @@ +import { afterAll, describe, expect } from 'vitest'; +import { cleanupChildProcesses, createEsmAndCjsTests } from '../../../utils/runner'; + +describe('httpServerSpans-streamed (no route)', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + createEsmAndCjsTests(__dirname, 'server.mjs', 'instrument.mjs', (createRunner, test) => { + test('names the server span after the request method, not the URL path', async () => { + const runner = createRunner() + .expect({ + span: container => { + const serverSpan = container.items.find( + item => + item.attributes['sentry.op']?.type === 'string' && item.attributes['sentry.op'].value === 'http.server', + ); + + expect(serverSpan).toBeDefined(); + expect(serverSpan?.is_segment).toBe(true); + // Without a route the name must not carry the URL path. + expect(serverSpan?.name).toBe('GET'); + expect(serverSpan?.attributes['sentry.segment.name.source']).toEqual({ type: 'string', value: 'url' }); + // The path is still available as an attribute, which is what `ignoreSpans`/`tracesSampler` match on. + expect(serverSpan?.attributes['url.path']).toEqual({ type: 'string', value: '/users/42' }); + }, + }) + .start(); + + await runner.makeRequest('get', '/users/42'); + + await runner.completed(); + }); + }); +}); diff --git a/dev-packages/node-integration-tests/suites/tracing/ignoreSpans-streamed/segments/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/ignoreSpans-streamed/segments/instrument.mjs index 4d14e615745b..812ba9fb86a8 100644 --- a/dev-packages/node-integration-tests/suites/tracing/ignoreSpans-streamed/segments/instrument.mjs +++ b/dev-packages/node-integration-tests/suites/tracing/ignoreSpans-streamed/segments/instrument.mjs @@ -7,6 +7,6 @@ Sentry.init({ tracesSampleRate: 1.0, transport: loggingTransport, traceLifecycle: 'stream', - ignoreSpans: [/\/health/], + ignoreSpans: [{ attributes: { 'url.path': '/health' } }], clientReportFlushInterval: 1_000, }); diff --git a/dev-packages/node-integration-tests/suites/tracing/sampling-streamed/instrument.mjs b/dev-packages/node-integration-tests/suites/tracing/sampling-streamed/instrument.mjs index 713a676ede3d..0ce3c59cf745 100644 --- a/dev-packages/node-integration-tests/suites/tracing/sampling-streamed/instrument.mjs +++ b/dev-packages/node-integration-tests/suites/tracing/sampling-streamed/instrument.mjs @@ -4,8 +4,9 @@ import { loggingTransport } from '@sentry-internal/node-integration-tests'; Sentry.init({ dsn: 'https://public@dsn.ingest.sentry.io/1337', release: '1.0', - tracesSampler: ({ inheritOrSampleWith, name }) => { - if (name === 'GET /health') { + tracesSampler: ({ inheritOrSampleWith, attributes }) => { + // The span name is low cardinality with span streaming, so match on `url.path` instead. + if (attributes?.['url.path'] === '/health') { return inheritOrSampleWith(0); } return inheritOrSampleWith(1); diff --git a/packages/bun/test/integrations/bunHttpServer.test.ts b/packages/bun/test/integrations/bunHttpServer.test.ts index ed73e0c61a3c..48d5f255d821 100644 --- a/packages/bun/test/integrations/bunHttpServer.test.ts +++ b/packages/bun/test/integrations/bunHttpServer.test.ts @@ -52,7 +52,9 @@ describe('Bun HTTP Server Integration', () => { expect(span).toBeDefined(); expect(span?.attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP]).toBe('http.server'); - expect(span?.name).toBe('GET /users'); + // No router resolves a route here, so with span streaming the name is the request method. + expect(span?.name).toBe('GET'); + expect(span?.attributes['url.path']).toBe('/users'); expect(span?.attributes['sentry.origin']).toBe('auto.http.server'); }); @@ -81,7 +83,8 @@ describe('Bun HTTP Server Integration', () => { expect(span).toBeDefined(); expect(span?.attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP]).toBe('http.server'); - expect(span?.name).toBe('QUERY /search'); + expect(span?.name).toBe('QUERY'); + expect(span?.attributes['url.path']).toBe('/search'); expect(span?.attributes[HTTP_REQUEST_METHOD]).toBe('QUERY'); }); diff --git a/packages/core/src/integrations/express/patch-layer.ts b/packages/core/src/integrations/express/patch-layer.ts index 5ccd63ace2fa..5318c7494732 100644 --- a/packages/core/src/integrations/express/patch-layer.ts +++ b/packages/core/src/integrations/express/patch-layer.ts @@ -27,7 +27,13 @@ * limitations under the License. */ -import { SENTRY_OP } from '@sentry/conventions/attributes'; +import { + HTTP_METHOD, + HTTP_REQUEST_METHOD, + HTTP_ROUTE, + SENTRY_OP, + SENTRY_SEGMENT_NAME_SOURCE, +} from '@sentry/conventions/attributes'; import { MIDDLEWARE } from '@sentry/conventions/op'; import { DEBUG_BUILD } from '../../debug-build'; import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '../../semanticAttributes'; @@ -37,7 +43,7 @@ import { 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'; -import { getActiveSpan } from '../../utils/spanUtils'; +import { getActiveSpan, getRootSpan, spanToJSON } from '../../utils/spanUtils'; import { getStoredLayers, storeLayer } from './request-layer-store'; import { type ExpressRequest, @@ -142,6 +148,13 @@ export function patchLayer( attributes[ATTR_HTTP_ROUTE] = actualMatchedRoute; } + // Propagate the route to the root `http.server` span before the ignore check, so the span is still + // named when the layer's own span is ignored. Runs for every layer that matched a route, not just + // request handlers: mounted middleware (`app.use('/trpc', ...)`) resolves a route too. + if (actualMatchedRoute) { + applyRouteToRootSpan(actualMatchedRoute); + } + // verify against the config if the layer should be ignored if (isLayerIgnored(metadata.attributes[ATTR_EXPRESS_NAME], type, options)) { // XXX: the isLayerPathStored guard here is *not* present in the @@ -289,3 +302,34 @@ export function patchLayer( value: layerHandlePatched, }); } + +/** + * Write the resolved route onto the root `http.server` span. + * + * With span streaming the root span starts out named after the request method only, because no route + * is known at that point. Unlike the Node SDK — which goes through `setHttpServerSpanRouteAttribute` — + * nothing else on this path renames it, so a routed request would otherwise keep the method-only name. + */ +function applyRouteToRootSpan(route: string): void { + const client = getClient(); + if (!client || !hasSpanStreamingEnabled(client)) { + return; + } + + const activeSpan = getActiveSpan(); + const rootSpan = activeSpan && getRootSpan(activeSpan); + if (!rootSpan) { + return; + } + + const attributes = spanToJSON(rootSpan).attributes; + if (attributes[SENTRY_OP] !== 'http.server') { + return; + } + + // eslint-disable-next-line typescript/no-deprecated + const method = attributes[HTTP_REQUEST_METHOD] || attributes[HTTP_METHOD] || 'GET'; + rootSpan.updateName(`${method} ${route}`); + rootSpan.setAttribute(HTTP_ROUTE, route); + rootSpan.setAttribute(SENTRY_SEGMENT_NAME_SOURCE, 'route'); +} diff --git a/packages/core/src/integrations/http/server-subscription.ts b/packages/core/src/integrations/http/server-subscription.ts index 055c1c359fbf..d86e2dc8b06f 100644 --- a/packages/core/src/integrations/http/server-subscription.ts +++ b/packages/core/src/integrations/http/server-subscription.ts @@ -37,6 +37,8 @@ import { recordRequestSession } from './record-request-session'; import { generateSpanId, generateTraceId } from '../../utils/propagationContext'; import { continueTrace, startSpanManual } from '../../tracing/trace'; import { getSpanStatusFromHttpCode, SPAN_STATUS_ERROR } from '../../tracing'; +import { hasSpanStreamingEnabled } from '../../tracing/spans/hasSpanStreamingEnabled'; +import { HTTP_SPAN_NAME_FALLBACK } from '../../tracing/spans/spanNames'; import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '../../semanticAttributes'; import { safeMathRandom } from '../../utils/randomSafeContext'; import type { SpanStatus } from '../../types/spanStatus'; @@ -295,7 +297,11 @@ function buildServerSpanWrap( const urlObj = parseStringToURLObject(fullUrl); const httpTargetWithoutQueryFragment = urlObj ? urlObj.pathname : stripUrlQueryAndFragment(fullUrl); const method = (request.method || 'GET').toUpperCase(); - const name = `${method} ${httpTargetWithoutQueryFragment}`; + // With span streaming, span names have to be low cardinality, so we can't fall back to the URL path. + // Route instrumentations rename the span to `${method} ${route}` once a route is known. + const name = hasSpanStreamingEnabled(client) + ? request.method?.toUpperCase() || HTTP_SPAN_NAME_FALLBACK + : `${method} ${httpTargetWithoutQueryFragment}`; const headers = request.headers; const userAgent = headers['user-agent']; const ips = headers['x-forwarded-for']; 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..63b42ac3ec7b 100644 --- a/packages/core/test/lib/integrations/express/patch-layer.test.ts +++ b/packages/core/test/lib/integrations/express/patch-layer.test.ts @@ -70,6 +70,7 @@ vi.mock('../../../../src/defaultScopes', () => ({ const mockSpans: MockSpan[] = []; beforeEach(() => (mockSpans.length = 0)); +beforeEach(() => (transactionNames.length = 0)); class MockSpan { ended = false; status: { code: number; message: string } = { code: 0, message: 'OK' }; @@ -131,12 +132,34 @@ const checkSpans = (expectations: Partial[]) => { }; let hasActiveSpan = true; -const parentSpan = {}; +// Stands in for the root `http.server` span so the route-to-root-span write can be asserted. +const parentSpan = { + name: 'GET', + attributes: { 'sentry.op': 'http.server' } as Record, + updateName(name: string) { + this.name = name; + return this; + }, + setAttribute(key: string, value: unknown) { + this.attributes[key] = value; + return this; + }, +}; +beforeEach(() => { + parentSpan.name = 'GET'; + parentSpan.attributes = { 'sentry.op': 'http.server' }; +}); vi.mock('../../../../src/utils/spanUtils', async () => ({ ...(await import('../../../../src/utils/spanUtils')), getActiveSpan() { return hasActiveSpan ? parentSpan : undefined; }, + getRootSpan(span: unknown) { + return span; + }, + spanToJSON(span: { attributes?: Record }) { + return { attributes: span.attributes ?? {} }; + }, })); vi.mock('../../../../src/tracing', () => ({ @@ -367,6 +390,90 @@ describe('patchLayer', () => { checkSpans([]); }); + it('writes the resolved route onto the root http.server span when span streaming is enabled', () => { + // Regression guard: with streaming the root span starts named `GET`, and nothing else on this + // path renames it — a routed request would otherwise keep the method-only name. + spanStreamingEnabled = true; + + const req = Object.assign(new EventEmitter(), { + originalUrl: '/a/b/c/layerPath', + method: 'get', + }) as unknown as ExpressRequest; + const res = Object.assign(new EventEmitter(), {}) as unknown as ExpressResponse; + const layer = { name: 'handle', handle: vi.fn() } as unknown as ExpressLayer; + + storeLayer(req, 'a'); + storeLayer(req, '/:boo'); + + patchLayer(() => ({}), layer); + layer.handle(req, res); + + expect(parentSpan.name).toBe('GET /a/:boo'); + expect(parentSpan.attributes['http.route']).toBe('/a/:boo'); + expect(parentSpan.attributes['sentry.segment.name.source']).toBe('route'); + }); + + it('names the root route `GET /` rather than leaving the route empty', () => { + // `getConstructedRoute` skips `/`, so the root handler must take its route from the matched route. + spanStreamingEnabled = true; + + const req = Object.assign(new EventEmitter(), { + originalUrl: '/', + method: 'get', + }) as unknown as ExpressRequest; + const res = Object.assign(new EventEmitter(), {}) as unknown as ExpressResponse; + const layer = { name: 'handle', handle: vi.fn() } as unknown as ExpressLayer; + + storeLayer(req, '/'); + + patchLayer(() => ({}), layer); + layer.handle(req, res); + + expect(parentSpan.name).toBe('GET /'); + expect(parentSpan.attributes['http.route']).toBe('/'); + }); + + it('applies the route from mounted middleware, not only from request handlers', () => { + // `app.use('/trpc', handler)` matches a route without being a request handler. + spanStreamingEnabled = true; + + const req = Object.assign(new EventEmitter(), { + originalUrl: '/trpc/foo', + method: 'get', + }) as unknown as ExpressRequest; + const res = Object.assign(new EventEmitter(), {}) as unknown as ExpressResponse; + // A layer name other than `handle`/`bound dispatch`/`router` is treated as middleware. + const layer = { name: 'trpcMiddleware', handle: vi.fn() } as unknown as ExpressLayer; + + storeLayer(req, '/trpc'); + + patchLayer(() => ({}), layer); + layer.handle(req, res); + + expect(parentSpan.name).toBe('GET /trpc'); + expect(parentSpan.attributes['http.route']).toBe('/trpc'); + }); + + it('leaves the root span name alone without span streaming', () => { + spanStreamingEnabled = false; + + const req = Object.assign(new EventEmitter(), { + originalUrl: '/a/b/c/layerPath', + method: 'get', + }) as unknown as ExpressRequest; + const res = Object.assign(new EventEmitter(), {}) as unknown as ExpressResponse; + const layer = { name: 'handle', handle: vi.fn() } as unknown as ExpressLayer; + + storeLayer(req, 'a'); + storeLayer(req, '/:boo'); + + patchLayer(() => ({}), layer); + layer.handle(req, res); + + expect(parentSpan.name).toBe('GET'); + expect(parentSpan.attributes['http.route']).toBeUndefined(); + }); + it('sets tx name in isolation scope', async () => { DEBUG_BUILD = true; expect( diff --git a/packages/core/test/lib/integrations/http/server-subscription.test.ts b/packages/core/test/lib/integrations/http/server-subscription.test.ts index 628d84fc757a..3b749134bd9d 100644 --- a/packages/core/test/lib/integrations/http/server-subscription.test.ts +++ b/packages/core/test/lib/integrations/http/server-subscription.test.ts @@ -19,6 +19,8 @@ import type { AddressInfo } from 'node:net'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { getIsolationScope } from '../../../../src/currentScopes'; +import { Scope } from '../../../../src/scope'; +import { spanToJSON } from '../../../../src/utils/spanUtils'; import { setCurrentClient } from '../../../../src/sdk'; import { HTTP_ON_SERVER_REQUEST } from '../../../../src/integrations/http/constants'; import { getHttpServerSubscriptions } from '../../../../src/integrations/http/server-subscription'; @@ -58,7 +60,7 @@ describe('getHttpServerSubscriptions', () => { async function makeRequest( path: string, - method: 'GET' | 'HEAD' | 'OPTIONS' = 'GET', + method: 'GET' | 'HEAD' | 'OPTIONS' | 'POST' = 'GET', extraHeaders: Record = {}, ): Promise { const { port } = server.address() as AddressInfo; @@ -308,4 +310,46 @@ describe('getHttpServerSubscriptions', () => { const transaction = await waitForTransaction(); expect(transaction.transaction).toBe('GET /now-traced'); }); + + describe('with span streaming enabled', () => { + let streamingClient: TestClient; + + beforeEach(() => { + streamingClient = new TestClient(getDefaultTestClientOptions({ tracesSampleRate: 1, traceLifecycle: 'stream' })); + setCurrentClient(streamingClient); + streamingClient.init(); + getIsolationScope().setClient(streamingClient); + }); + + async function startedSpanName(path: string, method: 'GET' | 'POST' = 'GET'): Promise { + let spanName: string | undefined; + streamingClient.on('spanStart', span => { + spanName ??= spanToJSON(span).name; + }); + + server = http.createServer((_req, res) => res.end('ok')); + await new Promise(resolve => server.listen(0, '127.0.0.1', () => resolve())); + instrument(true); + + await makeRequest(path, method); + await vi.waitUntil(() => spanName !== undefined, { timeout: 1000, interval: 10 }); + return spanName!; + } + + it('names the span after the request method instead of the URL path', async () => { + expect(await startedSpanName('/users/42?foo=bar')).toBe('GET'); + }); + + it('keeps the method distinct per request', async () => { + expect(await startedSpanName('/users/42', 'POST')).toBe('POST'); + }); + + it('keeps the raw URL path as the scope transaction name', async () => { + const setTransactionName = vi.spyOn(Scope.prototype, 'setTransactionName'); + + expect(await startedSpanName('/users/42?foo=bar')).toBe('GET'); + + expect(setTransactionName).toHaveBeenCalledWith('GET /users/42'); + }); + }); }); diff --git a/packages/node/src/integrations/http/httpServerSpansIntegration.ts b/packages/node/src/integrations/http/httpServerSpansIntegration.ts index 73e093334446..8633c6f23938 100644 --- a/packages/node/src/integrations/http/httpServerSpansIntegration.ts +++ b/packages/node/src/integrations/http/httpServerSpansIntegration.ts @@ -52,6 +52,8 @@ import { getUrlQuery, filterCollectedUrl, filterCollectedUrlQuery, + hasSpanStreamingEnabled, + HTTP_SPAN_NAME_FALLBACK, } from '@sentry/core'; import { DEBUG_BUILD } from '../../debug-build'; import type { NodeClient } from '../../sdk/client'; @@ -146,7 +148,8 @@ const _httpServerSpansIntegration = ((options: HttpServerSpansIntegrationOptions const scheme = fullUrl.startsWith('https') ? 'https' : 'http'; - const method = normalizedRequest.method || request.method?.toUpperCase() || 'GET'; + const requestMethod = normalizedRequest.method || request.method?.toUpperCase(); + const method = requestMethod || 'GET'; const httpTargetWithoutQueryFragment = urlObj ? urlObj.pathname : stripUrlQueryAndFragment(fullUrl); const bestEffortTransactionName = `${method} ${httpTargetWithoutQueryFragment}`; @@ -154,7 +157,11 @@ const _httpServerSpansIntegration = ((options: HttpServerSpansIntegrationOptions const fragment = getUrlFragment(urlObj?.hash); const span = startInactiveSpan({ - name: bestEffortTransactionName, + // With span streaming, span names have to be low cardinality, so we can't fall back to the URL path. + // Route instrumentations rename the span to `${method} ${route}` once a route is known. + name: hasSpanStreamingEnabled(client) + ? requestMethod || HTTP_SPAN_NAME_FALLBACK + : bestEffortTransactionName, attributes: { // Sentry specific attributes [SENTRY_KIND]: 'server',