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

docs(sdk): update earliest support node version #2860

Merged

Conversation

svetlanabrennan
Copy link
Contributor

@svetlanabrennan svetlanabrennan commented Mar 24, 2022

Signed-off-by: Svetlana Brennan svetlana.svn@gmail.com

Which problem is this PR solving?

Spans use performance.timeOrigin in perf_hooks to create startTime and endTime but performance.timeOrigin is set incorrectly before v8.12.0. Node introduced a fix for it that was backported to v8.12.0.

Fixes Issues # 2857 & 2834

Short description of the changes

Updated node support section in the sdk readme to say 8.12.0 as the earliest supported node version.

Type of change

Doc update

How Has This Been Tested?

NA

Checklist:

  • Followed the style guidelines of this project
  • Documentation has been updated

Signed-off-by: Svetlana Brennan <svetlana.svn@gmail.com>
@svetlanabrennan svetlanabrennan requested a review from a team as a code owner March 24, 2022 22:36
@codecov
Copy link

codecov bot commented Mar 24, 2022

Codecov Report

Merging #2860 (343d586) into main (1ff9688) will increase coverage by 0.74%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2860      +/-   ##
==========================================
+ Coverage   92.63%   93.38%   +0.74%     
==========================================
  Files         161      149      -12     
  Lines        5487     4428    -1059     
  Branches     1162      920     -242     
==========================================
- Hits         5083     4135     -948     
+ Misses        404      293     -111     
Impacted Files Coverage Δ
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 84.48% <0.00%> (-15.52%) ⬇️
...elemetry-exporter-zipkin/src/platform/node/util.ts
...emetry-instrumentation-grpc/src/instrumentation.ts
...es/opentelemetry-instrumentation-grpc/src/utils.ts
...elemetry-instrumentation-grpc/src/grpc-js/index.ts
...ges/opentelemetry-instrumentation-http/src/http.ts
...ry-instrumentation-grpc/src/grpc-js/serverUtils.ts
...es/opentelemetry-instrumentation-http/src/utils.ts
...entelemetry-instrumentation-grpc/src/grpc/index.ts
packages/opentelemetry-sdk-node/src/sdk.ts
... and 10 more

Copy link
Member

@vmarchaud vmarchaud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@svetlanabrennan
Copy link
Contributor Author

I think we should update the minimum version there: https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-sdk-trace-node/package.json#L33

Just in that package? Or in all the packages?

@dyladan
Copy link
Member

dyladan commented Mar 25, 2022

I think we should update the minimum version there: https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-sdk-trace-node/package.json#L33

Just in that package? Or in all the packages?

All

@svetlanabrennan
Copy link
Contributor Author

All packages (package.json engines node version) updated.

@dyladan
Copy link
Member

dyladan commented Mar 25, 2022

Test failure is not related

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

Successfully merging this pull request may close these issues.

None yet

6 participants