opentelemetry-rust icon indicating copy to clipboard operation
opentelemetry-rust copied to clipboard

Add a derive feature to allow deriving conversions into common types

Open Sufflope opened this issue 1 year ago • 4 comments
trafficstars

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.md files updated for non-trivial, user-facing changes
  • [ ] Changes in public API reviewed (if applicable)

Sufflope avatar Apr 28 '24 16:04 Sufflope

CLA Signed

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 :)

lalitb avatar Apr 29 '24 17:04 lalitb

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.

cijothomas avatar Apr 30 '24 17:04 cijothomas

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.

Sufflope avatar May 07 '24 13:05 Sufflope

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.

cijothomas avatar Aug 09 '24 17:08 cijothomas