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

* Remove URL escaping from `HTTPHeadersCarrier` and add a test for it
* Support `HTTPHeaders` format in `MockTracer` with URL escaping
* Fix `go vet` complaints
* Add usage examples for `HTTPHeadersCarrier`

ext/tags.go

@@ -24,12 +24,12 @@ var (
 	// SpanKindRPCClient marks a span representing the client-side of an RPC
 	// or other remote call
 	SpanKindRPCClientEnum = SpanKindEnum("client")
-	SpanKindRPCClient     = opentracing.Tag{string(SpanKind), SpanKindRPCClientEnum}
+	SpanKindRPCClient     = opentracing.Tag{Key: string(SpanKind), Value: SpanKindRPCClientEnum}
 
 	// SpanKindRPCServer marks a span representing the server-side of an RPC
 	// or other remote call
 	SpanKindRPCServerEnum = SpanKindEnum("server")
-	SpanKindRPCServer     = opentracing.Tag{string(SpanKind), SpanKindRPCServerEnum}
+	SpanKindRPCServer     = opentracing.Tag{Key: string(SpanKind), Value: SpanKindRPCServerEnum}
 
 	//////////////////////////////////////////////////////////////////////
 	// Component name

mocktracer/mocktracer.go

@@ -19,8 +19,10 @@ func New() *MockTracer {
 	textPropagator := new(TextMapPropagator)
 	t.RegisterInjector(opentracing.TextMap, textPropagator)
 	t.RegisterExtractor(opentracing.TextMap, textPropagator)
-	t.RegisterInjector(opentracing.HTTPHeaders, textPropagator)
-	t.RegisterExtractor(opentracing.HTTPHeaders, textPropagator)
+
+	httpPropagator := &TextMapPropagator{HTTPHeaders: true}
+	t.RegisterInjector(opentracing.HTTPHeaders, httpPropagator)
+	t.RegisterExtractor(opentracing.HTTPHeaders, httpPropagator)
 
 	return t
 }

mocktracer/mocktracer_test.go

@@ -1,6 +1,7 @@
 package mocktracer
 
 import (
+	"net/http"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -95,28 +96,48 @@ func TestMockSpan_Logs(t *testing.T) {
 	span.LogEventWithPayload("y", "z")
 	span.Log(opentracing.LogData{Event: "a"})
 	span.FinishWithOptions(opentracing.FinishOptions{
-		BulkLogData: []opentracing.LogData{opentracing.LogData{Event: "f"}}})
+		BulkLogData: []opentracing.LogData{{Event: "f"}}})
 	spans := tracer.FinishedSpans()
 	assert.Equal(t, 1, len(spans))
 	assert.Equal(t, []opentracing.LogData{
-		opentracing.LogData{Event: "x"},
-		opentracing.LogData{Event: "y", Payload: "z"},
-		opentracing.LogData{Event: "a"},
-		opentracing.LogData{Event: "f"},
+		{Event: "x"},
+		{Event: "y", Payload: "z"},
+		{Event: "a"},
+		{Event: "f"},
 	}, spans[0].Logs())
 }
 
 func TestMockTracer_Propagation(t *testing.T) {
+	textCarrier := func() interface{} {
+		return opentracing.TextMapCarrier(make(map[string]string))
+	}
+	textLen := func(c interface{}) int {
+		return len(c.(opentracing.TextMapCarrier))
+	}
+
+	httpCarrier := func() interface{} {
+		httpHeaders := http.Header(make(map[string][]string))
+		return opentracing.HTTPHeadersCarrier(httpHeaders)
+	}
+	httpLen := func(c interface{}) int {
+		return len(c.(opentracing.HTTPHeadersCarrier))
+	}
+
 	tests := []struct {
 		sampled bool
+		format  opentracing.BuiltinFormat
+		carrier func() interface{}
+		len     func(interface{}) int
 	}{
-		{true},
-		{false},
+		{sampled: true, format: opentracing.TextMap, carrier: textCarrier, len: textLen},
+		{sampled: false, format: opentracing.TextMap, carrier: textCarrier, len: textLen},
+		{sampled: true, format: opentracing.HTTPHeaders, carrier: httpCarrier, len: httpLen},
+		{sampled: false, format: opentracing.HTTPHeaders, carrier: httpCarrier, len: httpLen},
 	}
 	for _, test := range tests {
 		tracer := New()
 		span := tracer.StartSpan("x")
-		span.SetBaggageItem("x", "y")
+		span.SetBaggageItem("x", "y:z") // colon should be URL encoded as %3A
 		if !test.sampled {
 			ext.SamplingPriority.Set(span, 0)
 		}
@@ -127,22 +148,26 @@ func TestMockTracer_Propagation(t *testing.T) {
 		assert.Equal(t, opentracing.ErrInvalidCarrier,
 			tracer.Inject(span.Context(), opentracing.TextMap, span))
 
-		carrier := make(map[string]string)
+		carrier := test.carrier()
 
-		err := tracer.Inject(span.Context(), opentracing.TextMap, opentracing.TextMapCarrier(carrier))
+		err := tracer.Inject(span.Context(), test.format, carrier)
 		require.NoError(t, err)
-		assert.Equal(t, 4, len(carrier), "expect baggage + 2 ids + sampled")
+		assert.Equal(t, 4, test.len(carrier), "expect baggage + 2 ids + sampled")
+		if test.format == opentracing.HTTPHeaders {
+			c := carrier.(opentracing.HTTPHeadersCarrier)
+			assert.Equal(t, "y%3Az", c["Mockpfx-Baggage-X"][0])
+		}
 
 		_, err = tracer.Extract(opentracing.Binary, nil)
 		assert.Equal(t, opentracing.ErrUnsupportedFormat, err)
 		_, err = tracer.Extract(opentracing.TextMap, tracer)
 		assert.Equal(t, opentracing.ErrInvalidCarrier, err)
 
-		extractedContext, err := tracer.Extract(opentracing.TextMap, opentracing.TextMapCarrier(carrier))
+		extractedContext, err := tracer.Extract(test.format, carrier)
 		require.NoError(t, err)
 		assert.Equal(t, mSpan.SpanContext.TraceID, extractedContext.(MockSpanContext).TraceID)
 		assert.Equal(t, mSpan.SpanContext.SpanID, extractedContext.(MockSpanContext).SpanID)
 		assert.Equal(t, test.sampled, extractedContext.(MockSpanContext).Sampled)
-		assert.Equal(t, "y", extractedContext.(MockSpanContext).Baggage["x"])
+		assert.Equal(t, "y:z", extractedContext.(MockSpanContext).Baggage["x"])
 	}
 }

mocktracer/propagation.go

@@ -2,6 +2,7 @@ package mocktracer
 
 import (
 	"fmt"
+	"net/url"
 	"strconv"
 	"strings"
 
@@ -37,8 +38,10 @@ type Extractor interface {
 	Extract(carrier interface{}) (MockSpanContext, error)
 }
 
-// TextMapPropagator implements Injector/Extractor for TextMap format.
-type TextMapPropagator struct{}
+// TextMapPropagator implements Injector/Extractor for TextMap and HTTPHeaders formats.
+type TextMapPropagator struct {
+	HTTPHeaders bool
+}
 
 // Inject implements the Injector interface
 func (t *TextMapPropagator) Inject(spanContext MockSpanContext, carrier interface{}) error {
@@ -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)
 	}
 	return nil
 }
