Remove URL escaping from HTTPHeadersCarrier and add a test for it #113

Merged
merged 4 commits into from Sep 7, 2016

Projects

None yet

2 participants

@yurishkuro
Contributor

Support HTTPHeaders format in MockTracer with URL escaping

Addresses opentracing/opentracing.io#113

@yurishkuro yurishkuro Remove URL escaping from HTTPHeadersCarrier and add a test for it
Support HTTPHeaders format in MockTracer with URL escaping
3af5a0d
@bensigelman bensigelman commented on the diff Sep 6, 2016
mocktracer/propagation.go
@@ -52,7 +55,11 @@ func (t *TextMapPropagator) Inject(spanContext MockSpanContext, carrier interfac
writer.Set(mockTextMapIdsPrefix+"sampled", fmt.Sprint(spanContext.Sampled))
// Baggage:
for baggageKey, baggageVal := range spanContext.Baggage {
- writer.Set(mockTextMapBaggagePrefix+baggageKey, baggageVal)
+ safeVal := baggageVal
+ if t.HTTPHeaders {
+ safeVal = url.QueryEscape(baggageVal)
+ }
+ writer.Set(mockTextMapBaggagePrefix+baggageKey, safeVal)
@bensigelman
bensigelman Sep 6, 2016 Contributor

The keys are a problem, too. IMO, the only truly robust thing is to have the Tracer implementation completely control the keyspace. This either becomes ot-baggage-item-0, ot-baggage-item-1, etc, or we encode the entire lot as ot-baggage-map or similar. I prefer the latter, FWIW.

@yurishkuro
yurishkuro Sep 6, 2016 Contributor

agreed, but one thing at a time. The fact that HTTPHeadersCarrier was applying url-encoding was an actual bug in the API.

yurishkuro added some commits Sep 7, 2016
@yurishkuro yurishkuro go fmt 244b5c1
@yurishkuro yurishkuro Fix go vet complaint
76fa178
@yurishkuro
Contributor

@bensigelman ok to merge?

@yurishkuro
Contributor

also, this looks like v0.10.0, rather than v0.9.1 due to a change in the behavior.

@bensigelman bensigelman commented on an outdated diff Sep 7, 2016
mocktracer/mocktracer_test.go
@@ -111,23 +111,23 @@ func TestMockTracer_Propagation(t *testing.T) {
textCarrier := func() interface{} {
return opentracing.TextMapCarrier(make(map[string]string))
}
- textLen := func (c interface{}) int {
@bensigelman
bensigelman Sep 7, 2016 Contributor

(weird that go fmt didn't get applied to this?!)

@bensigelman bensigelman commented on the diff Sep 7, 2016
propagation.go
@@ -147,21 +146,14 @@ type HTTPHeadersCarrier http.Header
// Set conforms to the TextMapWriter interface.
func (c HTTPHeadersCarrier) Set(key, val string) {
@bensigelman
bensigelman Sep 7, 2016 Contributor

github won't let me comment on line 144, but I think we ought to include a quick example of how HTTPHeadersCarrier should be used in practice; or, alternatively, point to the examples in Tracer.Inject and Tracer.Extract.

@bensigelman
Contributor

Sure, it's an improvement, let's 🚢 it. Thanks for fixing.

I do want the various Tracer impls of the world to do something smarter about HTTP headers, but that's not a core OpenTracing-Go thing (and this is).

@yurishkuro yurishkuro Add usage examples
84b0296
@yurishkuro yurishkuro merged commit ca4c9c0 into master Sep 7, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@yurishkuro
Contributor

also, this looks like v0.10.0, rather than v0.9.1 due to a change in the behavior.

@bensigelman thoughts on ^^^?

@bensigelman
Contributor

@yurishkuro I agree about v0.10.0, yeah...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment