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

Minimum NodeJS version is 8.5.0 instead 8.0.0 #711

Closed
Flarna opened this issue Jan 20, 2020 · 17 comments · Fixed by #1003
Closed

Minimum NodeJS version is 8.5.0 instead 8.0.0 #711

Flarna opened this issue Jan 20, 2020 · 17 comments · Fixed by #1003
Assignees
Labels
bug Something isn't working Discussion Issue or PR that needs/is extended discussion.

Comments

@Flarna
Copy link
Member

Flarna commented Jan 20, 2020

What version of OpenTelemetry are you using?

0.3.2

What version of Node are you using?

<8.5.0

What did you do?

create a Span (e.g. via example basic-tracer-node)

What did you expect to see?

it should work

What did you see instead?

crash because of Error: Cannot find module 'perf_hooks'

Additional context

perf_hooks module was added in 8.5.0 - see https://nodejs.org/docs/latest-v13.x/api/perf_hooks.html

Maybe minimum NodeJS version should be bumped to 8.5.0 - or even 10.0.0.
I don't expect it's really worth to write a polyfill as Node.JS 8 has reached EOL.

@Flarna Flarna added the bug Something isn't working label Jan 20, 2020
@vmarchaud
Copy link
Member

Do we bump to support only 10.0.0 directly or update to 8.5.0 ?

@OlivierAlbertini
Copy link
Member

is there some statistics about what is running currently ? You can still push serverless functions to Google cloud with node 8 and node 10 is in beta ^^.

@dyladan
Copy link
Member

dyladan commented Jan 21, 2020

I think we can support 8 since the tests run on it and it is not so much of a burden. I think we should update the node 8 tests to run using the minimum version, instead of the latest version as they do right now.

@Flarna
Copy link
Member Author

Flarna commented Jan 22, 2020

Some numbers from our side:
NodeJs 8: 38.6%
NodeJs 10: 54.4%
NodeJs 12: 9.8%
Since ~ 09.2019 we see more Node.js 10 then Node.Js 8.
NodeJs 12 started to get used in significant amount since 10.2019 (most likely as it is marked as LTS since this time)

@OlivierAlbertini
Copy link
Member

I'm for keeping node 8 and set 8.5.0.

@dyladan
Copy link
Member

dyladan commented Jan 22, 2020

@Flarna can you provide more detailed statistics on how many of the node8 users are using versions older than 8.5? ~40% is a large number of users

@Flarna
Copy link
Member Author

Flarna commented Jan 23, 2020

Around 5% of the NodeJs 8 processes seen use <8.5

@dyladan
Copy link
Member

dyladan commented Jan 23, 2020

@mayurkale22 @open-telemetry/javascript-approvers given that over 35% of @Flarna's users are using node8 >=8.5, and under 2% are using versions of node8 < 8.5, I think the best course of action is to change our supported minimum version to 8.5. Furthermore, we should modify the circle tests to use node 8.5 rather than node latest so that we can be sure we are actually targeting what we claim to be targeting.

@OlivierAlbertini
Copy link
Member

also packages (package.json file) should have engines field to >=8.5

@mayurkale22
Copy link
Member

This is the Node Releases, Node 8 is EOL after Jan 2020:
Screen Shot 2020-01-23 at 10 33 18 AM

I am up changing minimum supported version to 8.5.

@mayurkale22 mayurkale22 added up-for-grabs Good for taking. Extra help will be provided by maintainers Discussion Issue or PR that needs/is extended discussion. labels Jan 23, 2020
@dyladan
Copy link
Member

dyladan commented Jan 23, 2020

It may be EOL but it is still the used by a very large number of users, including being the default node runtime on some serverless platforms.

@dyladan dyladan added the Agreed label Jan 23, 2020
@Ema93sh
Copy link
Contributor

Ema93sh commented Feb 5, 2020

@mayurkale22 @dyladan I would like to pick this up if its alright with yall

@mayurkale22 mayurkale22 removed the up-for-grabs Good for taking. Extra help will be provided by maintainers label Feb 5, 2020
@dyladan
Copy link
Member

dyladan commented Apr 29, 2020

@Flarna have you seen any drop in node 8 customers since EOL?

@Flarna
Copy link
Member Author

Flarna commented Apr 29, 2020

Current state:
Node 8: ~30% (~3.6% <8.5)
Node 10: ~47%
Node 12: ~19%
Node 12 is increasing, Node 8 is slightly going down.

I would not care much about node <8.5. First of all the number is low and besides that I'm quite sure that the at last most processes using old versions have not been changed/restarted for a long time.
Whoever introduces OTel has to touch he app and restart it. At this time it's quite common to revisit also dependencies and the node version used.

@dyladan
Copy link
Member

dyladan commented Apr 29, 2020

30% is still a large portion of users. I think 8.5.0 is still a decent minimum. The tests are still running against 8 latest. I'll update circle to run against 8.5

@Flarna
Copy link
Member Author

Flarna commented Apr 29, 2020

Yes, node 8 seems to be longer alive then expected... But as mentioned the concrete minor version is maybe not that important.
For whatever reason it seems that 8.9 and 8.11 are the most popular.

@dyladan
Copy link
Member

dyladan commented Apr 29, 2020

Hmm so maybe we're ok with only testing on 8 latest since it is unsupported anyways. No need to overcomplicate

pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Dec 15, 2023
Co-authored-by: Daniel Dyla <dyladan@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Discussion Issue or PR that needs/is extended discussion.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants