Skip to content

Commit f8024fd

Browse files
Lms24claude
authored andcommitted
fix(remix): Treat static route span names as low-cardinality names (#23504)
Same as #23503 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 4cf58c2 commit f8024fd

7 files changed

Lines changed: 26 additions & 32 deletions

File tree

dev-packages/e2e-tests/test-applications/create-remix-app-express-vite-dev/tests/client-transactions.test.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,12 @@ test('Sends a pageload transaction to Sentry', async ({ page }) => {
1414
expect(transactionEvent.contexts?.trace?.data).toEqual(
1515
expect.objectContaining({
1616
'sentry.origin': 'auto.pageload.remix',
17-
'sentry.source': 'url',
17+
'sentry.source': 'route',
1818
'url.full': expect.stringMatching(/^https?:\/\/localhost:\d+\/$/),
1919
'url.path': '/',
20+
'url.template': '/',
2021
}),
2122
);
22-
// no url.template because the route isn't parameterized (sentry.source: 'url')
23-
expect(transactionEvent.contexts?.trace?.data).not.toHaveProperty('url.template');
2423
});
2524

2625
test('Sends a navigation transaction to Sentry', async ({ page }) => {

dev-packages/e2e-tests/test-applications/create-remix-app-express/tests/client-transactions.test.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,13 +14,12 @@ test('Sends a pageload transaction to Sentry', async ({ page }) => {
1414
expect(transactionEvent.contexts?.trace?.data).toEqual(
1515
expect.objectContaining({
1616
'sentry.origin': 'auto.pageload.remix',
17-
'sentry.source': 'url',
17+
'sentry.source': 'route',
1818
'url.full': expect.stringMatching(/^https?:\/\/localhost:\d+\/$/),
1919
'url.path': '/',
20+
'url.template': '/',
2021
}),
2122
);
22-
// no url.template because the route isn't parameterized (sentry.source: 'url')
23-
expect(transactionEvent.contexts?.trace?.data).not.toHaveProperty('url.template');
2423
});
2524

2625
test('Sends a navigation transaction to Sentry', async ({ page }) => {

dev-packages/e2e-tests/test-applications/create-remix-app-v2/tests/client-transactions.test.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,12 @@ test('Sends a pageload transaction to Sentry', async ({ page }) => {
1313
expect(transactionEvent).toBeDefined();
1414
expect(transactionEvent.contexts?.trace?.data).toEqual(
1515
expect.objectContaining({
16-
'sentry.source': 'url',
16+
'sentry.source': 'route',
1717
'url.full': expect.stringMatching(/^https?:\/\/localhost:\d+\/$/),
1818
'url.path': '/',
19+
'url.template': '/',
1920
}),
2021
);
21-
// no url.template because the route isn't parameterized (sentry.source: 'url')
22-
expect(transactionEvent.contexts?.trace?.data).not.toHaveProperty('url.template');
2322
});
2423

2524
test('Sends a navigation transaction to Sentry', async ({ page }) => {

dev-packages/e2e-tests/test-applications/remix-hydrogen/tests/client-transactions.test.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,15 +16,14 @@ test('Sends a pageload transaction to Sentry', async ({ page }) => {
1616
contexts: {
1717
trace: {
1818
data: {
19-
'sentry.source': 'url',
19+
'sentry.source': 'route',
2020
'url.full': expect.stringMatching(/^https?:\/\/localhost:\d+\/$/),
2121
'url.path': '/',
22+
'url.template': '/',
2223
},
2324
},
2425
},
2526
});
26-
// no url.template because the route isn't parameterized (sentry.source: 'url')
27-
expect(transactionEvent.contexts?.trace?.data).not.toHaveProperty('url.template');
2827
});
2928

3029
test('Sends a navigation transaction to Sentry', async ({ page }) => {

packages/remix/src/client/performance.tsx

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,8 @@ function getInitPathName(): string | undefined {
6262
/**
6363
* Determines the transaction name and source for a route.
6464
* Handles three cases:
65-
* 1. Dynamic routes with manifest (Vite apps): Use parameterized path with source 'route'
66-
* 2. Static routes with manifest (Vite apps): Use pathname with source 'url'
65+
* 1. Routes resolved from the manifest (Vite apps): Use the route template with source 'route'
66+
* 2. Routes the manifest doesn't know, e.g. 404s (Vite apps): Use pathname with source 'url'
6767
* 3. Legacy apps without manifest: Use route ID with source 'route'
6868
*/
6969
function getTransactionNameAndSource(
@@ -73,13 +73,11 @@ function getTransactionNameAndSource(
7373
const parameterizedRoute = pathname ? maybeParameterizeRemixRoute(pathname) : undefined;
7474

7575
if (parameterizedRoute) {
76-
// We have a parameterized route from the manifest (dynamic route)
7776
return { name: parameterizedRoute, source: 'route' };
7877
}
7978

8079
if (hasManifest()) {
81-
// We have a manifest but no parameterization (static route)
82-
// Use the pathname with source 'url'
80+
// The manifest doesn't know this route, so the pathname may well be high cardinality.
8381
return { name: pathname || routeId, source: 'url' };
8482
}
8583

packages/remix/src/client/remixRouteParameterization.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,9 +112,9 @@ function findMatchingRoutes(
112112
): string[] {
113113
const matches: string[] = [];
114114

115-
// Static routes don't need parameterization - return empty to keep source as 'url'
115+
// Static routes don't need parameterization, return the route itself as already parameterized
116116
if (staticRoutes.some(r => r.path === route)) {
117-
return matches;
117+
return [route];
118118
}
119119

120120
// Check dynamic routes
@@ -142,7 +142,7 @@ export function hasManifest(): boolean {
142142
* Parameterize a route using the route manifest.
143143
*
144144
* @param route - The route to parameterize.
145-
* @returns The parameterized route or undefined if no parameterization is needed.
145+
* @returns The parameterized route or undefined if not able to parameterize.
146146
*/
147147
export const maybeParameterizeRemixRoute = (route: string): string | undefined => {
148148
const manifest = getManifest();

packages/remix/test/client/remixRouteParameterization.test.ts

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -25,17 +25,17 @@ describe('maybeParameterizeRemixRoute', () => {
2525
});
2626

2727
describe('when manifest has static routes', () => {
28-
it('should return undefined for static routes', () => {
28+
it('returns the route itself for static routes', () => {
2929
const manifest: RouteManifest = {
3030
staticRoutes: [{ path: '/' }, { path: '/about' }, { path: '/contact' }, { path: '/blog/posts' }],
3131
dynamicRoutes: [],
3232
};
3333
globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
3434

35-
expect(maybeParameterizeRemixRoute('/')).toBeUndefined();
36-
expect(maybeParameterizeRemixRoute('/about')).toBeUndefined();
37-
expect(maybeParameterizeRemixRoute('/contact')).toBeUndefined();
38-
expect(maybeParameterizeRemixRoute('/blog/posts')).toBeUndefined();
35+
expect(maybeParameterizeRemixRoute('/')).toBe('/');
36+
expect(maybeParameterizeRemixRoute('/about')).toBe('/about');
37+
expect(maybeParameterizeRemixRoute('/contact')).toBe('/contact');
38+
expect(maybeParameterizeRemixRoute('/blog/posts')).toBe('/blog/posts');
3939
});
4040
});
4141

@@ -84,8 +84,8 @@ describe('maybeParameterizeRemixRoute', () => {
8484
};
8585
globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
8686

87-
expect(maybeParameterizeRemixRoute('/')).toBeUndefined();
88-
expect(maybeParameterizeRemixRoute('/about')).toBeUndefined();
87+
expect(maybeParameterizeRemixRoute('/')).toBe('/');
88+
expect(maybeParameterizeRemixRoute('/about')).toBe('/about');
8989
});
9090

9191
it('should handle splat/catch-all routes', () => {
@@ -222,7 +222,7 @@ describe('maybeParameterizeRemixRoute', () => {
222222
};
223223
globalWithInjectedManifest._sentryRemixRouteManifest = JSON.stringify(manifest);
224224

225-
expect(maybeParameterizeRemixRoute('/')).toBeUndefined();
225+
expect(maybeParameterizeRemixRoute('/')).toBe('/');
226226
});
227227

228228
it('should handle complex nested dynamic routes', () => {
@@ -246,10 +246,10 @@ describe('maybeParameterizeRemixRoute', () => {
246246

247247
describe('realistic Remix patterns', () => {
248248
it.each([
249-
['/', undefined],
250-
['/about', undefined],
251-
['/contact', undefined],
252-
['/blog/posts', undefined],
249+
['/', '/'],
250+
['/about', '/about'],
251+
['/contact', '/contact'],
252+
['/blog/posts', '/blog/posts'],
253253

254254
['/users/123', '/users/:id'],
255255
['/users/john-doe', '/users/:id'],

0 commit comments

Comments
 (0)