Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

supported node versions #7

Closed
mayurkale22 opened this issue May 29, 2019 · 11 comments
Closed

supported node versions #7

mayurkale22 opened this issue May 29, 2019 · 11 comments
Milestone

Comments

@mayurkale22
Copy link
Member

The opencensus-node only support node 8 and newer. The opentracing/basictracer-javascript does not specify what are the node versions supported in package.json nor documentation. There are implicit versions supported in the Travis file : https://github.com/opentracing/basictracer-javascript/blob/master/.travis.yml#L3

The current status of Node versions:
image

Need to decide and document supported node versions.

@mayurkale22 mayurkale22 added the Discussion Issue or PR that needs/is extended discussion. label May 29, 2019
@rochdev
Copy link
Member

rochdev commented May 30, 2019

We currently support Node 4 at Datadog. We cannot realistically use OpenTelemetry without at the very least support for it. However, we can easily port our work for these older versions to OpenTelemetry. In general, it's not too difficult to support Node 4 compared to Node 8. The biggest issues are context propagation and native addons, but in our experience it's not too difficult to support it.

There was also a discussion to bump the requirement from 0.10 to 4 about a year ago in opentracing/opentracing-javascript#76 but it was rejected since some of the maintainers were using 0.10/0.12.

cc @yurishkuro

@hekike
Copy link
Member

hekike commented May 31, 2019

The best would be following Node's active releases cycle and supporting the active versions (8, 10 and 12 today). But as I guess OpenTelemetry will be in TypeScript too it will kind of define the minimum version, which is 4.x today: https://github.com/microsoft/TypeScript/blob/master/package.json#L29
Although looks like their CI runs only on active Node releases, which I found weird, so maybe I'm missing something: https://github.com/microsoft/TypeScript/blob/master/.travis.yml

In general, using higher Node version would allow us to configure TypeScript to use more built-in ES features instead of shims which would lead to better performance and lower instrumentation overhead.

You can find some info about Node version usage in this survey, look like active would cover 98%, but I'm pretty sure DataDog have more accurate data from their currently instrumented applications.
https://nodejs.org/en/user-survey-report/#version

@vmarchaud
Copy link
Member

Maybe could we make a specific build of open-telemetry to support really old version (0.10/12, 4, 6) which would not follow the master branch ? It would avoid adding shims on newer versions.

@rochdev
Copy link
Member

rochdev commented May 31, 2019

The best would be following Node's active releases cycle and supporting the active versions (8, 10 and 12 today).

This approach cannot work for APM vendors since we don't control which version of Node our customers use. Every time the minimum requirement is bumped, it should be done carefully and we have to discuss the impacts.

But as I guess OpenTelemetry will be in TypeScript too it will kind of define the minimum version, which is 4.x today

This is one of the many reasons I'm not a fan of using TypeScript as the main language for this type of project.

In general, using higher Node version would allow us to configure TypeScript to use more built-in ES features instead of shims which would lead to better performance and lower instrumentation overhead.

This is not necessarily true as you can always have different code paths depending on the version of Node. It would definitely result in a slightly larger payload for browsers, but usually you need the shims in browsers anyway. Not using TypeScript would give us total control over this and make it really easy to have different code paths. The solution proposed above would also work but it may make testing more difficult.

You can find some info about Node version usage in this survey, look like active would cover 98%

Where does the 98% number come from? I can't find it in the article.

I'm pretty sure DataDog have more accurate data from their currently instrumented applications

I'll try to see if we can share these numbers. Will update next week.

@hekike
Copy link
Member

hekike commented May 31, 2019

I think everyone understands that a holistic instrumentation alignment like OpenTelemetry is not possible without having the main APM players on board. I'd recommend to take a more data-driven approach here and make a final decision based on it. Please collect the data and make it available.

The 98% comes from the 55% LTS + 43% current, but as I said, data from actual instrumentation would be probably more accurate data. People who fill a form are not necessarily the audience who run DataDog.

@danielkhan
Copy link
Contributor

danielkhan commented Jun 6, 2019

This is our (Dynatrace's) release policy: https://www.dynatrace.com/support/help/shortlink/nodejs#support--desupport

It is based on real usage stats and it works well.

@vmarchaud
Copy link
Member

@danielkhan Cool ! So following your document we can target node 8 since the plan is to release a first version in september (which will not be e-o-l for node 6 but close enough)

@danielkhan
Copy link
Contributor

@vmarchaud - yes supporting 8-12 (maybe 13) in September looks like a safe bet.

@mayurkale22
Copy link
Member Author

Just to have more data points, @bg451 (lightstep) promised to share stats about active node versions. @bg451 Please update the thread once you get time. Thanks

@bg451
Copy link
Member

bg451 commented Jun 6, 2019

I looked at our metrics of reporting lightstep tracers and broke it down by node version with these results.
v8.x - 53%
v10.x - 39%
v11.x - 5%
v12.x - 3%

No tracers are running on anything below v8.x

@mayurkale22
Copy link
Member Author

We have decided (in today's meeting) to start with Node 8 and newer versions and switch back to Node 6 if needed.

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

No branches or pull requests

6 participants