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: use system clock for timestamps and perf clock for durations #1019

Closed

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented May 5, 2020

Fixes #852

Because the timestamps generated from the performance timer cannot be trusted, we can only use them to generate durations, not real timestamps. This PR introduces the following behavior:

User provides start AND end timestamps

  • Start Time from user
  • Duration is calculated from input times
  • End time is calculated from start time input and duration

User provides start timestamp

  • Start time from user
  • Duration calculated from input start time and system clock
  • End time is calculated from start time input and duration

User provides end timestamp

  • Start time from system clock
  • Duration calculated from system clock start time and input end time
  • End time is calculated from system clock start time and duration

User provides no timestamps

  • Start time from system clock
  • Duration is calculated using the performance clock
  • End time is calculated using system clock start time and duration

Copy link
Member

@obecny obecny left a comment

Choose a reason for hiding this comment

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

unit tests ?

@codecov-io
Copy link

codecov-io commented May 5, 2020

Codecov Report

Merging #1019 into master will decrease coverage by 0.03%.
The diff coverage is 92.18%.

@@            Coverage Diff             @@
##           master    #1019      +/-   ##
==========================================
- Coverage   95.00%   94.96%   -0.04%     
==========================================
  Files         212      212              
  Lines        8806     8862      +56     
  Branches      796      802       +6     
==========================================
+ Hits         8366     8416      +50     
- Misses        440      446       +6     
Impacted Files Coverage Δ
packages/opentelemetry-core/src/common/time.ts 88.73% <16.66%> (-6.66%) ⬇️
packages/opentelemetry-tracing/src/Span.ts 98.92% <100.00%> (-1.08%) ⬇️
...telemetry-tracing/test/BasicTracerProvider.test.ts 100.00% <100.00%> (ø)

@dyladan dyladan added the bug Something isn't working label May 5, 2020
) {
this.name = spanName;
this.spanContext = spanContext;
this.parentSpanId = parentSpanId;
this.kind = kind;
this.links = links;
this.startTime = timeInputToHrTime(startTime);

if (startTime != null) {
Copy link
Member

Choose a reason for hiding this comment

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

i believe we should check startTime for undefined instead of null

Copy link
Member

Choose a reason for hiding this comment

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

Mind the missing second = - startTime != null is the same as (startTime !== null) || (startTime !== undefined).

Copy link
Member

Choose a reason for hiding this comment

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

Didn't see that indeed, i would prefer to have strict check so its easier to read the code though

@Flarna
Copy link
Member

Flarna commented May 6, 2020

My feeling is that this is quite a heavy change for a bug which was fixed in NodeJs 8.12.0.
For browsers this may be different as they run on machins getting hibernated frequently.
Mixing different time sources tends to be problematic.
What about making the time source to use configurable? I could even think about allow users to provide their time provider.

@pichlermarc
Copy link
Member

Looks like this was replaced by #3129, which was replaced by #3134, should we close this @dyladan? Looks like stale bot never gets so far. 🙂

@dyladan dyladan closed this Sep 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sleeping computer causes span timings to be incorrect in browser
6 participants