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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ref(serverless): Added option ignoreSentryErrors for lambda WrapperOptions #4620

Merged
merged 3 commits into from
Feb 23, 2022
Merged

ref(serverless): Added option ignoreSentryErrors for lambda WrapperOptions #4620

merged 3 commits into from
Feb 23, 2022

Conversation

ichina
Copy link
Contributor

@ichina ichina commented Feb 23, 2022

This PR was built on top of #4582 so the changes here will allow sdk consumers to ignore sentry internal errors like 429 - too many requests, or 413 payload too large, etc...

in our case 429 error happens quite often per day since we added rate limit for each lambda project (that is recommended as a sentry quota management strategy ).
so, as u can find in tests, even for successful lambda executions, 429 error can fail the whole lambda invocation, which heavily affects our clients user experience

@ichina ichina marked this pull request as ready for review February 23, 2022 04:53
@@ -314,7 +316,12 @@ export function wrapHandler<TEvent, TResult>(
clearTimeout(timeoutWarningTimer);
transaction.finish();
hub.popScope();
await flush(options.flushTimeout);
Copy link
Contributor Author

@ichina ichina Feb 23, 2022

Choose a reason for hiding this comment

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

as an argument, here is similar error nullification on flushing operation for google functions

void flush(options.flushTimeout)
.then(() => {
callback(...args);
})
.then(null, e => {
logger.error(e);
});

Copy link
Contributor Author

@ichina ichina Feb 23, 2022

Choose a reason for hiding this comment

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

for traditional ExpressJS/Koa

void flush(options.flushTimeout)
.then(() => {
_end.call(this, chunk, encoding, cb);
})
.then(null, e => {
logger.error(e);
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

additionally, as i understand, 'flush' operation may be time consuming, so here it can affect lambda execution time even on successful invocation

Copy link
Contributor

Choose a reason for hiding this comment

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

Afaik this will not work in the case of AWS, as it will freeze the process immediately after reaching return value of the function.
Why it works for GCP, is that there is an explicit callback that tells the runtime that it's done executing.

Copy link
Contributor Author

@ichina ichina Feb 23, 2022

Choose a reason for hiding this comment

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

yep, you are right, with freezing the process, we can't do anything after the event, only via Lambda Extension, which is not our case. btw, in this PR i only propose to ignore sentry errors, same as in GCP and regular expressJs/Koa, they ignore sentry errors as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kamilogorek may i ask you to clarify what exact updates you requested

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I had a brain-fart for a second, as I was almost certain you changed from await flush to void flush 😅
It's all good! Thanks :)

@@ -224,6 +225,7 @@ export function wrapHandler<TEvent, TResult>(
captureTimeoutWarning: true,
timeoutWarningLimit: 500,
captureAllSettledReasons: false,
ignoreSentryErrors: false,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

default value does not change current behaviour

@@ -314,7 +316,12 @@ export function wrapHandler<TEvent, TResult>(
clearTimeout(timeoutWarningTimer);
transaction.finish();
hub.popScope();
await flush(options.flushTimeout);
Copy link
Contributor

Choose a reason for hiding this comment

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

Afaik this will not work in the case of AWS, as it will freeze the process immediately after reaching return value of the function.
Why it works for GCP, is that there is an explicit callback that tells the runtime that it's done executing.

@AbhiPrasad
Copy link
Member

Can we open a docs PR to document the option? https://docs.sentry.io/platforms/node/guides/aws-lambda/

We’ll then merge that in after we make a release.

@AbhiPrasad
Copy link
Member

You’ll also need to rebase as we’ve fixed some test failures on master.

@ichina
Copy link
Contributor Author

ichina commented Feb 23, 2022

@AbhiPrasad i've rebased to the latest master, but there are still some tests failures, can u advice please on it. i don't think my changes relate to this errors

@AbhiPrasad
Copy link
Member

The ember failures are flakes, we can re-rerun. There also seems to be lint failures that you can check by running yarn lint

@aali-fanfix
Copy link

Is this is the correct way of using this new flag?

import * as Sentry from '@sentry/nextjs';

Sentry.init({
dsn: 'https://my DNS here',
// Adjust this value in production, or use tracesSampler for greater control
tracesSampleRate: 0.1,
ignoreSentryErrors: true,
debug: false,
replaysOnErrorSampleRate: 0.1,
// This sets the sample rate to be 10%. You may want this to be 100% while
// in development and sample at a lower rate in production
replaysSessionSampleRate: process.env.NODE_ENV === 'production' ? 0.001 : 1,
// You can remove this option if you're not planning to use the Sentry Session Replay feature:
integrations: [
new Sentry.Replay({
// Additional Replay configuration goes in here, for example:
maskAllText: true,
blockAllMedia: true
})
]
});

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.

4 participants