-
Notifications
You must be signed in to change notification settings - Fork 424
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
Metric stdout exporter to print timestamp in human readable format #1192
Metric stdout exporter to print timestamp in human readable format #1192
Conversation
…/cijothomas/opentelemetry-rust into cijothomas/stdout-td-humanformat
…/cijothomas/opentelemetry-rust into cijothomas/stdout-td-humanformat
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1192 +/- ##
=====================================
Coverage 49.6% 49.7%
=====================================
Files 164 172 +8
Lines 20653 20713 +60
=====================================
+ Hits 10258 10304 +46
- Misses 10395 10409 +14
☔ View full report in Codecov by Sentry. |
Hm maybe this should be based on features? the convenient thing about the current output is it matches the file-exporter, and otlp protobuf json encodings so the common case json format is what you may expect. |
The file exporter and the json/protobuf are intended for machine consumption, so it makes sense for them to follow the specified format. The stdout exporter is purely for users to learn/debug OpenTelemetry, and not meant for production use. This stdout exporter is more like the "log" exporter in OTel Collector: https://github.com/open-telemetry/opentelemetry-collector/blob/main/exporter/loggingexporter/internal/otlptext/traces.go#L42 I will propose the following:
Thoughts on above? |
Another thought would be, as stdout exporter is purely for users to learn/debug OpenTelemetry, and not meant for production use - it allows us to extend it for better debugging. So we can also keep the exiting epoch representation, and add new field for human readable format. Sometime, for debugging purpose, customers may want to configure both stdout, and production exporter and compare the contents to validate the production exporter export values. Or else, keeping it on feature based as @jtescher suggested also works. |
Yeah agree that it's about debugging (mostly local) and not production. What I've seen people use it for most is debugging other exports or other integration issues, in which case I think it's mostly around having a second exporter to compare what you expect to see, in which case a json format for what you'd most expect the data to look like for machines and a human readable version like what go has which is more multi-line log oriented and likely not json at all both make sense to me. |
Thanks for the feedbacks. I do see some value in keeping the unix_nano format. |
@jtescher modified the PR to keep the existing format, and added user-friendly time as an additional field. Could you review/merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good thanks @cijothomas
In open-telemetry#1192, `chrono` was added as a dependency of the `opentelemetry-stdout` crate in order to support outputting timestamps in human readable format. In that PR, all Chrono features were disabled apart from the `clock` feature. However, since that change landed, `chrono` has added support for an even finer-grained feature named `now`, which is a subset of the `clock` feature which excludes timezone support, and so avoids pulling in many timezone related crates. `opentelemetry-stdout` only uses chrono's UTC features, so we can switch from using the `clock` feature to using `now` instead. After this change, the following transitive dependencies are no longer pulled in: - `android-tzdata` - `android_system_properties` - `cc` - `core-foundation-sys` - `iana-time-zone` - `iana-time-zone-haiku` - `windows-core` - `windows-targets` - `windows_aarch64_gnullvm` - `windows_aarch64_msvc` - `windows_i686_gnu` - `windows_i686_msvc` - `windows_x86_64_gnu` - `windows_x86_64_gnullvm` - `windows_x86_64_msvc` See: chronotope/chrono#1343 https://github.com/chronotope/chrono/blob/main/README.md#crate-features
In open-telemetry#1192, `chrono` was added as a dependency of the `opentelemetry-stdout` crate in order to support outputting timestamps in human readable format. In that PR, all Chrono features were disabled apart from the `clock` feature. However, since that change landed, `chrono` has added support for an even finer-grained feature named `now`, which is a subset of the `clock` feature which excludes timezone support, and so avoids pulling in many timezone related crates. `opentelemetry-stdout` only uses chrono's UTC features, so we can switch from using the `clock` feature to using `now` instead. After this change, the following transitive dependencies are no longer pulled in: - `android-tzdata` - `android_system_properties` - `cc` - `core-foundation-sys` - `iana-time-zone` - `iana-time-zone-haiku` - `windows-core` - `windows-targets` - `windows_aarch64_gnullvm` - `windows_aarch64_msvc` - `windows_i686_gnu` - `windows_i686_msvc` - `windows_x86_64_gnu` - `windows_x86_64_gnullvm` - `windows_x86_64_msvc` See: chronotope/chrono#1343 https://github.com/chronotope/chrono/blob/main/README.md#crate-features
In #1192, chrono was added as a dependency of the opentelemetry-stdout crate in order to support outputting timestamps in human readable format. In that PR, all Chrono features were disabled apart from the clock feature. However, since that change landed, chrono v0.4.32 has added support for an even finer-grained feature named now, which is a subset of the clock feature - that excludes timezone support, and so avoids pulling in many timezone related crates.
Addresses #1191 for metrics.
Changes
Update stdout metric exporter to write timestamp in human readable format.Writes timestamp in human-friendly format as well. The existing unix_ns format is kept as-is.
Example outout with this PR:
"startTime":"2023-08-08 00:02:41.404","time":"2023-08-08 00:02:41.404"
before this PR:
"startTimeUnixNano":1691453709811505677,"timeUnixNano":1691453709811786077
Merge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes