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
3 changes: 3 additions & 0 deletions MIGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -626,6 +626,7 @@ The following span names were adjusted:
| Span op | Before | After |
| ------------ | --------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------- |
| `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 |
| `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 |

Expand All @@ -639,6 +640,8 @@ Because a low-cardinality name cannot say which part of request processing a spa

For the same reason, `useOperationNameForRootSpan` no longer renames the enclosing root span (`GET /graphql` stays `GET /graphql`, instead of becoming `GET /graphql (query GetUser)`). The operations are still recorded on that span's `sentry.graphql.operation` attribute, as long as the option stays enabled (the default). Disabling it skips both, as before.

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`.

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.
Expand Down
6 changes: 5 additions & 1 deletion packages/angular/src/tracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ import { FUNCTION } from '@sentry/conventions/op';
import type { Integration, Span } from '@sentry/core';
import {
debug,
hasSpanStreamingEnabled,
parseStringToURLObject,
ROUTER_SPAN_NAME_FALLBACK,
stripUrlQueryAndFragment,
timestampInSeconds,
filterCollectedUrl,
Expand Down Expand Up @@ -136,7 +138,9 @@ export class TraceService implements OnDestroy {
this._routingSpan =
runOutsideAngular(() =>
startInactiveSpan({
name: `${navigationEvent.url}`,
// With span streaming, span names have to be low cardinality. The parameterized route is only
// known at `ResolveEnd`, well after this span starts, so there is nothing but the fallback.
name: hasSpanStreamingEnabled(client) ? ROUTER_SPAN_NAME_FALLBACK : `${navigationEvent.url}`,
attributes: {
// TODO(conventions): Replace `'router'` with the `router` span op constant once it is released in `@sentry/conventions`.
[SENTRY_OP]: 'router',
Expand Down
12 changes: 10 additions & 2 deletions packages/core/src/integrations/express/patch-layer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ import { MIDDLEWARE } from '@sentry/conventions/op';
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 { startSpanManual } from '../../tracing/trace';
import { debug } from '../../utils/debug-logger';
import type { SpanAttributes } from '../../types/span';
Expand All @@ -56,7 +58,7 @@ import {
getLayerMetadata,
isLayerIgnored,
} from './utils';
import { getIsolationScope } from '../../currentScopes';
import { getClient, getIsolationScope } from '../../currentScopes';
import { getDefaultIsolationScope } from '../../defaultScopes';
import { getOriginalFunction, markFunctionWrapped } from '../../utils/object';
import { setSDKProcessingMetadata } from './set-sdk-processing-metadata';
Expand Down Expand Up @@ -165,7 +167,13 @@ export function patchLayer(
DEBUG_BUILD && debug.warn('Isolation scope is still default isolation scope - skipping setting transactionName');
}

return startSpanManual({ name, attributes }, span => {
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);

const spanName = isStreamedRouterSpan ? actualMatchedRoute || ROUTER_SPAN_NAME_FALLBACK : name;

return startSpanManual({ name: spanName, attributes }, span => {
let spanHasEnded = false;
// TODO: Fix router spans (getRouterPath does not work properly) to
// have useful names before removing this branch
Expand Down
74 changes: 74 additions & 0 deletions packages/core/test/lib/integrations/express/patch-layer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,15 @@ const defaultIsolationScope = {
this._scopeData.sdkProcessingMetadata = data;
},
};
let spanStreamingEnabled = false;
beforeEach(() => (spanStreamingEnabled = false));
vi.mock('../../../../src/currentScopes', () => ({
getIsolationScope() {
return inDefaultIsolationScope ? defaultIsolationScope : notDefaultIsolationScope;
},
getClient() {
return { getOptions: () => ({ traceLifecycle: spanStreamingEnabled ? 'stream' : 'static' }) };
},
}));
vi.mock('../../../../src/defaultScopes', () => ({
getDefaultIsolationScope() {
Expand Down Expand Up @@ -468,6 +473,75 @@ describe('patchLayer', () => {
checkSpans([]);
});

it('names router 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: 'router',
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': '/c',
'express.type': 'router',
'http.route': '/a/b/c',
'sentry.op': 'router',
'sentry.origin': 'auto.http.express',
},
description: '/a/b/c',
},
]);
});

it('falls back to a static router 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: 'router',
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': '/c',
'express.type': 'router',
'sentry.op': 'router',
'sentry.origin': 'auto.http.express',
},
description: 'Router',
},
]);
});
Comment thread
cursor[bot] marked this conversation as resolved.

it('handles case when route does not match url', () => {
const onRouteResolved = vi.fn();
const options: ExpressPatchLayerOptions = { onRouteResolved };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
getCurrentScope,
hasSpanStreamingEnabled,
PAGELOAD_SPAN_NAME_FALLBACK,
ROUTER_SPAN_NAME_FALLBACK,
spanToJSON,
type Client,
type Span,
Expand Down Expand Up @@ -148,7 +149,9 @@ export function instrumentEmberAppInstanceForPerformance(
[SENTRY_OP]: 'router',
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.ui.ember',
},
name: `route:${fromRoute} -> route:${toRoute}`,
// With span streaming, span names have to be low cardinality, and Ember gives us no route
// template for the transition itself, so it's the fallback.
name: hasSpanStreamingEnabled(client) ? ROUTER_SPAN_NAME_FALLBACK : `route:${fromRoute} -> route:${toRoute}`,
onlyIfParent: true,
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,11 @@ import type { Span } from '@sentry/core';
import {
debug,
getActiveSpan,
getClient,
getDefaultIsolationScope,
getIsolationScope,
hasSpanStreamingEnabled,
ROUTER_SPAN_NAME_FALLBACK,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
startInactiveSpan,
stringMatchesSomePattern,
Expand Down Expand Up @@ -224,8 +227,12 @@ function getSpanForLayer(data: HandleChannelContext, options: ExpressIntegration
return undefined;
}

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);

const span = startInactiveSpan({
name,
name: isStreamedRouterSpan ? matchedRoute || ROUTER_SPAN_NAME_FALLBACK : name,
attributes: {
[SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: ORIGIN,
[SENTRY_OP]: EXPRESS_TYPE_TO_SPAN_OP[type],
Expand Down
21 changes: 19 additions & 2 deletions packages/server-utils/src/integrations/hapi-utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,14 @@
* is replaced with `getActiveSpan()`.
*/

import { getActiveSpan, SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, startSpan } from '@sentry/core';
import {
getActiveSpan,
getClient,
hasSpanStreamingEnabled,
ROUTER_SPAN_NAME_FALLBACK,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
startSpan,
} from '@sentry/core';
import { SENTRY_OP } from '@sentry/conventions/attributes';
import { MIDDLEWARE } from '@sentry/conventions/op';
import type {
Expand Down Expand Up @@ -74,7 +81,17 @@ export const getRouteMetadata = (route: ServerRoute, pluginName?: string): SpanM
attributes[AttributeNames.HAPI_TYPE] = HapiLayerType.ROUTER;
}

return { attributes, name: `${route.method.toUpperCase()} ${route.path}` };
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);

return {
attributes,
name: isStreamedRouterSpan
? route.path || ROUTER_SPAN_NAME_FALLBACK
: `${route.method.toUpperCase()} ${route.path}`,
};
};

/** Build the span name and attributes for a Hapi server extension. */
Expand Down
10 changes: 9 additions & 1 deletion packages/server-utils/src/integrations/koa.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,11 @@ import {
debug,
defineIntegration,
getActiveSpan,
getClient,
getDefaultIsolationScope,
getIsolationScope,
hasSpanStreamingEnabled,
ROUTER_SPAN_NAME_FALLBACK,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
startSpan,
} from '@sentry/core';
Expand Down Expand Up @@ -173,7 +176,12 @@ function patchLayer(
const koaName = metadata.attributes[KOA_NAME];
// Somehow, name is sometimes `''` for middleware spans.
// See: https://github.com/open-telemetry/opentelemetry-js-contrib/issues/2220
const name = typeof koaName === 'string' ? koaName || '< unknown >' : metadata.name;
const staticName = typeof koaName === 'string' ? koaName || '< unknown >' : metadata.name;

const client = getClient();
// With span streaming, span names have to be low cardinality, so router spans are named after their route.
const isStreamedRouterSpan = layerType === LAYER_TYPE.ROUTER && !!client && hasSpanStreamingEnabled(client);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoidable getClient calls

Low Severity

Flagged because it was mentioned in the review rules file — this is more an "is this necessary" check than a hard violation. These sites newly call getClient() to gate span streaming, while the Koa, Express, and Hapi integrations already receive a client in setup. Prefering that existing reference would avoid relying on ambient current-client state in multi-client setups.

Additional Locations (2)
Fix in Cursor Fix in Web

Triggered by project rule: PR Review Guidelines for Cursor Bot

Reviewed by Cursor Bugbot for commit be3a06a. Configure here.

const name = isStreamedRouterSpan ? metadata.attributes[HTTP_ROUTE] || ROUTER_SPAN_NAME_FALLBACK : staticName;

return startSpan(
{
Expand Down
22 changes: 21 additions & 1 deletion packages/server-utils/test/integrations/hapi-utils.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
import { describe, expect, it } from 'vitest';
import { setCurrentClient } from '@sentry/core';
import { afterEach, describe, expect, it } from 'vitest';
import { getExtMetadata, getRouteMetadata } from '../../src/integrations/hapi-utils';
import { getDefaultTestClientOptions, TestClient } from '../mocks/client';

describe('getRouteMetadata', () => {
const route = { path: '/users/{id}', method: 'get' } as any;

afterEach(() => {
setCurrentClient(undefined as unknown as TestClient);
});

it('describes a directly-registered route as a router layer', () => {
expect(getRouteMetadata(route)).toEqual({
name: 'GET /users/{id}',
Expand All @@ -26,6 +32,20 @@ describe('getRouteMetadata', () => {
},
});
});

it('drops the method from the router span name when span streaming is enabled', () => {
const client = new TestClient(getDefaultTestClientOptions({ traceLifecycle: 'stream' }));
setCurrentClient(client);

expect(getRouteMetadata(route).name).toBe('/users/{id}');
});

it('keeps 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}');
});
});

describe('getExtMetadata', () => {
Expand Down
5 changes: 4 additions & 1 deletion packages/sveltekit/src/client/svelte4BrowserTracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { Client, Span } from '@sentry/core';
import {
hasSpanStreamingEnabled,
PAGELOAD_SPAN_NAME_FALLBACK,
ROUTER_SPAN_NAME_FALLBACK,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
} from '@sentry/core';
Expand Down Expand Up @@ -132,7 +133,9 @@ function _instrumentNavigations(client: Client, navigatingStore: Readable<Naviga
);

routingSpan = startInactiveSpan({
name: 'SvelteKit Route Change',
// With span streaming, span names have to be low cardinality, and this span carries no route
// of its own, so it's the fallback.
name: hasSpanStreamingEnabled(client) ? ROUTER_SPAN_NAME_FALLBACK : 'SvelteKit Route Change',
attributes: {
// TODO(conventions): Replace `'router'` with the `router` span op constant once it is released in `@sentry/conventions`.
[SENTRY_OP]: 'router',
Expand Down
5 changes: 4 additions & 1 deletion packages/sveltekit/src/client/svelte5BrowserTracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import type { Client, Span } from '@sentry/core';
import {
hasSpanStreamingEnabled,
PAGELOAD_SPAN_NAME_FALLBACK,
ROUTER_SPAN_NAME_FALLBACK,
SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN,
SEMANTIC_ATTRIBUTE_SENTRY_SOURCE,
} from '@sentry/core';
Expand Down Expand Up @@ -112,7 +113,9 @@ function _instrumentNavigations(client: Client): void {
);

routingSpan = startInactiveSpan({
name: 'SvelteKit Route Change',
// With span streaming, span names have to be low cardinality, and this span carries no route
// of its own, so it's the fallback.
name: hasSpanStreamingEnabled(client) ? ROUTER_SPAN_NAME_FALLBACK : 'SvelteKit Route Change',
attributes: {
// TODO(conventions): Replace `'router'` with the `router` span op constant once it is released in `@sentry/conventions`.
[SENTRY_OP]: 'router',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,37 @@ describe('browserTracingIntegration', () => {
expect(routingSpanEndSpy).toHaveBeenCalledTimes(1);
});

it('names the routing span with the low cardinality fallback when span streaming is enabled', async () => {
const streamingClient = {
getOptions: () => ({ traceLifecycle: 'stream' }),
on: () => {},
addEventProcessor: () => {},
addIntegration: () => {},
};
const integration = browserTracingIntegration({
instrumentPageLoad: false,
});
// @ts-expect-error - the fakeClient doesn't satisfy Client but that's fine
integration.afterAllSetup(streamingClient);
await vi.dynamicImportSettled();

// TODO(v11): switch to `navigating` from `$app/state`
// @ts-expect-error - navigating is a writable but the types say it's just readable
// eslint-disable-next-line typescript/no-deprecated
navigating.set({
from: { route: { id: '/users' }, url: { pathname: '/users' } },
to: { route: { id: '/users/[id]' }, url: { pathname: '/users/7762', href: 'https://sentry-test.io/users/7762' } },
type: 'link',
});

expect(startInactiveSpanSpy).toHaveBeenCalledWith(
expect.objectContaining({
name: 'Router',
attributes: expect.objectContaining({ [SENTRY_OP]: 'router' }),
}),
);
});

describe('handling same origin and destination navigations', () => {
it("doesn't start a navigation span if the raw navigation origin and destination are equal", async () => {
const integration = browserTracingIntegration({
Expand Down
Loading