From 34d111aa26fd53a0e29f45f0a90a2e5ecc3c2062 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 8 Oct 2024 07:26:31 +0000 Subject: [PATCH 1/2] ref(nextjs): Remove dead code --- packages/nextjs/src/common/types.ts | 5 -- .../src/common/wrapApiHandlerWithSentry.ts | 56 ++++++------------- 2 files changed, 16 insertions(+), 45 deletions(-) diff --git a/packages/nextjs/src/common/types.ts b/packages/nextjs/src/common/types.ts index 05d89d3e3159..517c3677d556 100644 --- a/packages/nextjs/src/common/types.ts +++ b/packages/nextjs/src/common/types.ts @@ -55,11 +55,6 @@ export type WrappedNextApiHandler = { __sentry_route__?: string; __sentry_wrapped__?: boolean; }; - -export type AugmentedNextApiRequest = NextApiRequest & { - __withSentry_applied__?: boolean; -}; - export type AugmentedNextApiResponse = NextApiResponse & { __sentryTransaction?: SentrySpan; }; diff --git a/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts b/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts index f658a4736857..cef85c320ae0 100644 --- a/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts +++ b/packages/nextjs/src/common/wrapApiHandlerWithSentry.ts @@ -6,21 +6,24 @@ import { startSpanManual, withIsolationScope, } from '@sentry/core'; -import { consoleSandbox, isString, logger, objectify, vercelWaitUntil } from '@sentry/utils'; - import { SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN } from '@sentry/core'; -import type { AugmentedNextApiRequest, AugmentedNextApiResponse, NextApiHandler } from './types'; +import { isString, logger, objectify, vercelWaitUntil } from '@sentry/utils'; +import type { NextApiRequest } from 'next'; +import type { AugmentedNextApiResponse, NextApiHandler } from './types'; import { flushSafelyWithTimeout } from './utils/responseEnd'; import { escapeNextjsTracing } from './utils/tracingUtils'; +export type AugmentedNextApiRequest = NextApiRequest & { + __withSentry_applied__?: boolean; +}; + /** - * Wrap the given API route handler for tracing and error capturing. Thin wrapper around `withSentry`, which only - * applies it if it hasn't already been applied. + * Wrap the given API route handler with error nad performance monitoring. * * @param apiHandler The handler exported from the user's API page route file, which may or may not already be * wrapped with `withSentry` * @param parameterizedRoute The page's parameterized route. - * @returns The wrapped handler + * @returns The wrapped handler which will always return a Promise. */ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameterizedRoute: string): NextApiHandler { return new Proxy(apiHandler, { @@ -44,9 +47,7 @@ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameteriz return wrappingTarget.apply(thisArg, args); } - // We're now auto-wrapping API route handlers using `wrapApiHandlerWithSentry` (which uses `withSentry` under the hood), but - // users still may have their routes manually wrapped with `withSentry`. This check makes `sentryWrappedHandler` - // idempotent so that those cases don't break anything. + // Prevent double wrapping of the same request. if (req.__withSentry_applied__) { return wrappingTarget.apply(thisArg, args); } @@ -55,7 +56,6 @@ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameteriz return withIsolationScope(isolationScope => { return continueTrace( { - // TODO(v8): Make it so that continue trace will allow null as sentryTrace value and remove this fallback here sentryTrace: req.headers && isString(req.headers['sentry-trace']) ? req.headers['sentry-trace'] : undefined, baggage: req.headers?.baggage, @@ -80,31 +80,15 @@ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameteriz // eslint-disable-next-line @typescript-eslint/unbound-method res.end = new Proxy(res.end, { apply(target, thisArg, argArray) { - if (span.isRecording()) { - setHttpStatus(span, res.statusCode); - span.end(); - } + setHttpStatus(span, res.statusCode); + span.end(); vercelWaitUntil(flushSafelyWithTimeout()); - target.apply(thisArg, argArray); + return target.apply(thisArg, argArray); }, }); try { - const handlerResult = await wrappingTarget.apply(thisArg, args); - if ( - process.env.NODE_ENV === 'development' && - !process.env.SENTRY_IGNORE_API_RESOLUTION_ERROR && - !res.writableEnded - ) { - 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 `wrapApiHandlerWithSentry` 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; + return await wrappingTarget.apply(thisArg, args); } 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 @@ -123,16 +107,8 @@ export function wrapApiHandlerWithSentry(apiHandler: NextApiHandler, parameteriz }, }); - // 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'; - - if (span.isRecording()) { - setHttpStatus(span, res.statusCode); - span.end(); - } + setHttpStatus(span, 500); + span.end(); vercelWaitUntil(flushSafelyWithTimeout()); From 9a63d6838155acd762eb0de97da2d52b5cb7337d Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Tue, 8 Oct 2024 07:29:09 +0000 Subject: [PATCH 2/2] Add changelog entry --- CHANGELOG.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 65597758e81f..c03cb6556afe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,13 @@ ## Unreleased +### Important Changes + +- **ref(nextjs): Remove dead code ([#13828](https://github.com/getsentry/sentry-javascript/pull/13903))** + +Relevant for users of the `@sentry/nextjs` package: If you have previously configured a +`SENTRY_IGNORE_API_RESOLUTION_ERROR` environment variable, it is now safe to unset it. + - "You miss 100 percent of the chances you don't take. — Wayne Gretzky" — Michael Scott Work in this release was contributed by @trzeciak and @lizhiyao. Thank you for your contributions!