Skip to content
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

Add a derive feature to allow deriving conversions into common types #1686

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Sufflope
Copy link

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 Sufflope requested a review from a team as a code owner April 28, 2024 16:07
Copy link

linux-foundation-easycla bot commented Apr 28, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Sufflope / name: Jean-Sébastien Bour (91235f2)

@lalitb
Copy link
Member

lalitb commented Apr 29, 2024

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

@cijothomas
Copy link
Member

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 #1685 for more feedbacks while we wait.

Copy link
Member

@cijothomas cijothomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sufflope
Copy link
Author

Sufflope commented May 7, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: add derives for Key/Value conversions
3 participants