Skip to content

Commit f2795a3

Browse files
chargomeclaude
andcommitted
fix(core): Write the resolved express route onto the root http.server span
Unlike the Node SDK, which renames the root span via `setHttpServerSpanRouteAttribute`, core's express integration only set the isolation scope's transaction name. With span streaming that left routed requests on Bun and Deno stuck with the method-only span name they start with. Gated on span streaming so `static` mode names are unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 35f6a44 commit f2795a3

4 files changed

Lines changed: 160 additions & 6 deletions

File tree

dev-packages/node-integration-tests/suites/tracing/httpServerSpans-streamed-unrouted/test.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ describe('httpServerSpans-streamed (no route)', () => {
2020
expect(serverSpan?.is_segment).toBe(true);
2121
// Without a route the name must not carry the URL path.
2222
expect(serverSpan?.name).toBe('GET');
23-
expect(serverSpan?.attributes['sentry.source']).toEqual({ type: 'string', value: 'url' });
23+
expect(serverSpan?.attributes['sentry.segment.name.source']).toEqual({ type: 'string', value: 'url' });
2424
// The path is still available as an attribute, which is what `ignoreSpans`/`tracesSampler` match on.
2525
expect(serverSpan?.attributes['url.path']).toEqual({ type: 'string', value: '/users/42' });
2626
},

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/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(

0 commit comments

Comments
 (0)