Skip to content

Commit d4e9ccd

Browse files
chargomeclaude
andauthored
feat(core): Emit low-cardinality http.server span names when streaming (#23596)
Names `http.server` spans after the request method instead of the URL path when span streaming is enabled and no route resolved. Routed requests are unchanged, `static` mode is byte-identical, and the scope's transaction name keeps the full path so error grouping is unaffected. Refs #23527 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 941ebc0 commit d4e9ccd

11 files changed

Lines changed: 281 additions & 12 deletions

File tree

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
import * as Sentry from '@sentry/node';
2+
import { loggingTransport } from '@sentry-internal/node-integration-tests';
3+
4+
Sentry.init({
5+
dsn: 'https://public@dsn.ingest.sentry.io/1337',
6+
release: '1.0',
7+
tracesSampleRate: 1.0,
8+
transport: loggingTransport,
9+
traceLifecycle: 'stream',
10+
});
Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
import { sendPortToRunner } from '@sentry-internal/node-integration-tests';
2+
import http from 'http';
3+
4+
// A bare `node:http` server: no framework ever resolves a route, so the server span
5+
// keeps whatever name it was given at span start.
6+
const server = http.createServer((_request, response) => {
7+
response.end('Hello Node.js Server!');
8+
});
9+
10+
server.listen(0, () => {
11+
sendPortToRunner(server.address().port);
12+
});
Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
import { afterAll, describe, expect } from 'vitest';
2+
import { cleanupChildProcesses, createEsmAndCjsTests } from '../../../utils/runner';
3+
4+
describe('httpServerSpans-streamed (no route)', () => {
5+
afterAll(() => {
6+
cleanupChildProcesses();
7+
});
8+
9+
createEsmAndCjsTests(__dirname, 'server.mjs', 'instrument.mjs', (createRunner, test) => {
10+
test('names the server span after the request method, not the URL path', async () => {
11+
const runner = createRunner()
12+
.expect({
13+
span: container => {
14+
const serverSpan = container.items.find(
15+
item =>
16+
item.attributes['sentry.op']?.type === 'string' && item.attributes['sentry.op'].value === 'http.server',
17+
);
18+
19+
expect(serverSpan).toBeDefined();
20+
expect(serverSpan?.is_segment).toBe(true);
21+
// Without a route the name must not carry the URL path.
22+
expect(serverSpan?.name).toBe('GET');
23+
expect(serverSpan?.attributes['sentry.segment.name.source']).toEqual({ type: 'string', value: 'url' });
24+
// The path is still available as an attribute, which is what `ignoreSpans`/`tracesSampler` match on.
25+
expect(serverSpan?.attributes['url.path']).toEqual({ type: 'string', value: '/users/42' });
26+
},
27+
})
28+
.start();
29+
30+
await runner.makeRequest('get', '/users/42');
31+
32+
await runner.completed();
33+
});
34+
});
35+
});

‎dev-packages/node-integration-tests/suites/tracing/ignoreSpans-streamed/segments/instrument.mjs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,6 @@ Sentry.init({
77
tracesSampleRate: 1.0,
88
transport: loggingTransport,
99
traceLifecycle: 'stream',
10-
ignoreSpans: [/\/health/],
10+
ignoreSpans: [{ attributes: { 'url.path': '/health' } }],
1111
clientReportFlushInterval: 1_000,
1212
});

‎dev-packages/node-integration-tests/suites/tracing/sampling-streamed/instrument.mjs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,9 @@ import { loggingTransport } from '@sentry-internal/node-integration-tests';
44
Sentry.init({
55
dsn: 'https://public@dsn.ingest.sentry.io/1337',
66
release: '1.0',
7-
tracesSampler: ({ inheritOrSampleWith, name }) => {
8-
if (name === 'GET /health') {
7+
tracesSampler: ({ inheritOrSampleWith, attributes }) => {
8+
// The span name is low cardinality with span streaming, so match on `url.path` instead.
9+
if (attributes?.['url.path'] === '/health') {
910
return inheritOrSampleWith(0);
1011
}
1112
return inheritOrSampleWith(1);

‎packages/bun/test/integrations/bunHttpServer.test.ts‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,9 @@ describe('Bun HTTP Server Integration', () => {
5252

5353
expect(span).toBeDefined();
5454
expect(span?.attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP]).toBe('http.server');
55-
expect(span?.name).toBe('GET /users');
55+
// No router resolves a route here, so with span streaming the name is the request method.
56+
expect(span?.name).toBe('GET');
57+
expect(span?.attributes['url.path']).toBe('/users');
5658
expect(span?.attributes['sentry.origin']).toBe('auto.http.server');
5759
});
5860

@@ -81,7 +83,8 @@ describe('Bun HTTP Server Integration', () => {
8183

8284
expect(span).toBeDefined();
8385
expect(span?.attributes[SEMANTIC_ATTRIBUTE_SENTRY_OP]).toBe('http.server');
84-
expect(span?.name).toBe('QUERY /search');
86+
expect(span?.name).toBe('QUERY');
87+
expect(span?.attributes['url.path']).toBe('/search');
8588
expect(span?.attributes[HTTP_REQUEST_METHOD]).toBe('QUERY');
8689
});
8790

‎packages/core/src/integrations/express/patch-layer.ts‎

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -27,7 +27,13 @@
2727
* limitations under the License.
2828
*/
2929

30-
import { SENTRY_OP } from '@sentry/conventions/attributes';
30+
import {
31+
HTTP_METHOD,
32+
HTTP_REQUEST_METHOD,
33+
HTTP_ROUTE,
34+
SENTRY_OP,
35+
SENTRY_SEGMENT_NAME_SOURCE,
36+
} from '@sentry/conventions/attributes';
3137
import { MIDDLEWARE } from '@sentry/conventions/op';
3238
import { DEBUG_BUILD } from '../../debug-build';
3339
import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '../../semanticAttributes';
@@ -37,7 +43,7 @@ import { ROUTER_SPAN_NAME_FALLBACK } from '../../tracing/spans/spanNames';
3743
import { startSpanManual } from '../../tracing/trace';
3844
import { debug } from '../../utils/debug-logger';
3945
import type { SpanAttributes } from '../../types/span';
40-
import { getActiveSpan } from '../../utils/spanUtils';
46+
import { getActiveSpan, getRootSpan, spanToJSON } from '../../utils/spanUtils';
4147
import { getStoredLayers, storeLayer } from './request-layer-store';
4248
import {
4349
type ExpressRequest,
@@ -142,6 +148,13 @@ export function patchLayer(
142148
attributes[ATTR_HTTP_ROUTE] = actualMatchedRoute;
143149
}
144150

151+
// Propagate the route to the root `http.server` span before the ignore check, so the span is still
152+
// named when the layer's own span is ignored. Runs for every layer that matched a route, not just
153+
// request handlers: mounted middleware (`app.use('/trpc', ...)`) resolves a route too.
154+
if (actualMatchedRoute) {
155+
applyRouteToRootSpan(actualMatchedRoute);
156+
}
157+
145158
// verify against the config if the layer should be ignored
146159
if (isLayerIgnored(metadata.attributes[ATTR_EXPRESS_NAME], type, options)) {
147160
// XXX: the isLayerPathStored guard here is *not* present in the
@@ -289,3 +302,34 @@ export function patchLayer(
289302
value: layerHandlePatched,
290303
});
291304
}
305+
306+
/**
307+
* Write the resolved route onto the root `http.server` span.
308+
*
309+
* With span streaming the root span starts out named after the request method only, because no route
310+
* is known at that point. Unlike the Node SDK — which goes through `setHttpServerSpanRouteAttribute` —
311+
* nothing else on this path renames it, so a routed request would otherwise keep the method-only name.
312+
*/
313+
function applyRouteToRootSpan(route: string): void {
314+
const client = getClient();
315+
if (!client || !hasSpanStreamingEnabled(client)) {
316+
return;
317+
}
318+
319+
const activeSpan = getActiveSpan();
320+
const rootSpan = activeSpan && getRootSpan(activeSpan);
321+
if (!rootSpan) {
322+
return;
323+
}
324+
325+
const attributes = spanToJSON(rootSpan).attributes;
326+
if (attributes[SENTRY_OP] !== 'http.server') {
327+
return;
328+
}
329+
330+
// eslint-disable-next-line typescript/no-deprecated
331+
const method = attributes[HTTP_REQUEST_METHOD] || attributes[HTTP_METHOD] || 'GET';
332+
rootSpan.updateName(`${method} ${route}`);
333+
rootSpan.setAttribute(HTTP_ROUTE, route);
334+
rootSpan.setAttribute(SENTRY_SEGMENT_NAME_SOURCE, 'route');
335+
}

‎packages/core/src/integrations/http/server-subscription.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,8 @@ import { recordRequestSession } from './record-request-session';
3737
import { generateSpanId, generateTraceId } from '../../utils/propagationContext';
3838
import { continueTrace, startSpanManual } from '../../tracing/trace';
3939
import { getSpanStatusFromHttpCode, SPAN_STATUS_ERROR } from '../../tracing';
40+
import { hasSpanStreamingEnabled } from '../../tracing/spans/hasSpanStreamingEnabled';
41+
import { HTTP_SPAN_NAME_FALLBACK } from '../../tracing/spans/spanNames';
4042
import { SEMANTIC_ATTRIBUTE_SENTRY_OP, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '../../semanticAttributes';
4143
import { safeMathRandom } from '../../utils/randomSafeContext';
4244
import type { SpanStatus } from '../../types/spanStatus';
@@ -295,7 +297,11 @@ function buildServerSpanWrap(
295297
const urlObj = parseStringToURLObject(fullUrl);
296298
const httpTargetWithoutQueryFragment = urlObj ? urlObj.pathname : stripUrlQueryAndFragment(fullUrl);
297299
const method = (request.method || 'GET').toUpperCase();
298-
const name = `${method} ${httpTargetWithoutQueryFragment}`;
300+
// With span streaming, span names have to be low cardinality, so we can't fall back to the URL path.
301+
// Route instrumentations rename the span to `${method} ${route}` once a route is known.
302+
const name = hasSpanStreamingEnabled(client)
303+
? request.method?.toUpperCase() || HTTP_SPAN_NAME_FALLBACK
304+
: `${method} ${httpTargetWithoutQueryFragment}`;
299305
const headers = request.headers;
300306
const userAgent = headers['user-agent'];
301307
const ips = headers['x-forwarded-for'];

‎packages/core/test/lib/integrations/express/patch-layer.test.ts‎

Lines changed: 108 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,7 @@ vi.mock('../../../../src/defaultScopes', () => ({
7070

7171
const mockSpans: MockSpan[] = [];
7272
beforeEach(() => (mockSpans.length = 0));
73+
beforeEach(() => (transactionNames.length = 0));
7374
class MockSpan {
7475
ended = false;
7576
status: { code: number; message: string } = { code: 0, message: 'OK' };
@@ -131,12 +132,34 @@ const checkSpans = (expectations: Partial<MockSpanJSON>[]) => {
131132
};
132133

133134
let hasActiveSpan = true;
134-
const parentSpan = {};
135+
// Stands in for the root `http.server` span so the route-to-root-span write can be asserted.
136+
const parentSpan = {
137+
name: 'GET',
138+
attributes: { 'sentry.op': 'http.server' } as Record<string, unknown>,
139+
updateName(name: string) {
140+
this.name = name;
141+
return this;
142+
},
143+
setAttribute(key: string, value: unknown) {
144+
this.attributes[key] = value;
145+
return this;
146+
},
147+
};
148+
beforeEach(() => {
149+
parentSpan.name = 'GET';
150+
parentSpan.attributes = { 'sentry.op': 'http.server' };
151+
});
135152
vi.mock('../../../../src/utils/spanUtils', async () => ({
136153
...(await import('../../../../src/utils/spanUtils')),
137154
getActiveSpan() {
138155
return hasActiveSpan ? parentSpan : undefined;
139156
},
157+
getRootSpan(span: unknown) {
158+
return span;
159+
},
160+
spanToJSON(span: { attributes?: Record<string, unknown> }) {
161+
return { attributes: span.attributes ?? {} };
162+
},
140163
}));
141164

142165
vi.mock('../../../../src/tracing', () => ({
@@ -367,6 +390,90 @@ describe('patchLayer', () => {
367390
checkSpans([]);
368391
});
369392

393+
it('writes the resolved route onto the root http.server span when span streaming is enabled', () => {
394+
// Regression guard: with streaming the root span starts named `GET`, and nothing else on this
395+
// path renames it — a routed request would otherwise keep the method-only name.
396+
spanStreamingEnabled = true;
397+
398+
const req = Object.assign(new EventEmitter(), {
399+
originalUrl: '/a/b/c/layerPath',
400+
method: 'get',
401+
}) as unknown as ExpressRequest;
402+
const res = Object.assign(new EventEmitter(), {}) as unknown as ExpressResponse;
403+
const layer = { name: 'handle', handle: vi.fn() } as unknown as ExpressLayer;
404+
405+
storeLayer(req, 'a');
406+
storeLayer(req, '/:boo');
407+
408+
patchLayer(() => ({}), layer);
409+
layer.handle(req, res);
410+
411+
expect(parentSpan.name).toBe('GET /a/:boo');
412+
expect(parentSpan.attributes['http.route']).toBe('/a/:boo');
413+
expect(parentSpan.attributes['sentry.segment.name.source']).toBe('route');
414+
});
415+
416+
it('names the root route `GET /` rather than leaving the route empty', () => {
417+
// `getConstructedRoute` skips `/`, so the root handler must take its route from the matched route.
418+
spanStreamingEnabled = true;
419+
420+
const req = Object.assign(new EventEmitter(), {
421+
originalUrl: '/',
422+
method: 'get',
423+
}) as unknown as ExpressRequest;
424+
const res = Object.assign(new EventEmitter(), {}) as unknown as ExpressResponse;
425+
const layer = { name: 'handle', handle: vi.fn() } as unknown as ExpressLayer;
426+
427+
storeLayer(req, '/');
428+
429+
patchLayer(() => ({}), layer);
430+
layer.handle(req, res);
431+
432+
expect(parentSpan.name).toBe('GET /');
433+
expect(parentSpan.attributes['http.route']).toBe('/');
434+
});
435+
436+
it('applies the route from mounted middleware, not only from request handlers', () => {
437+
// `app.use('/trpc', handler)` matches a route without being a request handler.
438+
spanStreamingEnabled = true;
439+
440+
const req = Object.assign(new EventEmitter(), {
441+
originalUrl: '/trpc/foo',
442+
method: 'get',
443+
}) as unknown as ExpressRequest;
444+
const res = Object.assign(new EventEmitter(), {}) as unknown as ExpressResponse;
445+
// A layer name other than `handle`/`bound dispatch`/`router` is treated as middleware.
446+
const layer = { name: 'trpcMiddleware', handle: vi.fn() } as unknown as ExpressLayer;
447+
448+
storeLayer(req, '/trpc');
449+
450+
patchLayer(() => ({}), layer);
451+
layer.handle(req, res);
452+
453+
expect(parentSpan.name).toBe('GET /trpc');
454+
expect(parentSpan.attributes['http.route']).toBe('/trpc');
455+
});
456+
457+
it('leaves the root span name alone without span streaming', () => {
458+
spanStreamingEnabled = false;
459+
460+
const req = Object.assign(new EventEmitter(), {
461+
originalUrl: '/a/b/c/layerPath',
462+
method: 'get',
463+
}) as unknown as ExpressRequest;
464+
const res = Object.assign(new EventEmitter(), {}) as unknown as ExpressResponse;
465+
const layer = { name: 'handle', handle: vi.fn() } as unknown as ExpressLayer;
466+
467+
storeLayer(req, 'a');
468+
storeLayer(req, '/:boo');
469+
470+
patchLayer(() => ({}), layer);
471+
layer.handle(req, res);
472+
473+
expect(parentSpan.name).toBe('GET');
474+
expect(parentSpan.attributes['http.route']).toBeUndefined();
475+
});
476+
370477
it('sets tx name in isolation scope', async () => {
371478
DEBUG_BUILD = true;
372479
expect(

‎packages/core/test/lib/integrations/http/server-subscription.test.ts‎

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,8 @@ import type { AddressInfo } from 'node:net';
1919
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
2020

2121
import { getIsolationScope } from '../../../../src/currentScopes';
22+
import { Scope } from '../../../../src/scope';
23+
import { spanToJSON } from '../../../../src/utils/spanUtils';
2224
import { setCurrentClient } from '../../../../src/sdk';
2325
import { HTTP_ON_SERVER_REQUEST } from '../../../../src/integrations/http/constants';
2426
import { getHttpServerSubscriptions } from '../../../../src/integrations/http/server-subscription';
@@ -58,7 +60,7 @@ describe('getHttpServerSubscriptions', () => {
5860

5961
async function makeRequest(
6062
path: string,
61-
method: 'GET' | 'HEAD' | 'OPTIONS' = 'GET',
63+
method: 'GET' | 'HEAD' | 'OPTIONS' | 'POST' = 'GET',
6264
extraHeaders: Record<string, string> = {},
6365
): Promise<void> {
6466
const { port } = server.address() as AddressInfo;
@@ -308,4 +310,46 @@ describe('getHttpServerSubscriptions', () => {
308310
const transaction = await waitForTransaction();
309311
expect(transaction.transaction).toBe('GET /now-traced');
310312
});
313+
314+
describe('with span streaming enabled', () => {
315+
let streamingClient: TestClient;
316+
317+
beforeEach(() => {
318+
streamingClient = new TestClient(getDefaultTestClientOptions({ tracesSampleRate: 1, traceLifecycle: 'stream' }));
319+
setCurrentClient(streamingClient);
320+
streamingClient.init();
321+
getIsolationScope().setClient(streamingClient);
322+
});
323+
324+
async function startedSpanName(path: string, method: 'GET' | 'POST' = 'GET'): Promise<string> {
325+
let spanName: string | undefined;
326+
streamingClient.on('spanStart', span => {
327+
spanName ??= spanToJSON(span).name;
328+
});
329+
330+
server = http.createServer((_req, res) => res.end('ok'));
331+
await new Promise<void>(resolve => server.listen(0, '127.0.0.1', () => resolve()));
332+
instrument(true);
333+
334+
await makeRequest(path, method);
335+
await vi.waitUntil(() => spanName !== undefined, { timeout: 1000, interval: 10 });
336+
return spanName!;
337+
}
338+
339+
it('names the span after the request method instead of the URL path', async () => {
340+
expect(await startedSpanName('/users/42?foo=bar')).toBe('GET');
341+
});
342+
343+
it('keeps the method distinct per request', async () => {
344+
expect(await startedSpanName('/users/42', 'POST')).toBe('POST');
345+
});
346+
347+
it('keeps the raw URL path as the scope transaction name', async () => {
348+
const setTransactionName = vi.spyOn(Scope.prototype, 'setTransactionName');
349+
350+
expect(await startedSpanName('/users/42?foo=bar')).toBe('GET');
351+
352+
expect(setTransactionName).toHaveBeenCalledWith('GET /users/42');
353+
});
354+
});
311355
});

0 commit comments

Comments
 (0)