-
Notifications
You must be signed in to change notification settings - Fork 431
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
[Log SDK] Cost associated with populating LogRecord::observed_timestamp #2046
Comments
You are right! Obtaining the timestamp is indeed quite expensive. Wondering if there are solutions like checking system Ticks since start and using that to populate timestamp. |
The same challenge will exist for Exemplars too (when it arrives.), so any solution here would help Metric Exemplars too. |
Some old thread in rust community - https://users.rust-lang.org/t/fast-get-current-time/10381/2. It talks about using coarsetime or directly using libc.
Yes, need to see if there is platform/arch-independent way to achieve it. |
Some comparison between fn get_libc_clock(tv: &mut timespec) {
unsafe {
libc::clock_gettime(libc::CLOCK_MONOTONIC_COARSE, tv);
}
}
fn benchmark_time_methods(c: &mut Criterion) {
c.bench_function("SystemTime::now", |b| {
b.iter(|| black_box(std::time::SystemTime::now()));
});
let mut tv = libc::timespec {
tv_sec: 0,
tv_nsec: 0,
};
c.bench_function("libc::clock_gettime", |b| {
b.iter(|| black_box(get_libc_clock(&mut tv)));
});
} o/p is:
So, Also, if we don't want to use |
In Log SDK, the LogRecord::observed_timestamp is populated as:
opentelemetry-rust/opentelemetry-sdk/src/logs/log_emitter.rs
Line 270 in 673c328
However, after all the optimizations, the cost associated with this call is visible considerably ~20%:
A quick test by commenting it out increases stress test throughout to ~48k. Not sure about the solution for now, raising this issue to track it.
The text was updated successfully, but these errors were encountered: