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

fix(core): fix precision loss in numberToHrtime #3480

Merged
merged 2 commits into from Jan 3, 2023

Conversation

legendecas
Copy link
Member

Which problem is this PR solving?

Fixes #3459

Avoid unnecessary stringify of number values as the precision loss is inevitable with toFixed too. This also fixes precision loss on numbers without fractional parts.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Fix precision loss: should convert Date hrTime

Checklist:

  • Followed the style guidelines of this project
  • Unit tests have been added
  • Documentation has been updated

@legendecas legendecas changed the title refactor(core): avoid unnecessary stringify in numberToHrtime fix(core): fix precision loss in numberToHrtime Dec 9, 2022
@legendecas legendecas marked this pull request as ready for review December 9, 2022 02:46
@legendecas legendecas requested a review from a team as a code owner December 9, 2022 02:46
@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Merging #3480 (33d7f73) into main (3fd6fb8) will increase coverage by 0.06%.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3480      +/-   ##
==========================================
+ Coverage   93.70%   93.77%   +0.06%     
==========================================
  Files         247      249       +2     
  Lines        7439     7612     +173     
  Branches     1554     1587      +33     
==========================================
+ Hits         6971     7138     +167     
- Misses        468      474       +6     
Impacted Files Coverage Δ
packages/opentelemetry-core/src/common/time.ts 95.58% <100.00%> (+0.13%) ⬆️
...-trace-base/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️
...s/opentelemetry-instrumentation-fetch/src/fetch.ts 97.00% <0.00%> (ø)
...-instrumentation-fetch/src/enums/AttributeNames.ts 100.00% <0.00%> (ø)

@legendecas legendecas force-pushed the core/time branch 2 times, most recently from 44d0967 to 775a4b1 Compare December 9, 2022 02:54
@legendecas legendecas merged commit c93ab9e into open-telemetry:main Jan 3, 2023
@legendecas legendecas deleted the core/time branch January 3, 2023 15:53
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.

I want to know whether the nanos length returned by the numberToHrtime method is a fixed length?
4 participants