Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(handlers): prevent errors thrown on flush from breaking response #4667

Merged
merged 7 commits into from
Mar 3, 2022

Conversation

Ignigena
Copy link
Contributor

@Ignigena Ignigena commented Mar 2, 2022

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

We are using Sentry.Handlers.requestHandler with the flushTimeout parameter set. After a recent upgrade of the @sentry/node package we noticed a steady amount of failed requests on our application and determined that res.end was never called on these failed requests. Further complicating the diagnosis: no corresponding Issues reported in our Sentry account 馃檭

After some digging, it appears that the Sentry.flush command was actually throwing in these cases. In our case this appears to be due to throttling on Sentry's side based on rate limits associated with our account. However, when this happens it breaks the entire server response -- even if no errors have been captured on that particular session.

The more concerning part of this, however, is if Sentry ever goes down for any reason this would essentially result in every single request to our application never being served. I believe we hit this issue with one of our applications where our Sentry was only reporting 1,062 dropped events due to spike protection versus our server logs reporting nearly 1,500 such failed requests.

This appears to be a similar situation to #4620 which was resolved for the @sentry/serverless package.

For now I've just amended the default behaviour to always call the original res.end, whether the call to flush succeeds or fails. I am certainly willing to make this opt-in with the same option flag as the recent change to @sentry/serverless -- however, this seems like it should be default behaviour since it can otherwise lead to a very broken app that is very hard to diagnose.

Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a very reasonable change to me. Will cc my team to get their thoughts as well.

packages/node/src/handlers.ts Outdated Show resolved Hide resolved
@AbhiPrasad AbhiPrasad requested review from a team, Lms24, AbhiPrasad and lobsterkatie and removed request for a team March 2, 2022 17:41
Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! This looks good to me.

(It's what we do in the nextjs SDK also:

function wrapEndMethod(origEnd: ResponseEndMethod): WrappedResponseEndMethod {
return async function newEnd(this: AugmentedNextApiResponse, ...args: unknown[]) {
await finishSentryProcessing(this);
return origEnd.call(this, ...args);
};
}
/**
* Close the open transaction (if any) and flush events to Sentry.
*
* @param res The outgoing response for this request, on which the transaction is stored
*/
async function finishSentryProcessing(res: AugmentedNextApiResponse): Promise<void> {
const { __sentryTransaction: transaction } = res;
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<void> = 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. If there was an error, rethrow it so that the normal exception-handling mechanisms can apply.
try {
logger.log('Flushing events...');
await flush(2000);
logger.log('Done flushing events');
} catch (e) {
logger.log(`Error while flushing events:\n`, e);
}
}
.)

packages/node/test/handlers.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annnnnd... my edits broke the tests. This is what I get for making edits in the GH UI. Hopefully this fixes it.

packages/node/test/handlers.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once again, from the top, this time hopefully with a happy linter.

packages/node/test/handlers.test.ts Outdated Show resolved Hide resolved
Copy link
Member

@lobsterkatie lobsterkatie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃槗

(P.S., yes, I know that in theory I should be able to do this locally and push but... clearly not my GH day today.)

packages/node/test/handlers.test.ts Outdated Show resolved Hide resolved
@lobsterkatie
Copy link
Member

Now blocked by #4675.

@kamilogorek kamilogorek enabled auto-merge (squash) March 3, 2022 09:52
@kamilogorek
Copy link
Contributor

Thanks @Ignigena!

@kamilogorek kamilogorek merged commit 0b2221d into getsentry:master Mar 3, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants