Permalink
Browse files

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`
  • Loading branch information...
1 parent 77a31d6 commit ca4c9c0010bd9155b6d067b33865d849fe533628 @yurishkuro yurishkuro committed on GitHub Sep 7, 2016
Showing with 78 additions and 31 deletions.
  1. +2 −2 ext/tags.go
  2. +4 −2 mocktracer/mocktracer.go
  3. +38 −13 mocktracer/mocktracer_test.go
  4. +18 −4 mocktracer/propagation.go
  5. +16 −10 propagation.go
View
@@ -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
@@ -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
}
@@ -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"])
}
}
@@ -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
})
View
@@ -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
}
}

0 comments on commit ca4c9c0

Please sign in to comment.