Skip to content
This repository has been archived by the owner on Sep 13, 2022. It is now read-only.

Adds a nanosecond timer #122

Closed
wants to merge 1 commit into from
Closed

Conversation

sul4bh
Copy link

@sul4bh sul4bh commented May 31, 2017

This PR addresses #31

This is not ready for a merging yet. I want to discuss few thing with you guys:

  1. Does the opentracing spec say anything about the timer resolution? I have used a nanosecond timer here. Does doing so violate the spec?
  2. I am getting the following error with flow. I am not very familiar with it so can I pick your brain?
    Running npm run flow gives me the following error. Any idea how to fix it?
src/tracer.js:328
328:             const hrtime = process.hrtime();
                                        ^^^^^^ property `hrtime`. Property not found in
328:             const hrtime = process.hrtime();
                                ^^^^^^^ object type

src/util.js:146
146:         return (typeof process !== 'undefined' && process.hrtime);
                                                               ^^^^^^ property `hrtime`. Property not found in
146:         return (typeof process !== 'undefined' && process.hrtime);
                                                       ^^^^^^^ object type

@CLAassistant
Copy link

CLAassistant commented May 31, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@yurishkuro
Copy link
Member

OpenTracing does not dictate the resolution of timer used by the tracer internally, but it does specify the format of the timestamps passed into methods like span.finish() - those cannot change.

Also, the meaning of the explicitly passed timestamps is always milliseconds since epoch (higher precision can be provided via fractional decimal values). The Jaeger's internal representation is always microseconds since epoch.

Given the above, your current implementation is incorrect, because hrtime() does not return anything "since epoch", but since an arbitrary time point. It does not mean we cannot use it, but it needs to be used with care. I suggest checking jaeger-client-java implementation on how that was done.

Lastly, I think switching to hire-res timer should only happen on user request (when constructing the tracer), not by default.

@sul4bh
Copy link
Author

sul4bh commented Jun 2, 2017

I looked at the java implementation and I understand what you are saying. I will change my implementation to match the specs. Thanks for your input.

@yurishkuro
Copy link
Member

Please note there was an issue opened recently in the Java client repo that shows that even Java implementation is flawed.

@sul4bh
Copy link
Author

sul4bh commented Jun 2, 2017

This jaegertracing/jaeger-client-java#192 issue yes?

What you say in the comment about "startTime" and "nano second offsets from arbitrary time" is true for javascript implementation too. So I will run into the same problem...I will look more but do you have any suggestions on how to deal with it?

@yurishkuro
Copy link
Member

Let's discuss on jaegertracing/jaeger-client-java#192, if we find the solution there I assume it will apply to Node.js as well

@black-adder
Copy link
Contributor

@sul4bh we decided to go with:

Store the wall clock at the start of a process in the SpanContext. Pass it to child spans. Use nanoTime + offset to calculate start_time and duration. This will ensure that inside a single trace within a single process the start times are accurate with nanosecond precision.

Are you still available to work on this?

@sul4bh
Copy link
Author

sul4bh commented Jul 14, 2017

Yes, I am available.
(I will be available later in the evening to discuss additional details if need be.)

@yurishkuro
Copy link
Member

NB: this PR has a lot of odd formatting changes that actually go against our common coding style. Let's keep it to functional changes only.

@sul4bh
Copy link
Author

sul4bh commented Jul 16, 2017

From what I understand, this is what we are doing?

SpanContext.wallClock = now();
SpanContext.nano = hrtime();

span.startTime = spanContext.wallClock + hrtime(spanContext.nano);  // starTime derived by adding the nanosecond offset and the wallClock from spanContext ( hrtime(time) gives the offset )
span.nano = hrtime();

span.duration = hrtime(span.nano);

Basically, calculate startTime and duration for Spans using wallClock from SpanContext and nano-time offset?

@yurishkuro
Copy link
Member

Yes, with an additional assumption that every time a new SpanContext is created from an existing context it inherits wallClock + nano

@sul4bh
Copy link
Author

sul4bh commented Feb 27, 2018

I haven't been following the developments. I will close this.

@sul4bh sul4bh closed this Feb 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants