From 9cf8f14f7a63011ac4530eee281badac81139788 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 23 Oct 2024 09:49:49 +0000 Subject: [PATCH] cleanup --- .../nextjs-13/instrumentation.ts | 1 - .../server/excluded-api-endpoints.test.ts | 28 ++++--------- .../tests/server/getServerSideProps.test.ts | 14 +++---- .../nextjs-app-dir/next-env.d.ts | 2 +- .../nextjs-app-dir/tests/edge-route.test.ts | 11 +++-- .../nextjs-app-dir/tests/edge.test.ts | 21 +++++++--- .../tests/route-handlers.test.ts | 22 ++++++---- .../tests/server-components.test.ts | 4 +- .../src/common/wrapRouteHandlerWithSentry.ts | 42 ++++++++++++++++--- 9 files changed, 91 insertions(+), 54 deletions(-) diff --git a/dev-packages/e2e-tests/test-applications/nextjs-13/instrumentation.ts b/dev-packages/e2e-tests/test-applications/nextjs-13/instrumentation.ts index b8abbf9fd765..180685d41b4a 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-13/instrumentation.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-13/instrumentation.ts @@ -12,7 +12,6 @@ export function register() { // We are doing a lot of events at once in this test app bufferSize: 1000, }, - debug: false, }); } } diff --git a/dev-packages/e2e-tests/test-applications/nextjs-13/tests/server/excluded-api-endpoints.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-13/tests/server/excluded-api-endpoints.test.ts index b328f35d0721..2d3854e2a2a4 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-13/tests/server/excluded-api-endpoints.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-13/tests/server/excluded-api-endpoints.test.ts @@ -1,9 +1,7 @@ import { expect, test } from '@playwright/test'; import { waitForTransaction } from '@sentry-internal/test-utils'; -// TODO(lforst): This cannot make it into production - Make sure to fix this test -// The problem is that if we are not applying build time instrumentation Next.js will still emit spans (which is fine, but we need to find a different way of testing that build time instrumentation is successfully disabled - maybe with an attribute or something if build-time instrumentation is applied) -test.skip('should not automatically create transactions for routes that were excluded from auto wrapping (string)', async ({ +test('should not apply build-time instrumentation for routes that were excluded from auto wrapping (string)', async ({ request, }) => { const transactionPromise = waitForTransaction('nextjs-13', async transactionEvent => { @@ -15,19 +13,13 @@ test.skip('should not automatically create transactions for routes that were exc expect(await (await request.get(`/api/endpoint-excluded-with-string`)).text()).toBe('{"success":true}'); - let transactionPromiseReceived = false; - transactionPromise.then(() => { - transactionPromiseReceived = true; - }); - - await new Promise(resolve => setTimeout(resolve, 5_000)); + const transaction = await transactionPromise; - expect(transactionPromiseReceived).toBe(false); + expect(transaction.contexts?.trace?.data?.['sentry.origin']).toBeDefined(); + expect(transaction.contexts?.trace?.data?.['sentry.origin']).not.toBe('auto.http.nextjs'); // This is the origin set by the build time instrumentation }); -// TODO(lforst): This cannot make it into production - Make sure to fix this test -// The problem is that if we are not applying build time instrumentation Next.js will still emit spans (which is fine, but we need to find a different way of testing that build time instrumentation is successfully disabled - maybe with an attribute or something if build-time instrumentation is applied) -test.skip('should not automatically create transactions for routes that were excluded from auto wrapping (regex)', async ({ +test('should not apply build-time instrumentation for routes that were excluded from auto wrapping (regex)', async ({ request, }) => { const transactionPromise = waitForTransaction('nextjs-13', async transactionEvent => { @@ -39,12 +31,8 @@ test.skip('should not automatically create transactions for routes that were exc expect(await (await request.get(`/api/endpoint-excluded-with-regex`)).text()).toBe('{"success":true}'); - let transactionPromiseReceived = false; - transactionPromise.then(() => { - transactionPromiseReceived = true; - }); - - await new Promise(resolve => setTimeout(resolve, 5_000)); + const transaction = await transactionPromise; - expect(transactionPromiseReceived).toBe(false); + expect(transaction.contexts?.trace?.data?.['sentry.origin']).toBeDefined(); + expect(transaction.contexts?.trace?.data?.['sentry.origin']).not.toBe('auto.http.nextjs'); // This is the origin set by the build time instrumentation }); diff --git a/dev-packages/e2e-tests/test-applications/nextjs-13/tests/server/getServerSideProps.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-13/tests/server/getServerSideProps.test.ts index 44546d6b4668..9ae79d7bd4b0 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-13/tests/server/getServerSideProps.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-13/tests/server/getServerSideProps.test.ts @@ -60,11 +60,11 @@ test('Should report an error event for errors thrown in getServerSideProps', asy data: { 'http.response.status_code': 500, 'sentry.op': 'http.server', - 'sentry.origin': expect.stringMatching(/^(auto|auto\.http\.otel\.http)$/), + 'sentry.origin': 'auto', 'sentry.source': 'route', }, op: 'http.server', - origin: expect.stringMatching(/^(auto|auto\.http\.otel\.http)$/), + origin: 'auto', span_id: expect.any(String), status: 'internal_error', trace_id: expect.any(String), @@ -81,8 +81,7 @@ test('Should report an error event for errors thrown in getServerSideProps', asy start_timestamp: expect.any(Number), timestamp: expect.any(Number), transaction: 'GET /[param]/error-getServerSideProps', - // TODO: This test fails depending on the next version (next 13: 'custom', next >14: 'route') - // transaction_info: { source: 'custom' }, + transaction_info: { source: 'route' }, type: 'transaction', }); }); @@ -148,11 +147,11 @@ test('Should report an error event for errors thrown in getServerSideProps in pa data: { 'http.response.status_code': 500, 'sentry.op': 'http.server', - 'sentry.origin': expect.stringMatching(/^auto(\.http\.otel\.http)?$/), + 'sentry.origin': 'auto', 'sentry.source': 'route', }, op: 'http.server', - origin: expect.stringMatching(/^auto(\.http\.otel\.http)?$/), + origin: 'auto', span_id: expect.any(String), status: 'internal_error', trace_id: expect.any(String), @@ -169,8 +168,7 @@ test('Should report an error event for errors thrown in getServerSideProps in pa start_timestamp: expect.any(Number), timestamp: expect.any(Number), transaction: 'GET /[param]/customPageExtension', - // TODO: This test fails depending on the next version (next 13: 'custom', next >14: 'route') - // transaction_info: { source: 'custom' }, + transaction_info: { source: 'route' }, type: 'transaction', }); }); diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/next-env.d.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/next-env.d.ts index 725dd6f24515..fd36f9494e2c 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/next-env.d.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/next-env.d.ts @@ -3,4 +3,4 @@ /// // NOTE: This file should not be edited -// see https://nextjs.org/docs/app/building-your-application/configuring/typescript for more information. +// see https://nextjs.org/docs/basic-features/typescript for more information. diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/edge-route.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/edge-route.test.ts index 6233688317e4..88460e3ab533 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/edge-route.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/edge-route.test.ts @@ -25,11 +25,17 @@ test('Should create a transaction for edge routes', async ({ request }) => { test('Faulty edge routes', async ({ request }) => { const edgerouteTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { - return transactionEvent?.transaction === 'GET /api/error-edge-endpoint'; + return ( + transactionEvent?.transaction === 'GET /api/error-edge-endpoint' && + transactionEvent.contexts?.runtime?.name === 'vercel-edge' + ); }); const errorEventPromise = waitForError('nextjs-app-dir', errorEvent => { - return errorEvent?.exception?.values?.[0]?.value === 'Edge Route Error'; + return ( + errorEvent?.exception?.values?.[0]?.value === 'Edge Route Error' && + errorEvent.contexts?.runtime?.name === 'vercel-edge' + ); }); request.get('/api/error-edge-endpoint').catch(() => { @@ -44,7 +50,6 @@ test('Faulty edge routes', async ({ request }) => { test.step('should create transactions with the right fields', () => { expect(edgerouteTransaction.contexts?.trace?.status).toBe('unknown_error'); expect(edgerouteTransaction.contexts?.trace?.op).toBe('http.server'); - expect(edgerouteTransaction.contexts?.runtime?.name).toBe('vercel-edge'); }); test.step('should have scope isolation', () => { diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/edge.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/edge.test.ts index a34d415ee4bf..934cfa2e472d 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/edge.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/edge.test.ts @@ -1,6 +1,8 @@ import { expect, test } from '@playwright/test'; import { waitForError, waitForTransaction } from '@sentry-internal/test-utils'; +const packageJson = require('../package.json'); + test('Should record exceptions for faulty edge server components', async ({ page }) => { const errorEventPromise = waitForError('nextjs-app-dir', errorEvent => { return errorEvent?.exception?.values?.[0]?.value === 'Edge Server Component Error'; @@ -19,8 +21,10 @@ test('Should record exceptions for faulty edge server components', async ({ page expect(errorEvent.transaction).toBe(`Page Server Component (/edge-server-components/error)`); }); -// TODO(lforst): Fix this test -test.skip('Should record transaction for edge server components', async ({ page }) => { +test('Should record transaction for edge server components', async ({ page }) => { + const nextjsVersion = packageJson.dependencies.next; + const nextjsMajor = Number(nextjsVersion.split('.')[0]); + const serverComponentTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { return ( transactionEvent?.transaction === 'GET /edge-server-components' && @@ -33,9 +37,14 @@ test.skip('Should record transaction for edge server components', async ({ page const serverComponentTransaction = await serverComponentTransactionPromise; expect(serverComponentTransaction).toBeDefined(); - expect(serverComponentTransaction.request?.headers).toBeDefined(); + expect(serverComponentTransaction.contexts?.trace?.op).toBe('http.server'); - // Assert that isolation scope works properly - expect(serverComponentTransaction.tags?.['my-isolated-tag']).toBe(true); - expect(serverComponentTransaction.tags?.['my-global-scope-isolated-tag']).not.toBeDefined(); + // For some reason headers aren't picked up on Next.js 13 - also causing scope isolation to be broken + if (nextjsMajor >= 14) { + expect(serverComponentTransaction.request?.headers).toBeDefined(); + + // Assert that isolation scope works properly + expect(serverComponentTransaction.tags?.['my-isolated-tag']).toBe(true); + expect(serverComponentTransaction.tags?.['my-global-scope-isolated-tag']).not.toBeDefined(); + } }); diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts index 90d71865dd3b..7e6dc5fbe300 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/route-handlers.test.ts @@ -63,11 +63,13 @@ test('Should record exceptions and transactions for faulty route handlers', asyn expect(routehandlerError.transaction).toBe('PUT /route-handlers/[param]/error'); }); -// TODO(lforst): This cannot make it into production - Make sure to fix this test -test.describe.skip('Edge runtime', () => { +test.describe('Edge runtime', () => { test('should create a transaction for route handlers', async ({ request }) => { const routehandlerTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { - return transactionEvent?.transaction === 'PATCH /route-handlers/[param]/edge'; + return ( + transactionEvent?.transaction === 'PATCH /route-handlers/[param]/edge' && + transactionEvent.contexts?.runtime?.name === 'vercel-edge' + ); }); const response = await request.patch('/route-handlers/bar/edge'); @@ -81,11 +83,17 @@ test.describe.skip('Edge runtime', () => { test('should record exceptions and transactions for faulty route handlers', async ({ request }) => { const errorEventPromise = waitForError('nextjs-app-dir', errorEvent => { - return errorEvent?.exception?.values?.[0]?.value === 'route-handler-edge-error'; + return ( + errorEvent?.exception?.values?.[0]?.value === 'route-handler-edge-error' && + errorEvent.contexts?.runtime?.name === 'vercel-edge' + ); }); const routehandlerTransactionPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { - return transactionEvent?.transaction === 'DELETE /route-handlers/[param]/edge'; + return ( + transactionEvent?.transaction === 'DELETE /route-handlers/[param]/edge' && + transactionEvent.contexts?.runtime?.name === 'vercel-edge' + ); }); await request.delete('/route-handlers/baz/edge').catch(() => { @@ -101,12 +109,10 @@ test.describe.skip('Edge runtime', () => { expect(routehandlerError.tags?.['my-isolated-tag']).toBe(true); expect(routehandlerError.tags?.['my-global-scope-isolated-tag']).not.toBeDefined(); - expect(routehandlerTransaction.contexts?.trace?.status).toBe('internal_error'); + expect(routehandlerTransaction.contexts?.trace?.status).toBe('unknown_error'); expect(routehandlerTransaction.contexts?.trace?.op).toBe('http.server'); - expect(routehandlerTransaction.contexts?.runtime?.name).toBe('vercel-edge'); expect(routehandlerError.exception?.values?.[0].value).toBe('route-handler-edge-error'); - expect(routehandlerError.contexts?.runtime?.name).toBe('vercel-edge'); expect(routehandlerError.transaction).toBe('DELETE /route-handlers/[param]/edge'); }); diff --git a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts index 3970bd586679..75f30075a47f 100644 --- a/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts +++ b/dev-packages/e2e-tests/test-applications/nextjs-app-dir/tests/server-components.test.ts @@ -16,7 +16,7 @@ test('Sends a transaction for a request to app router', async ({ page }) => { expect(transactionEvent.contexts?.trace).toEqual({ data: expect.objectContaining({ 'sentry.op': 'http.server', - 'sentry.origin': expect.stringMatching(/^(auto|auto\.http\.otel\.http)$/), + 'sentry.origin': 'auto', 'sentry.sample_rate': 1, 'sentry.source': 'route', 'http.method': 'GET', @@ -27,7 +27,7 @@ test('Sends a transaction for a request to app router', async ({ page }) => { 'otel.kind': 'SERVER', }), op: 'http.server', - origin: expect.stringMatching(/^(auto|auto\.http\.otel\.http)$/), + origin: 'auto', span_id: expect.any(String), status: 'ok', trace_id: expect.any(String), diff --git a/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts b/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts index a789cb80aadf..6ff4e314b17b 100644 --- a/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts +++ b/packages/nextjs/src/common/wrapRouteHandlerWithSentry.ts @@ -1,4 +1,6 @@ import { + SEMANTIC_ATTRIBUTE_SENTRY_OP, + SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, Scope, captureException, getActiveSpan, @@ -6,6 +8,7 @@ import { getRootSpan, handleCallbackErrors, setCapturedScopesOnSpan, + setHttpStatus, withIsolationScope, withScope, } from '@sentry/core'; @@ -14,7 +17,7 @@ import type { RouteHandlerContext } from './types'; import { propagationContextFromHeaders, winterCGHeadersToDict } from '@sentry/utils'; -import { isRedirectNavigationError } from './nextNavigationErrorUtils'; +import { isNotFoundNavigationError, isRedirectNavigationError } from './nextNavigationErrorUtils'; import { commonObjectToIsolationScope, commonObjectToPropagationContext } from './utils/tracingUtils'; /** @@ -49,22 +52,36 @@ export function wrapRouteHandlerWithSentry any>( const propagationContext = commonObjectToPropagationContext(headers, incomingPropagationContext); const activeSpan = getActiveSpan(); - if (activeSpan) { - const rootSpan = getRootSpan(activeSpan); + const rootSpan = activeSpan ? getRootSpan(activeSpan) : undefined; + if (rootSpan) { const { scope } = getCapturedScopesOnSpan(rootSpan); setCapturedScopesOnSpan(rootSpan, scope ?? new Scope(), isolationScope); + + if (process.env.NEXT_RUNTIME === 'edge') { + rootSpan.updateName(`${method} ${parameterizedRoute}`); + rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, 'route'); + rootSpan.setAttribute(SEMANTIC_ATTRIBUTE_SENTRY_OP, 'http.server'); + } } return withIsolationScope(isolationScope, () => { - return withScope(scope => { + return withScope(async scope => { scope.setTransactionName(`${method} ${parameterizedRoute}`); scope.setPropagationContext(propagationContext); - return handleCallbackErrors( + + const response: Response = await handleCallbackErrors( () => originalFunction.apply(thisArg, args), error => { // Next.js throws errors when calling `redirect()`. We don't wanna report these. if (isRedirectNavigationError(error)) { // Don't do anything + } else if (isNotFoundNavigationError(error)) { + if (activeSpan) { + setHttpStatus(activeSpan, 404); + } + if (rootSpan) { + setHttpStatus(rootSpan, 404); + } } else { captureException(error, { mechanism: { @@ -74,6 +91,21 @@ export function wrapRouteHandlerWithSentry any>( } }, ); + + try { + if (response.status) { + if (activeSpan) { + setHttpStatus(activeSpan, response.status); + } + if (rootSpan) { + setHttpStatus(rootSpan, response.status); + } + } + } catch { + // best effort - response may be undefined? + } + + return response; }); }); },