JupiterOne
Multiple resolves from node-fetch logged as error
The Qualys integration uses the node-fetch library to make API requests. node-fetch resolves the same promise multiple times in the case of a timeout. This does not cause any problems in the integration itself, but a multipleResolves event is published and logs an error.
Interesting. According to the implementation, it should only invoke the callback (publish / log an error) if type === 'reject', not if type === 'resolve'. Maybe I messed up the implementation?
https://github.com/JupiterOne/sdk/blob/master/packages/integration-sdk-runtime/src/logger/registerEventHandlers.ts#L21
@ndowmon node-fetch rejects the promise twice. My use of the word "resolve" was "a promise callback was called"; I didn't mean to imply which callback it was. I should have made that clear, my bad.
Also, I am working on identifying a fix in node-fetch itself since it should not be rejecting the error twice. Maybe the change we should make here is log warn instead of error for multipleResolves? I'm sure @aiwilliams has thoughts on that idea.
I didn't myself study the multipleResolves Node docs, but if this is indeed a bad thing to do, we should certainly not log as a level: 30, but level: 50 is ideally reserved for things we should be looking to fix ASAP (who doesn't love level = 50 in CloudWatch as a quick health test?), and which end up in Sentry. Did this end up in Sentry? This is not worthy of Sentry (doesn't actually break anything), and in fact is probably not a concern at all really (though it would be nice to provide a patch to node-fetch). It certainly led us to believe we had to spend half a day figuring out what was broken in the integration.
Summary: can we safely make all of these log at level: 40 so we know it's happening, don't have to fix immediately, and don't get bothered in Sentry?
My only question is - would making that change reduce all multiple resolves to level: 40, or could this change be scoped to target only the node-fetch known issue? I'd fear making a change that affects all multipleResolves events and stops sending others to sentry.
It sounds like multipleResolves was an error in the context where this feature was developed, so we should keep it as level: 50 and get the broken libraries patched ASAP, and perhaps have the ability to register a filter to the logger so the skipping is in the integration that uses node-fetch until the patched version is in place.
OK then, I think the work for this issue should be to allow callers to specify error types to ignore (E.G., "FetchError"). Do y'all concur?
@fomentia that sounds reasonable to me. I guess it will use a string to compare to the error code or something like that? I'd be cautious of any instanceof checking since, well, you know, NPM modules allow for different instances of what appears to be the same constant.
Reading the docs for Node process.on('multipleResolves'):
This is useful for tracking potential errors in an application while using the constructor. Otherwise such mistakes are silently swallowed due to being in a
Promiseconstructor, as multiple resolutions are silently swallowed. However, the occurrence of this event does not necessarily indicate an error. For example, [Promise.race()][] can trigger a'multipleResolves'event.
Particularly:
the occurrence of this event does not necessarily indicate an error
This suggests to me that we really don't need to submit a PR to node-fetch; they're not doing anything wrong. We just don't like the logging that the SDK assumes it should do for all cases of this event. I'm not seeing a clear way to intercept this in the graph-qualys code to prevent it from bubbling to other registered listeners.
Chatting with @ndowmon IRL, the best approach:
- Code the solution in the graph-qualys project so that we don't modify the SDK, nor do we only have this covered when running in our managed environment
- Consider instrumenting the
logger.error/logger.onFailurefunction(s) as appropriate when execution starts to intercept thisnode-fetchbehavior so that it isn't logged
Put some effort into understanding multipleResolves: https://jupiterone.atlassian.net/browse/INT-1813?focusedCommentId=11574