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

feat(serverless): Mark errors caught in Serverless handlers as unhandled #8907

Merged
merged 2 commits into from
Sep 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 3 additions & 3 deletions packages/serverless/src/awslambda.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import { performance } from 'perf_hooks';
import { types } from 'util';

import { AWSServices } from './awsservices';
import { serverlessEventProcessor } from './utils';
import { markEventUnhandled, serverlessEventProcessor } from './utils';

export * from '@sentry/node';

Expand Down Expand Up @@ -312,11 +312,11 @@ export function wrapHandler<TEvent, TResult>(
if (options.captureAllSettledReasons && Array.isArray(rv) && isPromiseAllSettledResult(rv)) {
const reasons = getRejectedReasons(rv);
reasons.forEach(exception => {
captureException(exception);
captureException(exception, scope => markEventUnhandled(scope));
});
}
} catch (e) {
captureException(e);
captureException(e, scope => markEventUnhandled(scope));
throw e;
} finally {
clearTimeout(timeoutWarningTimer);
Expand Down
8 changes: 4 additions & 4 deletions packages/serverless/src/gcpfunction/cloud_events.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { captureException, flush, getCurrentHub } from '@sentry/node';
import { isThenable, logger } from '@sentry/utils';

import { domainify, proxyFunction } from '../utils';
import { domainify, markEventUnhandled, proxyFunction } from '../utils';
import type { CloudEventFunction, CloudEventFunctionWithCallback, WrapperOptions } from './general';

export type CloudEventFunctionWrapperOptions = WrapperOptions;
Expand Down Expand Up @@ -50,7 +50,7 @@ function _wrapCloudEventFunction(

const newCallback = domainify((...args: unknown[]) => {
if (args[0] !== null && args[0] !== undefined) {
captureException(args[0]);
captureException(args[0], scope => markEventUnhandled(scope));
}
transaction?.finish();

Expand All @@ -68,13 +68,13 @@ function _wrapCloudEventFunction(
try {
fnResult = (fn as CloudEventFunctionWithCallback)(context, newCallback);
} catch (err) {
captureException(err);
captureException(err, scope => markEventUnhandled(scope));
throw err;
}

if (isThenable(fnResult)) {
fnResult.then(null, err => {
captureException(err);
captureException(err, scope => markEventUnhandled(scope));
throw err;
});
}
Expand Down
8 changes: 4 additions & 4 deletions packages/serverless/src/gcpfunction/events.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { captureException, flush, getCurrentHub } from '@sentry/node';
import { isThenable, logger } from '@sentry/utils';

import { domainify, proxyFunction } from '../utils';
import { domainify, markEventUnhandled, proxyFunction } from '../utils';
import type { EventFunction, EventFunctionWithCallback, WrapperOptions } from './general';

export type EventFunctionWrapperOptions = WrapperOptions;
Expand Down Expand Up @@ -52,7 +52,7 @@ function _wrapEventFunction<F extends EventFunction | EventFunctionWithCallback>

const newCallback = domainify((...args: unknown[]) => {
if (args[0] !== null && args[0] !== undefined) {
captureException(args[0]);
captureException(args[0], scope => markEventUnhandled(scope));
}
transaction?.finish();

Expand All @@ -72,13 +72,13 @@ function _wrapEventFunction<F extends EventFunction | EventFunctionWithCallback>
try {
fnResult = (fn as EventFunctionWithCallback)(data, context, newCallback);
} catch (err) {
captureException(err);
captureException(err, scope => markEventUnhandled(scope));
throw err;
}

if (isThenable(fnResult)) {
fnResult.then(null, err => {
captureException(err);
captureException(err, scope => markEventUnhandled(scope));
throw err;
});
}
Expand Down
6 changes: 3 additions & 3 deletions packages/serverless/src/gcpfunction/http.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { AddRequestDataToEventOptions } from '@sentry/node';
import { captureException, flush, getCurrentHub } from '@sentry/node';
import { isString, isThenable, logger, stripUrlQueryAndFragment, tracingContextFromHeaders } from '@sentry/utils';

import { domainify, proxyFunction } from './../utils';
import { domainify, markEventUnhandled, proxyFunction } from './../utils';
import type { HttpFunction, WrapperOptions } from './general';

// TODO (v8 / #5257): Remove this whole old/new business and just use the new stuff
Expand Down Expand Up @@ -122,13 +122,13 @@ function _wrapHttpFunction(fn: HttpFunction, wrapOptions: Partial<HttpFunctionWr
try {
fnResult = fn(req, res);
} catch (err) {
captureException(err);
captureException(err, scope => markEventUnhandled(scope));
throw err;
}

if (isThenable(fnResult)) {
fnResult.then(null, err => {
captureException(err);
captureException(err, scope => markEventUnhandled(scope));
throw err;
});
}
Expand Down
13 changes: 13 additions & 0 deletions packages/serverless/src/utils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { runWithAsyncContext } from '@sentry/core';
import type { Event } from '@sentry/node';
import type { Scope } from '@sentry/types';
import { addExceptionMechanism } from '@sentry/utils';

/**
Expand Down Expand Up @@ -55,3 +56,15 @@ export function proxyFunction<A extends any[], R, F extends (...args: A) => R>(

return new Proxy(source, handler);
}

/**
* Marks an event as unhandled by adding a span processor to the passed scope.
*/
export function markEventUnhandled(scope: Scope): Scope {
scope.addEventProcessor(event => {
addExceptionMechanism(event, { handled: false });
return event;
});

return scope;
}
41 changes: 35 additions & 6 deletions packages/serverless/test/awslambda.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// 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 * as SentryNode from '@sentry/node';
import type { Event } from '@sentry/types';
// eslint-disable-next-line import/no-unresolved
import type { Callback, Handler } from 'aws-lambda';

Expand Down Expand Up @@ -175,8 +176,8 @@ describe('AWSLambda', () => {
]);
const wrappedHandler = wrapHandler(handler, { flushTimeout: 1337, captureAllSettledReasons: true });
await wrappedHandler(fakeEvent, fakeContext, fakeCallback);
expect(SentryNode.captureException).toHaveBeenNthCalledWith(1, error);
expect(SentryNode.captureException).toHaveBeenNthCalledWith(2, error2);
expect(SentryNode.captureException).toHaveBeenNthCalledWith(1, error, expect.any(Function));
expect(SentryNode.captureException).toHaveBeenNthCalledWith(2, error2, expect.any(Function));
expect(SentryNode.captureException).toBeCalledTimes(2);
});
});
Expand Down Expand Up @@ -229,7 +230,7 @@ describe('AWSLambda', () => {
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext);
expectScopeSettings(fakeTransactionContext);
expect(SentryNode.captureException).toBeCalledWith(error);
expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function));
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeTransaction.finish).toBeCalled();
expect(SentryNode.flush).toBeCalledWith(2000);
Expand Down Expand Up @@ -308,7 +309,7 @@ describe('AWSLambda', () => {
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext);
expectScopeSettings(fakeTransactionContext);
expect(SentryNode.captureException).toBeCalledWith(e);
expect(SentryNode.captureException).toBeCalledWith(e, expect.any(Function));
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeTransaction.finish).toBeCalled();
expect(SentryNode.flush).toBeCalled();
Expand Down Expand Up @@ -375,7 +376,7 @@ describe('AWSLambda', () => {
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext);
expectScopeSettings(fakeTransactionContext);
expect(SentryNode.captureException).toBeCalledWith(error);
expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function));
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeTransaction.finish).toBeCalled();
expect(SentryNode.flush).toBeCalled();
Expand Down Expand Up @@ -457,14 +458,42 @@ describe('AWSLambda', () => {
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext);
expectScopeSettings(fakeTransactionContext);
expect(SentryNode.captureException).toBeCalledWith(error);
expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function));
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeTransaction.finish).toBeCalled();
expect(SentryNode.flush).toBeCalled();
}
});
});

