adds timestamps #2231
adds timestamps #2231
Conversation
The committers listed above are authorized under a signed CLA. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2231 +/- ##
=====================================
Coverage 79.1% 79.1%
=====================================
Files 121 121
Lines 21082 21173 +91
=====================================
+ Hits 16680 16762 +82
- Misses 4402 4411 +9 ☔ View full report in Codecov by Sentry. |
| @@ -173,6 +173,8 @@ where | |||
| log_record.set_event_name(meta.name()); | |||
| log_record.set_severity_number(severity_of_level(meta.level())); | |||
| log_record.set_severity_text(meta.level().as_str()); | |||
| log_record.set_timestamp(std::time::SystemTime::now()); | |||
There was a problem hiding this comment.
As per the specs - This field is optional, it may be missing if the source timestamp is unknown.
| @@ -173,6 +173,8 @@ where | |||
| log_record.set_event_name(meta.name()); | |||
| log_record.set_severity_number(severity_of_level(meta.level())); | |||
| log_record.set_severity_text(meta.level().as_str()); | |||
| log_record.set_timestamp(std::time::SystemTime::now()); | |||
| log_record.set_observed_timestamp(std::time::SystemTime::now()); | |||
There was a problem hiding this comment.
This is already done in SDK, why do we need it here?
There was a problem hiding this comment.
Is this path in SDK hit when this Bridge is added as a layer into tracing_subscriber ? On the QuickWit side I see that this timestamp was not populated which needed a workaround: quickwit-oss/quickwit#5366
However this fix is not ideal since it will set the log timestamp at the received time, not at the emit time which will lead to all sorts of troubles.
There was a problem hiding this comment.
Is this path in SDK hit when this Bridge is added as a layer into tracing_subscriber
yes it does. You can try running the example to verify - https://github.com/open-telemetry/opentelemetry-rust/blob/main/opentelemetry-appender-tracing/examples/basic.rs
|
timestamp setting is costly (ref - #2046), let's not do it unless really required by specs. |
|
@montekki I am closing this PR as it looks like there is no missing functionality. If you believe otherwise, we can re-open to discuss more. |
Fixes #
Design discussion issue (if applicable) #
Changes
Unset timestamps in logs break services such as Quickwit, although if my understanding is correct these timestamps must be set as a part of OTEL spec.
Merge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes