OpenTracing Bridge: allow more generic carriers#2141
OpenTracing Bridge: allow more generic carriers#2141bboreham wants to merge 6 commits intoopen-telemetry:mainfrom
Conversation
bboreham
requested review from
Aneurysm9,
MadVikingGod,
MrAlias,
XSAM,
dashpole,
evantorrie,
jmacd,
paivagustavo and
pellared
as code owners
Codecov Report
@@ Coverage Diff @@
## main #2141 +/- ##
=======================================
+ Coverage 75.7% 75.9% +0.2%
=======================================
Files 178 178
Lines 11703 11740 +37
=======================================
+ Hits 8864 8922 +58
+ Misses 2614 2584 -30
- Partials 225 234 +9
|
|
I can confirm this fixes our issue, propagation context can be injected and extracted without error now 🙂 |
MrAlias
left a comment
MrAlias
left a comment
There was a problem hiding this comment.
Thanks for the contribution, it is definitely welcomed as we ultimately want to support more than just HTTP carriers for the OT bridge.
These changes need to include tests. Primarily the tests need to ensure behavior of the introduced types are preserved.
|
Ping @bboreham. Rebase needed and some feedback to look at. |
|
Ack. It hasn't been high enough up my priority list lately. |
Instead of insisting that the carrier is an HTTPHeaders, cast it or adapt it to the interface we need - TextMapCarrier. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Suggested by Tyler Yahn <MrAlias@users.noreply.github.com> Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
Values stored in http.Header get title-cased, i.e. `traceparent` will turn into `Traceparent`. Since HTTPHeadersCarrier.ForeachKey does not undo this change, special-case that one type with a different wrapper that will allow the value to be fetched. Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
bboreham
force-pushed
the
ot-bridge-carrier
branch
from
f73fe19 to
fcb9b2a
Compare
This is not currently used in normal functioning of Inject and Extract, but might get used in future so give it some testing now. Test adapted from `open-telemetry/opentelemetry-go/propagation/propagation_test.go` Signed-off-by: Bryan Boreham <bjboreham@gmail.com>
bboreham
force-pushed
the
ot-bridge-carrier
branch
from
205fd52 to
344c66b
Compare
|
Rebased and added unit tests for the Inject/Extract functionality added in this PR. Note that It appears baggage was never propagated by |
|
Does #2911 meet the needs here, or is there more required? |
Strictly no, however I think #2911 makes it easier to work around the deficiency.
Do you have any response to #2141 (comment) ? |
|
@MrAlias Do you have any further feedback for this PR please? |
| return ot.ErrUnsupportedFormat | ||
| } | ||
| hhcarrier, ok := carrier.(ot.HTTPHeadersCarrier) | ||
| // If carrier implements the required interface directly, use that |
There was a problem hiding this comment.
| // If carrier implements the required interface directly, use that | |
| // If carrier implements the required interface directly, use that. |
| tmCarrier, ok := carrier.(propagation.TextMapCarrier) | ||
| if !ok { | ||
| return ot.ErrInvalidCarrier | ||
| tmWriter, ok := carrier.(ot.TextMapWriter) // otherwise see if we can wrap it |
There was a problem hiding this comment.
| tmWriter, ok := carrier.(ot.TextMapWriter) // otherwise see if we can wrap it | |
| tmWriter, ok := carrier.(ot.TextMapWriter) // Otherwise, see if we can wrap it. |
|
Closing this as stale. |
|
Hey @bboreham, if you stop working on this, can I take over it? |
8 participants
Instead of insisting that the carrier is an
HTTPHeaders, cast it or adapt it to the interface we need -TextMapCarrier.It's a bit long-winded to wrap the read and write side separately, but this gives maximum flexibility.Fixes #2137 - @kvrhdn has used this fork to create a working program.