From d7b27de3229f13ce373e39c1e62679fe24eacc22 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 5 Sep 2025 09:01:45 +0200 Subject: [PATCH 1/7] fix(astro): Ensure traces are correctly propagated for static routes This refactors the astro middleware a bit to be more correct & future proof. Right now, http.server spans are not emitted by the node http integration because import-in-the-middle does not work with Astro. So we emit our own. This PR makes astro ready to also handle the base http.server spans and enhance them with routing information. It also handles static routes better: these do not emit spans anymore now, and do not continue traces, but instead only propagate the parametrized route to the client, no trace data. --- .../test-applications/astro-4/package.json | 2 +- .../astro-4/playwright.config.mjs | 2 +- .../astro-4/tests/errors.client.test.ts | 1 - .../astro-4/tests/tracing.static.test.ts | 16 +- .../test-applications/astro-5/package.json | 1 + .../astro-5/playwright.config.mjs | 2 +- .../astro-5/tests/errors.client.test.ts | 1 - .../tests/tracing.serverIslands.test.ts | 21 +- .../astro-5/tests/tracing.static.test.ts | 16 +- packages/astro/src/server/middleware.ts | 449 +++++++++++------- packages/astro/src/server/sdk.ts | 27 +- 11 files changed, 333 insertions(+), 205 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/astro-4/package.json b/dev-packages/e2e-tests/test-applications/astro-4/package.json index d355f35e6315..df0750ee226c 100644 --- a/dev-packages/e2e-tests/test-applications/astro-4/package.json +++ b/dev-packages/e2e-tests/test-applications/astro-4/package.json @@ -4,9 +4,9 @@ "version": "0.0.1", "scripts": { "dev": "astro dev --force", - "start": "astro dev", "build": "astro check && astro build", "preview": "astro preview", + "start": "node ./dist/server/entry.mjs", "astro": "astro", "test:build": "pnpm install && pnpm build", "test:assert": "TEST_ENV=production playwright test" diff --git a/dev-packages/e2e-tests/test-applications/astro-4/playwright.config.mjs b/dev-packages/e2e-tests/test-applications/astro-4/playwright.config.mjs index cd6ed611fb4a..ae58e4ff3ddc 100644 --- a/dev-packages/e2e-tests/test-applications/astro-4/playwright.config.mjs +++ b/dev-packages/e2e-tests/test-applications/astro-4/playwright.config.mjs @@ -7,7 +7,7 @@ if (!testEnv) { } const config = getPlaywrightConfig({ - startCommand: 'node ./dist/server/entry.mjs', + startCommand: 'pnpm start', }); export default config; diff --git a/dev-packages/e2e-tests/test-applications/astro-4/tests/errors.client.test.ts b/dev-packages/e2e-tests/test-applications/astro-4/tests/errors.client.test.ts index 730122f5c208..afa786c9fcd1 100644 --- a/dev-packages/e2e-tests/test-applications/astro-4/tests/errors.client.test.ts +++ b/dev-packages/e2e-tests/test-applications/astro-4/tests/errors.client.test.ts @@ -70,7 +70,6 @@ test.describe('client-side errors', () => { contexts: { trace: { trace_id: expect.stringMatching(/[a-f0-9]{32}/), - parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), }, }, diff --git a/dev-packages/e2e-tests/test-applications/astro-4/tests/tracing.static.test.ts b/dev-packages/e2e-tests/test-applications/astro-4/tests/tracing.static.test.ts index 30bcbee1a026..7af2115dc29e 100644 --- a/dev-packages/e2e-tests/test-applications/astro-4/tests/tracing.static.test.ts +++ b/dev-packages/e2e-tests/test-applications/astro-4/tests/tracing.static.test.ts @@ -21,14 +21,14 @@ test.describe('tracing in static/pre-rendered routes', () => { const clientPageloadTraceId = clientPageloadTxn.contexts?.trace?.trace_id; const clientPageloadParentSpanId = clientPageloadTxn.contexts?.trace?.parent_span_id; - const sentryTraceMetaTagContent = await page.locator('meta[name="sentry-trace"]').getAttribute('content'); - const baggageMetaTagContent = await page.locator('meta[name="baggage"]').getAttribute('content'); + const sentryTraceMetaTags = await page.locator('meta[name="sentry-trace"]').count(); + expect(sentryTraceMetaTags).toBe(0); - const [metaTraceId, metaParentSpanId, metaSampled] = sentryTraceMetaTagContent?.split('-') || []; + const baggageMetaTags = await page.locator('meta[name="baggage"]').count(); + expect(baggageMetaTags).toBe(0); expect(clientPageloadTraceId).toMatch(/[a-f0-9]{32}/); - expect(clientPageloadParentSpanId).toMatch(/[a-f0-9]{16}/); - expect(metaSampled).toBe('1'); + expect(clientPageloadParentSpanId).toBeUndefined(); expect(clientPageloadTxn).toMatchObject({ contexts: { @@ -40,9 +40,8 @@ test.describe('tracing in static/pre-rendered routes', () => { }), op: 'pageload', origin: 'auto.pageload.astro', - parent_span_id: metaParentSpanId, span_id: expect.stringMatching(/[a-f0-9]{16}/), - trace_id: metaTraceId, + trace_id: expect.stringMatching(/[a-f0-9]{32}/), }, }, platform: 'javascript', @@ -53,9 +52,6 @@ test.describe('tracing in static/pre-rendered routes', () => { type: 'transaction', }); - expect(baggageMetaTagContent).toContain('sentry-transaction=GET%20%2Ftest-static'); // URL-encoded for 'GET /test-static' - expect(baggageMetaTagContent).toContain('sentry-sampled=true'); - await page.waitForTimeout(1000); // wait another sec to ensure no server transaction is sent }); }); diff --git a/dev-packages/e2e-tests/test-applications/astro-5/package.json b/dev-packages/e2e-tests/test-applications/astro-5/package.json index 1c669cbc041b..6695d3c9434c 100644 --- a/dev-packages/e2e-tests/test-applications/astro-5/package.json +++ b/dev-packages/e2e-tests/test-applications/astro-5/package.json @@ -7,6 +7,7 @@ "build": "astro build", "preview": "astro preview", "astro": "astro", + "start": "node ./dist/server/entry.mjs", "test:build": "pnpm install && pnpm build", "test:assert": "TEST_ENV=production playwright test" }, diff --git a/dev-packages/e2e-tests/test-applications/astro-5/playwright.config.mjs b/dev-packages/e2e-tests/test-applications/astro-5/playwright.config.mjs index cd6ed611fb4a..ae58e4ff3ddc 100644 --- a/dev-packages/e2e-tests/test-applications/astro-5/playwright.config.mjs +++ b/dev-packages/e2e-tests/test-applications/astro-5/playwright.config.mjs @@ -7,7 +7,7 @@ if (!testEnv) { } const config = getPlaywrightConfig({ - startCommand: 'node ./dist/server/entry.mjs', + startCommand: 'pnpm start', }); export default config; diff --git a/dev-packages/e2e-tests/test-applications/astro-5/tests/errors.client.test.ts b/dev-packages/e2e-tests/test-applications/astro-5/tests/errors.client.test.ts index 19e9051ddf69..09ef627bf56c 100644 --- a/dev-packages/e2e-tests/test-applications/astro-5/tests/errors.client.test.ts +++ b/dev-packages/e2e-tests/test-applications/astro-5/tests/errors.client.test.ts @@ -70,7 +70,6 @@ test.describe('client-side errors', () => { contexts: { trace: { trace_id: expect.stringMatching(/[a-f0-9]{32}/), - parent_span_id: expect.stringMatching(/[a-f0-9]{16}/), span_id: expect.stringMatching(/[a-f0-9]{16}/), }, }, diff --git a/dev-packages/e2e-tests/test-applications/astro-5/tests/tracing.serverIslands.test.ts b/dev-packages/e2e-tests/test-applications/astro-5/tests/tracing.serverIslands.test.ts index 202051e7a57e..c7496d4e6247 100644 --- a/dev-packages/e2e-tests/test-applications/astro-5/tests/tracing.serverIslands.test.ts +++ b/dev-packages/e2e-tests/test-applications/astro-5/tests/tracing.serverIslands.test.ts @@ -4,28 +4,27 @@ import { waitForTransaction } from '@sentry-internal/test-utils'; test.describe('tracing in static routes with server islands', () => { test('only sends client pageload transaction and server island endpoint transaction', async ({ page }) => { const clientPageloadTxnPromise = waitForTransaction('astro-5', txnEvent => { - return txnEvent?.transaction === '/server-island'; + return txnEvent.transaction === '/server-island'; }); const serverIslandEndpointTxnPromise = waitForTransaction('astro-5', evt => { - return !!evt.transaction?.startsWith('GET /_server-islands'); + return evt.transaction === 'GET /_server-islands/[name]'; }); await page.goto('/server-island'); const clientPageloadTxn = await clientPageloadTxnPromise; - const clientPageloadTraceId = clientPageloadTxn.contexts?.trace?.trace_id; const clientPageloadParentSpanId = clientPageloadTxn.contexts?.trace?.parent_span_id; - const sentryTraceMetaTagContent = await page.locator('meta[name="sentry-trace"]').getAttribute('content'); - const baggageMetaTagContent = await page.locator('meta[name="baggage"]').getAttribute('content'); + const sentryTraceMetaTags = await page.locator('meta[name="sentry-trace"]').count(); + expect(sentryTraceMetaTags).toBe(0); - const [metaTraceId, metaParentSpanId, metaSampled] = sentryTraceMetaTagContent?.split('-') || []; + const baggageMetaTags = await page.locator('meta[name="baggage"]').count(); + expect(baggageMetaTags).toBe(0); expect(clientPageloadTraceId).toMatch(/[a-f0-9]{32}/); - expect(clientPageloadParentSpanId).toMatch(/[a-f0-9]{16}/); - expect(metaSampled).toBe('1'); + expect(clientPageloadParentSpanId).toBeUndefined(); expect(clientPageloadTxn).toMatchObject({ contexts: { @@ -37,9 +36,8 @@ test.describe('tracing in static routes with server islands', () => { }), op: 'pageload', origin: 'auto.pageload.astro', - parent_span_id: metaParentSpanId, span_id: expect.stringMatching(/[a-f0-9]{16}/), - trace_id: metaTraceId, + trace_id: clientPageloadTraceId, }, }, platform: 'javascript', @@ -63,9 +61,6 @@ test.describe('tracing in static routes with server islands', () => { ]), ); - expect(baggageMetaTagContent).toContain('sentry-transaction=GET%20%2Fserver-island'); // URL-encoded for 'GET /server-island' - expect(baggageMetaTagContent).toContain('sentry-sampled=true'); - const serverIslandEndpointTxn = await serverIslandEndpointTxnPromise; expect(serverIslandEndpointTxn).toMatchObject({ diff --git a/dev-packages/e2e-tests/test-applications/astro-5/tests/tracing.static.test.ts b/dev-packages/e2e-tests/test-applications/astro-5/tests/tracing.static.test.ts index 7593d6823d9b..4563dc2f306d 100644 --- a/dev-packages/e2e-tests/test-applications/astro-5/tests/tracing.static.test.ts +++ b/dev-packages/e2e-tests/test-applications/astro-5/tests/tracing.static.test.ts @@ -21,14 +21,14 @@ test.describe('tracing in static/pre-rendered routes', () => { const clientPageloadTraceId = clientPageloadTxn.contexts?.trace?.trace_id; const clientPageloadParentSpanId = clientPageloadTxn.contexts?.trace?.parent_span_id; - const sentryTraceMetaTagContent = await page.locator('meta[name="sentry-trace"]').getAttribute('content'); - const baggageMetaTagContent = await page.locator('meta[name="baggage"]').getAttribute('content'); + const sentryTraceMetaTags = await page.locator('meta[name="sentry-trace"]').count(); + expect(sentryTraceMetaTags).toBe(0); - const [metaTraceId, metaParentSpanId, metaSampled] = sentryTraceMetaTagContent?.split('-') || []; + const baggageMetaTags = await page.locator('meta[name="baggage"]').count(); + expect(baggageMetaTags).toBe(0); expect(clientPageloadTraceId).toMatch(/[a-f0-9]{32}/); - expect(clientPageloadParentSpanId).toMatch(/[a-f0-9]{16}/); - expect(metaSampled).toBe('1'); + expect(clientPageloadParentSpanId).toBeUndefined(); expect(clientPageloadTxn).toMatchObject({ contexts: { @@ -40,9 +40,8 @@ test.describe('tracing in static/pre-rendered routes', () => { }), op: 'pageload', origin: 'auto.pageload.astro', - parent_span_id: metaParentSpanId, span_id: expect.stringMatching(/[a-f0-9]{16}/), - trace_id: metaTraceId, + trace_id: expect.stringMatching(/[a-f0-9]{32}/), }, }, platform: 'javascript', @@ -53,9 +52,6 @@ test.describe('tracing in static/pre-rendered routes', () => { type: 'transaction', }); - expect(baggageMetaTagContent).toContain('sentry-transaction=GET%20%2Ftest-static'); // URL-encoded for 'GET /test-static' - expect(baggageMetaTagContent).toContain('sentry-sampled=true'); - await page.waitForTimeout(1000); // wait another sec to ensure no server transaction is sent }); }); diff --git a/packages/astro/src/server/middleware.ts b/packages/astro/src/server/middleware.ts index fbf6720c23b8..4f90a413f263 100644 --- a/packages/astro/src/server/middleware.ts +++ b/packages/astro/src/server/middleware.ts @@ -1,9 +1,12 @@ -import type { RequestEventData, Scope, SpanAttributes } from '@sentry/core'; +/* eslint-disable max-lines */ +import type { Span, SpanAttributes } from '@sentry/core'; import { addNonEnumerableProperty, - extractQueryParamsFromUrl, flushIfServerless, + getIsolationScope, + getRootSpan, objectify, + spanToJSON, stripUrlQueryAndFragment, winterCGRequestToRequestData, } from '@sentry/core'; @@ -66,23 +69,60 @@ export const handleRequest: (options?: MiddlewareOptions) => MiddlewareResponseH }; return async (ctx, next) => { - // if there is an active span, we know that this handle call is nested and hence - // we don't create a new domain for it. If we created one, nested server calls would - // create new transactions instead of adding a child span to the currently active span. - if (getActiveSpan()) { - return instrumentRequest(ctx, next, handlerOptions); + // If no Sentry client exists, just bail + // Apart from the case when no Sentry.init() is called at all, this also happens + // if a prerendered page is hit first before a ssr page is called + // For regular prerendered pages, this is fine as we do not want to instrument them at runtime anyhow + // BUT for server-islands requests on a static page, this can be problematic... + // TODO: Today, this leads to inconsistent behavior: If a prerendered page is hit first (before _any_ ssr page is called), + // Sentry.init() has not been called yet (as this is only injected in SSR pages), so server-island requests are not instrumented + // If any SSR route is hit before, the client will already be set up and everything will work as expected :O + // To reproduce this: Run the astro-5 "tracing.serverIslands.test" only + if (!getClient()) { + return next(); } - return withIsolationScope(isolationScope => { - return instrumentRequest(ctx, next, handlerOptions, isolationScope); - }); + + const isDynamicPageRequest = checkIsDynamicPageRequest(ctx); + + // For static (prerendered) routes, we only want to inject the parametrized route meta tags + if (!isDynamicPageRequest) { + return handleStaticRoute(ctx, next); + } + + const activeSpan = getActiveSpan(); + const rootSpan = activeSpan ? getRootSpan(activeSpan) : undefined; + + // if there is an active span, we just want to enhance it with routing data etc. + if (rootSpan && spanToJSON(rootSpan).op === 'http.server') { + return enhanceHttpServerSpan(ctx, next, rootSpan); + } + + return instrumentRequestStartHttpServerSpan(ctx, next, handlerOptions); }; }; -async function instrumentRequest( +async function handleStaticRoute( ctx: Parameters[0], next: Parameters[1], - options: MiddlewareOptions, - isolationScope?: Scope, +): Promise { + const parametrizedRoute = getParametrizedRoute(ctx); + try { + const originalResponse = await next(); + + // We never want to continue a trace here, so we do not inject trace data + // But we do want to inject the parametrized route, as this is used for client-side route parametrization + const metaTagsStr = getMetaTagsStr({ injectTraceData: false, parametrizedRoute }); + return injectMetaTagsInResponse(originalResponse, metaTagsStr); + } catch (e) { + sendErrorToSentry(e); + throw e; + } +} + +async function enhanceHttpServerSpan( + ctx: Parameters[0], + next: Parameters[1], + rootSpan: Span, ): Promise { // Make sure we don't accidentally double wrap (e.g. user added middleware and integration auto added it) const locals = ctx.locals as AstroLocalsWithSentry | undefined; @@ -93,179 +133,171 @@ async function instrumentRequest( addNonEnumerableProperty(locals, '__sentry_wrapped__', true); } - const isDynamicPageRequest = checkIsDynamicPageRequest(ctx); - const request = ctx.request; + const isolationScope = getIsolationScope(); + const method = request.method; - const { method, headers } = isDynamicPageRequest - ? request - : // headers can only be accessed in dynamic routes. Accessing `request.headers` in a static route - // will make the server log a warning. - { method: request.method, headers: undefined }; + try { + const parametrizedRoute = getParametrizedRoute(ctx); - return continueTrace( - { - sentryTrace: headers?.get('sentry-trace') || undefined, - baggage: headers?.get('baggage'), - }, - async () => { - getCurrentScope().setSDKProcessingMetadata({ - // We store the request on the current scope, not isolation scope, - // because we may have multiple requests nested inside each other - normalizedRequest: (isDynamicPageRequest - ? winterCGRequestToRequestData(request) - : { - method, - url: request.url, - query_string: extractQueryParamsFromUrl(request.url), - }) satisfies RequestEventData, - }); - - if (options.trackClientIp && isDynamicPageRequest) { - getCurrentScope().setUser({ ip_address: ctx.clientAddress }); - } - - try { - // `routePattern` is available after Astro 5 - const contextWithRoutePattern = ctx as Parameters[0] & { routePattern?: string }; - const rawRoutePattern = contextWithRoutePattern.routePattern; - - // @ts-expect-error Implicit any on Symbol.for (This is available in Astro 5) - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - const routesFromManifest = ctx?.[Symbol.for('context.routes')]?.manifest?.routes; - - // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access - const matchedRouteSegmentsFromManifest = routesFromManifest?.find( - (route: { routeData?: { route?: string } }) => route?.routeData?.route === rawRoutePattern, - )?.routeData?.segments; - - const parametrizedRoute = - // Astro v5 - Joining the segments to get the correct casing of the parametrized route - (matchedRouteSegmentsFromManifest && joinRouteSegments(matchedRouteSegmentsFromManifest)) || - // Fallback (Astro v4 and earlier) - interpolateRouteFromUrlAndParams(ctx.url.pathname, ctx.params); - - const source = parametrizedRoute ? 'route' : 'url'; - // storing res in a variable instead of directly returning is necessary to - // invoke the catch block if next() throws - - const attributes: SpanAttributes = { - [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.astro', - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source, - method, - url: stripUrlQueryAndFragment(ctx.url.href), - }; - - if (ctx.url.search) { - attributes['http.query'] = ctx.url.search; - } + rootSpan.setAttributes({ + // This is here for backwards compatibility, we used to set this here before + method, + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.astro', + }); - if (ctx.url.hash) { - attributes['http.fragment'] = ctx.url.hash; - } + if (!parametrizedRoute) { + return next(); + } - isolationScope?.setTransactionName(`${method} ${parametrizedRoute || ctx.url.pathname}`); - - const res = await startSpan( - { - attributes, - name: `${method} ${parametrizedRoute || ctx.url.pathname}`, - op: 'http.server', - }, - async span => { - try { - const originalResponse = await next(); - if (originalResponse.status) { - setHttpStatus(span, originalResponse.status); - } + rootSpan.setAttributes({ + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + 'http.route': parametrizedRoute, + }); - const client = getClient(); - const contentType = originalResponse.headers.get('content-type'); + isolationScope.setTransactionName(`${method} ${parametrizedRoute}`); - const isPageloadRequest = contentType?.startsWith('text/html'); - if (!isPageloadRequest || !client) { - return originalResponse; - } + try { + const originalResponse = await next(); + const metaTagsStr = getMetaTagsStr({ injectTraceData: true, parametrizedRoute }); + return injectMetaTagsInResponse(originalResponse, metaTagsStr); + } catch (e) { + sendErrorToSentry(e); + throw e; + } + } finally { + await flushIfServerless(); + } - // Type case necessary b/c the body's ReadableStream type doesn't include - // the async iterator that is actually available in Node - // We later on use the async iterator to read the body chunks - // see https://github.com/microsoft/TypeScript/issues/39051 - const originalBody = originalResponse.body as NodeJS.ReadableStream | null; - if (!originalBody) { - return originalResponse; - } + // TODO: flush if serverless (first extract function) +} - const decoder = new TextDecoder(); - - const newResponseStream = new ReadableStream({ - start: async controller => { - // Assign to a new variable to avoid TS losing the narrower type checked above. - const body = originalBody; - - async function* bodyReporter(): AsyncGenerator { - try { - for await (const chunk of body) { - yield chunk; - } - } catch (e) { - // Report stream errors coming from user code or Astro rendering. - sendErrorToSentry(e); - throw e; - } - } - - try { - for await (const chunk of bodyReporter()) { - const html = typeof chunk === 'string' ? chunk : decoder.decode(chunk, { stream: true }); - const modifiedHtml = addMetaTagToHead(html, parametrizedRoute); - controller.enqueue(new TextEncoder().encode(modifiedHtml)); - } - } catch (e) { - controller.error(e); - } finally { - controller.close(); - } - }, - }); - - return new Response(newResponseStream, originalResponse); - } catch (e) { - sendErrorToSentry(e); - throw e; - } - }, - ); - return res; - } finally { - await flushIfServerless(); - } - // TODO: flush if serverless (first extract function) - }, - ); +async function instrumentRequestStartHttpServerSpan( + ctx: Parameters[0], + next: Parameters[1], + options: MiddlewareOptions, +): Promise { + // Make sure we don't accidentally double wrap (e.g. user added middleware and integration auto added it) + const locals = ctx.locals as AstroLocalsWithSentry | undefined; + if (locals?.__sentry_wrapped__) { + return next(); + } + if (locals) { + addNonEnumerableProperty(locals, '__sentry_wrapped__', true); + } + + const request = ctx.request; + + // Note: We guard outside of this function call that the request is dynamic + // accessing headers on a static route would throw + const { method, headers } = request; + + return withIsolationScope(isolationScope => { + return continueTrace( + { + sentryTrace: headers?.get('sentry-trace') || undefined, + baggage: headers?.get('baggage'), + }, + async () => { + getCurrentScope().setSDKProcessingMetadata({ + // We store the request on the current scope, not isolation scope, + // because we may have multiple requests nested inside each other + normalizedRequest: winterCGRequestToRequestData(request), + }); + + if (options.trackClientIp) { + isolationScope.setUser({ ip_address: ctx.clientAddress }); + } + + try { + const parametrizedRoute = getParametrizedRoute(ctx); + + const source = parametrizedRoute ? 'route' : 'url'; + // storing res in a variable instead of directly returning is necessary to + // invoke the catch block if next() throws + + const attributes: SpanAttributes = { + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.astro', + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source, + method, + url: stripUrlQueryAndFragment(ctx.url.href), + }; + + if (parametrizedRoute) { + attributes['http.route'] = parametrizedRoute; + } + + if (ctx.url.search) { + attributes['http.query'] = ctx.url.search; + } + + if (ctx.url.hash) { + attributes['http.fragment'] = ctx.url.hash; + } + + isolationScope.setTransactionName(`${method} ${parametrizedRoute || ctx.url.pathname}`); + + const res = await startSpan( + { + attributes, + name: `${method} ${parametrizedRoute || ctx.url.pathname}`, + op: 'http.server', + }, + async span => { + try { + const originalResponse = await next(); + if (originalResponse.status) { + setHttpStatus(span, originalResponse.status); + } + + const metaTagsStr = getMetaTagsStr({ injectTraceData: true, parametrizedRoute }); + return injectMetaTagsInResponse(originalResponse, metaTagsStr); + } catch (e) { + sendErrorToSentry(e); + throw e; + } + }, + ); + return res; + } finally { + await flushIfServerless(); + } + // TODO: flush if serverless (first extract function) + }, + ); + }); } /** * This function optimistically assumes that the HTML coming in chunks will not be split * within the tag. If this still happens, we simply won't replace anything. */ -function addMetaTagToHead(htmlChunk: string, parametrizedRoute?: string): string { - if (typeof htmlChunk !== 'string') { +function addMetaTagToHead(htmlChunk: string, metaTagsStr: string): string { + if (typeof htmlChunk !== 'string' || !metaTagsStr) { return htmlChunk; } - const metaTags = parametrizedRoute - ? `${getTraceMetaTags()}\n\n` - : getTraceMetaTags(); - - if (!metaTags) { - return htmlChunk; - } - - const content = `${metaTags}`; + const content = `${metaTagsStr}`; return htmlChunk.replace('', content); } +function getMetaTagsStr({ + injectTraceData, + parametrizedRoute, +}: { + injectTraceData: boolean; + parametrizedRoute: string | undefined; +}): string { + const parts = []; + if (injectTraceData) { + parts.push(getTraceMetaTags()); + } + if (parametrizedRoute) { + parts.push(``); + } + return parts.join('\n'); +} + /** * Interpolates the route from the URL and the passed params. * Best we can do to get a route name instead of a raw URL. @@ -376,3 +408,90 @@ function joinRouteSegments(segments: RoutePart[][]): string { return `/${parthArray.join('/')}`; } + +function getParametrizedRoute( + ctx: Parameters[0] & { routePattern?: string }, +): string | undefined { + try { + // `routePattern` is available after Astro 5 + const contextWithRoutePattern = ctx; + const rawRoutePattern = contextWithRoutePattern.routePattern; + + // @ts-expect-error Implicit any on Symbol.for (This is available in Astro 5) + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + const routesFromManifest = ctx?.[Symbol.for('context.routes')]?.manifest?.routes; + + // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access + const matchedRouteSegmentsFromManifest = routesFromManifest?.find( + (route: { routeData?: { route?: string } }) => route?.routeData?.route === rawRoutePattern, + )?.routeData?.segments; + + return ( + // Astro v5 - Joining the segments to get the correct casing of the parametrized route + (matchedRouteSegmentsFromManifest && joinRouteSegments(matchedRouteSegmentsFromManifest)) || + // Fallback (Astro v4 and earlier) + interpolateRouteFromUrlAndParams(ctx.url.pathname, ctx.params) + ); + } catch { + return undefined; + } +} + +function injectMetaTagsInResponse(originalResponse: Response, metaTagsStr: string): Response { + try { + const client = getClient(); + const contentType = originalResponse.headers.get('content-type'); + + const isPageloadRequest = contentType?.startsWith('text/html'); + if (!isPageloadRequest || !client) { + return originalResponse; + } + + // Type case necessary b/c the body's ReadableStream type doesn't include + // the async iterator that is actually available in Node + // We later on use the async iterator to read the body chunks + // see https://github.com/microsoft/TypeScript/issues/39051 + const originalBody = originalResponse.body as NodeJS.ReadableStream | null; + if (!originalBody) { + return originalResponse; + } + + const decoder = new TextDecoder(); + + const newResponseStream = new ReadableStream({ + start: async controller => { + // Assign to a new variable to avoid TS losing the narrower type checked above. + const body = originalBody; + + async function* bodyReporter(): AsyncGenerator { + try { + for await (const chunk of body) { + yield chunk; + } + } catch (e) { + // Report stream errors coming from user code or Astro rendering. + sendErrorToSentry(e); + throw e; + } + } + + try { + for await (const chunk of bodyReporter()) { + const html = typeof chunk === 'string' ? chunk : decoder.decode(chunk, { stream: true }); + const modifiedHtml = addMetaTagToHead(html, metaTagsStr); + controller.enqueue(new TextEncoder().encode(modifiedHtml)); + } + } catch (e) { + controller.error(e); + } finally { + controller.close(); + } + }, + }); + + return new Response(newResponseStream, originalResponse); + } catch (e) { + sendErrorToSentry(e); + throw e; + } +} diff --git a/packages/astro/src/server/sdk.ts b/packages/astro/src/server/sdk.ts index 884747dcf72a..25dbb9416fe6 100644 --- a/packages/astro/src/server/sdk.ts +++ b/packages/astro/src/server/sdk.ts @@ -1,5 +1,5 @@ import { applySdkMetadata } from '@sentry/core'; -import type { NodeClient, NodeOptions } from '@sentry/node'; +import type { Event, NodeClient, NodeOptions } from '@sentry/node'; import { init as initNodeSdk } from '@sentry/node'; /** @@ -13,5 +13,28 @@ export function init(options: NodeOptions): NodeClient | undefined { applySdkMetadata(opts, 'astro', ['astro', 'node']); - return initNodeSdk(opts); + const client = initNodeSdk(opts); + + client?.addEventProcessor( + Object.assign( + (event: Event) => { + // For http.server spans that did not go though the astro middleware, + // we want to drop them + // this is the case with http.server spans of prerendered pages + // we do not care about those, as they are effectively static + if ( + event.type === 'transaction' && + event.contexts?.trace?.op === 'http.server' && + event.contexts?.trace?.origin === 'auto.http.otel.http' + ) { + return null; + } + + return event; + }, + { id: 'AstroHttpEventProcessor' }, + ), + ); + + return client; } From 635f73fb42c5c204103da063c4a931ff84d0df2b Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 5 Sep 2025 11:20:25 +0200 Subject: [PATCH 2/7] remove unneeded thing --- packages/astro/src/server/sdk.ts | 27 ++------------------------- 1 file changed, 2 insertions(+), 25 deletions(-) diff --git a/packages/astro/src/server/sdk.ts b/packages/astro/src/server/sdk.ts index 25dbb9416fe6..884747dcf72a 100644 --- a/packages/astro/src/server/sdk.ts +++ b/packages/astro/src/server/sdk.ts @@ -1,5 +1,5 @@ import { applySdkMetadata } from '@sentry/core'; -import type { Event, NodeClient, NodeOptions } from '@sentry/node'; +import type { NodeClient, NodeOptions } from '@sentry/node'; import { init as initNodeSdk } from '@sentry/node'; /** @@ -13,28 +13,5 @@ export function init(options: NodeOptions): NodeClient | undefined { applySdkMetadata(opts, 'astro', ['astro', 'node']); - const client = initNodeSdk(opts); - - client?.addEventProcessor( - Object.assign( - (event: Event) => { - // For http.server spans that did not go though the astro middleware, - // we want to drop them - // this is the case with http.server spans of prerendered pages - // we do not care about those, as they are effectively static - if ( - event.type === 'transaction' && - event.contexts?.trace?.op === 'http.server' && - event.contexts?.trace?.origin === 'auto.http.otel.http' - ) { - return null; - } - - return event; - }, - { id: 'AstroHttpEventProcessor' }, - ), - ); - - return client; + return initNodeSdk(opts); } From 247c50a1bc10885959d8ac384bb322ca086aeb03 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 5 Sep 2025 11:26:42 +0200 Subject: [PATCH 3/7] fix case without parametrized route --- packages/astro/src/server/middleware.ts | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/packages/astro/src/server/middleware.ts b/packages/astro/src/server/middleware.ts index 4f90a413f263..345f9f925c61 100644 --- a/packages/astro/src/server/middleware.ts +++ b/packages/astro/src/server/middleware.ts @@ -146,16 +146,14 @@ async function enhanceHttpServerSpan( [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.astro', }); - if (!parametrizedRoute) { - return next(); - } - - rootSpan.setAttributes({ - [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', - 'http.route': parametrizedRoute, - }); + if (parametrizedRoute) { + rootSpan.setAttributes({ + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + 'http.route': parametrizedRoute, + }); - isolationScope.setTransactionName(`${method} ${parametrizedRoute}`); + isolationScope.setTransactionName(`${method} ${parametrizedRoute}`); + } try { const originalResponse = await next(); From eb9947b63f4318209887e4aec4ea1e50d76d8406 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 5 Sep 2025 11:30:23 +0200 Subject: [PATCH 4/7] no need to check client again --- packages/astro/src/server/middleware.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/astro/src/server/middleware.ts b/packages/astro/src/server/middleware.ts index 345f9f925c61..0a6d8e513bdc 100644 --- a/packages/astro/src/server/middleware.ts +++ b/packages/astro/src/server/middleware.ts @@ -437,11 +437,10 @@ function getParametrizedRoute( function injectMetaTagsInResponse(originalResponse: Response, metaTagsStr: string): Response { try { - const client = getClient(); const contentType = originalResponse.headers.get('content-type'); const isPageloadRequest = contentType?.startsWith('text/html'); - if (!isPageloadRequest || !client) { + if (!isPageloadRequest) { return originalResponse; } From 66aa46fa98f901199062d5f0b85d108913428bf3 Mon Sep 17 00:00:00 2001 From: Francesco Gringl-Novy Date: Fri, 5 Sep 2025 12:47:01 +0200 Subject: [PATCH 5/7] Apply suggestion from @Lms24 Co-authored-by: Lukas Stracke --- packages/astro/src/server/middleware.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/astro/src/server/middleware.ts b/packages/astro/src/server/middleware.ts index 0a6d8e513bdc..f78526671d3b 100644 --- a/packages/astro/src/server/middleware.ts +++ b/packages/astro/src/server/middleware.ts @@ -167,7 +167,6 @@ async function enhanceHttpServerSpan( await flushIfServerless(); } - // TODO: flush if serverless (first extract function) } async function instrumentRequestStartHttpServerSpan( From 72f1a793371899d806273da2f9cfc47eb3751cd7 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 5 Sep 2025 12:46:37 +0200 Subject: [PATCH 6/7] set method better --- packages/astro/src/server/middleware.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/astro/src/server/middleware.ts b/packages/astro/src/server/middleware.ts index f78526671d3b..e80020ba0913 100644 --- a/packages/astro/src/server/middleware.ts +++ b/packages/astro/src/server/middleware.ts @@ -6,6 +6,7 @@ import { getIsolationScope, getRootSpan, objectify, + SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD, spanToJSON, stripUrlQueryAndFragment, winterCGRequestToRequestData, @@ -166,7 +167,6 @@ async function enhanceHttpServerSpan( } finally { await flushIfServerless(); } - } async function instrumentRequestStartHttpServerSpan( @@ -216,6 +216,8 @@ async function instrumentRequestStartHttpServerSpan( const attributes: SpanAttributes = { [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.astro', [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: source, + [SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD]: method, + // This is here for backwards compatibility, we used to set this here before method, url: stripUrlQueryAndFragment(ctx.url.href), }; From ccb50604b9706f09f5d64d7bf25cabed1d6afde7 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Fri, 5 Sep 2025 13:39:15 +0200 Subject: [PATCH 7/7] test fixes --- packages/astro/test/server/middleware.test.ts | 249 ++++++++++-------- 1 file changed, 143 insertions(+), 106 deletions(-) diff --git a/packages/astro/test/server/middleware.test.ts b/packages/astro/test/server/middleware.test.ts index 6430a5f47eb7..13e54d1537cb 100644 --- a/packages/astro/test/server/middleware.test.ts +++ b/packages/astro/test/server/middleware.test.ts @@ -5,26 +5,48 @@ import * as SentryNode from '@sentry/node'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { handleRequest, interpolateRouteFromUrlAndParams } from '../../src/server/middleware'; -vi.mock('../../src/server/meta', () => ({ - getTracingMetaTagValues: () => ({ - sentryTrace: '', - baggage: '', - }), -})); +const DYNAMIC_REQUEST_CONTEXT = { + clientAddress: '192.168.0.1', + request: { + method: 'GET', + url: '/users', + headers: new Headers(), + }, + params: {}, + url: new URL('https://myDomain.io/users/'), +}; + +const STATIC_REQUEST_CONTEXT = { + request: { + method: 'GET', + url: '/users', + headers: new Headers({ + 'some-header': 'some-value', + }), + }, + get clientAddress() { + throw new Error('clientAddress.get() should not be called in static page requests'); + }, + params: {}, + url: new URL('https://myDomain.io/users/'), +}; describe('sentryMiddleware', () => { const startSpanSpy = vi.spyOn(SentryNode, 'startSpan'); const getSpanMock = vi.fn(() => { - return {} as Span | undefined; + return { + spanContext: () => ({ + spanId: '123', + traceId: '123', + }), + } as Span | undefined; }); - const setUserMock = vi.fn(); const setSDKProcessingMetadataMock = vi.fn(); beforeEach(() => { vi.spyOn(SentryNode, 'getCurrentScope').mockImplementation(() => { return { - setUser: setUserMock, setPropagationContext: vi.fn(), getSpan: getSpanMock, setSDKProcessingMetadata: setSDKProcessingMetadataMock, @@ -42,6 +64,9 @@ describe('sentryMiddleware', () => { vi.spyOn(SentryCore, 'getDynamicSamplingContextFromSpan').mockImplementation(() => ({ transaction: 'test', })); + + // Ensure this is wiped + SentryCore.setUser(null); }); const nextResult = Promise.resolve(new Response(null, { status: 200, headers: new Headers() })); @@ -53,6 +78,7 @@ describe('sentryMiddleware', () => { it('creates a span for an incoming request', async () => { const middleware = handleRequest(); const ctx = { + ...DYNAMIC_REQUEST_CONTEXT, request: { method: 'GET', url: '/users/123/details', @@ -75,6 +101,8 @@ describe('sentryMiddleware', () => { method: 'GET', url: 'https://mydomain.io/users/123/details', [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + [SentryCore.SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD]: 'GET', + 'http.route': '/users/[id]/details', }, name: 'GET /users/[id]/details', op: 'http.server', @@ -89,6 +117,7 @@ describe('sentryMiddleware', () => { it("sets source route if the url couldn't be decoded correctly", async () => { const middleware = handleRequest(); const ctx = { + ...DYNAMIC_REQUEST_CONTEXT, request: { method: 'GET', url: '/a%xx', @@ -109,6 +138,7 @@ describe('sentryMiddleware', () => { method: 'GET', url: 'http://localhost:1234/a%xx', [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'url', + [SentryCore.SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD]: 'GET', }, name: 'GET a%xx', op: 'http.server', @@ -125,13 +155,7 @@ describe('sentryMiddleware', () => { const middleware = handleRequest(); const ctx = { - request: { - method: 'GET', - url: '/users', - headers: new Headers(), - }, - url: new URL('https://myDomain.io/users/'), - params: {}, + ...DYNAMIC_REQUEST_CONTEXT, }; const error = new Error('Something went wrong'); @@ -153,13 +177,7 @@ describe('sentryMiddleware', () => { const middleware = handleRequest(); const ctx = { - request: { - method: 'GET', - url: '/users', - headers: new Headers(), - }, - url: new URL('https://myDomain.io/users/'), - params: {}, + ...DYNAMIC_REQUEST_CONTEXT, }; const error = new Error('Something went wrong'); @@ -195,50 +213,31 @@ describe('sentryMiddleware', () => { it('attaches client IP if `trackClientIp=true` when handling dynamic page requests', async () => { const middleware = handleRequest({ trackClientIp: true }); const ctx = { - request: { - method: 'GET', - url: '/users', - headers: new Headers({ - 'some-header': 'some-value', - }), - }, - clientAddress: '192.168.0.1', - params: {}, - url: new URL('https://myDomain.io/users/'), + ...DYNAMIC_REQUEST_CONTEXT, }; - const next = vi.fn(() => nextResult); // @ts-expect-error, a partial ctx object is fine here - await middleware(ctx, next); - - expect(setUserMock).toHaveBeenCalledWith({ ip_address: '192.168.0.1' }); + await middleware(ctx, async () => { + expect(SentryCore.getIsolationScope().getScopeData().user).toEqual({ ip_address: '192.168.0.1' }); + return nextResult; + }); }); it("doesn't attach a client IP if `trackClientIp=true` when handling static page requests", async () => { const middleware = handleRequest({ trackClientIp: true }); - const ctx = { - request: { - method: 'GET', - url: '/users', - headers: new Headers({ - 'some-header': 'some-value', - }), - }, - get clientAddress() { - throw new Error('clientAddress.get() should not be called in static page requests'); - }, - params: {}, - url: new URL('https://myDomain.io/users/'), - }; - - const next = vi.fn(() => nextResult); + const ctx = STATIC_REQUEST_CONTEXT; // @ts-expect-error, a partial ctx object is fine here - await middleware(ctx, next); - - expect(setUserMock).not.toHaveBeenCalled(); - expect(next).toHaveBeenCalledTimes(1); + await middleware(ctx, async () => { + expect(SentryCore.getIsolationScope().getScopeData().user).toEqual({ + email: undefined, + id: undefined, + ip_address: undefined, + username: undefined, + }); + return nextResult; + }); }); }); @@ -246,6 +245,7 @@ describe('sentryMiddleware', () => { it('attaches request as SDK processing metadata in dynamic page requests', async () => { const middleware = handleRequest({}); const ctx = { + ...DYNAMIC_REQUEST_CONTEXT, request: { method: 'GET', url: '/users', @@ -253,9 +253,6 @@ describe('sentryMiddleware', () => { 'some-header': 'some-value', }), }, - clientAddress: '192.168.0.1', - params: {}, - url: new URL('https://myDomain.io/users/'), }; const next = vi.fn(() => nextResult); @@ -276,46 +273,52 @@ describe('sentryMiddleware', () => { it("doesn't attach request headers as processing metadata for static page requests", async () => { const middleware = handleRequest({}); - const ctx = { - request: { - method: 'GET', - url: '/users', - headers: new Headers({ - 'some-header': 'some-value', - }), - }, - get clientAddress() { - throw new Error('clientAddress.get() should not be called in static page requests'); - }, - params: {}, - url: new URL('https://myDomain.io/users/'), - }; + const ctx = STATIC_REQUEST_CONTEXT; const next = vi.fn(() => nextResult); // @ts-expect-error, a partial ctx object is fine here await middleware(ctx, next); - expect(setSDKProcessingMetadataMock).toHaveBeenCalledWith({ - normalizedRequest: { - method: 'GET', - url: '/users', - }, - }); + expect(setSDKProcessingMetadataMock).not.toHaveBeenCalled(); expect(next).toHaveBeenCalledTimes(1); }); }); - it('injects tracing tags into the HTML of a pageload response', async () => { + it('does not inject tracing tags if route is static', async () => { + const middleware = handleRequest(); + + const ctx = STATIC_REQUEST_CONTEXT; + const next = vi.fn(() => + Promise.resolve( + new Response('', { + headers: new Headers({ 'content-type': 'text/html' }), + }), + ), + ); + + // @ts-expect-error, a partial ctx object is fine here + const resultFromNext = await middleware(ctx, next); + + expect(resultFromNext?.headers.get('content-type')).toEqual('text/html'); + + const html = await resultFromNext?.text(); + + expect(html).toContain(''); + expect(html).toContain(''); + // parametrized route is injected + expect(html).toContain(''); + // trace data is not injected + expect(html).not.toContain(''); + expect(html).not.toContain(''); + expect(html).not.toContain('', { + headers: new Headers({ 'content-type': 'text/html' }), + }), + ), + ); + + // @ts-expect-error, a partial ctx object is fine here + const resultFromNext = await middleware(ctx, next); + + expect(resultFromNext?.headers.get('content-type')).toEqual('text/html'); + + const html = await resultFromNext?.text(); + + expect(html).toContain(''); + expect(html).toContain(''); + expect(html).toContain(''); expect(html).toContain(' { const middleware = handleRequest(); const ctx = { - request: { - method: 'GET', - url: '/users', - headers: new Headers(), - }, - params: {}, - url: new URL('https://myDomain.io/users/'), + ...DYNAMIC_REQUEST_CONTEXT, }; const originalHtml = '

no head

'; @@ -399,7 +421,9 @@ describe('sentryMiddleware', () => { it('starts a new async context if no span is active', async () => { getSpanMock.mockReturnValueOnce(undefined); const handler = handleRequest(); - const ctx = {}; + const ctx = { + ...DYNAMIC_REQUEST_CONTEXT, + }; const next = vi.fn(); try { @@ -413,11 +437,24 @@ describe('sentryMiddleware', () => { }); it("doesn't start a new async context if a span is active", async () => { - // @ts-expect-error, a empty span is fine here - getSpanMock.mockReturnValueOnce({}); + getSpanMock.mockReturnValueOnce({ + spanContext: () => ({ + spanId: '123', + traceId: '123', + traceFlags: 1, + }), + // @ts-expect-error, this is fine + getSpanJSON: () => ({ + span_id: '123', + trace_id: '123', + op: 'http.server', + }), + }); const handler = handleRequest(); - const ctx = {}; + const ctx = { + ...DYNAMIC_REQUEST_CONTEXT, + }; const next = vi.fn(); try {