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

feat: anchored clock #3134

Merged
merged 38 commits into from Sep 10, 2022
Merged

feat: anchored clock #3134

merged 38 commits into from Sep 10, 2022

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Aug 1, 2022

Introduce an anchored clock which is resistant to time drift. Create a single anchored clock instance per local trace and store the anchored clocks in a weak map on the tracer to ensure they don't memory leak.

@codecov
Copy link

codecov bot commented Aug 1, 2022

Codecov Report

Merging #3134 (7f056da) into main (32cb123) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3134   +/-   ##
=======================================
  Coverage   93.28%   93.28%           
=======================================
  Files         201      202    +1     
  Lines        6579     6599   +20     
  Branches     1379     1384    +5     
=======================================
+ Hits         6137     6156   +19     
- Misses        442      443    +1     
Impacted Files Coverage Δ
...es/opentelemetry-core/src/common/anchored-clock.ts 100.00% <100.00%> (ø)
packages/opentelemetry-sdk-trace-base/src/Span.ts 99.19% <100.00%> (+<0.01%) ⬆️
...ackages/opentelemetry-sdk-trace-base/src/Tracer.ts 98.68% <100.00%> (+0.22%) ⬆️
...-trace-base/src/platform/node/RandomIdGenerator.ts 87.50% <0.00%> (-6.25%) ⬇️

2q Outdated Show resolved Hide resolved
@dyladan dyladan added bug Something isn't working priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect labels Aug 5, 2022
@Flarna
Copy link
Member

Flarna commented Sep 9, 2022

Seems like browser tests are unhappy:

ERROR in ./test/common/anchored-clock.test.ts
Module not found: Error: Can't resolve 'perf_hooks' in '/__w/opentelemetry-js/opentelemetry-js/packages/opentelemetry-core/test/common'
 @ ./test/common/anchored-clock.test.ts 19:21-42

CHANGELOG.md Outdated Show resolved Hide resolved
@dyladan
Copy link
Member Author

dyladan commented Sep 9, 2022

Seems like browser tests are unhappy:

ERROR in ./test/common/anchored-clock.test.ts
Module not found: Error: Can't resolve 'perf_hooks' in '/__w/opentelemetry-js/opentelemetry-js/packages/opentelemetry-core/test/common'
 @ ./test/common/anchored-clock.test.ts 19:21-42

and thats why we have them

@vmarchaud vmarchaud merged commit 5818129 into open-telemetry:main Sep 10, 2022
@dyladan dyladan deleted the anchored-clock branch September 12, 2022 15:32
@itai-codefresh
Copy link

i am getting these errors
Inconsistent start and end time, startTime > endTime [ 1663917517, 902433872 ] [ 1663917517, 901867218 ]

@pichlermarc
Copy link
Member

Hi @itai-codefresh, thanks for reporting this. Do you have a way to reproduce this issue? 🤔
If so, could you please open a bug ticket so that we can take a look? 🙂

@Flarna
Copy link
Member

Flarna commented Sep 28, 2022

At least instrumentation express provides a timestamp to span.end() but not to startSpan(). Therefore two different time sources are used resulting in undefined results.

See open-telemetry/opentelemetry-js-contrib#1209 and open-telemetry/opentelemetry-js-contrib#1193

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p2 Bugs and spec inconsistencies which cause telemetry to be incomplete or incorrect
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants