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..e80020ba0913 100644 --- a/packages/astro/src/server/middleware.ts +++ b/packages/astro/src/server/middleware.ts @@ -1,9 +1,13 @@ -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, + SEMANTIC_ATTRIBUTE_HTTP_REQUEST_METHOD, + spanToJSON, stripUrlQueryAndFragment, winterCGRequestToRequestData, } from '@sentry/core'; @@ -66,23 +70,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 +134,169 @@ 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, + rootSpan.setAttributes({ + // This is here for backwards compatibility, we used to set this here before + method, + [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.http.astro', + }); + + if (parametrizedRoute) { + rootSpan.setAttributes({ + [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'route', + 'http.route': parametrizedRoute, }); - 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; - } + isolationScope.setTransactionName(`${method} ${parametrizedRoute}`); + } - if (ctx.url.hash) { - attributes['http.fragment'] = ctx.url.hash; - } + try { + const originalResponse = await next(); + const metaTagsStr = getMetaTagsStr({ injectTraceData: true, parametrizedRoute }); + return injectMetaTagsInResponse(originalResponse, metaTagsStr); + } catch (e) { + sendErrorToSentry(e); + throw e; + } + } finally { + await flushIfServerless(); + } +} - 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); - } +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 client = getClient(); - const contentType = originalResponse.headers.get('content-type'); + const request = ctx.request; - const isPageloadRequest = contentType?.startsWith('text/html'); - if (!isPageloadRequest || !client) { - return originalResponse; - } + // 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; - // 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; - } + 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 }); + } - 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) - }, - ); + 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, + [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), + }; + + 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 +407,89 @@ 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 contentType = originalResponse.headers.get('content-type'); + + const isPageloadRequest = contentType?.startsWith('text/html'); + if (!isPageloadRequest) { + 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/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 {