test('marks the captured error as unhandled', async () => {
expect.assertions(3);

const error = new Error('wat');
const handler: Handler = async (_event, _context, _callback) => {
throw error;
};
const wrappedHandler = wrapHandler(handler);

try {
await wrappedHandler(fakeEvent, fakeContext, fakeCallback);
} catch (e) {
expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function));
// @ts-ignore see "Why @ts-ignore" note
const scopeFunction = SentryNode.captureException.mock.calls[0][1];
const event: Event = { exception: { values: [{}] } };
let evtProcessor: ((e: Event) => Event) | undefined = undefined;
scopeFunction({ addEventProcessor: jest.fn().mockImplementation(proc => (evtProcessor = proc)) });

expect(evtProcessor).toBeInstanceOf(Function);
// @ts-ignore just mocking around...
expect(evtProcessor(event).exception.values[0].mechanism).toEqual({
handled: false,
type: 'generic',
});
}
});

describe('init()', () => {
test('calls Sentry.init with correct sdk info metadata', () => {
Sentry.AWSLambda.init({});
Expand Down
44 changes: 35 additions & 9 deletions packages/serverless/test/gcpfunction.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as SentryNode from '@sentry/node';
import type { Event } from '@sentry/types';
import * as domain from 'domain';

import * as Sentry from '../src';
Expand All @@ -12,7 +13,6 @@ import type {
Request,
Response,
} from '../src/gcpfunction/general';

/**
* Why @ts-ignore some Sentry.X calls
*
Expand Down Expand Up @@ -198,7 +198,7 @@ describe('GCPFunction', () => {
expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext);
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeScope.setSpan).toBeCalledWith(fakeTransaction);
expect(SentryNode.captureException).toBeCalledWith(error);
expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function));
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeTransaction.finish).toBeCalled();
expect(SentryNode.flush).toBeCalled();
Expand Down Expand Up @@ -317,7 +317,7 @@ describe('GCPFunction', () => {
expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext);
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeScope.setSpan).toBeCalledWith(fakeTransaction);
expect(SentryNode.captureException).toBeCalledWith(error);
expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function));
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeTransaction.finish).toBeCalled();
expect(SentryNode.flush).toBeCalled();
Expand Down Expand Up @@ -382,7 +382,7 @@ describe('GCPFunction', () => {
expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext);
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeScope.setSpan).toBeCalledWith(fakeTransaction);
expect(SentryNode.captureException).toBeCalledWith(error);
expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function));
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeTransaction.finish).toBeCalled();
expect(SentryNode.flush).toBeCalled();
Expand Down Expand Up @@ -440,7 +440,7 @@ describe('GCPFunction', () => {
expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext);
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeScope.setSpan).toBeCalledWith(fakeTransaction);
expect(SentryNode.captureException).toBeCalledWith(error);
expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function));
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeTransaction.finish).toBeCalled();
expect(SentryNode.flush).toBeCalled();
Expand Down Expand Up @@ -469,7 +469,33 @@ describe('GCPFunction', () => {
expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext);
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeScope.setSpan).toBeCalledWith(fakeTransaction);
expect(SentryNode.captureException).toBeCalledWith(error);
expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function));
});
});

test('marks the captured error as unhandled', async () => {
expect.assertions(4);

const error = new Error('wat');
const handler: EventFunctionWithCallback = (_data, _context, _cb) => {
throw error;
};
const wrappedHandler = wrapEventFunction(handler);
await expect(handleEvent(wrappedHandler)).rejects.toThrowError(error);

expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function));

// @ts-ignore just mocking around...
const scopeFunction = SentryNode.captureException.mock.calls[0][1];
const event: Event = { exception: { values: [{}] } };
let evtProcessor: ((e: Event) => Event) | undefined = undefined;
scopeFunction({ addEventProcessor: jest.fn().mockImplementation(proc => (evtProcessor = proc)) });

expect(evtProcessor).toBeInstanceOf(Function);
// @ts-ignore just mocking around...
expect(evtProcessor(event).exception.values[0].mechanism).toEqual({
handled: false,
type: 'generic',
});
});

Expand Down Expand Up @@ -537,7 +563,7 @@ describe('GCPFunction', () => {
expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext);
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeScope.setSpan).toBeCalledWith(fakeTransaction);
expect(SentryNode.captureException).toBeCalledWith(error);
expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function));
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeTransaction.finish).toBeCalled();
expect(SentryNode.flush).toBeCalled();
Expand Down Expand Up @@ -595,7 +621,7 @@ describe('GCPFunction', () => {
expect(SentryNode.fakeHub.startTransaction).toBeCalledWith(fakeTransactionContext);
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeScope.setSpan).toBeCalledWith(fakeTransaction);
expect(SentryNode.captureException).toBeCalledWith(error);
expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function));
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeTransaction.finish).toBeCalled();
expect(SentryNode.flush).toBeCalled();
Expand Down Expand Up @@ -625,7 +651,7 @@ describe('GCPFunction', () => {
// @ts-ignore see "Why @ts-ignore" note
expect(SentryNode.fakeScope.setSpan).toBeCalledWith(fakeTransaction);

expect(SentryNode.captureException).toBeCalledWith(error);
expect(SentryNode.captureException).toBeCalledWith(error, expect.any(Function));
});
});

Expand Down