Skip to content

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

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Feb 23, 2022
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
52d463b
added ignoreSentryErrors option with tests
ichina Feb 23, 2022
b5d3a4d
added error logging
ichina Feb 23, 2022
df39880
lint fixes
ichina Feb 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions packages/serverless/src/awslambda.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
} from '@sentry/node';
import { extractTraceparentData } from '@sentry/tracing';
import { Integration } from '@sentry/types';
import { isString, logger } from '@sentry/utils';
import { isString, logger, SentryError } from '@sentry/utils';
// NOTE: I have no idea how to fix this right now, and don't want to waste more time, as it builds just fine — Kamil
// eslint-disable-next-line import/no-unresolved
import { Context, Handler } from 'aws-lambda';
Expand Down Expand Up @@ -53,6 +53,7 @@ export interface WrapperOptions {
* @default false
*/
captureAllSettledReasons: boolean;
ignoreSentryErrors: boolean;
}

export const defaultIntegrations: Integration[] = [...Sentry.defaultIntegrations, new AWSServices({ optional: true })];
Expand Down Expand Up @@ -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

...wrapOptions,
};
let timeoutWarningTimer: NodeJS.Timeout;
Expand Down Expand Up @@ -314,7 +316,13 @@ export function wrapHandler<TEvent, TResult>(
clearTimeout(timeoutWarningTimer);
transaction.finish();
hub.popScope();
await flush(options.flushTimeout);
Copy link
Contributor Author

@ichina ichina Feb 23, 2022 •
edited
Loading

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

sentry-javascript/packages/serverless/src/gcpfunction/events.ts

Lines 57 to 63 in 1bf9883

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

Copy link
Contributor Author

@ichina ichina Feb 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

for traditional ExpressJS/Koa

sentry-javascript/packages/node/src/handlers.ts

Lines 403 to 409 in 1bf9883

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 •
edited
Loading

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 :)

await flush(options.flushTimeout).catch(e => {
if (options.ignoreSentryErrors && e instanceof SentryError) {
logger.error(e);
return;
}
throw e;
});
}
return rv;
};
Expand Down
39 changes: 39 additions & 0 deletions packages/serverless/test/awslambda.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { SentryError } from '@sentry/utils';
// NOTE: I have no idea how to fix this right now, and don't want to waste more time, as it builds just fine — Kamil
// eslint-disable-next-line import/no-unresolved
import { Callback, Handler } from 'aws-lambda';
Expand Down Expand Up @@ -177,6 +178,44 @@ describe('AWSLambda', () => {
expect(Sentry.captureException).toHaveBeenNthCalledWith(2, error2);
expect(Sentry.captureException).toBeCalledTimes(2);
});

test('ignoreSentryErrors - with successful handler', async () => {
const sentryError = new SentryError('HTTP Error (429)');
jest.spyOn(Sentry, 'flush').mockRejectedValueOnce(sentryError);

const handledError = new Error('handled error, and we want to monitor it');
const expectedSuccessStatus = { status: 'success', reason: 'we handled error, so success' };
const handler = () => {
Sentry.captureException(handledError);
return Promise.resolve(expectedSuccessStatus);
};
const wrappedHandlerWithoutIgnoringSentryErrors = wrapHandler(handler, { ignoreSentryErrors: false });
const wrappedHandlerWithIgnoringSentryErrors = wrapHandler(handler, { ignoreSentryErrors: true });

await expect(wrappedHandlerWithoutIgnoringSentryErrors(fakeEvent, fakeContext, fakeCallback)).rejects.toThrow(
sentryError,
);
await expect(wrappedHandlerWithIgnoringSentryErrors(fakeEvent, fakeContext, fakeCallback)).resolves.toBe(
expectedSuccessStatus,
);
});

test('ignoreSentryErrors - with failed handler', async () => {
const sentryError = new SentryError('HTTP Error (429)');
jest.spyOn(Sentry, 'flush').mockRejectedValueOnce(sentryError);

const criticalUnhandledError = new Error('critical unhandled error ');
const handler = () => Promise.reject(criticalUnhandledError);
const wrappedHandlerWithoutIgnoringSentryErrors = wrapHandler(handler, { ignoreSentryErrors: false });
const wrappedHandlerWithIgnoringSentryErrors = wrapHandler(handler, { ignoreSentryErrors: true });

await expect(wrappedHandlerWithoutIgnoringSentryErrors(fakeEvent, fakeContext, fakeCallback)).rejects.toThrow(
sentryError,
);
await expect(wrappedHandlerWithIgnoringSentryErrors(fakeEvent, fakeContext, fakeCallback)).rejects.toThrow(
criticalUnhandledError,
);
});
});

describe('wrapHandler() on sync handler', () => {
Expand Down