From 2956c47b730bd11cff722682f13dda08cb5e466e Mon Sep 17 00:00:00 2001 From: James Murty Date: Mon, 11 Oct 2021 13:38:02 +1100 Subject: [PATCH 1/3] Ensure `withSentry` reports API (serverless) errors from Vercel Potential fix for #3917 to ensure Sentry error reporting clean-up is done -- transaction finished and error flushed -- when run from a Vercel deployment, by explicitly calling the clean-up code in two places: directly from the `withSentry` handler wrapper function, as well as the existing monkey-patched `res.end()`. In Vercel the `res.end()` function is not (or is not always) called, which means the prior approach that relied on monkey-patching of that function for clean-up did not work in Vercel. Note 1: this is a naive fix: I'm not sure why res.end() isn't called as expected or if there might be a better target for monkey- patching. Note 2: a new `__flushed` variable is used to avoid running the clean-up code twice, should both the explicit and the monkey-patched path be run. See the TODO asking whether this flag should be set at the beginning, instead of the end, of the clean-up function `finishTransactionAndFlush()` --- packages/nextjs/src/utils/withSentry.ts | 65 ++++++++++++++++--------- 1 file changed, 41 insertions(+), 24 deletions(-) diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index dbccfceb1724..a1510c59b2c0 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/packages/nextjs/src/utils/withSentry.ts @@ -10,7 +10,7 @@ const { parseRequest } = Handlers; // purely for clarity type WrappedNextApiHandler = NextApiHandler; -type AugmentedResponse = NextApiResponse & { __sentryTransaction?: Transaction }; +type AugmentedResponse = NextApiResponse & { __sentryTransaction?: Transaction, __flushed?: boolean }; // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { @@ -84,6 +84,10 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { return event; }); captureException(e); + + // Explicitly call function to finish transaction and flush in case the monkeypatched `res.end()` is + // never called; it isn't always called for Vercel deployment + await finishTransactionAndFlush(res); } throw e; } @@ -93,35 +97,48 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { }; }; +async function finishTransactionAndFlush(res: AugmentedResponse) { + const transaction = res.__sentryTransaction; + + if (transaction) { + transaction.setHttpStatus(res.statusCode); + + // Push `transaction.finish` to the next event loop so open spans have a better chance of finishing before the + // transaction closes, and make sure to wait until that's done before flushing events + const transactionFinished: Promise = new Promise((resolve) => { + setImmediate(() => { + transaction.finish(); + resolve(); + }); + }); + await transactionFinished; + } + + // flush the event queue to ensure that events get sent to Sentry before the response is finished and the lambda + // ends + try { + logger.log('Flushing events...'); + await flush(2000); + logger.log('Done flushing events'); + } catch (e) { + logger.log(`Error while flushing events:\n${e}`); + } finally { + // Flag response as already finished and flushed, to avoid double-flushing + // TODO Set at beginning of this function to better avoid double runs? + res.__flushed = true; + } +} + type ResponseEndMethod = AugmentedResponse['end']; type WrappedResponseEndMethod = AugmentedResponse['end']; function wrapEndMethod(origEnd: ResponseEndMethod): WrappedResponseEndMethod { return async function newEnd(this: AugmentedResponse, ...args: unknown[]) { - const transaction = this.__sentryTransaction; - - if (transaction) { - transaction.setHttpStatus(this.statusCode); - - // Push `transaction.finish` to the next event loop so open spans have a better chance of finishing before the - // transaction closes, and make sure to wait until that's done before flushing events - const transactionFinished: Promise = new Promise(resolve => { - setImmediate(() => { - transaction.finish(); - resolve(); - }); - }); - await transactionFinished; - } - // flush the event queue to ensure that events get sent to Sentry before the response is finished and the lambda - // ends - try { - logger.log('Flushing events...'); - await flush(2000); - logger.log('Done flushing events'); - } catch (e) { - logger.log(`Error while flushing events:\n${e}`); + if (this.__flushed) { + logger.log('Skip finish transaction and flush, already done'); + } else { + await finishTransactionAndFlush(this); } return origEnd.call(this, ...args); From 7ab43f2890be125819502f637886ebf84b815cf0 Mon Sep 17 00:00:00 2001 From: James Murty Date: Mon, 11 Oct 2021 13:40:26 +1100 Subject: [PATCH 2/3] Linting fix --- packages/nextjs/src/utils/withSentry.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index a1510c59b2c0..b77e0dcc6257 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/packages/nextjs/src/utils/withSentry.ts @@ -10,7 +10,7 @@ const { parseRequest } = Handlers; // purely for clarity type WrappedNextApiHandler = NextApiHandler; -type AugmentedResponse = NextApiResponse & { __sentryTransaction?: Transaction, __flushed?: boolean }; +type AugmentedResponse = NextApiResponse & { __sentryTransaction?: Transaction; __flushed?: boolean }; // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { @@ -105,7 +105,7 @@ async function finishTransactionAndFlush(res: AugmentedResponse) { // Push `transaction.finish` to the next event loop so open spans have a better chance of finishing before the // transaction closes, and make sure to wait until that's done before flushing events - const transactionFinished: Promise = new Promise((resolve) => { + const transactionFinished: Promise = new Promise(resolve => { setImmediate(() => { transaction.finish(); resolve(); @@ -134,7 +134,6 @@ type WrappedResponseEndMethod = AugmentedResponse['end']; function wrapEndMethod(origEnd: ResponseEndMethod): WrappedResponseEndMethod { return async function newEnd(this: AugmentedResponse, ...args: unknown[]) { - if (this.__flushed) { logger.log('Skip finish transaction and flush, already done'); } else { From c0a32719108c88b05cbb64a15f20ed2920c3bb92 Mon Sep 17 00:00:00 2001 From: James Murty Date: Mon, 11 Oct 2021 13:54:50 +1100 Subject: [PATCH 3/3] Add missing function return type --- packages/nextjs/src/utils/withSentry.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/nextjs/src/utils/withSentry.ts b/packages/nextjs/src/utils/withSentry.ts index b77e0dcc6257..69e2109b8967 100644 --- a/packages/nextjs/src/utils/withSentry.ts +++ b/packages/nextjs/src/utils/withSentry.ts @@ -97,7 +97,7 @@ export const withSentry = (handler: NextApiHandler): WrappedNextApiHandler => { }; }; -async function finishTransactionAndFlush(res: AugmentedResponse) { +async function finishTransactionAndFlush(res: AugmentedResponse): Promise { const transaction = res.__sentryTransaction; if (transaction) {