-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[bridge/ot] Fall-back to TextMap carrier when it's not ot.HttpHeaders #3679
Conversation
Signed-off-by: Yuri Shkuro <github@ysh.us>
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #3679 +/- ##
=====================================
Coverage 79.6% 79.6%
=====================================
Files 171 171
Lines 12657 12661 +4
=====================================
+ Hits 10085 10089 +4
Misses 2359 2359
Partials 213 213
|
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.
It looks like this assumes that if the format is HTTPHeaders
and we fall back to the TextMapCarrier, the backing type will do the appropriate URL encoding. I would suggest we document this edge case.
@MadVikingGod it's a good question. I don't see how OTEL handles this in general since it does not have a dedicated HTTP propagator. In OT it was the responsibility of the Tracer to encode values for http before passing them to the carrier. While it could be made a responsibility of the carrier in OTEL, I don't see it being done in OTEL. This is how r = r.WithContext(ctx)
span.SetAttributes(semconv.HTTPClientAttributesFromHTTPRequest(r)...)
t.propagators.Inject(ctx, propagation.HeaderCarrier(r.Header)) Here Where does the encoding of values happen in OTEL? |
|
can this be merged? |
Resolves #3678