Regression in 6.4.2 (with nock-vcr-recorder + node 13.8.0)

[papandreou]

After upgrading to newrelic 6.4.2 I have a test that fails with:

the boolean true was thrown, throw an Error :)
Error: the boolean true was thrown, throw an Error :)
  at Runner.fail (node_modules/mocha-clean/index.js:37:14)
  at tryCatch (node_modules/rsvp/dist/rsvp.js:525:12)
  at invokeCallback (node_modules/rsvp/dist/rsvp.js:538:13)
  at publish (node_modules/rsvp/dist/rsvp.js:508:7)
  at publishRejection (node_modules/rsvp/dist/rsvp.js:443:3)
  at flush (node_modules/rsvp/dist/rsvp.js:2415:5)
  at processTicksAndRejections (internal/process/task_queues.js:79:11)

I used git bisect and narrowed it down to https://github.com/newrelic/node-newrelic/commit/245b8e95532b10092c8b815e58cc361e682e3817:

245b8e95532b10092c8b815e58cc361e682e3817 is the first bad commit
commit 245b8e95532b10092c8b815e58cc361e682e3817
Author: Kenneth Aasan <k.aasan@sportradar.com>
Date:   Wed Dec 18 12:58:47 2019 -0800

    316: Adds support for http/https APIs for node >= 10

 lib/instrumentation/core/http-outbound.js       |  18 ++-
 lib/instrumentation/core/http.js                |  54 +++++----
 package-lock.json                               | 148 +++++-------------------
 package.json                                    |   4 +-
 test/unit/instrumentation/http/outbound.test.js | 116 +++++++++++++++++--
 5 files changed, 184 insertions(+), 156 deletions(-)

This particular test (out of a suite of 10000) happens to be using nock-vcr-recorder to mock out a certificate from sns-validator. Could it be that this new instrumentation isn't hardened against other tools also wrapping the http/https modules?

It's a bit involved to make a standalone reproduction given all the moving parts, but it seems like it's the wrapped https.request parameter resolution that concludes that options is the url (and _url comes out as undefined) here:

I know that you don't support odd-numbered officially supported, but I thought you'd still like to know :)

[papandreou]

Oh, it looks like instrumentOutbound is supposed to compensate for that, so it doesn't help if I arrange for _url to come out right. Anyway, the symptom seems to be that my code gets a 404 response from the actual service rather than the mocked out response. Might make sense to make a standalone test with just nock-vcr-recorder and newrelic. Unfortunately I don't have time to do that myself, so I hope this helps you locate it.

[astormnewrelic]

Great to meet you @papandreou, sorry to hear you ran into an issue with nock-vcr-recorder.

As you figured out, the PR in question was specifically to handle the multitude of argument arrangements available in modern versions of node. It's possible we missed one, and it's also possible that nock-vcr-recorder is doing something that's not supported in Node 8+.

In order to help us reproduce this, could you

1. Let us know what version(s) of node you're seeing this with?
2. Provide us with a stand-alone reproduction of the issue?

Once we have a reproducible test case we can usually get to the bottom of this sort of thing pretty easily and either fix it, or get you a workaround.

Also -- if you're looking for a formal support from New Relic, you'll need to submit this request via our official support channels. I know -- it's a bit of bureaucratese, but getting a request in via support ensures the right folks see the request, and they can often get you a response faster than we can. GitHub issues is currently a loophole that no one's noticed yet, but we can't always check in here as promptly as this we did for this specific request.

[papandreou]

I dug some more into this and tried to make a small repro:

require('newrelic');
const nock = require('nock');

nock('https://thisdomaindefinitelydoesnotexist.com')
  .get('/hello')
  .reply(418, { delicious: '🍵' });

const https = require('https');

https
  .get('https://thisdomaindefinitelydoesnotexist.com/hello', async response => {
    console.log('Got', response.statusCode);
  })
  .on('error', err => console.error(err.message));

To my great surprise, the above actually works, but stops working when I pull out require('newrelic'), which was kind of the opposite of what I expected to see. It turns out that nock-vcr-recorder is using a very old version of nock that doesn't support node > 8. However, having newrelic 6.4.1 or below somehow makes it work anyway! As far as I can tell, this unintended side effect then got removed in 6.4.2. I haven't tried to figure out exactly what changed on that front, but on paper that seems more likely to be an improvement than anything else.

I'll try to get nock-vcr-recorder to upgrade instead: https://github.com/poetic/nock-vcr-recorder/pull/14

Sorry for the noise! 😅

[astormnewrelic]

@papandreou the joys of working in a language where everyone decides it's ok for them to monkey patch a function but not anyone else :)

Glad you have some next steps and thanks again for bringing this to our attention. Even if the problem's downstream it's always good to hear about these things. Good luck!