search

lightstep / lightstep-tracer-go

The Lightstep distributed tracing library for Go
https://lightstep.com
MIT License
98 stars 54 forks source link

fix http double headers #160

Closed vetinari closed 6 years ago

vetinari commented 6 years ago

fix double headers when reusing the http.Request … because of https://github.com/opentracing/opentracing-go/issues/159 the headers in a reused http.Request are appended instead of set which results in headers like this after injecting the current span context:

ot-tracer-spanid: 911775291efa49de,46fbd688b66dc96d
ot-tracer-traceid: 24080003fd414b3d,24080003fd414b3d
ot-tracer-sampled: true,true

With these double headers at least the java client is unable to extract the current span context.

vetinari commented 6 years ago

see also https://github.com/opentracing/opentracing-go/pull/191

joeblubaugh commented 6 years ago

@vetinari thanks for the PR. Would you also like to file an issue against the Java repository? It's an open question to me how we should handle a list in the header format, but crashing definitely isn't it.

MatthewDolan commented 6 years ago

@vetinari Apologies if I am mis-reading this, but I think this change is a no-op in that it doesn't resolve the issue you pointed out.

To re-iterate what I am seeing, httpHeadersPropagator.Inject is a copy of textMapPropagator.Inject in that they both call Set(...) except that in httpHeadersPropagator the carrier is cast to an implementation (opentracing.HTTPHeadersCarrier) and in textMapPropagator the carrier is cast to a interface (opentracing.TextMapWriter). Since they both call Set(...) the same code will get run regardless.

The issue you pointed out is real and should be resolved by (https://github.com/opentracing/opentracing-go/pull/191). By that I mean, if you use a version of opentracing-go after that merge, you shouldn't see the double values in the headers anymore.

Does that make sense, @vetinari ?

MatthewDolan commented 6 years ago

I went back and realized that we do have a Gopkg.lock file. We should update that file to go beyond opentracing/opentracing-go#191, let me dig in deeper here.