-
Notifications
You must be signed in to change notification settings - Fork 599
feat: implement FromStr and Display for Protocol
#2758
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
base: main
Are you sure you want to change the base?
feat: implement FromStr and Display for Protocol
#2758
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. 🚀 New features to boost your workflow:
|
|
Possibly interesting to note that the (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.) |
gruebel
left a comment
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.
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 { |
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.
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)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.
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.
| HttpJson, | ||
| } | ||
|
|
||
| impl Display for Protocol { |
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.
What's the motivation behind adding a Display implementation for Protocol?
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.
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).
Done, please let me know if you want it adjusted. |
|
Sorry, fat finger error led to closing PR by mistake 🤦 |
I opened #2765 for this as a separate PR, because it's potentially breaking and deserves its own visibility / changelog note. |
Changes
Implements
DisplayandFromStrforProtocol, using the values documented in https://opentelemetry.io/docs/languages/sdk-configuration/otlp-exporter/#otel_exporter_otlp_protocolMerge requirement checklist
CHANGELOG.mdfiles updated for non-trivial, user-facing changes