Skip to content
This repository was archived by the owner on Oct 3, 2023. It is now read-only.

fix(span): make child span clock relative to root span#628

Merged
mayurkale22 merged 3 commits into
census-instrumentation:masterfrom
hekike:fix/child-span-clock
Jul 19, 2019
Merged

fix(span): make child span clock relative to root span#628
mayurkale22 merged 3 commits into
census-instrumentation:masterfrom
hekike:fix/child-span-clock

Conversation

@hekike

@hekike hekike commented Jul 12, 2019

Copy link
Copy Markdown
Contributor

new Date() is vulnerable to clock drifts. This PR changes child span clocks to start relative to root span by using process.hrtime. Otherwise in-process spans can have an invalid casualty.

@hekike

hekike commented Jul 13, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@mayurkale22 @draffensperger I don't really like the idea of making clock/span properties public (9192b10) just for testing. Do you have a recommendation on how to test it without changing these types? Is it usual in TS adding public methods which are there just for testing? Like _setClock?

@hekike hekike force-pushed the fix/child-span-clock branch from 9192b10 to 665d555 Compare July 13, 2019 14:44
@hekike hekike force-pushed the fix/child-span-clock branch from 665d555 to 3dad28c Compare July 19, 2019 02:26
@hekike

hekike commented Jul 19, 2019

Copy link
Copy Markdown
Contributor Author

I fixed CI

draffensperger
draffensperger approved these changes Jul 19, 2019
View reviewed changes

@draffensperger draffensperger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This sort of thing is why I'm hoping we can standardize on performance.now() for all time measurments in OpenTelemetry JS!

@mayurkale22 mayurkale22 merged commit 7fe73e9 into census-instrumentation:master Jul 19, 2019
@hekike hekike deleted the fix/child-span-clock branch July 19, 2019 22:30
@hekike

hekike commented Jul 19, 2019

Copy link
Copy Markdown
Contributor Author

Thanks! Can I ask a release with this fix?

@mayurkale22

mayurkale22 commented Jul 19, 2019

Copy link
Copy Markdown
Member

Thanks! Can I ask a release with this fix?

Sure, I can do that. By any chance, do you know fix for : https://github.com/census-instrumentation/opencensus-node/network/alerts? I wanted to include fix in next release.

@mayurkale22 mayurkale22 added this to the Release 0.0.16 milestone Jul 19, 2019
@hekike

hekike commented Jul 19, 2019

Copy link
Copy Markdown
Contributor Author

hmm, this link says 404 for me.

@mayurkale22

mayurkale22 commented Jul 20, 2019

Copy link
Copy Markdown
Member

Hmm, np. I will take care of it later.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@justindsmith justindsmith Awaiting requested review from justindsmith
2 more reviewers
@draffensperger draffensperger draffensperger approved these changes
@mayurkale22 mayurkale22 mayurkale22 approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla: yes

Projects

None yet

Milestone

Release 0.0.16

Development

Successfully merging this pull request may close these issues.

4 participants

@hekike @mayurkale22 @draffensperger @googlebot