diff --git a/dev-packages/e2e-tests/test-applications/remix-v3/tests/client.test.ts b/dev-packages/e2e-tests/test-applications/remix-v3/tests/client.test.ts index 70f77fb7627f..393a3e8e3195 100644 --- a/dev-packages/e2e-tests/test-applications/remix-v3/tests/client.test.ts +++ b/dev-packages/e2e-tests/test-applications/remix-v3/tests/client.test.ts @@ -15,6 +15,25 @@ test('sends a pageload span', async ({ page }) => { expect(span.attributes?.['sentry.segment.name.source']?.value).toBe('route'); }); +test('continues the server trace on a page load', async ({ page }) => { + const serverSpanPromise = waitForStreamedSpan( + APP_NAME, + span => + getSpanOp(span) === 'http.server' && span.is_segment === true && span.attributes?.['http.route']?.value === '/', + ); + const pageloadPromise = waitForStreamedSpan( + APP_NAME, + span => getSpanOp(span) === 'pageload' && span.is_segment === true, + ); + + await page.goto('/'); + + const [serverSpan, pageload] = await Promise.all([serverSpanPromise, pageloadPromise]); + // The document's response carried the trace in `Server-Timing`, which the browser SDK picks up. + expect(pageload.trace_id).toBe(serverSpan.trace_id); + expect(pageload.parent_span_id).toBe(serverSpan.span_id); +}); + test('parameterizes a page load on a route with params', async ({ page }) => { const spanPromise = waitForStreamedSpan(APP_NAME, span => getSpanOp(span) === 'pageload' && span.is_segment === true); diff --git a/packages/remix/src/v3/server/middleware.ts b/packages/remix/src/v3/server/middleware.ts index 7ae9dc989362..f2ce59c48f01 100644 --- a/packages/remix/src/v3/server/middleware.ts +++ b/packages/remix/src/v3/server/middleware.ts @@ -4,6 +4,7 @@ import { getIsolationScope, getRootSpan, getSpanStatusFromHttpCode, + getTraceData, INTERNAL_setSegmentNameSourceIfSegment, type Scope, updateSpanName, @@ -44,9 +45,7 @@ export function sentryRemixMiddleware(matcher: MatcherLike): MiddlewareLike { } setResponseStatus(response); - if (route) { - addRouteHeader(response, context.request, route); - } + addServerTimingHeaders(response, context.request, route); return response; }; @@ -70,19 +69,96 @@ function applyRoute(isolationScope: Scope, route: string, method: string): void } /** - * Tells the browser SDK which route served a document, so it can name its page load and navigation - * spans after the pattern. The browser has no route table of its own. Only HTML responses carry it, - * which is what document loads and the runtime's frame fetches ask for. + * What the browser SDK needs from the server on a document: the trace to continue, so a page load + * joins the request's trace, and the route that served it, so spans are named after the pattern. + * The browser reads both off the navigation timing entry. Only HTML responses carry them, which is + * what document loads and the runtime's frame fetches ask for. */ -function addRouteHeader(response: Response, request: Request, route: string): void { +function addServerTimingHeaders(response: Response, request: Request, route: string | undefined): void { if (!request.headers.get('accept')?.includes('text/html')) { return; } - try { - response.headers.append('Server-Timing', formatRouteTiming(route)); - } catch { - // Immutable headers, e.g. a response passed through from `fetch()`. + + const entries: string[] = []; + // A shared cache would hand this request's trace to every later page load, so a cacheable response + // carries the route only. The route is the same for every request to it. + if (!isSharedCacheable(response)) { + const traceData = getTraceData(); + if (traceData['sentry-trace']) { + entries.push(`sentry-trace;desc="${traceData['sentry-trace']}"`); + } + if (traceData.baggage) { + entries.push(`baggage;desc="${traceData.baggage}"`); + } + } + if (route) { + entries.push(formatRouteTiming(route)); + } + + for (const entry of entries) { + try { + response.headers.append('Server-Timing', entry); + } catch { + // Immutable headers, e.g. a response passed through from `fetch()`. + return; + } + } +} + +// Cache-control fields a CDN reads instead of `Cache-Control`. +const CDN_CACHE_CONTROL_HEADERS = [ + 'cdn-cache-control', + 'cloudflare-cdn-cache-control', + 'vercel-cdn-cache-control', + 'surrogate-control', +]; + +/** + * Whether a cache in front of the app may store this response and serve it to other users. + * + * A CDN that reads a targeted field ignores `Cache-Control`; a cache without targeted support reads + * `Cache-Control`. Any of them storing the response is enough. + */ +function isSharedCacheable(response: Response): boolean { + for (const name of CDN_CACHE_CONTROL_HEADERS) { + const value = response.headers.get(name); + if (value !== null && sharedCachingVerdict(value.toLowerCase()) === true) { + return true; + } + } + + const verdict = sharedCachingVerdict(response.headers.get('cache-control')?.toLowerCase() ?? ''); + if (verdict !== undefined) { + return verdict; + } + // Without a lifetime in `Cache-Control`, a cache falls back to `Expires`. + const expires = response.headers.get('expires'); + return expires !== null && Date.parse(expires) > Date.now(); +} + +/** Whether these directives let a shared cache reuse the response, or `undefined` when they say nothing. */ +function sharedCachingVerdict(directives: string): boolean | undefined { + if (/\b(?:no-store|private)\b/.test(directives)) { + return false; + } + // A stale copy is served even after a zero lifetime. + if (/\b(?:stale-while-revalidate|stale-if-error)\s*=\s*[1-9]/.test(directives)) { + return true; + } + // `s-maxage` overrides `max-age` for shared caches. A zero lifetime means the cache revalidates + // every time, so it never serves this response to anyone else. + const sharedMaxAge = directives.match(/\bs-maxage\s*=\s*(\d+)/)?.[1]; + if (sharedMaxAge !== undefined) { + return Number(sharedMaxAge) > 0; + } + if (/\bpublic\b/.test(directives)) { + return true; + } + const maxAge = directives.match(/\bmax-age\s*=\s*(\d+)/)?.[1]; + if (maxAge !== undefined) { + return Number(maxAge) > 0; } + return undefined; } function setResponseStatus(response: Response): void { diff --git a/packages/remix/test/v3/middleware.test.ts b/packages/remix/test/v3/middleware.test.ts index 88c8f1fc42db..3bc7031300bf 100644 --- a/packages/remix/test/v3/middleware.test.ts +++ b/packages/remix/test/v3/middleware.test.ts @@ -1,4 +1,13 @@ -import { describe, expect, it } from 'vitest'; +import type * as SentryCore from '@sentry/core'; +import { beforeEach, describe, expect, it, vi } from 'vitest'; + +// Trace data as the SDK would produce it for the active request; empty when nothing is active. +const getTraceData = vi.hoisted(() => vi.fn<() => { 'sentry-trace'?: string; baggage?: string }>(() => ({}))); + +vi.mock('@sentry/core', async importOriginal => ({ + ...(await importOriginal()), + getTraceData, +})); import { sentryRemixMiddleware } from '../../src/v3/server/middleware'; import type { MatcherLike, RequestContextLike } from '../../src/v3/types'; @@ -16,6 +25,146 @@ function contextFor(url: string, headers: Record = {}): RequestC } describe('sentryRemixMiddleware', () => { + beforeEach(() => { + getTraceData.mockReturnValue({}); + }); + + it('propagates the trace to HTML responses through Server-Timing, ahead of the route', async () => { + getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1', baggage: 'sentry-trace_id=abc' }); + const middleware = sentryRemixMiddleware(matcherFor('/users/:id')); + + const response = await middleware( + contextFor('http://x/users/1', { accept: 'text/html' }), + async () => new Response(''), + ); + + expect(response.headers.get('server-timing')).toBe( + 'sentry-trace;desc="abc-def-1", baggage;desc="sentry-trace_id=abc", sentry-route;desc="/users/:id"', + ); + }); + + it('propagates the trace even when no route matched', async () => { + getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1' }); + const middleware = sentryRemixMiddleware(matcherFor(undefined)); + + const response = await middleware( + contextFor('http://x/nope', { accept: 'text/html' }), + async () => new Response(''), + ); + + expect(response.headers.get('server-timing')).toBe('sentry-trace;desc="abc-def-1"'); + }); + + it.each([ + ['public', 'public, max-age=60'], + ['s-maxage', 's-maxage=300'], + ['max-age', 'max-age=60'], + ])('leaves the trace off a response a shared cache may store (%s), keeping the route', async (_why, cacheControl) => { + getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1', baggage: 'sentry-trace_id=abc' }); + const middleware = sentryRemixMiddleware(matcherFor('/')); + + const response = await middleware( + contextFor('http://x/', { accept: 'text/html' }), + async () => new Response('', { headers: { 'cache-control': cacheControl } }), + ); + + expect(response.headers.get('server-timing')).toBe('sentry-route;desc="/"'); + }); + + it.each([ + ['stale-while-revalidate after a zero lifetime', { 'cache-control': 'max-age=0, stale-while-revalidate=60' }], + ['stale-if-error after a zero shared lifetime', { 'cache-control': 's-maxage=0, stale-if-error=600' }], + [ + 'CDN-Cache-Control over a private Cache-Control', + { 'cache-control': 'private', 'cdn-cache-control': 'max-age=3600' }, + ], + [ + 'Vercel-CDN-Cache-Control over no-store', + { 'cache-control': 'no-store', 'vercel-cdn-cache-control': 's-maxage=3600' }, + ], + ['Cloudflare-CDN-Cache-Control', { 'cloudflare-cdn-cache-control': 'public, max-age=60' }], + ['Surrogate-Control', { 'surrogate-control': 'max-age=3600' }], + ])('leaves the trace off a response a CDN or stale-serving cache may reuse (%s)', async (_why, headers) => { + getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1' }); + const middleware = sentryRemixMiddleware(matcherFor('/')); + + const response = await middleware( + contextFor('http://x/', { accept: 'text/html' }), + async () => new Response('', { headers }), + ); + + expect(response.headers.get('server-timing')).toBe('sentry-route;desc="/"'); + }); + + it('propagates the trace when the CDN field forbids caching and nothing else allows it', async () => { + getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1' }); + const middleware = sentryRemixMiddleware(matcherFor('/')); + + const response = await middleware( + contextFor('http://x/', { accept: 'text/html' }), + async () => new Response('', { headers: { 'cdn-cache-control': 'max-age=0' } }), + ); + + expect(response.headers.get('server-timing')).toContain('sentry-trace;desc="abc-def-1"'); + }); + + it('leaves the trace off a response with a future Expires and no Cache-Control', async () => { + getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1' }); + const middleware = sentryRemixMiddleware(matcherFor('/')); + + const response = await middleware( + contextFor('http://x/', { accept: 'text/html' }), + async () => new Response('', { headers: { expires: new Date(Date.now() + 60_000).toUTCString() } }), + ); + + expect(response.headers.get('server-timing')).toBe('sentry-route;desc="/"'); + }); + + it('propagates the trace when max-age=0 overrides a future Expires', async () => { + getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1' }); + const middleware = sentryRemixMiddleware(matcherFor('/')); + + const response = await middleware( + contextFor('http://x/', { accept: 'text/html' }), + async () => + new Response('', { + headers: { 'cache-control': 'max-age=0', expires: new Date(Date.now() + 60_000).toUTCString() }, + }), + ); + + expect(response.headers.get('server-timing')).toContain('sentry-trace;desc="abc-def-1"'); + }); + + it('propagates the trace when Expires is in the past', async () => { + getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1' }); + const middleware = sentryRemixMiddleware(matcherFor('/')); + + const response = await middleware( + contextFor('http://x/', { accept: 'text/html' }), + async () => new Response('', { headers: { expires: 'Thu, 01 Jan 1970 00:00:00 GMT' } }), + ); + + expect(response.headers.get('server-timing')).toContain('sentry-trace;desc="abc-def-1"'); + }); + + it.each([ + ['private', 'private, max-age=60'], + ['no-store', 'no-store'], + ['max-age=0', 'max-age=0, must-revalidate'], + ['s-maxage=0', 'max-age=3600, s-maxage=0'], + ['no cache header', undefined], + ])('propagates the trace on a response only this user can get back (%s)', async (_why, cacheControl) => { + getTraceData.mockReturnValue({ 'sentry-trace': 'abc-def-1' }); + const middleware = sentryRemixMiddleware(matcherFor('/')); + + const response = await middleware( + contextFor('http://x/', { accept: 'text/html' }), + async () => new Response('', { headers: cacheControl ? { 'cache-control': cacheControl } : {} }), + ); + + expect(response.headers.get('server-timing')).toContain('sentry-trace;desc="abc-def-1"'); + }); + it('reports the route on HTML responses through Server-Timing', async () => { const middleware = sentryRemixMiddleware(matcherFor('/users/:id')); @@ -60,7 +209,7 @@ describe('sentryRemixMiddleware', () => { expect(response.headers.has('server-timing')).toBe(false); }); - it('adds nothing when no route matched', async () => { + it('adds nothing when no route matched and no trace is active', async () => { const middleware = sentryRemixMiddleware(matcherFor(undefined)); const response = await middleware(