Add instrumentation for Q

[mdlavin]

My project uses Q for promises and I've noticed that when Q and newrelic are used together, the recorded segments are sometimes lost or placed on the wrong transaction.

It turns out that Q has it's own internal queue and depending on the ordering of calls to Q APIs, the transactions can get lost or the wrong one picked up.

[lykkin]

thanks, @mdlavin! i'll roll this into the next release.

[lykkin]

@mdlavin i seem to have missed the second commit on this branch, i'll roll that into the next release. sorry for the mix up!

[mdlavin]

The second commit is pretty important. Q will throw exceptions without it
if a certain error flow happens. I'd highly recommended putting a version
out with it to avoid bugs.

By the way, thanks for the quick merging.
On Feb 17, 2016 5:24 PM, "Bryan Clement" [email protected] wrote:

@mdlavin https://github.com/mdlavin i seem to have missed the second
commit on this branch, i'll roll that into the next release. sorry for the
mix up!

—
Reply to this email directly or view it on GitHub
#206 (comment)
.

[lykkin]

yeah, i'm planning on putting out a release out next week with a few bug fixes - this being one of them. should be early in the week, maybe monday or tuesday.

[mdlavin]

OK. If there are users that are using Q today, they will likely open an
issue for the missing change. Maybe next week is soon enough to avoid too
many reports. I'm not sure how fast people adopt new versions
On Feb 17, 2016 6:08 PM, "Bryan Clement" [email protected] wrote:

yeah, i'm planning on putting out a release out next week with a few bug
fixes - this being one of them. should be early in the week, maybe monday
or tuesday.

—
Reply to this email directly or view it on GitHub
#206 (comment)
.

[lykkin]

alright, this has been merged out of band. thanks for being patience!

[benjamingr]

Hey, @lykkin @martinkuba your promise instrumentation in node-newrelic is really weird. You override native promise methods. We want to use newrelic but can't because of how it instruments promises.

Wouldn't it be better to add a "unhandledRejection" handler to node-newrelic globally for all promise implementations? I can help you with that if I know a pull request would be entertained.

I'm a bluebird and Q contributor, I wrote the unhandledRejection hook code for Q and the specification for native promises in Node and would love to help you get this instrumentation right since I want to use it for our projects at TipRanks.

[benjamingr]

You can also contact me at [email protected] and I would love to chat about this.

[hayes]

@benjamingr could you elaborate on what it is about the way New Relic instruments promises that prevents you from using it? as for wrapping unhandledRejection, it probably is something that should be supported, but does not replace the need for implementation specific instrumentation. The instrumentation is not to report errors, it is designed to propagate transactions across async boundaries.

it is fairly trivial to report errors for unhandled rejections:

process.on('unhandledRejection', function(reason, p) {
    newrelic.noticeError(new Error('UnhandledRejection: ' + reason))
});

[martinkuba]

@benjamingr Thanks for reaching out. This is good timing, as we are looking at Bluebird as well. Our main goal at this point is to make sure that state is properly preserved when using promises, so that different transactions don't get mixed up. We thought this PR accomplished that, but we are definitely open to suggestions. PRs are always welcome! I would be happy to collaborate on this.

[benjamingr]

The instrumentation is not to report errors, it is designed to propagate transactions across async boundaries.

I'm confused, wouldn't it make more sense to use domains or the newer AsyncWrap API for that?

it is fairly trivial to report errors for unhandled rejections:

I think for the very least the default should be to noticeError on unhandledRejection.

As for async boundaries - I'm not sure how you think that'd work in a reasonable way with regards to performance - it would likely cause a performance slowdown (like capturing async stack traces does).

Bluebird has a monitoring API and you can use that + a WeakMap to track async boundaries and following of promises.

[benjamingr]

Or, if you just want to run code whenever bluebird schedules stuff - you can use .setScheduler which supports this use case directly.

Note that bluebird may omit async scheduling when it deems that actions have already happened asynchronously anyway (for example, running then callbacks after a readFile will execute them synchronously and avoid the extra scheduling).

[benjamingr]

Also, welcome to the (Node) foundation :)