From e06908dd03975bb5b0d5153919a98763afbf3420 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 20 Dec 2023 15:51:36 +0000 Subject: [PATCH 1/6] ref(nextjs): Replace `startTrancsaction` in `withSentry` --- .../src/common/wrapApiHandlerWithSentry.ts | 263 ++++++++---------- 1 file changed, 112 insertions(+), 151 deletions(-) diff --git a/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts b/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts index dfd6d888fbef..b6c79ef2cd23 100644 --- a/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts +++ b/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts @@ -1,25 +1,15 @@ import { addTracingExtensions, captureException, - getClient, - getCurrentScope, + continueTrace, runWithAsyncContext, - startTransaction, + startSpanManual, } from '@sentry/core'; -import type { Transaction } from '@sentry/types'; -import { - consoleSandbox, - isString, - logger, - objectify, - stripUrlQueryAndFragment, - tracingContextFromHeaders, -} from '@sentry/utils'; - -import { DEBUG_BUILD } from './debug-build'; +import { consoleSandbox, isString, logger, objectify, stripUrlQueryAndFragment } from '@sentry/utils'; + import type { AugmentedNextApiRequest, AugmentedNextApiResponse, NextApiHandler } from './types'; import { platformSupportsStreaming } from './utils/platformSupportsStreaming'; -import { autoEndTransactionOnResponseEnd, finishTransaction, flushQueue } from './utils/responseEnd'; +import { flushQueue } from './utils/responseEnd'; /** * Wrap the given API route handler for tracing and error capturing. Thin wrapper around `withSentry`, which only @@ -84,151 +74,122 @@ export function withSentry(apiHandler: NextApiHandler, parameterizedRoute?: stri addTracingExtensions(); - // eslint-disable-next-line complexity, @typescript-eslint/no-explicit-any - const boundHandler = runWithAsyncContext( - // eslint-disable-next-line complexity - async () => { - let transaction: Transaction | undefined; - const currentScope = getCurrentScope(); - const options = getClient()?.getOptions(); - - currentScope.setSDKProcessingMetadata({ request: req }); - - if (options?.instrumenter === 'sentry') { - const sentryTrace = - req.headers && isString(req.headers['sentry-trace']) ? req.headers['sentry-trace'] : undefined; - const baggage = req.headers?.baggage; - const { traceparentData, dynamicSamplingContext, propagationContext } = tracingContextFromHeaders( - sentryTrace, - baggage, - ); - currentScope.setPropagationContext(propagationContext); - - if (DEBUG_BUILD && traceparentData) { - logger.log(`[Tracing] Continuing trace ${traceparentData.traceId}.`); + return runWithAsyncContext(async () => { + const transactionContext = continueTrace({ + sentryTrace: req.headers && isString(req.headers['sentry-trace']) ? req.headers['sentry-trace'] : undefined, + baggage: req.headers?.baggage, + }); + + // prefer the parameterized route, if we have it (which we will if we've auto-wrapped the route handler) + let reqPath = parameterizedRoute; + + // If not, fake it by just replacing parameter values with their names, hoping that none of them match either + // each other or any hard-coded parts of the path + if (!reqPath) { + const url = `${req.url}`; + // pull off query string, if any + reqPath = stripUrlQueryAndFragment(url); + // Replace with placeholder + if (req.query) { + for (const [key, value] of Object.entries(req.query)) { + reqPath = reqPath.replace(`${value}`, `[${key}]`); } - - // prefer the parameterized route, if we have it (which we will if we've auto-wrapped the route handler) - let reqPath = parameterizedRoute; - - // If not, fake it by just replacing parameter values with their names, hoping that none of them match either - // each other or any hard-coded parts of the path - if (!reqPath) { - const url = `${req.url}`; - // pull off query string, if any - reqPath = stripUrlQueryAndFragment(url); - // Replace with placeholder - if (req.query) { - for (const [key, value] of Object.entries(req.query)) { - reqPath = reqPath.replace(`${value}`, `[${key}]`); + } + } + + const reqMethod = `${(req.method || 'GET').toUpperCase()} `; + + return startSpanManual( + { + ...transactionContext, + name: `${reqMethod}${reqPath}`, + op: 'http.server', + origin: 'auto.http.nextjs', + metadata: { + ...transactionContext.metadata, + source: 'route', + request: req, + }, + }, + async (_span, finishSpan) => { + // eslint-disable-next-line @typescript-eslint/unbound-method + res.end = new Proxy(res.end, { + apply(target, thisArg, argArray) { + finishSpan(); + if (platformSupportsStreaming() && !wrappingTarget.__sentry_test_doesnt_support_streaming__) { + target.apply(thisArg, argArray); + } else { + // flushQueue will not reject + // eslint-disable-next-line @typescript-eslint/no-floating-promises + flushQueue().then(() => { + target.apply(thisArg, argArray); + }); } - } - } - - const reqMethod = `${(req.method || 'GET').toUpperCase()} `; - - transaction = startTransaction( - { - name: `${reqMethod}${reqPath}`, - op: 'http.server', - origin: 'auto.http.nextjs', - ...traceparentData, - metadata: { - dynamicSamplingContext: traceparentData && !dynamicSamplingContext ? {} : dynamicSamplingContext, - source: 'route', - request: req, - }, }, - // extra context passed to the `tracesSampler` - { request: req }, - ); - currentScope.setSpan(transaction); - if (platformSupportsStreaming() && !wrappingTarget.__sentry_test_doesnt_support_streaming__) { - autoEndTransactionOnResponseEnd(transaction, res); - } else { - // If we're not on a platform that supports streaming, we're blocking res.end() until the queue is flushed. - // res.json() and res.send() will implicitly call res.end(), so it is enough to wrap res.end(). - - // eslint-disable-next-line @typescript-eslint/unbound-method - const origResEnd = res.end; - res.end = async function (this: unknown, ...args: unknown[]) { - if (transaction) { - finishTransaction(transaction, res); - await flushQueue(); - } + }); - origResEnd.apply(this, args); - }; - } - } + try { + const handlerResult = await wrappingTarget.apply(thisArg, args); + if ( + process.env.NODE_ENV === 'development' && + !process.env.SENTRY_IGNORE_API_RESOLUTION_ERROR && + !res.finished + // TODO(v8): Remove this warning? + // This can only happen (not always) when the user is using `withSentry` manually, which we're deprecating. + // Warning suppression on Next.JS is only necessary in that case. + ) { + consoleSandbox(() => { + // eslint-disable-next-line no-console + console.warn( + '[sentry] If Next.js logs a warning "API resolved without sending a response", it\'s a false positive, which may happen when you use `withSentry` manually to wrap your routes. To suppress this warning, set `SENTRY_IGNORE_API_RESOLUTION_ERROR` to 1 in your env. To suppress the nextjs warning, use the `externalResolver` API route option (see https://nextjs.org/docs/api-routes/api-middlewares#custom-config for details).', + ); + }); + } - try { - const handlerResult = await wrappingTarget.apply(thisArg, args); - - if ( - process.env.NODE_ENV === 'development' && - !process.env.SENTRY_IGNORE_API_RESOLUTION_ERROR && - !res.finished - // This can only happen (not always) when the user is using `withSentry` manually, which we're deprecating. - // Warning suppression on Next.JS is only necessary in that case. - ) { - consoleSandbox(() => { - // eslint-disable-next-line no-console - console.warn( - `[sentry] If Next.js logs a warning "API resolved without sending a response", it's a false positive, which may happen when you use \`withSentry\` manually to wrap your routes. - To suppress this warning, set \`SENTRY_IGNORE_API_RESOLUTION_ERROR\` to 1 in your env. - To suppress the nextjs warning, use the \`externalResolver\` API route option (see https://nextjs.org/docs/api-routes/api-middlewares#custom-config for details).`, - ); + return handlerResult; + } catch (e) { + // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can + // store a seen flag on it. (Because of the one-way-on-Vercel-one-way-off-of-Vercel approach we've been forced + // to take, it can happen that the same thrown object gets caught in two different ways, and flagging it is a + // way to prevent it from actually being reported twice.) + const objectifiedErr = objectify(e); + + captureException(objectifiedErr, { + mechanism: { + type: 'instrument', + handled: false, + data: { + wrapped_handler: wrappingTarget.name, + function: 'withSentry', + }, + }, }); - } - return handlerResult; - } catch (e) { - // In case we have a primitive, wrap it in the equivalent wrapper class (string -> String, etc.) so that we can - // store a seen flag on it. (Because of the one-way-on-Vercel-one-way-off-of-Vercel approach we've been forced - // to take, it can happen that the same thrown object gets caught in two different ways, and flagging it is a - // way to prevent it from actually being reported twice.) - const objectifiedErr = objectify(e); - - captureException(objectifiedErr, { - mechanism: { - type: 'instrument', - handled: false, - data: { - wrapped_handler: wrappingTarget.name, - function: 'withSentry', - }, - }, - }); + // Because we're going to finish and send the transaction before passing the error onto nextjs, it won't yet + // have had a chance to set the status to 500, so unless we do it ourselves now, we'll incorrectly report that + // the transaction was error-free + res.statusCode = 500; + res.statusMessage = 'Internal Server Error'; + + finishSpan(); + + // Make sure we have a chance to finish the transaction and flush events to Sentry before the handler errors + // out. (Apps which are deployed on Vercel run their API routes in lambdas, and those lambdas will shut down the + // moment they detect an error, so it's important to get this done before rethrowing the error. Apps not + // deployed serverlessly will run into this cleanup code again in `res.end(), but the transaction will already + // be finished and the queue will already be empty, so effectively it'll just no-op.) + if (platformSupportsStreaming() && !wrappingTarget.__sentry_test_doesnt_support_streaming__) { + await flushQueue(); + } - // Because we're going to finish and send the transaction before passing the error onto nextjs, it won't yet - // have had a chance to set the status to 500, so unless we do it ourselves now, we'll incorrectly report that - // the transaction was error-free - res.statusCode = 500; - res.statusMessage = 'Internal Server Error'; - - finishTransaction(transaction, res); - - // Make sure we have a chance to finish the transaction and flush events to Sentry before the handler errors - // out. (Apps which are deployed on Vercel run their API routes in lambdas, and those lambdas will shut down the - // moment they detect an error, so it's important to get this done before rethrowing the error. Apps not - // deployed serverlessly will run into this cleanup code again in `res.end(), but the transaction will already - // be finished and the queue will already be empty, so effectively it'll just no-op.) - if (platformSupportsStreaming() && !wrappingTarget.__sentry_test_doesnt_support_streaming__) { - await flushQueue(); + // We rethrow here so that nextjs can do with the error whatever it would normally do. (Sometimes "whatever it + // would normally do" is to allow the error to bubble up to the global handlers - another reason we need to mark + // the error as already having been captured.) + throw objectifiedErr; } - - // We rethrow here so that nextjs can do with the error whatever it would normally do. (Sometimes "whatever it - // would normally do" is to allow the error to bubble up to the global handlers - another reason we need to mark - // the error as already having been captured.) - throw objectifiedErr; - } - }, - ); - - // Since API route handlers are all async, nextjs always awaits the return value (meaning it's fine for us to return - // a promise here rather than a real result, and it saves us the overhead of an `await` call.) - return boundHandler; + }, + ); + }); }, }); } From f6fdbbb01dcf2e76dc1e9d3d3299fc4da2fa44e0 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 22 Dec 2023 11:12:40 +0000 Subject: [PATCH 2/6] Move to newr api --- packages/nextjs/src/common/wrapApiHandlerWithSentry.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts b/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts index b6c79ef2cd23..dc2285fc0b8b 100644 --- a/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts +++ b/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts @@ -111,11 +111,11 @@ export function withSentry(apiHandler: NextApiHandler, parameterizedRoute?: stri request: req, }, }, - async (_span, finishSpan) => { + async span => { // eslint-disable-next-line @typescript-eslint/unbound-method res.end = new Proxy(res.end, { apply(target, thisArg, argArray) { - finishSpan(); + span?.end(); if (platformSupportsStreaming() && !wrappingTarget.__sentry_test_doesnt_support_streaming__) { target.apply(thisArg, argArray); } else { @@ -171,7 +171,7 @@ export function withSentry(apiHandler: NextApiHandler, parameterizedRoute?: stri res.statusCode = 500; res.statusMessage = 'Internal Server Error'; - finishSpan(); + span?.end(); // Make sure we have a chance to finish the transaction and flush events to Sentry before the handler errors // out. (Apps which are deployed on Vercel run their API routes in lambdas, and those lambdas will shut down the From 279bf5b41681643a98bd304369c4a0155fd12ba6 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 22 Dec 2023 11:26:38 +0000 Subject: [PATCH 3/6] Fix test --- .../nextjs/test/config/withSentry.test.ts | 45 ++----------------- 1 file changed, 4 insertions(+), 41 deletions(-) diff --git a/packages/nextjs/test/config/withSentry.test.ts b/packages/nextjs/test/config/withSentry.test.ts index d997419a88a2..da43ec724944 100644 --- a/packages/nextjs/test/config/withSentry.test.ts +++ b/packages/nextjs/test/config/withSentry.test.ts @@ -1,6 +1,5 @@ import * as SentryCore from '@sentry/core'; import { addTracingExtensions } from '@sentry/core'; -import type { Client, ClientOptions } from '@sentry/types'; import type { NextApiRequest, NextApiResponse } from 'next'; import type { AugmentedNextApiResponse, NextApiHandler } from '../../src/common/types'; @@ -10,37 +9,7 @@ import { withSentry } from '../../src/server'; // constructor but the client isn't used in these tests. addTracingExtensions(); -const FLUSH_DURATION = 200; - -async function sleep(ms: number): Promise { - await new Promise(resolve => setTimeout(resolve, ms)); -} -/** - * Helper to prevent tests from ending before `flush()` has finished its work. - * - * This is necessary because, like its real-life counterpart, our mocked `res.send()` below doesn't await `res.end() - * (which becomes async when we wrap it in `withSentry` in order to give `flush()` time to run). In real life, the - * request/response cycle is held open as subsequent steps wait for `end()` to emit its `prefinished` event. Here in - * tests, without any of that other machinery, we have to hold it open ourselves. - * - * @param wrappedHandler - * @param req - * @param res - */ -async function callWrappedHandler(wrappedHandler: NextApiHandler, req: NextApiRequest, res: NextApiResponse) { - await wrappedHandler(req, res); - - // we know we need to wait at least this long for `flush()` to finish - await sleep(FLUSH_DURATION); - - // should be <1 second, just long enough the `flush()` call to return, the original (pre-wrapping) `res.end()` to be - // called, and the response to be marked as done - while (!res.finished) { - await sleep(100); - } -} - -const startTransactionSpy = jest.spyOn(SentryCore, 'startTransaction'); +const startSpanManualSpy = jest.spyOn(SentryCore, 'startSpanManual'); describe('withSentry', () => { let req: NextApiRequest, res: NextApiResponse; @@ -70,24 +39,18 @@ describe('withSentry', () => { describe('tracing', () => { it('starts a transaction and sets metadata when tracing is enabled', async () => { - jest.spyOn(SentryCore.Hub.prototype, 'getClient').mockReturnValueOnce({ - getOptions: () => ({ tracesSampleRate: 1, instrumenter: 'sentry' }) as ClientOptions, - } as Client); - - await callWrappedHandler(wrappedHandlerNoError, req, res); - - expect(startTransactionSpy).toHaveBeenCalledWith( + await wrappedHandlerNoError(req, res); + expect(startSpanManualSpy).toHaveBeenCalledWith( { name: 'GET http://dogs.are.great', op: 'http.server', origin: 'auto.http.nextjs', - metadata: { source: 'route', request: expect.objectContaining({ url: 'http://dogs.are.great' }), }, }, - { request: expect.objectContaining({ url: 'http://dogs.are.great' }) }, + expect.any(Function), ); }); }); From 1d2f80bb502da9f2dec22a535e54d8598482a558 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 22 Dec 2023 11:55:43 +0000 Subject: [PATCH 4/6] Status code --- packages/nextjs/src/common/wrapApiHandlerWithSentry.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts b/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts index dc2285fc0b8b..cf4e92627406 100644 --- a/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts +++ b/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts @@ -115,6 +115,7 @@ export function withSentry(apiHandler: NextApiHandler, parameterizedRoute?: stri // eslint-disable-next-line @typescript-eslint/unbound-method res.end = new Proxy(res.end, { apply(target, thisArg, argArray) { + span?.setHttpStatus(res.statusCode); span?.end(); if (platformSupportsStreaming() && !wrappingTarget.__sentry_test_doesnt_support_streaming__) { target.apply(thisArg, argArray); @@ -171,6 +172,7 @@ export function withSentry(apiHandler: NextApiHandler, parameterizedRoute?: stri res.statusCode = 500; res.statusMessage = 'Internal Server Error'; + span?.setHttpStatus(res.statusCode); span?.end(); // Make sure we have a chance to finish the transaction and flush events to Sentry before the handler errors From 5167c3e0831c569df7199ba83ed401c32da68056 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 22 Dec 2023 12:16:35 +0000 Subject: [PATCH 5/6] Set request sdk processing metadata --- packages/nextjs/src/common/wrapApiHandlerWithSentry.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts b/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts index cf4e92627406..d1b51b6fc36e 100644 --- a/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts +++ b/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts @@ -2,6 +2,7 @@ import { addTracingExtensions, captureException, continueTrace, + getCurrentScope, runWithAsyncContext, startSpanManual, } from '@sentry/core'; @@ -112,6 +113,8 @@ export function withSentry(apiHandler: NextApiHandler, parameterizedRoute?: stri }, }, async span => { + getCurrentScope().setSDKProcessingMetadata({ request: req }); + // eslint-disable-next-line @typescript-eslint/unbound-method res.end = new Proxy(res.end, { apply(target, thisArg, argArray) { From af60c56b6739074876aa603a1454dfe3fa15e16a Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Fri, 22 Dec 2023 12:33:23 +0000 Subject: [PATCH 6/6] Set request sdk processing metadata --- packages/nextjs/src/common/wrapApiHandlerWithSentry.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts b/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts index d1b51b6fc36e..fc8f602f524b 100644 --- a/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts +++ b/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts @@ -100,6 +100,8 @@ export function withSentry(apiHandler: NextApiHandler, parameterizedRoute?: stri const reqMethod = `${(req.method || 'GET').toUpperCase()} `; + getCurrentScope().setSDKProcessingMetadata({ request: req }); + return startSpanManual( { ...transactionContext, @@ -113,8 +115,6 @@ export function withSentry(apiHandler: NextApiHandler, parameterizedRoute?: stri }, }, async span => { - getCurrentScope().setSDKProcessingMetadata({ request: req }); - // eslint-disable-next-line @typescript-eslint/unbound-method res.end = new Proxy(res.end, { apply(target, thisArg, argArray) {