Support http/https APIs for node >= 10

[kennethaasan]

Related issue: sindresorhus/got#876

Seems like https://github.com/newrelic/node-newrelic/blob/master/lib/instrumentation/core/http.js#L447-L451 breaks requests.

Also seems like the dependency node-agent-base that you have through https-proxy-agent doesn't support http/https APIs for node >= 10: TooTallNate/node-agent-base#29

[astormnewrelic]

@kennethaasan Nice to meet you, thanks for the bug report.

First, some official business: The best way to get support for the Node Agent is via our official support channels: https://support.newrelic.com/ -- reporting things via the official channels will ensure your issue gets in front the folks best situated to give you immediate help and make sure the issues routed properly through our org.

With that out of the way -- do you have some sample code/agent-configuration that shows how things break? I took a look at the links you provided and there's enough extra parties involved that I'm not 100% clear what the problem you're describing is. If you could provide a small program that demonstrates how things breaks we'll be in a much better position to help out.

[kennethaasan]

@astormnewrelic Thanks, I made a support ticket now.

I made a PR to fix this in #317

[astormnewrelic]

@kennethaasan

do you have some sample code/agent-configuration that shows how things break?

Would you be able to provide the above so we can better understanding the issue you're facing?

[kennethaasan]

@astormnewrelic,

The error message I get is TypeError [ERR_INVALID_ARG_TYPE]: The "listener" argument must be of type Function. Received type object.

The NR agent config I use is:

process.env.NEW_RELIC_NO_CONFIG_FILE = 'true';
process.env.NEW_RELIC_DISTRIBUTED_TRACING_ENABLED = 'true';
process.env.NEW_RELIC_LOG_ENABLED = 'false';
process.env.NEW_RELIC_ERROR_COLLECTOR_IGNORE_ERROR_CODES = '400';
import 'newrelic';

The error happens when I use got v10 (https://github.com/sindresorhus/got) to request JSON feeds:

const agent = new https.Agent({
  keepAlive: true,
  maxSockets: 50,
  maxFreeSockets: 50,
})

export async function gotGet<V extends object>(
  url: string
): Promise<Response<V>> {
  const newrelicHeader = newrelic
    .getTransaction()
    .createDistributedTracePayload()
    .httpSafe();

  return got.get<V>(url, {
    agent,
    headers: {
      newrelic: newrelicHeader,
    },
    timeout: 10000,
    followRedirect: true,
    retry: 3,
    responseType: 'json',
  });
}

The changed I made in #317 fixes this, but some tests are failing (feel free to take over from where I left it). Interesting that some of the tests are failing with the exact same error message as I have from this issue. Probably because node-agent-base is not overriding the default node API anymore.

[kennethaasan]

I made a sample app to show the problem: https://github.com/kennethaasan/node-newrelic-bug
It happens using both got and https.request.

[michaelgoin]

Hi @kennethaasan,

Thank you for all of your hard work making the issue clear.

We definitely have an issue with the http.request(url, options, callback) signature. I also think we may introduce some issues in the older signature when options is a proper URL object, but I haven't manually confirmed that.

I've created an internal bug to track this on our end more-closely. For our reference: NODE-2242.

Looking through things, it seems there are several permutations we want to ensure we get right with both signatures. The public PR is definitely on the right track. If you want to keep iterating on your PR, we are happy to help provide guidance and review. If not, totally understandable. It may be involved. Either way, you've set us up for success.

I can't guarantee timing of when we'll get a fix out. Given it crashes, it will be higher priority bug.

Thank you,

Michael

[kennethaasan]

Hi @michaelgoin ,

Thanks for the follow-up!

I'd love to finish my PR, but I'll have to dig into the whole codebase to understand why some of the tests break. Feel free to take over from where I left. When I have time to finish it, I'll give you an update here.

[kennethaasan]

I have some time to look at it today.

Looks like nock needs an update to v11 make tests pass: https://github.com/nock/nock/releases/tag/v11.3.2

Add support for the http.request signatures added in Node 10.9

[kennethaasan]

@michaelgoin, this turns out to be a complex issue. Nock, a lib you use for testing is also breaking the Node API: nock/nock#1588
http.get calls http.request, which is causing the tests to fail because wrapRequest is then called twice on 1 request. They claim they have fixed it, but their fix is not working for this library because of that reason. A temporary solution might be to not add the shim on http.get when running tests until Nock has fixed this.

Also looks like https-proxy-agent has released a new version that doesn't overwrite the Node API anymore, which is good!

Can the NR team take over this and prioritize it? This is blocking upgrades of open source software that are using this Node API.

EDIT: I think I'm close to solving it now with the comment below.

[kennethaasan]

nock/nock#1837 will fix the unit tests in #318

[pcwiek]

I'm just chiming in for 'visibility' - we're also waiting for a new release of the agent with this fix in place.

[kennethaasan]

@pcwiek, #318 is "ready" to be merged IMO, but the unit tests are breaking because of the nock issue above.

[nrcmkoh]

Any update on this?

[michaelgoin]

Hi @nrcmkoh,

We are hoping to put dedicated focus on getting this resolved/wrapped-up right after we finish our current effort. That probably puts it a few releases out but likely not very far out time-wise. We might get to put some focus on it prior to then, but that is what I can more confidently communicate. We'll update the issue and PR as that progresses.

Thank you,

Michael

[kennethaasan]

nock/nock#1837 is ready to be revied and merged. Hopefully, that happens soon so that I can continue with #318

[jrnelson90]

I am currently also seeing an issue using got 10.3.0 with newrelic 6.3.0 due to the dependency on [email protected], which causes a dependency on [email protected]. All got versions 10+ need agent-base 5+. This should be resolved once newrelic moves to [email protected]

I have to choose between having metrics or just being able to make HTTP requests at the moment.

See corresponding got issue 1039:
sindresorhus/got#1039

[jrnelson90]

I have also been able to locally change the package.json of newrelic within node-modules to depend on [email protected], and I was able to run my mocha tests that depend on got 10.3.0 and newrelic 6.3.0.

However, looking at #318, I can see that it will take a bit more refactoring for this to be fully compatible. I'm going to attempt to merge the updates those changes into 6.3.0 locally and see how they run for me.

[mike-dazn]

We're also seeing issues with got 10.3.0 and the newrelic agent (both directly, and via the newrelic lambda layer). I've raised a support ticket on our side with newrelic referencing this issue to hopefully help get it moving.

[carlo-808]

Thank you for putting the support ticket @mike-dazn. We are still in the middle of our current effort in updating the agent. Hopefully we can get to this soon.

[paulmelnikow]

@kennethaasan, @michaelgoin, or others, the ecosystem impact has just come to my attention. I'd like to get this patched in Nock in the next day or so.

I need a bit of help to understand clearly what is the undesirable behavior in Nock w.r.t. New Relic's overriding of http.request. See my question at nock/nock#1836 (comment). Thanks!

[kennethaasan]

@michaelgoin,

My PR is ready for review: #318

The nock issue is fixed. All the tests except the versioned tests pass. But they seem to break on master as well, so not sure if I should care?

[michaelgoin]

Thanks @kennethaasan, we plan to take a look at this soon. Not sure what is up with the public versioned tests. With our current setup, we have to pull things down to our private repo before shipping so we'll ensure the tests are in a good spot.

[maximilian-krauss]

Hi,
how is the progress on new relic side regarding this issue? We are blocked since the very beginning of this issue here and can't use the node integration in most of our services unfortunately 😐

Do you have an estimation when we can see this fix going into the official release?

Thanks in advance, cheers,
Max

[astormnewrelic]

Thanks for checking in @maximilian-krauss -- Last week we released version 6.4.2 of the node agent. This release incorporates this PR, as well as expanding it to cover most (hopefully all?) possible combinations of arguments for the http functions. (to preempt the obvious question -- we don't have a good public github/internal release pipeline setup yet so this PR remains unmerged -- we're are working on it)

Could you give this latest release a try and let us know if it solves your problems? If not, could you share an isolated example of an HTTP request (or other code) that breaks things? We've added significant test coverage to ensure we test every message signature, but it's possible we've missed one. An isolated reproduction case is the most actionable piece of feedback we can get.

[kennethaasan]

Thanks! I just deployed the new version of NR together with upgrading got. So far I don't see any issues, so for me we can close this ticket.

[nijotz]

Excellent! Sounds great, thank you so much for your help with this issue