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): stop rounding to nearest int in hrTimeTo*seconds() functions #4014

Merged
merged 3 commits into from
Jul 31, 2023

Conversation

aabmass
Copy link
Member

@aabmass aabmass commented Jul 21, 2023

Which problem is this PR solving?

Fixes #3986

Short description of the changes

Remove Math.round() calls from hrTimeTo{Milli,Micro}seconds() functions in order to preserve higher precision from floating point parts.

Type of change

Please delete options that are not relevant.

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

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

  • Existing tests are all passing
  • Would like to test this against contrib repo but not sure how to do that

Checklist:

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

@aabmass aabmass changed the title fix(core): stop rounding to nearest int in hrTimeTo*seconds() functions fix(core): stop rounding to nearest int in hrTimeTo*seconds() functions Jul 21, 2023
@aabmass aabmass changed the title fix(core): stop rounding to nearest int in hrTimeTo*seconds() functions fix(core): stop rounding to nearest int in hrTimeTo*seconds() functions Jul 21, 2023
@codecov
Copy link

codecov bot commented Jul 21, 2023

Codecov Report

Merging #4014 (0b4aaf9) into main (2b20565) will not change coverage.
The diff coverage is 100.00%.

❗ Current head 0b4aaf9 differs from pull request most recent head b356aa1. Consider uploading reports for the commit b356aa1 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4014   +/-   ##
=======================================
  Coverage   92.31%   92.31%           
=======================================
  Files         321      321           
  Lines        9189     9189           
  Branches     1953     1953           
=======================================
  Hits         8483     8483           
  Misses        706      706           
Files Changed Coverage Δ
packages/opentelemetry-core/src/common/time.ts 97.18% <100.00%> (ø)

@dyladan
Copy link
Member

dyladan commented Jul 21, 2023

Seems like it doesn't break any tests. Have you been able to try this fork in a real world app?

@aabmass
Copy link
Member Author

aabmass commented Jul 21, 2023

Seems like it doesn't break any tests. Have you been able to try this fork in a real world app?

I tried it in a little test express server with the http instrumentation and it fixed the striping I mentioned in the issue. Is there a way to run the contrib repo tests against this branch?

@dyladan
Copy link
Member

dyladan commented Jul 25, 2023

Seems like it doesn't break any tests. Have you been able to try this fork in a real world app?

I tried it in a little test express server with the http instrumentation and it fixed the striping I mentioned in the issue. Is there a way to run the contrib repo tests against this branch?

Not an easy way that I can think of. We could release a -alpha tag version and push it to NPM.

I am very familiar with the trace SDK and don't expect this to be a problem, but can someone with more familiarity with the metrics SDK sanity check this? @legendecas @pichlermarc

@legendecas
Copy link
Member

The only call site of the conversion functions is merging two accumulations, e.g. https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/sdk-metrics/src/aggregator/LastValue.ts#L70. I believe it is fine to accept non-integer numbers here.

@aabmass aabmass marked this pull request as ready for review July 26, 2023 16:40
@aabmass aabmass requested a review from a team July 26, 2023 16:40
@aabmass
Copy link
Member Author

aabmass commented Jul 28, 2023

Test failures api-eol-node-test are unrelated to this PR, they are failing on HEAD commit 2b20565

Unexpected token {
npm ERR! Test failed.  See above for more details.

@legendecas
Copy link
Member

I'm going to land this and track the api-eol failure in #4029.

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.

hrTimeToMilliseconds and hrTimeToMicroseconds lose precision
4 participants