opentelemetry-rust
opentelemetry-rust copied to clipboard
Add a derive feature to allow deriving conversions into common types
Fixes #1685
Changes
Implemented suggested derive macros for Key/Value related types.
Merge requirement checklist
- [ ] CONTRIBUTING guidelines followed
- [ ] Unit tests added/updated (if applicable)
- [ ] Appropriate
CHANGELOG.mdfiles updated for non-trivial, user-facing changes - [ ] Changes in public API reviewed (if applicable)
The committers listed above are authorized under a signed CLA.
- :white_check_mark: login: Sufflope / name: Jean-Sébastien Bour (91235f23db99104270205771dbee54c553a9fd67)
Thanks for the PR. Correct me, this derive attribute conversion would be useful only for the metrics, as we would be using tokio-tracing for logs, and possibly for traces too. May be I am not to able to visualize the use-case properly, the concern is whether the use-case is generic enough to extend the OpenTelemetry API surface (which we want to keep minimal). If not, we can also have this crate within contrib repo, without having an optional dependency of it within opentelemetry crate. The application which wants these conversions can in that case have dependency to both opentelemetry and opentelemetry-derive crates.
Good to get more feedbacks :)
I'd also suggest to hold off from adding new public API without sufficient demand. We are trying to keep API as lean as possible, to keep the scope for GA release minimum. Happy to add more things post initial stable release based on demand. Meanwhile, we are happy to accept additional components to the contrib repo. We can observe https://github.com/open-telemetry/opentelemetry-rust/issues/1685 for more feedbacks while we wait.
this derive attribute conversion would be useful only for the metrics, as we would be using tokio-tracing for logs, and possibly for traces too.
I admit I use otel for metrics, I don't know if the Key/Value related types are used in other contexts, but since they are in opentelemetry::common it's not clear they are restricted to metrics, I assumed they were quite transversal :).
May be I am not to able to visualize the use-case properly, the concern is whether the use-case is generic enough to extend the OpenTelemetry API surface (which we want to keep minimal). If not, we can also have this crate within contrib repo, without having an optional dependency of it within opentelemetry crate.
I implemented it this way because it is the usual Rust way. See e.g. serde with its derive feature where, if you enable it, you get both the type and the derive macro when you import De/Serialize. The optional dependency is due to the nature of proc-macro crates which must be separate, self-contained crates.
Making it another crate will require actually two crates (something like opentelemetry-macros and opentelemetry-macros-derive as per common conventions) and then
use opentelemetry::KeyValue;
use opentelemetry_macros::KeyValue;
if your code uses both the type and the macro.
But at the end of the day it is your call, I'll refactor it this way.
Closing the PR now. Lets observe the associate issue to gauge demand before adding anything to main repo. Okay to accept contribution to the contrib repo meanwhile.