Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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',
});
Original file line number Diff line number Diff line change
@@ -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);
});
Original file line number Diff line number Diff line change
@@ -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();
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@ Sentry.init({
tracesSampleRate: 1.0,
transport: loggingTransport,
traceLifecycle: 'stream',
ignoreSpans: [/\/health/],
ignoreSpans: [{ attributes: { 'url.path': '/health' } }],
clientReportFlushInterval: 1_000,
});
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
7 changes: 5 additions & 2 deletions packages/bun/test/integrations/bunHttpServer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});

Expand Down Expand Up @@ -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');
});

Expand Down
48 changes: 46 additions & 2 deletions packages/core/src/integrations/express/patch-layer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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');
}
8 changes: 7 additions & 1 deletion packages/core/src/integrations/http/server-subscription.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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}`;
Comment thread
chargome marked this conversation as resolved.
const headers = request.headers;
const userAgent = headers['user-agent'];
const ips = headers['x-forwarded-for'];
Expand Down
109 changes: 108 additions & 1 deletion packages/core/test/lib/integrations/express/patch-layer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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' };
Expand Down Expand Up @@ -131,12 +132,34 @@ const checkSpans = (expectations: Partial<MockSpanJSON>[]) => {
};

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<string, unknown>,
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<string, unknown> }) {
return { attributes: span.attributes ?? {} };
},
}));

vi.mock('../../../../src/tracing', () => ({
Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -58,7 +60,7 @@ describe('getHttpServerSubscriptions', () => {

async function makeRequest(
path: string,
method: 'GET' | 'HEAD' | 'OPTIONS' = 'GET',
method: 'GET' | 'HEAD' | 'OPTIONS' | 'POST' = 'GET',
extraHeaders: Record<string, string> = {},
): Promise<void> {
const { port } = server.address() as AddressInfo;
Expand Down Expand Up @@ -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<string> {
let spanName: string | undefined;
streamingClient.on('spanStart', span => {
spanName ??= spanToJSON(span).name;
});

server = http.createServer((_req, res) => res.end('ok'));
await new Promise<void>(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');
});
});
});
Loading
Loading