-
Notifications
You must be signed in to change notification settings - Fork 501
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
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.) |
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.
@@ -396,7 +406,53 @@ pub enum Protocol { | |||
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
Display
andFromStr
forProtocol
, using the values documented in https://opentelemetry.io/docs/languages/sdk-configuration/otlp-exporter/#otel_exporter_otlp_protocolMerge requirement checklist
CHANGELOG.md
files updated for non-trivial, user-facing changes