Ensure outbound tracing headers take precedence #683
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #683 +/- ##
==========================================
+ Coverage 86.55% 87.24% +0.68%
==========================================
Files 39 39
Lines 3958 3959 +1
==========================================
+ Hits 3426 3454 +28
+ Misses 410 386 -24
+ Partials 122 119 -3
Continue to review full report at Codecov.
|
Signed-off-by: Yuri Shkuro <ys@uber.com>
|
keep forgetting that |
| // Some applications propagate all inbound application headers to outbound calls (issue #682). | ||
| // If those headers include tracing headers we want to make sure to keep the new tracing headers. | ||
| if _, ok := newHeaders[k]; !ok { | ||
| newHeaders[k] = v |
There was a problem hiding this comment.
Do you know why we didn't get coverage from the added test here?
There was a problem hiding this comment.
Don't know. When I run this locally it shows the line as covered:
$ go test -v -run TestTracingSpanAttributes -coverprofile cover.out
=== RUN TestTracingSpanAttributes
--- PASS: TestTracingSpanAttributes (0.00s)
PASS
coverage: 48.8% of statements
ok github.com/uber/tchannel-go 0.042s
$ go tool cover -html=cover.out
I added another check to ensure that non-tracing headers are propagated.
tracing_test.go
Outdated
| customAppHeaderKey = "futurama" | ||
| customAppHeaderExpectedValue = "simpsons" | ||
| ) | ||
| var customAppHeaderValue atomic.Value |
There was a problem hiding this comment.
we use uber-go/atomic in tchannel, which has a atomic.String that you could use here
tracing_test.go
Outdated
| // will override them (https://github.com/uber/tchannel-go/issues/682). | ||
| headers := make(map[string]string) | ||
| assert.NoError(t, tracer.Inject(span.Context(), opentracing.TextMap, opentracing.TextMapCarrier(headers))) | ||
| tracingHeaders := map[string]string{ |
There was a problem hiding this comment.
should these be named the opposite? this seems like headers, while the thing named headers seems more like tracingHeaders
tracing_test.go
Outdated
| customAppHeaderKey: customAppHeaderExpectedValue, | ||
| } | ||
| for k, v := range headers { | ||
| tracingHeaders["$tracing$"+k] = v |
There was a problem hiding this comment.
can you set the value to something incorrect, like dummy so we know that it was replaced
|
Thanks Yuri for fixing this! Do we have a plan to release? |
|
@Yu-Xie This is now released in the latest release: |
|
Thanks @prashantv |
Resolves #682
Signed-off-by: Yuri Shkuro ys@uber.com
cc @Yu-Xie