Skip to content
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

fix http double headers #160

Closed
wants to merge 2 commits into from

Conversation

vetinari
Copy link
Contributor

@vetinari vetinari commented Sep 7, 2018

fix double headers when reusing the *http.Request …
because of opentracing/opentracing-go#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.

because of opentracing/opentracing-go#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
Copy link
Contributor Author

vetinari commented Sep 7, 2018

see also opentracing/opentracing-go#191

@joeblubaugh
Copy link
Contributor

@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
Copy link
Contributor

@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 (opentracing/opentracing-go#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
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants