Use CLOCK_BOOTTIME instead of CLOCK_REALTIME in published_at_ns in process context C/C++ impl#79
Conversation
…` in process context C/C++ impl **What does this PR do?** This PR updates the process context C/C++ implementation to match the change in the upstream specification in open-telemetry/opentelemetry-specification@15256ee to adopt a monotonic clock. **Motivation:** Using a monotonic clock means that readers can't get confused by clocks going backward, and we can trivially about the ABA issue (because if the clock can't ever go back, we can always add 1 to a previous read to ensure no two timestamps are ever the same). **Additional Notes:** In open-telemetry#66 (comment) we discussed that this could be implemented by directly using the `CLOCK_MONOTONIC`, or alternatively we could map the monotonic clock back to a regular ns from epoch. I had this commit already ready and it takes the first approach, but let's discuss? ;) **How to test the change?** In practice readers don't really need to change unless they were trying to interpret the timestamp and pretty-print it (as the bash script was).
|
Btw @christos68k now that I think of it, I think if we went with option 2:
And looking at the manpage https://manpages.opensuse.org/Tumbleweed/man-pages/clock_gettime.2.en.html#CLOCK_MONOTONIC
I think we could start getting weird values (either in the past or in the future) in the presence of updates to system time and VM suspension if we always used the same delta... 🤔 |
If we update the delta more than once for the lifetime of the process, we lose the "can't go backwards" guarantee. |
|
Ahh yeah I read your comment and completely glossed over the I guess in the end, if we really wanted to, rather than trying to fight the delta and the different clocks, we could bite the bullet and have the two timestamps (or one update counter and one timestamp), but it does seem slight overkill... |
Even if system time goes backwards, the calculated delta won't be affected (as it's only calculated once for the life of the process) and thus the resulting delta+monotonic values also won't be affected. The only affected part will be the tight correlation with the real time clock (which we don't really care about). In essence, this scheme trades that tight-synchronization-with-realtime guarantee for monotonicity and precision of relative measurements with respect to each other and the 'base' which remains unaffected and accurate to the nanosecond (also something we don't really need here but it's there for free). I think we can be flexible and don't need to overspecify the implementation (either here or in the OTEP). Instead, if we specify the clock used in the OTEP (e.g.
|
Yes! My "hmm not amazing" spider sense is more of the -- if the writer does the delta and puts as a timestamp a cooked value that includes the delta change, the semantics of "most of the time this is a timestamp that makes sense but sometimes it won't!" aren't great -- especially if not all writers agree on exactly that meaning. Which brings us to...
...if we put in I quite like that! I'll go ahead and update this PR and push a tiny tweak to the spec to just say "use |
CLOCK_MONOTONIC instead of CLOCK_REALTIME in published_at_ns in process context C/C++ implCLOCK_BOOTTIME instead of CLOCK_REALTIME in published_at_ns in process context C/C++ impl
|
@christos68k I've pushed open-telemetry/opentelemetry-specification@0c6caa7 + updated this PR. All still good on your side? |
…of reference library **What does this PR do?** This PR updates the vendored version of the OTel process context support with the latest code from open-telemetry/sig-profiling#79 (which will soon be merged upstream). **Motivation:** This latest version of the library fixes a number of small issues, but more importantly includes support for the thread context configuration keys which we need for #347 (so we will be able to shed a bunch of code once we rebase on top of this one). **Additional Notes:** From the Java API consumer side, not a lot changes with this version. **How to test the change?** Validate tests still pass!
...they don't always have a `#define` for this flag.
…ild`
In the `otel_process_ctx_read()` code used for testing, if somehow
there were repeated keys in the process context data, then we could
leak the existing value while overwriting it with the latter one.
(The key missing bit was `if (*field != NULL) { free(value); return
false; }`).
TL;DR if there were repeated keys when parsing the data in the testing code we could leak their values. This matches the latest version upstream in open-telemetry/sig-profiling#79 .
What does this PR do?
This PR updates the process context C/C++ implementation to match the change in the upstream specification in
open-telemetry/opentelemetry-specification@15256ee to adopt a monotonic clock.
Motivation:
Using a monotonic clock means that readers can't get confused by clocks going backward, and we can trivially about the ABA issue (because if the clock can't ever go back, we can always add 1 to a previous read to ensure no two timestamps are ever the same).
Additional Notes:
In #66 (comment) we discussed that this could be implemented by directly using the
CLOCK_MONOTONIC, or alternatively we could map the monotonic clock back to a regular ns from epoch.I had this commit already ready and it takes the first approach, but let's discuss? ;)
How to test the change?
In practice readers don't really need to change unless they were trying to interpret the timestamp and pretty-print it (as the bash script was).