Skip to content
This repository has been archived by the owner on Sep 18, 2023. It is now read-only.

Http module patches http.reqeust in a way thats incompatable with NodeJs > v10 #78

Closed
romansavchenko opened this issue Jan 5, 2021 · 18 comments
Assignees

Comments

@romansavchenko
Copy link

romansavchenko commented Jan 5, 2021

It overwrites http.request and https.request with only two arguments: options and callback, while node api > 10 provides url, options and callback.

This breaks common modern implementations like got sindresorhus/got#876 (comment)

@romansavchenko romansavchenko changed the title Bug: Http module patches http.reqeust in a way thats incompatable with NodeJs > v10 Http module patches http.reqeust in a way thats incompatable with NodeJs > v10 Jan 5, 2021
@jtmalinowski jtmalinowski self-assigned this Feb 18, 2021
@jtmalinowski
Copy link

Hi @romansavchenko, I've created an internal ticket for this. Do you mind emailing me (my email is in my profile) so I know if you're a customer?

@jtmalinowski
Copy link

jtmalinowski commented Feb 19, 2021

I think we'll need to rephrase it as support for NodeJS v12, which is the next LTS after v10. While possibly http.request is the only issue to be fixed, we need to be clear about which versions are supported.
Edit: @romansavchenko does this description seem accurate to you?

@rkburnett
Copy link

rkburnett commented Jul 1, 2021

Hi @jtmalinowski
Is there any update on this issue? I am experiencing the same issue on NodeJS v12

@jtmalinowski
Copy link

Hi @rkburnett, https://github.com/signalfx/splunk-otel-js is currently in beta, we provide support for it, and once it's 1.0, which should be soon, it'll replace signalfx-nodejs-tracing. I think you'll be more satisfied with it, because it's basically a small configuration layer on top of https://github.com/open-telemetry/opentelemetry-js. However, if you can't / don't want to switch, let me know.

@andrew-prior
Copy link

andrew-prior commented Jul 2, 2021

@jtmal-signalfx Thanks for responding. The migration link Migrate from the SignalFx Tracing Library for JS does not work.

@jtmalinowski
Copy link

@andrew-prior I'm working on it, I'll let you know when it's fixed.

@rkburnett
Copy link

@jtmalinowski Is it still a possibility to get support for Node 12 for signalfx-tracing to fix this issue? How long before that could be available? Thanks again for your help.

@jtmalinowski
Copy link

@rkburnett Of course, we still provide support for this library, but to start that, I'd need to open a support ticket for you (in which case please let me know which company you're at, the email address is in my profile). The problem with providing an ETA is that I can't just upgrade the http instrumentation to node 12+, it has to be done and tested well for all supported instrumentations - obviously I (or one of my colleagues) will do that if we open the ticket.

@rkburnett
Copy link

Is there any update on fixing the migration guide @jtmalinowski ?
Migrate from the SignalFx Tracing Library for JS

Thanks.

@jtmalinowski
Copy link

It's going through proof-reading and testing now, we should merge the fix tomorrow or the day after.

@rkburnett
Copy link

Thanks @jtmalinowski !

@jtmalinowski
Copy link

The PR is ready to merge but I need to sync it with a release, that will happen tomorrow afternoon CET. If you'd like a sneak peak then it's here signalfx/splunk-otel-js#169.

In addition to that, I'll be estimating the effort to support Node.js 12 some time next week.

@andrew-prior
Copy link

Thanks. I have read though the MR. I appreciate that you point out the "limitations" even though your colleague was a little wary of it. It helped me know what it is going to take to get feature parity with signalfx-nodejs-tracing library.

@jtmalinowski
Copy link

Guide is up now https://github.com/signalfx/splunk-otel-js/blob/main/MIGRATING.md. I'll be looking into support for Node.js 12 soon.

@andrew-prior
Copy link

Any estimation on what it will take to patch signalfx-nodejs-tracing to not clobber the request object.

@jtmalinowski
Copy link

@andrew-prior @rkburnett This is now in the backlog. I don't have an exact date yet, but I'd expect this to be done sometime within the next 2 weeks.

@sanknmFinicity
Copy link

@jtmalinowski Any updates on above ?

@jtmalinowski
Copy link

@sanknmFinicity right, I should have updated the issue. It's quite difficult to resolve this properly, because 1) we want to be clear about which versions are supported, and fixing this would mean committing to supporting Node.js v12, and that triggers a cascade of many necessary fixes 2) all Node.js development efforts now go towards https://github.com/signalfx/splunk-otel-js which supports 8.5 - latest LTS.

If switching to @splunk/otel doesn't work for you, email me or our tech support, we'll be resolving this on case-by-case basis.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants