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

feat: implement FromStr and Display for Protocol #2758

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

davidhewitt
Copy link

Changes

Implements Display and FromStr for Protocol, using the values documented in https://opentelemetry.io/docs/languages/sdk-configuration/otlp-exporter/#otel_exporter_otlp_protocol

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)

@davidhewitt davidhewitt requested a review from a team as a code owner March 5, 2025 14:43
Copy link

linux-foundation-easycla bot commented Mar 5, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: lalitb / name: Lalit Kumar Bhasin (82a6d3d)
  • ✅ login: davidhewitt / name: David Hewitt (e23f7c9, e6711a2)

Copy link

codecov bot commented Mar 5, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.6%. Comparing base (b33f0cc) to head (e23f7c9).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2758   +/-   ##
=====================================
  Coverage   79.6%   79.6%           
=====================================
  Files        123     123           
  Lines      23034   23061   +27     
=====================================
+ Hits       18356   18379   +23     
- Misses      4678    4682    +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@davidhewitt
Copy link
Author

Possibly interesting to note that the Serialize and Deserialize values are automatically derived and will not match these.

(Perhaps the serde implementations should be changed to use these constants? They seem to be universal across all OTEL SDKs. I am happy to do that as a follow-up.)

Copy link
Member

@gruebel gruebel left a comment

Choose a reason for hiding this comment

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

nice work 🍻 can you also add a short changelog entry for it? Regarding the serde part, it would be great to be consistent with other SDKs.

}
}

impl FromStr for Protocol {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? Why would a user have this awkward construct:

.with_protocol("grpc".parse::<Protocol>().unwrap())

when they could simply do this:

.with_protocol(Protocol::Grpc)

Copy link
Author

Choose a reason for hiding this comment

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

Completely agree if hard coded then users should use the enum variant.

I am working on a downstream SDK which uses opentelemetry, tracing and log. We configure an otel exporter and are working to support setting the protocol via environment, which is not yet supported by this library (although in future perhaps I can also contribute a solution to that issue).

Other downstream packages to set up an exporter, like init-tracing-opentelemetry, also have similar code doing this conversion.

It feels most correct to have the FromStr implementation here in the library for all downstream implementations to re-use. This is also how the Compression type is already parsed from env within this library.

@@ -396,7 +406,53 @@ pub enum Protocol {
HttpJson,
}

impl Display for Protocol {
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the motivation behind adding a Display implementation for Protocol?

Copy link
Author

Choose a reason for hiding this comment

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

After seeing the Compression implementation within this library had both FromStr and Display, I felt it was most correct to add Display to match the FromStr here too.

If you have concerns, I am happy to remove it - the FromStr is the functionality I need at present (as per above comment).

@davidhewitt
Copy link
Author

davidhewitt commented Mar 6, 2025

nice work 🍻 can you also add a short changelog entry for it?

Done, please let me know if you want it adjusted.

@davidhewitt davidhewitt closed this Mar 6, 2025
@davidhewitt davidhewitt reopened this Mar 6, 2025
@davidhewitt
Copy link
Author

Sorry, fat finger error led to closing PR by mistake 🤦

@davidhewitt
Copy link
Author

Regarding the serde part, it would be great to be consistent with other SDKs.

I opened #2765 for this as a separate PR, because it's potentially breaking and deserves its own visibility / changelog note.

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.

4 participants