@@ -92,7 +99,14 @@ func (t *TextMapPropagator) Extract(carrier interface{}) (MockSpanContext, error
 			if rval.Baggage == nil {
 				rval.Baggage = make(map[string]string)
 			}
-			rval.Baggage[lowerKey[len(mockTextMapBaggagePrefix):]] = val
+			safeVal := val
+			if t.HTTPHeaders {
+				// unescape errors are ignored, nothing can be done
+				if rawVal, err := url.QueryUnescape(val); err == nil {
+					safeVal = rawVal
+				}
+			}
+			rval.Baggage[lowerKey[len(mockTextMapBaggagePrefix):]] = safeVal
 		}
 		return nil
 	})

propagation.go

@@ -3,7 +3,6 @@ package opentracing
 import (
 	"errors"
 	"net/http"
-	"net/url"
 )
 
 ///////////////////////////////////////////////////////////////////////////////
@@ -142,26 +141,33 @@ func (c TextMapCarrier) Set(key, val string) {
 }
 
 // HTTPHeadersCarrier satisfies both TextMapWriter and TextMapReader.
+//
+// Example usage for server side:
+//
+//     carrier := opentracing.HttpHeadersCarrier(httpReq.Header)
+//     spanContext, err := tracer.Extract(opentracing.HttpHeaders, carrier)
+//
+// Example usage for client side:
+//
+//     carrier := opentracing.HTTPHeadersCarrier(httpReq.Header)
+//     err := tracer.Inject(
+//         span.Context(),
+//         opentracing.HttpHeaders,
+//         carrier)
+//
 type HTTPHeadersCarrier http.Header
 
 // Set conforms to the TextMapWriter interface.
 func (c HTTPHeadersCarrier) Set(key, val string) {
 	h := http.Header(c)
-	h.Add(key, url.QueryEscape(val))
+	h.Add(key, val)
 }
 
 // ForeachKey conforms to the TextMapReader interface.
 func (c HTTPHeadersCarrier) ForeachKey(handler func(key, val string) error) error {
 	for k, vals := range c {
 		for _, v := range vals {
-			rawV, err := url.QueryUnescape(v)
-			if err != nil {
-				// We don't know if there was an error escaping an
-				// OpenTracing-related header or something else; as such, we
-				// continue rather than return the error.
-				continue
-			}
-			if err = handler(k, rawV); err != nil {
+			if err := handler(k, v); err != nil {
 				return err
 			}
 		}