fix(node): Sets mechanism handled to false when it is a crash #3493
Conversation
ahmedetefy
changed the title
Set mechanism handled to false when error is triggered by an onuncaughtexception or onunhandledrejection. Also, add the appropriate exception type to mechanism.
ahmedetefy
force-pushed
the
ahmed-set-handled-false-when-crash
branch
from
33d0915 to
2194ac7
Compare
ahmedetefy
requested review from
HazAT and
rhcarvalho
and removed request for
kamilogorek
size-limit report
|
rhcarvalho
left a comment
rhcarvalho
left a comment
There was a problem hiding this comment.
A bit late to the party... but sharing my feedback anyway.
This is changing a fundamental aspect of the SDK, we should have a better description here and then a suitable change log entry.
It seems that it was either a long standing bug that the mechanism was "mechanism": {"handled": true, "type": "generic"}, or intentional behavior that was not clearly specified (that is, no test broke when we made this change).
There's no explanation whatsoever if that was an incorrect behavior / oversight or if we "just changed our mind". We'd do ourselves a favor to document why it is changing here and let users know in the change log in case their code depends on it / it will change what they see in Sentry / it will affect the session counts for Browser (?).
(All we have at the moment is a brief discussion on a call today which is not captured anywhere and nobody will remember in a matter of days)
| hub.captureException(error, { originalException: error }); | ||
| hub.captureException(error, { | ||
| originalException: error, | ||
| data: { mechanism: { handled: false, type: 'onuncaughtexception' } }, |
There was a problem hiding this comment.
"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.
| import { OnUncaughtException } from '../src/integrations/onuncaughtexception'; | ||
|
|
||
| jest.mock('@sentry/hub', () => { | ||
| // we just want to short-circuit it, so dont worry about types |
There was a problem hiding this comment.
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.
| test('install global listener', () => { | ||
| const integration = new OnUncaughtException(); | ||
| integration.setupOnce(); | ||
| expect(process.listeners('uncaughtException')).toHaveLength(1); |
There was a problem hiding this comment.
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);| expect(process.listeners('uncaughtException')).toHaveLength(1); | ||
| }); | ||
|
|
||
| test('sendUncaughtException', () => { |
There was a problem hiding this comment.
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/
This PR:
onuncaughtexceptionoronunhandledrejection.Also, add the appropriate exception type to mechanism whether it is
onuncaughtexceptionoronunhandledrejection.We decided that this behavior is the correct one we would expect in node.
So if an error bubbles up to the global error handlers for promise or error, we set the mechanism
handledtofalse.Why we didn't do this from the beginning is hard to tell but this is how it works in browser and other SDKs.