fix(astro): Fix astro trace propagation issues (#14501)

Noticed this while working on
#14481.

The way we try-catched the astro
server request code lead to the http.server span not being attached to
servers correctly - we had a try-catch block _outside_ of the
`startSpan` call, where we sent caught errors to sentry. but any error
caught this way would not have an active span (because by the time the
`catch` part triggers, `startSpan` is over), and thus the http.server
span would not be attached to the error. By moving this try-catch inside
of the `startSpan` call, we can correctly assign the span to errors. I
also tried to add some tests to this - there is still a problem in there
which the tests show, which I'll look at afterwards (and/or they may get
fixed by #14481)

Author: mydea

dev-packages/e2e-tests/test-applications/astro-4/tests/errors.server.test.ts

@@ -1,15 +1,32 @@
 import { expect, test } from '@playwright/test';
-import { waitForError } from '@sentry-internal/test-utils';
+import { waitForError, waitForTransaction } from '@sentry-internal/test-utils';
 
 test.describe('server-side errors', () => {
   test('captures SSR error', async ({ page }) => {
     const errorEventPromise = waitForError('astro-4', errorEvent => {
       return errorEvent?.exception?.values?.[0]?.value === "Cannot read properties of undefined (reading 'x')";
     });
 
+    const transactionEventPromise = waitForTransaction('astro-4', transactionEvent => {
+      return transactionEvent.transaction === 'GET /ssr-error';
+    });
+
     await page.goto('/ssr-error');
 
     const errorEvent = await errorEventPromise;
+    const transactionEvent = await transactionEventPromise;
+
+    expect(transactionEvent).toMatchObject({
+      transaction: 'GET /ssr-error',
+      spans: [],
+    });
+
+    const traceId = transactionEvent.contexts?.trace?.trace_id;
+    const spanId = transactionEvent.contexts?.trace?.span_id;
+
+    expect(traceId).toMatch(/[a-f0-9]{32}/);
+    expect(spanId).toMatch(/[a-f0-9]{16}/);
+    expect(transactionEvent.contexts?.trace?.parent_span_id).toBeUndefined();
 
     expect(errorEvent).toMatchObject({
       contexts: {
@@ -20,8 +37,8 @@ test.describe('server-side errors', () => {
         os: expect.any(Object),
         runtime: expect.any(Object),
         trace: {
-          span_id: '', //TODO: This is a bug! We should expect.stringMatching(/[a-f0-9]{16}/) instead of ''
-          trace_id: expect.stringMatching(/[a-f0-9]{32}/),
+          span_id: spanId,
+          trace_id: traceId,
         },
       },
       environment: 'qa',
@@ -69,18 +86,50 @@ test.describe('server-side errors', () => {
     const errorEventPromise = waitForError('astro-4', errorEvent => {
       return errorEvent?.exception?.values?.[0]?.value === 'Endpoint Error';
     });
+    const transactionEventApiPromise = waitForTransaction('astro-4', transactionEvent => {
+      return transactionEvent.transaction === 'GET /endpoint-error/api';
+    });
+    const transactionEventEndpointPromise = waitForTransaction('astro-4', transactionEvent => {
+      return transactionEvent.transaction === 'GET /endpoint-error';
+    });
 
     await page.goto('/endpoint-error');
     await page.getByText('Get Data').click();
 
     const errorEvent = await errorEventPromise;
+    const transactionEventApi = await transactionEventApiPromise;
+    const transactionEventEndpoint = await transactionEventEndpointPromise;
+
+    expect(transactionEventEndpoint).toMatchObject({
+      transaction: 'GET /endpoint-error',
+      spans: [],
+    });
+
+    const traceId = transactionEventEndpoint.contexts?.trace?.trace_id;
+    const endpointSpanId = transactionEventApi.contexts?.trace?.span_id;
+
+    expect(traceId).toMatch(/[a-f0-9]{32}/);
+    expect(endpointSpanId).toMatch(/[a-f0-9]{16}/);
+
+    expect(transactionEventApi).toMatchObject({
+      transaction: 'GET /endpoint-error/api',
+      spans: [],
+    });
+
+    const spanId = transactionEventApi.contexts?.trace?.span_id;
+    const parentSpanId = transactionEventApi.contexts?.trace?.parent_span_id;
+
+    expect(spanId).toMatch(/[a-f0-9]{16}/);
+    // TODO: This is incorrect, for whatever reason, it should be the endpointSpanId ideally
+    expect(parentSpanId).toMatch(/[a-f0-9]{16}/);
+    expect(parentSpanId).not.toEqual(endpointSpanId);
 
     expect(errorEvent).toMatchObject({
       contexts: {
         trace: {
-          parent_span_id: expect.stringMatching(/[a-f0-9]{16}/),
-          span_id: expect.stringMatching(/[a-f0-9]{16}/),
-          trace_id: expect.stringMatching(/[a-f0-9]{32}/),
+          parent_span_id: parentSpanId,
+          span_id: spanId,
+          trace_id: traceId,
         },
       },
       exception: {

package.json

@@ -17,7 +17,7 @@
     "clean:build": "lerna run clean",
     "clean:caches": "yarn rimraf eslintcache .nxcache && yarn jest --clearCache",
     "clean:deps": "lerna clean --yes && rm -rf node_modules && yarn",
-    "clean:tarballs": "rimraf -g **/*.tgz",
+    "clean:tarballs": "rimraf {packages,dev-packages}/*/*.tgz",
     "clean:all": "run-s clean:build clean:tarballs clean:caches clean:deps",
     "fix": "run-s fix:biome fix:prettier fix:lerna",
     "fix:lerna": "lerna run fix",

packages/astro/src/server/middleware.ts

@@ -155,49 +155,50 @@ async function instrumentRequest(
             op: 'http.server',
           },
           async span => {
-            const originalResponse = await next();
-
-            if (originalResponse.status) {
-              setHttpStatus(span, originalResponse.status);
-            }
-
-            const client = getClient();
-            const contentType = originalResponse.headers.get('content-type');
-
-            const isPageloadRequest = contentType && 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;
+            try {
+              const originalResponse = await next();
+              if (originalResponse.status) {
+                setHttpStatus(span, originalResponse.status);
+              }
+
+              const client = getClient();
+              const contentType = originalResponse.headers.get('content-type');
+
+              const isPageloadRequest = contentType && 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 => {
+                  for await (const chunk of originalBody) {
+                    const html = typeof chunk === 'string' ? chunk : decoder.decode(chunk, { stream: true });
+                    const modifiedHtml = addMetaTagToHead(html);
+                    controller.enqueue(new TextEncoder().encode(modifiedHtml));
+                  }
+                  controller.close();
+                },
+              });
+
+              return new Response(newResponseStream, originalResponse);
+            } catch (e) {
+              sendErrorToSentry(e);
+              throw e;
             }
-
-            const decoder = new TextDecoder();
-
-            const newResponseStream = new ReadableStream({
-              start: async controller => {
-                for await (const chunk of originalBody) {
-                  const html = typeof chunk === 'string' ? chunk : decoder.decode(chunk, { stream: true });
-                  const modifiedHtml = addMetaTagToHead(html);
-                  controller.enqueue(new TextEncoder().encode(modifiedHtml));
-                }
-                controller.close();
-              },
-            });
-
-            return new Response(newResponseStream, originalResponse);
           },
         );
         return res;
-      } catch (e) {
-        sendErrorToSentry(e);
-        throw e;
       } finally {
         vercelWaitUntil(
           (async () => {

packages/opentelemetry/src/utils/generateSpanContextForPropagationContext.ts

@@ -17,6 +17,7 @@ export function generateSpanContextForPropagationContext(propagationContext: Pro
 
   const spanContext: SpanContext = {
     traceId: propagationContext.traceId,
+    // TODO: Do not create an invalid span context here
     spanId: propagationContext.parentSpanId || '',
     isRemote: true,
     traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE,