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

hrTimeToMilliseconds and hrTimeToMicroseconds lose precision #3986

Closed
aabmass opened this issue Jul 13, 2023 · 1 comment · Fixed by #4014
Closed

hrTimeToMilliseconds and hrTimeToMicroseconds lose precision #3986

aabmass opened this issue Jul 13, 2023 · 1 comment · Fixed by #4014
Assignees
Labels
bug Something isn't working pkg:core triage

Comments

@aabmass
Copy link
Member

aabmass commented Jul 13, 2023

What happened?

These two util functions are using Math.round() to round to the nearest integer, which causes a huge loss of precision from hrTime:

export function hrTimeToMilliseconds(time: api.HrTime): number {
return Math.round(time[0] * 1e3 + time[1] / 1e6);
}

export function hrTimeToMicroseconds(time: api.HrTime): number {
return Math.round(time[0] * 1e6 + time[1] / 1e3);
}

They could instead convert to the correct unit without rounding to an integer.

Steps to Reproduce

const core = require("@opentelemetry/core");

async function main() {
  const start = process.hrtime();
  const end = await new Promise((resolve) => {
    setTimeout(() => resolve(process.hrtime()), 100);
  });
  const dur = core.hrTimeDuration(start, end);
  console.log(dur);
  console.log("Millis diff: %s", core.hrTimeToMilliseconds(dur));
}
main();

Expected Result

Print duration as a floating point milliseconds

[ 0, 100727047 ]
Millis diff: 100.727047

Actual Result

Everything after the decimal is thrown away

[ 0, 100727047 ]
Millis diff: 101

Additional Details

I noticed this while trying out ExponentialHistograms with an HTTP server. I noticed this weird striping in the histograms where any bucket not containing an integer will always have zero count:
image
because the http instrumentation uses hrTimeToMilliseconds().

OpenTelemetry Setup Code

No response

package.json

No response

Relevant log output

No response

@aabmass aabmass added bug Something isn't working triage labels Jul 13, 2023
@aabmass
Copy link
Member Author

aabmass commented Jul 19, 2023

From SIG: the next step is to make a draft PR fixing this and see if it breaks tests in this repo or contrib.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working pkg:core triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants