-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix(node): Ensure that self._handler
exists before calling it in LinkedErrors
#5497
Conversation
size-limit report 📦
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the linked Sentry error, it seems the event has no breadcrumbs. This leads me to believe that this error is happening right at the start so for some reason, very mysterious.
@@ -47,7 +47,7 @@ export class LinkedErrors implements Integration { | |||
const hub = getCurrentHub(); | |||
const self = hub.getIntegration(LinkedErrors); | |||
const client = hub.getClient<NodeClient>(); | |||
if (client && self) { | |||
if (client && self && self._handler && typeof self._handler === 'function') { | |||
await self._handler(client.getOptions().stackParser, event, hint); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand how this happens either but this should fix it for now. Will try to investigate this more on Monday. Since we're cutting a patch on Monday, I'll leave this open till then
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I had to guess it's due to multiple @sentry/node
s being installed - maybe through transitive deps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that a version conflict might cause this. Apart from that I don't see how this method would not exist while the class instance obviously does exist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merging this for now as I think the check doesn't hurt us
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked out package-lock.json when we had 7.8.0 installed and can't see any references to other versions of sentry/node in there. Happy to share the full file if you think it would be helpful.
@@ -47,7 +47,7 @@ export class LinkedErrors implements Integration { | |||
const hub = getCurrentHub(); | |||
const self = hub.getIntegration(LinkedErrors); | |||
const client = hub.getClient<NodeClient>(); | |||
if (client && self) { | |||
if (client && self && self._handler && typeof self._handler === 'function') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Lms24 @AbhiPrasad this should have included a check that event
was truthy and an object and if not, returned, since calling _handler
with an invalid event object does the same. see: #5622 (comment)
The package should also warn to console when something. like this happens, or at least output to a debug channel that can be enabled.
If the package had some better internal error handling and better debug logging, I could have narrowed this down further. Alas.
This patch brings back a check for
self._handler
in the global event processor of the LinkedErrors integration for Node.The check was removed in #4902 but apparently it is still necessary.
fixes #5493