Skip to content

fix(node): Sets mechanism handled to false when it is a crash #3493

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
ahmedetefy merged 1 commit into from
May 3, 2021
Merged

fix(node): Sets mechanism handled to false when it is a crash #3493

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 3 additions & 1 deletion packages/node/src/backend.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@ export class NodeBackend extends BaseBackend<NodeOptions> {
public eventFromException(exception: any, hint?: EventHint): PromiseLike<Event> {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
let ex: any = exception;
const mechanism: Mechanism = {
const providedMechanism: Mechanism | undefined =
hint && hint.data && (hint.data as { mechanism: Mechanism }).mechanism;
const mechanism: Mechanism = providedMechanism || {
handled: true,
type: 'generic',
};
Expand Down
5 changes: 4 additions & 1 deletion packages/node/src/integrations/onuncaughtexception.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,10 @@ export class OnUncaughtException implements Integration {
if (hub.getIntegration(OnUncaughtException)) {
hub.withScope((scope: Scope) => {
scope.setLevel(Severity.Fatal);
hub.captureException(error, { originalException: error });
hub.captureException(error, {
originalException: error,
data: { mechanism: { handled: false, type: 'onuncaughtexception' } },
Copy link
Contributor

@rhcarvalho rhcarvalho May 3, 2021

Choose a reason for hiding this comment

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

"on" is the JS way of saying it is registering an event handler... shouldn't this be simply "uncaught exception"?

https://develop.sentry.dev/sdk/event-payloads/exception/#exception-mechanism says "promise" as an example... that might be too short. A fix there might be in order too.

image

});
if (!calledFatalError) {
calledFatalError = true;
onFatalError(error);
Expand Down
5 changes: 4 additions & 1 deletion packages/node/src/integrations/onunhandledrejection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,10 @@ export class OnUnhandledRejection implements Integration {
scope.setExtras(context.extra);
}

hub.captureException(reason, { originalException: promise });
hub.captureException(reason, {
originalException: promise,
data: { mechanism: { handled: false, type: 'onunhandledrejection' } },
});
});
/* eslint-disable @typescript-eslint/no-unsafe-member-access */

Expand Down
34 changes: 34 additions & 0 deletions packages/node/test/onuncaughtexception.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
import { Hub } from '@sentry/hub';

import { OnUncaughtException } from '../src/integrations/onuncaughtexception';

jest.mock('@sentry/hub', () => {
// we just want to short-circuit it, so dont worry about types
Copy link
Contributor

@rhcarvalho rhcarvalho May 3, 2021

Choose a reason for hiding this comment

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

This is copy-pasted from somewhere else. This would be more useful if it explained why we do what we do and how it is relevant for the tests.

const original = jest.requireActual('@sentry/hub');
original.Hub.prototype.getIntegration = () => true;
return {
...original,
getCurrentHub: () => new Hub(),
};
});

describe('uncaught exceptions', () => {
test('install global listener', () => {
const integration = new OnUncaughtException();
integration.setupOnce();
expect(process.listeners('uncaughtException')).toHaveLength(1);
Copy link
Contributor

@rhcarvalho rhcarvalho May 3, 2021

Choose a reason for hiding this comment

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

We miss here comparing the steady state. If there was already a listener registered by any other means than calling setupOnce, the test would incorrectly pass.

If we want to test that the integration sets up an event listener, then we should compare the before-after.
Something along the lines of:

const before = process.listeners('uncaughtException').length;
...
integration.setupOnce();
const after = process.listeners('uncaughtException').length;
expect(after - before).toBe(1);

});

test('sendUncaughtException', () => {
Copy link
Contributor

@rhcarvalho rhcarvalho May 3, 2021

Choose a reason for hiding this comment

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

We could document better what's the intent of this test. One can see what it does, not why it does, what behavior is important and why is it important.

I'm guessing the intention is to check that integration.handler captures an exception with the appropriate mechanism type and handled bool.

Inspiration: https://hynek.me/articles/document-your-tests/

const integration = new OnUncaughtException({ onFatalError: jest.fn() });
integration.setupOnce();

const captureException = jest.spyOn(Hub.prototype, 'captureException');

integration.handler({ message: 'message', name: 'name' });

expect(captureException.mock.calls[0][1]?.data).toEqual({
mechanism: { handled: false, type: 'onuncaughtexception' },
});
});
});
3 changes: 3 additions & 0 deletions packages/node/test/onunhandledrejection.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@ describe('unhandled promises', () => {

integration.sendUnhandledPromise('bla', promise);

expect(captureException.mock.calls[0][1]?.data).toEqual({
mechanism: { handled: false, type: 'onunhandledrejection' },
});
expect(captureException.mock.calls[0][0]).toBe('bla');
expect(setUser.mock.calls[0][0]).toEqual({ id: 1 });
expect(setExtra.mock.calls[0]).toEqual(['unhandledPromiseRejection', true]);
Expand Down