From a6b6c007c474d1f3e4b18074471fe9edfab9cc3f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 27 Nov 2024 09:29:28 +0100 Subject: [PATCH 1/5] fix(astro): Fix astro trace propagation issues --- .../astro-4/tests/errors.server.test.ts | 61 ++++++++++++-- package.json | 2 +- packages/astro/src/server/middleware.ts | 79 ++++++++++--------- ...enerateSpanContextForPropagationContext.ts | 4 +- 4 files changed, 99 insertions(+), 47 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/astro-4/tests/errors.server.test.ts b/dev-packages/e2e-tests/test-applications/astro-4/tests/errors.server.test.ts index d5f07ebe239a..3faacdb30090 100644 --- a/dev-packages/e2e-tests/test-applications/astro-4/tests/errors.server.test.ts +++ b/dev-packages/e2e-tests/test-applications/astro-4/tests/errors.server.test.ts @@ -1,5 +1,5 @@ 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 }) => { @@ -7,9 +7,26 @@ test.describe('server-side errors', () => { 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: { diff --git a/package.json b/package.json index 39139d5faba0..099771f7be38 100644 --- a/package.json +++ b/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", diff --git a/packages/astro/src/server/middleware.ts b/packages/astro/src/server/middleware.ts index 781a5a75b7a9..14b48706c2cf 100644 --- a/packages/astro/src/server/middleware.ts +++ b/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 () => { diff --git a/packages/opentelemetry/src/utils/generateSpanContextForPropagationContext.ts b/packages/opentelemetry/src/utils/generateSpanContextForPropagationContext.ts index d2aa470213f7..9f21c6c20f8f 100644 --- a/packages/opentelemetry/src/utils/generateSpanContextForPropagationContext.ts +++ b/packages/opentelemetry/src/utils/generateSpanContextForPropagationContext.ts @@ -1,5 +1,6 @@ import type { SpanContext } from '@opentelemetry/api'; import { TraceFlags } from '@opentelemetry/api'; +import { uuid4 } from '@sentry/core'; import type { PropagationContext } from '@sentry/types'; import { makeTraceState } from './makeTraceState'; @@ -17,7 +18,8 @@ export function generateSpanContextForPropagationContext(propagationContext: Pro const spanContext: SpanContext = { traceId: propagationContext.traceId, - spanId: propagationContext.parentSpanId || '', + // If we have no parent span ID, just generate a random one + spanId: propagationContext.parentSpanId || uuid4().substring(16), isRemote: true, traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE, traceState, From 93b615439ae9332e4ac9acd1ddc4281052741ee9 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 27 Nov 2024 10:47:44 +0100 Subject: [PATCH 2/5] fix test --- packages/opentelemetry/test/trace.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index 0a709c9fb5da..bfa72087c4f2 100644 --- a/packages/opentelemetry/test/trace.test.ts +++ b/packages/opentelemetry/test/trace.test.ts @@ -1521,8 +1521,8 @@ describe('continueTrace', () => { const span = getActiveSpan()!; expect(span).toBeDefined(); expect(spanToJSON(span)).toEqual({ - span_id: '', - trace_id: expect.any(String), + span_id: expect.stringMatching(/^[0-9a-f]{16}$/), + trace_id: expect.stringMatching(/^[0-9a-f]{32}$/), }); expect(getSamplingDecision(span.spanContext())).toBe(undefined); expect(spanIsSampled(span)).toBe(false); From 6585a2da4e8ae977249ca5b1ef0438dded57415f Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 27 Nov 2024 12:19:53 +0100 Subject: [PATCH 3/5] revert otel change --- .../test-applications/astro-4/tests/errors.server.test.ts | 4 +++- .../src/utils/generateSpanContextForPropagationContext.ts | 5 ++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/astro-4/tests/errors.server.test.ts b/dev-packages/e2e-tests/test-applications/astro-4/tests/errors.server.test.ts index 3faacdb30090..a0a4aca96b6d 100644 --- a/dev-packages/e2e-tests/test-applications/astro-4/tests/errors.server.test.ts +++ b/dev-packages/e2e-tests/test-applications/astro-4/tests/errors.server.test.ts @@ -37,7 +37,9 @@ test.describe('server-side errors', () => { os: expect.any(Object), runtime: expect.any(Object), trace: { - span_id: spanId, + // TODO: This is a bug, this should be spanId + // This is due to generateSpanContextForPropagationContext() returning '' as fallback + span_id: '', trace_id: traceId, }, }, diff --git a/packages/opentelemetry/src/utils/generateSpanContextForPropagationContext.ts b/packages/opentelemetry/src/utils/generateSpanContextForPropagationContext.ts index 9f21c6c20f8f..7c1d94daa7d8 100644 --- a/packages/opentelemetry/src/utils/generateSpanContextForPropagationContext.ts +++ b/packages/opentelemetry/src/utils/generateSpanContextForPropagationContext.ts @@ -1,6 +1,5 @@ import type { SpanContext } from '@opentelemetry/api'; import { TraceFlags } from '@opentelemetry/api'; -import { uuid4 } from '@sentry/core'; import type { PropagationContext } from '@sentry/types'; import { makeTraceState } from './makeTraceState'; @@ -18,8 +17,8 @@ export function generateSpanContextForPropagationContext(propagationContext: Pro const spanContext: SpanContext = { traceId: propagationContext.traceId, - // If we have no parent span ID, just generate a random one - spanId: propagationContext.parentSpanId || uuid4().substring(16), + // TODO: Do not create an invalid span context here + spanId: propagationContext.parentSpanId || '', isRemote: true, traceFlags: propagationContext.sampled ? TraceFlags.SAMPLED : TraceFlags.NONE, traceState, From a2b37fbe282e7001550dc11dcd99196d5f5c508d Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 27 Nov 2024 12:20:03 +0100 Subject: [PATCH 4/5] Revert "fix test" This reverts commit 93b615439ae9332e4ac9acd1ddc4281052741ee9. --- packages/opentelemetry/test/trace.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry/test/trace.test.ts b/packages/opentelemetry/test/trace.test.ts index bfa72087c4f2..0a709c9fb5da 100644 --- a/packages/opentelemetry/test/trace.test.ts +++ b/packages/opentelemetry/test/trace.test.ts @@ -1521,8 +1521,8 @@ describe('continueTrace', () => { const span = getActiveSpan()!; expect(span).toBeDefined(); expect(spanToJSON(span)).toEqual({ - span_id: expect.stringMatching(/^[0-9a-f]{16}$/), - trace_id: expect.stringMatching(/^[0-9a-f]{32}$/), + span_id: '', + trace_id: expect.any(String), }); expect(getSamplingDecision(span.spanContext())).toBe(undefined); expect(spanIsSampled(span)).toBe(false); From 6be9fcfe76482ab8f3673af598995c9f73cb6eac Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Wed, 27 Nov 2024 12:41:00 +0100 Subject: [PATCH 5/5] fix test --- .../test-applications/astro-4/tests/errors.server.test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/astro-4/tests/errors.server.test.ts b/dev-packages/e2e-tests/test-applications/astro-4/tests/errors.server.test.ts index a0a4aca96b6d..3faacdb30090 100644 --- a/dev-packages/e2e-tests/test-applications/astro-4/tests/errors.server.test.ts +++ b/dev-packages/e2e-tests/test-applications/astro-4/tests/errors.server.test.ts @@ -37,9 +37,7 @@ test.describe('server-side errors', () => { os: expect.any(Object), runtime: expect.any(Object), trace: { - // TODO: This is a bug, this should be spanId - // This is due to generateSpanContextForPropagationContext() returning '' as fallback - span_id: '', + span_id: spanId, trace_id: traceId, }, },