Skip to content

Commit e99e6e1

Browse files
chargomeclaude
andauthored
feat(remix): Propagate the server trace to Remix 3 page loads (#25145)
Browser page loads in Remix 3 started a new trace. The server middleware now adds `sentry-trace` and `baggage` entries to the `Server-Timing` header of HTML responses, ahead of the route entry, the same channel the Remix 2 SDK and Nitro use. The browser SDK already reads both off the navigation timing entry for page loads, so no client change is needed. A response a shared cache may store (`public`, `s-maxage`, or a positive `max-age` without `private`/`no-store`) carries the route entry only: a cached document would otherwise hand one request's trace to every later page load, the ISR problem the Next.js SDK works around on the client. The e2e test asserts that the page load span shares the server span's trace id and has it as parent. Fixes #25137 --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
1 parent 2fdcb23 commit e99e6e1

3 files changed

Lines changed: 257 additions & 13 deletions

File tree

‎dev-packages/e2e-tests/test-applications/remix-v3/tests/client.test.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,25 @@ test('sends a pageload span', async ({ page }) => {
1515
expect(span.attributes?.['sentry.segment.name.source']?.value).toBe('route');
1616
});
1717

18+
test('continues the server trace on a page load', async ({ page }) => {
19+
const serverSpanPromise = waitForStreamedSpan(
20+
APP_NAME,
21+
span =>
22+
getSpanOp(span) === 'http.server' && span.is_segment === true && span.attributes?.['http.route']?.value === '/',
23+
);
24+
const pageloadPromise = waitForStreamedSpan(
25+
APP_NAME,
26+
span => getSpanOp(span) === 'pageload' && span.is_segment === true,
27+
);
28+
29+
await page.goto('/');
30+
31+
const [serverSpan, pageload] = await Promise.all([serverSpanPromise, pageloadPromise]);
32+
// The document's response carried the trace in `Server-Timing`, which the browser SDK picks up.
33+
expect(pageload.trace_id).toBe(serverSpan.trace_id);
34+
expect(pageload.parent_span_id).toBe(serverSpan.span_id);
35+
});
36+
1837
test('parameterizes a page load on a route with params', async ({ page }) => {
1938
const spanPromise = waitForStreamedSpan(APP_NAME, span => getSpanOp(span) === 'pageload' && span.is_segment === true);
2039

‎packages/remix/src/v3/server/middleware.ts‎

Lines changed: 87 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import {
44
getIsolationScope,
55
getRootSpan,
66
getSpanStatusFromHttpCode,
7+
getTraceData,
78
INTERNAL_setSegmentNameSourceIfSegment,
89
type Scope,
910
updateSpanName,
@@ -44,9 +45,7 @@ export function sentryRemixMiddleware(matcher: MatcherLike): MiddlewareLike {
4445
}
4546

4647
setResponseStatus(response);
47-
if (route) {
48-
addRouteHeader(response, context.request, route);
49-
}
48+
addServerTimingHeaders(response, context.request, route);
5049

5150
return response;
5251
};
@@ -70,19 +69,96 @@ function applyRoute(isolationScope: Scope, route: string, method: string): void
7069
}
7170

7271
/**
73-
* Tells the browser SDK which route served a document, so it can name its page load and navigation
74-
* spans after the pattern. The browser has no route table of its own. Only HTML responses carry it,
75-
* which is what document loads and the runtime's frame fetches ask for.
72+
* What the browser SDK needs from the server on a document: the trace to continue, so a page load
73+
* joins the request's trace, and the route that served it, so spans are named after the pattern.
74+
* The browser reads both off the navigation timing entry. Only HTML responses carry them, which is
75+
* what document loads and the runtime's frame fetches ask for.
7676
*/
77-
function addRouteHeader(response: Response, request: Request, route: string): void {
77+
function addServerTimingHeaders(response: Response, request: Request, route: string | undefined): void {
7878
if (!request.headers.get('accept')?.includes('text/html')) {
7979
return;
8080
}
81-
try {
82-
response.headers.append('Server-Timing', formatRouteTiming(route));
83-
} catch {
84-
// Immutable headers, e.g. a response passed through from `fetch()`.
81+
82+
const entries: string[] = [];
83+
// A shared cache would hand this request's trace to every later page load, so a cacheable response
84+
// carries the route only. The route is the same for every request to it.
85+
if (!isSharedCacheable(response)) {
86+
const traceData = getTraceData();
87+
if (traceData['sentry-trace']) {
88+
entries.push(`sentry-trace;desc="${traceData['sentry-trace']}"`);
89+
}
90+
if (traceData.baggage) {
91+
entries.push(`baggage;desc="${traceData.baggage}"`);
92+
}
93+
}
94+
if (route) {
95+
entries.push(formatRouteTiming(route));
96+
}
97+
98+
for (const entry of entries) {
99+
try {
100+
response.headers.append('Server-Timing', entry);
101+
} catch {
102+
// Immutable headers, e.g. a response passed through from `fetch()`.
103+
return;
104+
}
105+
}
106+
}
107+
108+
// Cache-control fields a CDN reads instead of `Cache-Control`.
109+
const CDN_CACHE_CONTROL_HEADERS = [
110+
'cdn-cache-control',
111+
'cloudflare-cdn-cache-control',
112+
'vercel-cdn-cache-control',
113+
'surrogate-control',
114+
];
115+
116+
/**
117+
* Whether a cache in front of the app may store this response and serve it to other users.
118+
*
119+
* A CDN that reads a targeted field ignores `Cache-Control`; a cache without targeted support reads
120+
* `Cache-Control`. Any of them storing the response is enough.
121+
*/
122+
function isSharedCacheable(response: Response): boolean {
123+
for (const name of CDN_CACHE_CONTROL_HEADERS) {
124+
const value = response.headers.get(name);
125+
if (value !== null && sharedCachingVerdict(value.toLowerCase()) === true) {
126+
return true;
127+
}
128+
}
129+
130+
const verdict = sharedCachingVerdict(response.headers.get('cache-control')?.toLowerCase() ?? '');
131+
if (verdict !== undefined) {
132+
return verdict;
133+
}
134+
// Without a lifetime in `Cache-Control`, a cache falls back to `Expires`.
135+
const expires = response.headers.get('expires');
136+
return expires !== null && Date.parse(expires) > Date.now();
137+
}
138+
139+
/** Whether these directives let a shared cache reuse the response, or `undefined` when they say nothing. */
140+
function sharedCachingVerdict(directives: string): boolean | undefined {
141+
if (/\b(?:no-store|private)\b/.test(directives)) {
142+
return false;
143+
}
144+
// A stale copy is served even after a zero lifetime.
145+
if (/\b(?:stale-while-revalidate|stale-if-error)\s*=\s*[1-9]/.test(directives)) {
146+
return true;
147+
}
148+
// `s-maxage` overrides `max-age` for shared caches. A zero lifetime means the cache revalidates
149+
// every time, so it never serves this response to anyone else.
150+
const sharedMaxAge = directives.match(/\bs-maxage\s*=\s*(\d+)/)?.[1];
151+
if (sharedMaxAge !== undefined) {
152+
return Number(sharedMaxAge) > 0;
153+
}
154+
if (/\bpublic\b/.test(directives)) {
155+
return true;
156+
}
157+
const maxAge = directives.match(/\bmax-age\s*=\s*(\d+)/)?.[1];
158+
if (maxAge !== undefined) {
159+
return Number(maxAge) > 0;
85160
}
161+
return undefined;
86162
}
87163

88164
function setResponseStatus(response: Response): void {

‎packages/remix/test/v3/middleware.test.ts‎

Lines changed: 151 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,13 @@
1-
import { describe, expect, it } from 'vitest';
1+
import type * as SentryCore from '@sentry/core';
2+
import { beforeEach, describe, expect, it, vi } from 'vitest';
3+
4+
// Trace data as the SDK would produce it for the active request; empty when nothing is active.
5+
const getTraceData = vi.hoisted(() => vi.fn<() => { 'sentry-trace'?: string; baggage?: string }>(() => ({})));
6+
7+
vi.mock('@sentry/core', async importOriginal => ({
8+
...(await importOriginal<typeof SentryCore>()),
9+
getTraceData,
10+
}));
211

312
import { sentryRemixMiddleware } from '../../src/v3/server/middleware';
413
import type { MatcherLike, RequestContextLike } from '../../src/v3/types';
@@ -16,6 +25,146 @@ function contextFor(url: string, headers: Record<string, string> = {}): RequestC
1625
}
1726

1827
describe('sentryRemixMiddleware', () => {
28+
beforeEach(() => {
29+
getTraceData.mockReturnValue({});
30+
});
31+
32+
it('propagates the trace to HTML responses through Server-Timing, ahead of the route', async () => {
33+
getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1', baggage: 'sentry-trace_id=abc' });
34+
const middleware = sentryRemixMiddleware(matcherFor('/users/:id'));
35+
36+
const response = await middleware(
37+
contextFor('http://x/users/1', { accept: 'text/html' }),
38+
async () => new Response(''),
39+
);
40+
41+
expect(response.headers.get('server-timing')).toBe(
42+
'sentry-trace;desc="abc-def-1", baggage;desc="sentry-trace_id=abc", sentry-route;desc="/users/:id"',
43+
);
44+
});
45+
46+
it('propagates the trace even when no route matched', async () => {
47+
getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1' });
48+
const middleware = sentryRemixMiddleware(matcherFor(undefined));
49+
50+
const response = await middleware(
51+
contextFor('http://x/nope', { accept: 'text/html' }),
52+
async () => new Response(''),
53+
);
54+
55+
expect(response.headers.get('server-timing')).toBe('sentry-trace;desc="abc-def-1"');
56+
});
57+
58+
it.each([
59+
['public', 'public, max-age=60'],
60+
['s-maxage', 's-maxage=300'],
61+
['max-age', 'max-age=60'],
62+
])('leaves the trace off a response a shared cache may store (%s), keeping the route', async (_why, cacheControl) => {
63+
getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1', baggage: 'sentry-trace_id=abc' });
64+
const middleware = sentryRemixMiddleware(matcherFor('/'));
65+
66+
const response = await middleware(
67+
contextFor('http://x/', { accept: 'text/html' }),
68+
async () => new Response('', { headers: { 'cache-control': cacheControl } }),
69+
);
70+
71+
expect(response.headers.get('server-timing')).toBe('sentry-route;desc="/"');
72+
});
73+
74+
it.each([
75+
['stale-while-revalidate after a zero lifetime', { 'cache-control': 'max-age=0, stale-while-revalidate=60' }],
76+
['stale-if-error after a zero shared lifetime', { 'cache-control': 's-maxage=0, stale-if-error=600' }],
77+
[
78+
'CDN-Cache-Control over a private Cache-Control',
79+
{ 'cache-control': 'private', 'cdn-cache-control': 'max-age=3600' },
80+
],
81+
[
82+
'Vercel-CDN-Cache-Control over no-store',
83+
{ 'cache-control': 'no-store', 'vercel-cdn-cache-control': 's-maxage=3600' },
84+
],
85+
['Cloudflare-CDN-Cache-Control', { 'cloudflare-cdn-cache-control': 'public, max-age=60' }],
86+
['Surrogate-Control', { 'surrogate-control': 'max-age=3600' }],
87+
])('leaves the trace off a response a CDN or stale-serving cache may reuse (%s)', async (_why, headers) => {
88+
getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1' });
89+
const middleware = sentryRemixMiddleware(matcherFor('/'));
90+
91+
const response = await middleware(
92+
contextFor('http://x/', { accept: 'text/html' }),
93+
async () => new Response('', { headers }),
94+
);
95+
96+
expect(response.headers.get('server-timing')).toBe('sentry-route;desc="/"');
97+
});
98+
99+
it('propagates the trace when the CDN field forbids caching and nothing else allows it', async () => {
100+
getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1' });
101+
const middleware = sentryRemixMiddleware(matcherFor('/'));
102+
103+
const response = await middleware(
104+
contextFor('http://x/', { accept: 'text/html' }),
105+
async () => new Response('', { headers: { 'cdn-cache-control': 'max-age=0' } }),
106+
);
107+
108+
expect(response.headers.get('server-timing')).toContain('sentry-trace;desc="abc-def-1"');
109+
});
110+
111+
it('leaves the trace off a response with a future Expires and no Cache-Control', async () => {
112+
getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1' });
113+
const middleware = sentryRemixMiddleware(matcherFor('/'));
114+
115+
const response = await middleware(
116+
contextFor('http://x/', { accept: 'text/html' }),
117+
async () => new Response('', { headers: { expires: new Date(Date.now() + 60_000).toUTCString() } }),
118+
);
119+
120+
expect(response.headers.get('server-timing')).toBe('sentry-route;desc="/"');
121+
});
122+
123+
it('propagates the trace when max-age=0 overrides a future Expires', async () => {
124+
getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1' });
125+
const middleware = sentryRemixMiddleware(matcherFor('/'));
126+
127+
const response = await middleware(
128+
contextFor('http://x/', { accept: 'text/html' }),
129+
async () =>
130+
new Response('', {
131+
headers: { 'cache-control': 'max-age=0', expires: new Date(Date.now() + 60_000).toUTCString() },
132+
}),
133+
);
134+
135+
expect(response.headers.get('server-timing')).toContain('sentry-trace;desc="abc-def-1"');
136+
});
137+
138+
it('propagates the trace when Expires is in the past', async () => {
139+
getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1' });
140+
const middleware = sentryRemixMiddleware(matcherFor('/'));
141+
142+
const response = await middleware(
143+
contextFor('http://x/', { accept: 'text/html' }),
144+
async () => new Response('', { headers: { expires: 'Thu, 01 Jan 1970 00:00:00 GMT' } }),
145+
);
146+
147+
expect(response.headers.get('server-timing')).toContain('sentry-trace;desc="abc-def-1"');
148+
});
149+
150+
it.each([
151+
['private', 'private, max-age=60'],
152+
['no-store', 'no-store'],
153+
['max-age=0', 'max-age=0, must-revalidate'],
154+
['s-maxage=0', 'max-age=3600, s-maxage=0'],
155+
['no cache header', undefined],
156+
])('propagates the trace on a response only this user can get back (%s)', async (_why, cacheControl) => {
157+
getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1' });
158+
const middleware = sentryRemixMiddleware(matcherFor('/'));
159+
160+
const response = await middleware(
161+
contextFor('http://x/', { accept: 'text/html' }),
162+
async () => new Response('', { headers: cacheControl ? { 'cache-control': cacheControl } : {} }),
163+
);
164+
165+
expect(response.headers.get('server-timing')).toContain('sentry-trace;desc="abc-def-1"');
166+
});
167+
19168
it('reports the route on HTML responses through Server-Timing', async () => {
20169
const middleware = sentryRemixMiddleware(matcherFor('/users/:id'));
21170

@@ -60,7 +209,7 @@ describe('sentryRemixMiddleware', () => {
60209
expect(response.headers.has('server-timing')).toBe(false);
61210
});
62211

63-
it('adds nothing when no route matched', async () => {
212+
it('adds nothing when no route matched and no trace is active', async () => {
64213
const middleware = sentryRemixMiddleware(matcherFor(undefined));
65214

66215
const response = await middleware(

0 commit comments

Comments
 (0)