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

How to represent timestamps? #19

Closed
draffensperger opened this issue Jun 6, 2019 · 15 comments
Closed

How to represent timestamps? #19

draffensperger opened this issue Jun 6, 2019 · 15 comments

Comments

@draffensperger
Copy link
Contributor

The OpenCensus Node project used Date for span start/end times (code link). That has the downside of only only having millisecond accuracy. OpenTracing JS uses number for milliseconds since the epoch (code link), which currently has ~microsecond accuracy - you can confirm that by testing Date.now() + 0.001 vs. Date.now() + 0.0001.

I think microsecond accuracy is pretty good, but perhaps for some high performance processes you might want higher accuracy, and I think since we are designing this from the ground up, we should ask the question.

Here's an alternative: we use timestamps from the performance.now() clock, which Node 8+ supports, and which also has broad browser support even from IE 10+.

Those timestamps can have nanosecond accuracy for processes that have been running for less than about 90 days (try running 90*365*24*3600*1000 + 1e-6 to check the accuracy).

We would need to convert those timestamps into a format that the exporter expects in a careful way to preserve the accuracy, see for example [this function]
(https://github.com/census-instrumentation/opencensus-web/blob/82464ed231ad829a7a8a72eab17e95577aa813d0/packages/opencensus-web-core/src/common/time-util.ts#L37) that converts a high-accuracy performance.now timestamp and a high-accuracy origin time into a nano-second accuracy ISO date string.

If we just want to do number as millis since the epoch, I'm fine with that too! But wanted to at least open this for discussion. I think use of performance.now as a clock function would be nice either way since it's one less incompatibility between Node and the browser we have to think about.

@bogdandrutu
Copy link
Member

More granularity better :) I would go with something that has ns but not available us is good.

@draffensperger
Copy link
Contributor Author

Another option is to use a tuple type of [seconds, nanos] like process.hrtime returns

@rochdev
Copy link
Member

rochdev commented Jun 7, 2019

Here is a quick summary of the differences between a Number object (for example returned by performance.now) and a tuple (for example returned by process.hrtime)

  • Number: Maximum granularity of 0.2µs. Easier to work with.
  • Tuple: Maximum granularity of 1ns. A bit clunkier to work with but it's just 2 numbers so not too bad.

What is the use case for granularity higher than 0.2µs?

@rochdev
Copy link
Member

rochdev commented Jun 7, 2019

In general I think I would vote for a tuple representation internally and accept both a tuple or a Number as input.

@rochdev
Copy link
Member

rochdev commented Jun 17, 2019

To clarify my above comment, the Number input would be as milliseconds with a decimal component. This is the precision that is used by JavaScript for timestamps.

@draffensperger
Copy link
Contributor Author

One of the weirdnesses with using Number for milliseconds is that gets progressively less accurate at the number increases. So if you get values from performance.now() they will have nanosecond accuracy since they are generally close to zero (assuming your browser window hasn't been open for too long and your Node process hasn't been running for < 90 days). However, for epoch millis you only have ~0.5 microsecond accuracy (try running Date.now() + 1e-6 vs Date.now() + 1e-3)

@rochdev
Copy link
Member

rochdev commented Jun 18, 2019

In any case we would always have 0.2µs precision since the result from performance.now() needs to be added to the timestamp when the process was started to get the correct start/end times. Personally I think this is an acceptable precision loss for the simplicity of using numbers. For more precision, we can additionally support tuples (which would also need to be added to the process start timestamp).

@draffensperger
Copy link
Contributor Author

draffensperger commented Jun 18, 2019

In any case we would always have 0.2µs precision since the result from performance.now() needs to be added to the timestamp when the process was started to get the correct start/end times.

Well, to convert a performance.now timestamp to a nanosecond-precision ISO timestamp, you can separate the fractional and whole number parts of both the origin and performance.now numbers, add them separately, and then create the ISO timestamp string based on the separately added whole millisecond and fractional millisecond parts, see this code that does that. That approach will still have 0.2 microsecond precision relative to the epoch, but it will have nanosecond precision for the relative events within the spans of the Node process.

Anyway, I'm fine with epoch millis everywhere and I think I'm convinced we should just go with that for simplicity.

@rochdev
Copy link
Member

rochdev commented Jun 19, 2019

This would mean a change of how the time is reported though. It means end() should be called with the duration and not the end time. This would indeed allow a higher precision in most cases if internally it's always stored as a tuple, depending on how the data is reported.

I think it's worth a discussion if that makes high precision possible with Number. The only downside is that precision would be lost over time.

@rochdev
Copy link
Member

rochdev commented Jun 19, 2019

This was discussed at today's SIG meeting, and for now a Number will be used as milliseconds for the end time.

This is (potentially) temporary however and we still need to discuss what would be the best approach. One discussion that probably needs to happen in the specification repository first is whether end() should accept a duration or an explicit end time.

@rochdev
Copy link
Member

rochdev commented Jun 19, 2019

Opened open-telemetry/opentelemetry-specification#139 to discuss end time VS duration and making the timestamps available on the cross-language API.

@draffensperger
Copy link
Contributor Author

I like the idea of just picking epoch millis and moving on for now.

But to clarify a bit of what I meant - what I was envisioning was that both the start and end times would be in performance.now timestamps such that both could be high-precision relative to each other and other events in the spans collected from the Node process for the request.

Only when they need to be converted to epoch timestamps when sent to an exporter would they become 0.2 microsecond accuracy relative to the epoch, but they would still all be nanosecond-accuracy relative to each other (true for both start and end times and for trace annotations).

By making storing start time and duration, we make the end time be nanosecond precise, but the start time is still just the 0.2 microsecond precision relative to other spans/events in the process for the request (portion of the overall trace).

@rochdev
Copy link
Member

rochdev commented Jun 20, 2019

I like this idea a lot. However, this may not be possible in other languages. How should this be described in the spec?

@draffensperger
Copy link
Contributor Author

Well, most other languages just have nice nanosecond-level timestamps, or at least native 64-bit ints that could make representing a nanosecond-level epoch time easy :(

If we were going to write something in the specs for this, my sense would be to say that implementations may choose to use a monotonic clock timestamp for span start/end times and event times if that helps improve accuracy or ease of use. So that for Node/browser we could use the performance.now timestamp.

One thing we could also do is keep the number type but allow either monotonic (millis since performance.timeOrigin) or epoch (millis since 1970) in the public API, but store all timestamps internally as monotonic performance.now timestamps. We can figure out which type of timestamp was intended from the public API based on its size (if it's > say last month's epoch timestamp then it's an epoch time, else it's a monotonic performance.now time). We can always determine that because there were no Node processes running in 1970....

@mayurkale22 mayurkale22 added question User is asking a question not related to a new feature or bug Discussion Issue or PR that needs/is extended discussion. and removed question User is asking a question not related to a new feature or bug labels Jul 8, 2019
@mayurkale22 mayurkale22 added Agreed and removed Discussion Issue or PR that needs/is extended discussion. labels Sep 30, 2019
@mayurkale22
Copy link
Member

Closed via #206

dyladan referenced this issue in dyladan/opentelemetry-js Sep 9, 2022
dyladan referenced this issue in dyladan/opentelemetry-js Sep 9, 2022
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this issue Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants