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

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

Closed
yuanman0109 opened this issue Dec 1, 2022 · 7 comments · Fixed by #3480
Assignees
Labels
bug Something isn't working pkg:core priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect

Comments

@yuanman0109
Copy link

function numberToHrtime(epochMillis: number): api.HrTime {

I saw this method used in many places, so I tested this method in the development environment. I found that sometimes the length of time obtained is unstable. Sometimes the length of the value 'nano' is 9, and sometimes it is 8. Is this normal? My Chrome version is the latest, and this is my screenshot:
image
image

@yuanman0109
Copy link
Author

yuanman0109 commented Dec 1, 2022

export function hrTimeToNanoseconds(time: api.HrTime): number {

The start and end time of the span are calculated by this method. If the precision of js will affect the calculation results here, our duration value will be very inaccurate, In about 1 second, the performance statistics will be affected @legendecas @dyladan

@legendecas
Copy link
Member

legendecas commented Dec 1, 2022

Duplicate of #2643 for hrTimeToNanoseconds.

@legendecas
Copy link
Member

numberToHrtime converts number of seconds to HrTime tuple ([seconds, nanoseconds]) without precision loss. Count of digits in the elements of the HrTime tuple can change depends on the leading zero counts of the element value.

@Flarna
Copy link
Member

Flarna commented Dec 1, 2022

A bit strange is that nanos have a fractional part. I have never seen this in node.js process.hrtime().

Also doing a conversion via a string might be not the fastest variant and looks a likely a bit misleading.

@yuanman0109
Copy link
Author

yuanman0109 commented Dec 1, 2022

const NANOSECOND_DIGITS = 6;
const SECOND_TO_NANOSECONDS = Math.pow(10, NANOSECOND_DIGITS);
function numberToHrtime(epochMillis) {
  const epochSeconds = epochMillis / 1000;
  // Decimals only.
  const seconds = Math.trunc(epochSeconds);
  const time = seconds * 1000;
  // Round sub-nanosecond accuracy to nanosecond.
  const nanos = (epochMillis - time) * SECOND_TO_NANOSECONDS;
  return [seconds, nanos];
}

Avoid using decimals to temporarily solve this problem

@dyladan dyladan added bug Something isn't working priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect labels Dec 7, 2022
@dyladan
Copy link
Member

dyladan commented Dec 7, 2022

@legendecas should this be closed as a dup of #2643 ?

@legendecas
Copy link
Member

Sorry, I might get it wrong in the above statement on numberToHrtime. I'll investigate the accuracy of the conversion of numberToHrtime with the fraction part (e.g. chrome performance.now() returns a number with the fractional part).

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 priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants