From 9e9989318ebf1450f65249d234cb88849779212b Mon Sep 17 00:00:00 2001 From: Ben Sigelman Date: Thu, 9 Jun 2016 18:42:59 -0700 Subject: [PATCH 1/3] Illustrate the ramifications of ot.SpanContext Benchmark diffs: ``` bhs@bhs-personal:~/git/lightstep/go/src$ ../bin/benchcmp /tmp/before /tmp/after benchmark old ns/op new ns/op delta BenchmarkSpan_Empty-8 629 632 +0.48% BenchmarkSpan_100Events-8 35847 35271 -1.61% BenchmarkSpan_1000Events-8 35587 35173 -1.16% BenchmarkSpan_100Tags-8 43618 44145 +1.21% BenchmarkSpan_1000Tags-8 44063 43838 -0.51% BenchmarkSpan_100BaggageItems-8 145397 32579 -77.59% BenchmarkTrimmedSpan_100Events_100Tags_100BaggageItems-8 168957 79144 -53.16% BenchmarkInject_TextMap_Empty-8 2221 2292 +3.20% BenchmarkInject_TextMap_100BaggageItems-8 85977 87857 +2.19% BenchmarkInject_Binary_Empty-8 388 390 +0.52% BenchmarkInject_Binary_100BaggageItems-8 17861 18968 +6.20% BenchmarkJoin_TextMap_Empty-8 2849 2872 +0.81% BenchmarkJoin_TextMap_100BaggageItems-8 148235 146900 -0.90% BenchmarkJoin_Binary_Empty-8 1175 1231 +4.77% BenchmarkJoin_Binary_100BaggageItems-8 37424 37700 +0.74% ``` I made these changes pretty casually and wonder if the huge improvements on the BaggageItems benchmarks are due to some sort of measurement error in the test itself (i.e., an invalid result). --- bench_test.go | 23 +++--- concurrency_test.go | 15 ++-- context.go | 56 +++++++++++-- examples/dapperish.go | 16 ++-- examples/dapperish/trivialrecorder.go | 2 +- propagation.go | 63 ++++++--------- propagation_ot.go | 110 ++++++++++---------------- propagation_test.go | 15 ++-- raw.go | 13 +-- recorder_test.go | 8 +- span.go | 50 +----------- span_test.go | 12 +-- tracer.go | 77 +++++++++++------- 13 files changed, 223 insertions(+), 237 deletions(-) diff --git a/bench_test.go b/bench_test.go index 22c6951..6873ffe 100644 --- a/bench_test.go +++ b/bench_test.go @@ -26,7 +26,7 @@ func executeOps(sp opentracing.Span, numEvent, numTag, numItems int) { sp.SetTag(tags[j], nil) } for j := 0; j < numItems; j++ { - sp.SetBaggageItem(tags[j], tags[j]) + sp.Context().SetBaggageItem(tags[j], tags[j]) } } @@ -108,14 +108,14 @@ func benchmarkInject(b *testing.B, format opentracing.BuiltinFormat, numItems in } b.ResetTimer() for i := 0; i < b.N; i++ { - err := tracer.Inject(sp, format, carrier) + err := tracer.Inject(sp.Context(), format, carrier) if err != nil { b.Fatal(err) } } } -func benchmarkJoin(b *testing.B, format opentracing.BuiltinFormat, numItems int) { +func benchmarkExtract(b *testing.B, format opentracing.BuiltinFormat, numItems int) { var r CountingRecorder tracer := New(&r) sp := tracer.StartSpan("testing") @@ -129,12 +129,12 @@ func benchmarkJoin(b *testing.B, format opentracing.BuiltinFormat, numItems int) default: b.Fatalf("unhandled format %d", format) } - if err := tracer.Inject(sp, format, carrier); err != nil { + if err := tracer.Inject(sp.Context(), format, carrier); err != nil { b.Fatal(err) } - // We create a new bytes.Buffer every time for tracer.Join() to keep this - // benchmark realistic. + // We create a new bytes.Buffer every time for tracer.Extract() to keep + // this benchmark realistic. var rawBinaryBytes []byte if format == opentracing.Binary { rawBinaryBytes = carrier.(*bytes.Buffer).Bytes() @@ -144,11 +144,10 @@ func benchmarkJoin(b *testing.B, format opentracing.BuiltinFormat, numItems int) if format == opentracing.Binary { carrier = bytes.NewBuffer(rawBinaryBytes) } - sp, err := tracer.Join("benchmark", format, carrier) + _, err := tracer.Extract(format, carrier) if err != nil { b.Fatal(err) } - sp.Finish() // feed back into buffer pool } } @@ -169,17 +168,17 @@ func BenchmarkInject_Binary_100BaggageItems(b *testing.B) { } func BenchmarkJoin_TextMap_Empty(b *testing.B) { - benchmarkJoin(b, opentracing.TextMap, 0) + benchmarkExtract(b, opentracing.TextMap, 0) } func BenchmarkJoin_TextMap_100BaggageItems(b *testing.B) { - benchmarkJoin(b, opentracing.TextMap, 100) + benchmarkExtract(b, opentracing.TextMap, 100) } func BenchmarkJoin_Binary_Empty(b *testing.B) { - benchmarkJoin(b, opentracing.Binary, 0) + benchmarkExtract(b, opentracing.Binary, 0) } func BenchmarkJoin_Binary_100BaggageItems(b *testing.B) { - benchmarkJoin(b, opentracing.Binary, 100) + benchmarkExtract(b, opentracing.Binary, 100) } diff --git a/concurrency_test.go b/concurrency_test.go index 267ea99..48c8b8c 100644 --- a/concurrency_test.go +++ b/concurrency_test.go @@ -83,11 +83,11 @@ func TestConcurrentUsage(t *testing.T) { sp := tracer.StartSpan(op) sp.LogEvent("test event") sp.SetTag("foo", "bar") - sp.SetBaggageItem("boo", "far") + sp.Context().SetBaggageItem("boo", "far") sp.SetOperationName("x") - csp := tracer.StartSpanWithOptions(opentracing.StartSpanOptions{ - Parent: sp, - }) + csp := tracer.StartSpan( + "csp", + opentracing.ChildOf(sp.Context())) csp.Finish() defer sp.Finish() } @@ -105,9 +105,8 @@ func TestDisableSpanPool(t *testing.T) { parent := tracer.StartSpan("parent") parent.Finish() // This shouldn't panic. - child := tracer.StartSpanWithOptions(opentracing.StartSpanOptions{ - Parent: parent, - OperationName: "child", - }) + child := tracer.StartSpan( + "child", + opentracing.ChildOf(parent.Context())) child.Finish() } diff --git a/context.go b/context.go index 18159c2..8fdb194 100644 --- a/context.go +++ b/context.go @@ -1,16 +1,62 @@ package basictracer -// Context holds the basic Span metadata. -type Context struct { +import ( + "sync" + + "github.com/opentracing/opentracing-go" +) + +// SpanContext holds the basic Span metadata. +type SpanContext struct { // A probabilistically unique identifier for a [multi-span] trace. TraceID uint64 // A probabilistically unique identifier for a span. SpanID uint64 - // The SpanID of this Context's parent, or 0 if there is no parent. - ParentSpanID uint64 - // Whether the trace is sampled. Sampled bool + + // The span's associated baggage. + baggageLock sync.Mutex + Baggage map[string]string // initialized on first use +} + +// BaggageItem belongs to the opentracing.SpanContext interface +func (c *SpanContext) BaggageItem(key string) string { + // TODO: if we want to support onBaggage, need a pointer to the bt.Span. + // s.onBaggage(canonicalKey, val) + // if s.trim() { + // return s + // } + + c.baggageLock.Lock() + defer c.baggageLock.Unlock() + + if c.Baggage == nil { + return "" + } + return c.Baggage[key] +} + +// SetBaggageItem belongs to the opentracing.SpanContext interface +func (c *SpanContext) SetBaggageItem(key, val string) opentracing.SpanContext { + c.baggageLock.Lock() + defer c.baggageLock.Unlock() + if c.Baggage == nil { + c.Baggage = make(map[string]string) + } + c.Baggage[key] = val + return c +} + +// ForeachBaggageItem belongs to the opentracing.SpanContext interface +func (c *SpanContext) ForeachBaggageItem(handler func(k, v string) bool) { + c.baggageLock.Lock() + defer c.baggageLock.Unlock() + for k, v := range c.Baggage { + if !handler(k, v) { + break + } + } } diff --git a/examples/dapperish.go b/examples/dapperish.go index d306e05..042f8a6 100644 --- a/examples/dapperish.go +++ b/examples/dapperish.go @@ -11,17 +11,20 @@ import ( "runtime" "strings" + "golang.org/x/net/context" + "github.com/opentracing/basictracer-go/examples/dapperish" opentracing "github.com/opentracing/opentracing-go" + "github.com/opentracing/opentracing-go/ext" ) func client() { reader := bufio.NewReader(os.Stdin) for { span := opentracing.StartSpan("getInput") - ctx := opentracing.BackgroundContextWithSpan(span) + ctx := opentracing.ContextWithSpan(context.Background(), span) // Make sure that global baggage propagation works. - span.SetBaggageItem("User", os.Getenv("USER")) + span.Context().SetBaggageItem("User", os.Getenv("USER")) span.LogEventWithPayload("ctx", ctx) fmt.Print("\n\nEnter text (empty string to exit): ") text, _ := reader.ReadString('\n') @@ -36,7 +39,7 @@ func client() { httpClient := &http.Client{} httpReq, _ := http.NewRequest("POST", "http://localhost:8080/", bytes.NewReader([]byte(text))) textCarrier := opentracing.HTTPHeaderTextMapCarrier(httpReq.Header) - err := span.Tracer().Inject(span, opentracing.TextMap, textCarrier) + err := span.Tracer().Inject(span.Context(), opentracing.TextMap, textCarrier) if err != nil { panic(err) } @@ -54,11 +57,14 @@ func client() { func server() { http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { textCarrier := opentracing.HTTPHeaderTextMapCarrier(req.Header) - serverSpan, err := opentracing.GlobalTracer().Join( - "serverSpan", opentracing.TextMap, textCarrier) + wireSpanContext, err := opentracing.GlobalTracer().Extract( + opentracing.TextMap, textCarrier) if err != nil { panic(err) } + serverSpan := opentracing.GlobalTracer().StartSpan( + "serverSpan", + ext.RPCServerOption(wireSpanContext)) serverSpan.SetTag("component", "server") defer serverSpan.Finish() diff --git a/examples/dapperish/trivialrecorder.go b/examples/dapperish/trivialrecorder.go index 26ea8a1..2fb1f2e 100644 --- a/examples/dapperish/trivialrecorder.go +++ b/examples/dapperish/trivialrecorder.go @@ -35,7 +35,7 @@ func (t *TrivialRecorder) RecordSpan(span basictracer.RawSpan) { fmt.Printf( "RecordSpan: %v[%v, %v us] --> %v logs. std context: %v; baggage: %v\n", span.Operation, span.Start, span.Duration, len(span.Logs), - span.Context, span.Baggage) + span.SpanContext, span.Baggage) for i, l := range span.Logs { fmt.Printf( " log %v @ %v: %v --> %v\n", i, l.Timestamp, l.Event, reflect.TypeOf(l.Payload)) diff --git a/propagation.go b/propagation.go index 39b68ae..55e44e6 100644 --- a/propagation.go +++ b/propagation.go @@ -1,10 +1,6 @@ package basictracer -import ( - "time" - - opentracing "github.com/opentracing/opentracing-go" -) +import opentracing "github.com/opentracing/opentracing-go" type accessorPropagator struct { tracer *tracerImpl @@ -21,54 +17,45 @@ type DelegatingCarrier interface { } func (p *accessorPropagator) Inject( - sp opentracing.Span, + spanContext opentracing.SpanContext, carrier interface{}, ) error { - ac, ok := carrier.(DelegatingCarrier) - if !ok || ac == nil { + dc, ok := carrier.(DelegatingCarrier) + if !ok || dc == nil { return opentracing.ErrInvalidCarrier } - si, ok := sp.(*spanImpl) + sc, ok := spanContext.(*SpanContext) if !ok { - return opentracing.ErrInvalidSpan + return opentracing.ErrInvalidSpanContext } - meta := si.raw.Context - ac.SetState(meta.TraceID, meta.SpanID, meta.Sampled) - for k, v := range si.raw.Baggage { - ac.SetBaggageItem(k, v) + dc.SetState(sc.TraceID, sc.SpanID, sc.Sampled) + for k, v := range sc.Baggage { + dc.SetBaggageItem(k, v) } return nil } -func (p *accessorPropagator) Join( - operationName string, +func (p *accessorPropagator) Extract( carrier interface{}, -) (opentracing.Span, error) { - ac, ok := carrier.(DelegatingCarrier) - if !ok || ac == nil { +) (opentracing.SpanContext, error) { + dc, ok := carrier.(DelegatingCarrier) + if !ok || dc == nil { return nil, opentracing.ErrInvalidCarrier } - sp := p.tracer.getSpan() - ac.GetBaggage(func(k, v string) { - if sp.raw.Baggage == nil { - sp.raw.Baggage = map[string]string{} + traceID, spanID, sampled := dc.State() + sc := &SpanContext{ + TraceID: traceID, + SpanID: spanID, + Sampled: sampled, + Baggage: nil, + } + dc.GetBaggage(func(k, v string) { + if sc.Baggage == nil { + sc.Baggage = map[string]string{} } - sp.raw.Baggage[k] = v + sc.Baggage[k] = v }) - traceID, parentSpanID, sampled := ac.State() - sp.raw.Context = Context{ - TraceID: traceID, - SpanID: randomID(), - ParentSpanID: parentSpanID, - Sampled: sampled, - } - - return p.tracer.startSpanInternal( - sp, - operationName, - time.Now(), - nil, - ), nil + return sc, nil } diff --git a/propagation_ot.go b/propagation_ot.go index be7771e..3d13ef1 100644 --- a/propagation_ot.go +++ b/propagation_ot.go @@ -5,7 +5,6 @@ import ( "io" "strconv" "strings" - "time" "github.com/gogo/protobuf/proto" "github.com/opentracing/basictracer-go/wire" @@ -30,39 +29,38 @@ const ( ) func (p *textMapPropagator) Inject( - sp opentracing.Span, + spanContext opentracing.SpanContext, opaqueCarrier interface{}, ) error { - sc, ok := sp.(*spanImpl) + sc, ok := spanContext.(*SpanContext) if !ok { - return opentracing.ErrInvalidSpan + return opentracing.ErrInvalidSpanContext } carrier, ok := opaqueCarrier.(opentracing.TextMapWriter) if !ok { return opentracing.ErrInvalidCarrier } - carrier.Set(fieldNameTraceID, strconv.FormatUint(sc.raw.TraceID, 16)) - carrier.Set(fieldNameSpanID, strconv.FormatUint(sc.raw.SpanID, 16)) - carrier.Set(fieldNameSampled, strconv.FormatBool(sc.raw.Sampled)) + carrier.Set(fieldNameTraceID, strconv.FormatUint(sc.TraceID, 16)) + carrier.Set(fieldNameSpanID, strconv.FormatUint(sc.SpanID, 16)) + carrier.Set(fieldNameSampled, strconv.FormatBool(sc.Sampled)) - sc.Lock() - for k, v := range sc.raw.Baggage { + sc.baggageLock.Lock() + for k, v := range sc.Baggage { carrier.Set(prefixBaggage+k, v) } - sc.Unlock() + sc.baggageLock.Unlock() return nil } -func (p *textMapPropagator) Join( - operationName string, +func (p *textMapPropagator) Extract( opaqueCarrier interface{}, -) (opentracing.Span, error) { +) (opentracing.SpanContext, error) { carrier, ok := opaqueCarrier.(opentracing.TextMapReader) if !ok { return nil, opentracing.ErrInvalidCarrier } requiredFieldCount := 0 - var traceID, propagatedSpanID uint64 + var traceID, spanID uint64 var sampled bool var err error decodedBaggage := make(map[string]string) @@ -71,17 +69,17 @@ func (p *textMapPropagator) Join( case fieldNameTraceID: traceID, err = strconv.ParseUint(v, 16, 64) if err != nil { - return opentracing.ErrTraceCorrupted + return opentracing.ErrSpanContextCorrupted } case fieldNameSpanID: - propagatedSpanID, err = strconv.ParseUint(v, 16, 64) + spanID, err = strconv.ParseUint(v, 16, 64) if err != nil { - return opentracing.ErrTraceCorrupted + return opentracing.ErrSpanContextCorrupted } case fieldNameSampled: sampled, err = strconv.ParseBool(v) if err != nil { - return opentracing.ErrTraceCorrupted + return opentracing.ErrSpanContextCorrupted } default: lowercaseK := strings.ToLower(k) @@ -99,37 +97,26 @@ func (p *textMapPropagator) Join( } if requiredFieldCount < tracerStateFieldCount { if requiredFieldCount == 0 { - return nil, opentracing.ErrTraceNotFound + return nil, opentracing.ErrSpanContextNotFound } - return nil, opentracing.ErrTraceCorrupted + return nil, opentracing.ErrSpanContextCorrupted } - sp := p.tracer.getSpan() - sp.raw = RawSpan{ - Context: Context{ - TraceID: traceID, - SpanID: randomID(), - ParentSpanID: propagatedSpanID, - Sampled: sampled, - }, + return &SpanContext{ + TraceID: traceID, + SpanID: spanID, + Sampled: sampled, Baggage: decodedBaggage, - } - - return p.tracer.startSpanInternal( - sp, - operationName, - time.Now(), - nil, - ), nil + }, nil } func (p *binaryPropagator) Inject( - sp opentracing.Span, + spanContext opentracing.SpanContext, opaqueCarrier interface{}, ) error { - sc, ok := sp.(*spanImpl) + sc, ok := spanContext.(*SpanContext) if !ok { - return opentracing.ErrInvalidSpan + return opentracing.ErrInvalidSpanContext } carrier, ok := opaqueCarrier.(io.Writer) if !ok { @@ -137,10 +124,10 @@ func (p *binaryPropagator) Inject( } state := wire.TracerState{} - state.TraceId = sc.raw.TraceID - state.SpanId = sc.raw.SpanID - state.Sampled = sc.raw.Sampled - state.BaggageItems = sc.raw.Baggage + state.TraceId = sc.TraceID + state.SpanId = sc.SpanID + state.Sampled = sc.Sampled + state.BaggageItems = sc.Baggage b, err := proto.Marshal(&state) if err != nil { @@ -157,10 +144,9 @@ func (p *binaryPropagator) Inject( return err } -func (p *binaryPropagator) Join( - operationName string, +func (p *binaryPropagator) Extract( opaqueCarrier interface{}, -) (opentracing.Span, error) { +) (opentracing.SpanContext, error) { carrier, ok := opaqueCarrier.(io.Reader) if !ok { return nil, opentracing.ErrInvalidCarrier @@ -172,37 +158,25 @@ func (p *binaryPropagator) Join( // the exact amount of bytes into it. var length uint32 if err := binary.Read(carrier, binary.BigEndian, &length); err != nil { - return nil, opentracing.ErrTraceCorrupted + return nil, opentracing.ErrSpanContextCorrupted } buf := make([]byte, length) if n, err := carrier.Read(buf); err != nil { if n > 0 { - return nil, opentracing.ErrTraceCorrupted + return nil, opentracing.ErrSpanContextCorrupted } - return nil, opentracing.ErrTraceNotFound + return nil, opentracing.ErrSpanContextNotFound } ctx := wire.TracerState{} if err := proto.Unmarshal(buf, &ctx); err != nil { - return nil, opentracing.ErrTraceCorrupted + return nil, opentracing.ErrSpanContextCorrupted } - sp := p.tracer.getSpan() - sp.raw = RawSpan{ - Context: Context{ - TraceID: ctx.TraceId, - SpanID: randomID(), - ParentSpanID: ctx.SpanId, - Sampled: ctx.Sampled, - }, - } - - sp.raw.Baggage = ctx.BaggageItems - - return p.tracer.startSpanInternal( - sp, - operationName, - time.Now(), - nil, - ), nil + return &SpanContext{ + TraceID: ctx.TraceId, + SpanID: ctx.SpanId, + Sampled: ctx.Sampled, + Baggage: ctx.BaggageItems, + }, nil } diff --git a/propagation_test.go b/propagation_test.go index c4b7f76..724185b 100644 --- a/propagation_test.go +++ b/propagation_test.go @@ -13,7 +13,7 @@ import ( ) type verbatimCarrier struct { - basictracer.Context + basictracer.SpanContext b map[string]string } @@ -30,11 +30,11 @@ func (vc *verbatimCarrier) GetBaggage(f func(string, string)) { } func (vc *verbatimCarrier) SetState(tID, sID uint64, sampled bool) { - vc.Context = basictracer.Context{TraceID: tID, SpanID: sID, Sampled: sampled} + vc.SpanContext = basictracer.SpanContext{TraceID: tID, SpanID: sID, Sampled: sampled} } func (vc *verbatimCarrier) State() (traceID, spanID uint64, sampled bool) { - return vc.Context.TraceID, vc.Context.SpanID, vc.Context.Sampled + return vc.SpanContext.TraceID, vc.SpanContext.SpanID, vc.SpanContext.Sampled } func TestSpanPropagator(t *testing.T) { @@ -43,7 +43,7 @@ func TestSpanPropagator(t *testing.T) { tracer := basictracer.New(recorder) sp := tracer.StartSpan(op) - sp.SetBaggageItem("foo", "bar") + sp.Context().SetBaggageItem("foo", "bar") tmc := opentracing.HTTPHeaderTextMapCarrier(http.Header{}) tests := []struct { @@ -55,13 +55,16 @@ func TestSpanPropagator(t *testing.T) { } for i, test := range tests { - if err := tracer.Inject(sp, test.typ, test.carrier); err != nil { + if err := tracer.Inject(sp.Context(), test.typ, test.carrier); err != nil { t.Fatalf("%d: %v", i, err) } - child, err := tracer.Join(op, test.typ, test.carrier) + injectedContext, err := tracer.Extract(test.typ, test.carrier) if err != nil { t.Fatalf("%d: %v", i, err) } + child := tracer.StartSpan( + op, + opentracing.ChildOf(injectedContext)) child.Finish() } sp.Finish() diff --git a/raw.go b/raw.go index eaac6a6..5f66015 100644 --- a/raw.go +++ b/raw.go @@ -8,9 +8,13 @@ import ( // RawSpan encapsulates all state associated with a (finished) Span. type RawSpan struct { - // The RawSpan embeds its Context. Those recording the RawSpan - // should also record the contents of its Context. - Context + // The RawSpan embeds its SpanContext. Those recording the RawSpan + // should also record the contents of its SpanContext. + SpanContext + + // The SpanID of this SpanContext's first intra-trace reference (i.e., + // "parent"), or 0 if there is no parent. + ParentSpanID uint64 // The name of the "operation" this span is an instance of. (Called a "span // name" in some implementations) @@ -27,7 +31,4 @@ type RawSpan struct { // The span's "microlog". Logs []opentracing.LogData - - // The span's associated baggage. - Baggage map[string]string // initialized on first use } diff --git a/recorder_test.go b/recorder_test.go index ab84a31..f7a8bf2 100644 --- a/recorder_test.go +++ b/recorder_test.go @@ -12,10 +12,10 @@ func TestInMemoryRecorderSpans(t *testing.T) { recorder := NewInMemoryRecorder() var apiRecorder SpanRecorder = recorder span := RawSpan{ - Context: Context{}, - Operation: "test-span", - Start: time.Now(), - Duration: -1, + SpanContext: SpanContext{}, + Operation: "test-span", + Start: time.Now(), + Duration: -1, } apiRecorder.RecordSpan(span) assert.Equal(t, []RawSpan{span}, recorder.GetSpans()) diff --git a/span.go b/span.go index c757f5b..c486525 100644 --- a/span.go +++ b/span.go @@ -1,7 +1,6 @@ package basictracer import ( - "fmt" "sync" "time" @@ -15,9 +14,6 @@ import ( type Span interface { opentracing.Span - // Context contains trace identifiers - Context() Context - // Operation names the work done by this span instance Operation() string @@ -152,54 +148,12 @@ func (s *spanImpl) FinishWithOptions(opts opentracing.FinishOptions) { } } -func (s *spanImpl) SetBaggageItem(restrictedKey, val string) opentracing.Span { - canonicalKey, valid := opentracing.CanonicalizeBaggageKey(restrictedKey) - if !valid { - panic(fmt.Errorf("Invalid key: %q", restrictedKey)) - } - - s.Lock() - defer s.Unlock() - s.onBaggage(canonicalKey, val) - if s.trim() { - return s - } - - if s.raw.Baggage == nil { - s.raw.Baggage = make(map[string]string) - } - s.raw.Baggage[canonicalKey] = val - return s -} - -func (s *spanImpl) BaggageItem(restrictedKey string) string { - canonicalKey, valid := opentracing.CanonicalizeBaggageKey(restrictedKey) - if !valid { - panic(fmt.Errorf("Invalid key: %q", restrictedKey)) - } - - s.Lock() - defer s.Unlock() - - return s.raw.Baggage[canonicalKey] -} - -func (s *spanImpl) ForeachBaggageItem(handler func(k, v string) bool) { - s.Lock() - defer s.Unlock() - for k, v := range s.raw.Baggage { - if !handler(k, v) { - break - } - } -} - func (s *spanImpl) Tracer() opentracing.Tracer { return s.tracer } -func (s *spanImpl) Context() Context { - return s.raw.Context +func (s *spanImpl) Context() opentracing.SpanContext { + return &s.raw.SpanContext } func (s *spanImpl) Operation() string { diff --git a/span_test.go b/span_test.go index f4937d9..2903864 100644 --- a/span_test.go +++ b/span_test.go @@ -14,8 +14,8 @@ func TestSpan_Baggage(t *testing.T) { ShouldSample: func(traceID uint64) bool { return true }, // always sample }) span := tracer.StartSpan("x") - span.SetBaggageItem("x", "y") - assert.Equal(t, "y", span.BaggageItem("x")) + span.Context().SetBaggageItem("x", "y") + assert.Equal(t, "y", span.Context().BaggageItem("x")) span.Finish() spans := recorder.GetSpans() assert.Equal(t, 1, len(spans)) @@ -23,17 +23,17 @@ func TestSpan_Baggage(t *testing.T) { recorder.Reset() span = tracer.StartSpan("x") - span.SetBaggageItem("x", "y") + span.Context().SetBaggageItem("x", "y") baggage := make(map[string]string) - span.ForeachBaggageItem(func(k, v string) bool { + span.Context().ForeachBaggageItem(func(k, v string) bool { baggage[k] = v return true }) assert.Equal(t, map[string]string{"x": "y"}, baggage) - span.SetBaggageItem("a", "b") + span.Context().SetBaggageItem("a", "b") baggage = make(map[string]string) - span.ForeachBaggageItem(func(k, v string) bool { + span.Context().ForeachBaggageItem(func(k, v string) bool { baggage[k] = v return false // exit early }) diff --git a/tracer.go b/tracer.go index fa158ff..78ec6e8 100644 --- a/tracer.go +++ b/tracer.go @@ -118,11 +118,13 @@ type tracerImpl struct { func (t *tracerImpl) StartSpan( operationName string, + opts ...opentracing.StartSpanOption, ) opentracing.Span { - return t.StartSpanWithOptions( - opentracing.StartSpanOptions{ - OperationName: operationName, - }) + sso := opentracing.StartSpanOptions{} + for _, o := range opts { + o.Apply(&sso) + } + return t.StartSpanWithOptions(operationName, sso) } func (t *tracerImpl) getSpan() *spanImpl { @@ -135,6 +137,7 @@ func (t *tracerImpl) getSpan() *spanImpl { } func (t *tracerImpl) StartSpanWithOptions( + operationName string, opts opentracing.StartSpanOptions, ) opentracing.Span { // Start time. @@ -147,31 +150,45 @@ func (t *tracerImpl) StartSpanWithOptions( tags := opts.Tags // Build the new span. This is the only allocation: We'll return this as - // a opentracing.Span. + // an opentracing.Span. sp := t.getSpan() - if opts.Parent == nil { - sp.raw.TraceID, sp.raw.SpanID = randomID2() - sp.raw.Sampled = t.options.ShouldSample(sp.raw.TraceID) - } else { - pr := opts.Parent.(*spanImpl) - sp.raw.TraceID = pr.raw.TraceID - sp.raw.SpanID = randomID() - sp.raw.ParentSpanID = pr.raw.SpanID - sp.raw.Sampled = pr.raw.Sampled - - pr.Lock() - if l := len(pr.raw.Baggage); l > 0 { - sp.raw.Baggage = make(map[string]string, len(pr.raw.Baggage)) - for k, v := range pr.raw.Baggage { - sp.raw.Baggage[k] = v + // Look for a parent in the list of References. + // + // TODO: would be nice if basictracer did something with all + // References, not just the first one. +ReferencesLoop: + for _, ref := range opts.References { + switch ref.Type { + case opentracing.ChildOfRef, + opentracing.FollowsFromRef: + + refMD := ref.Referee.(*SpanContext) + sp.raw.TraceID = refMD.TraceID + sp.raw.SpanID = randomID() + sp.raw.ParentSpanID = refMD.SpanID + sp.raw.Sampled = refMD.Sampled + + refMD.baggageLock.Lock() + if l := len(refMD.Baggage); l > 0 { + sp.raw.Baggage = make(map[string]string, len(refMD.Baggage)) + for k, v := range refMD.Baggage { + sp.raw.Baggage[k] = v + } } + refMD.baggageLock.Unlock() + break ReferencesLoop } - pr.Unlock() + } + if sp.raw.TraceID == 0 { + // No parent Span found; allocate new trace and span ids and determine + // the Sampled status. + sp.raw.TraceID, sp.raw.SpanID = randomID2() + sp.raw.Sampled = t.options.ShouldSample(sp.raw.TraceID) } return t.startSpanInternal( sp, - opts.OperationName, + operationName, startTime, tags, ) @@ -203,28 +220,28 @@ type delegatorType struct{} // Delegator is the format to use for DelegatingCarrier. var Delegator delegatorType -func (t *tracerImpl) Inject(sp opentracing.Span, format interface{}, carrier interface{}) error { +func (t *tracerImpl) Inject(sc opentracing.SpanContext, format interface{}, carrier interface{}) error { switch format { case opentracing.TextMap: - return t.textPropagator.Inject(sp, carrier) + return t.textPropagator.Inject(sc, carrier) case opentracing.Binary: - return t.binaryPropagator.Inject(sp, carrier) + return t.binaryPropagator.Inject(sc, carrier) } if _, ok := format.(delegatorType); ok { - return t.accessorPropagator.Inject(sp, carrier) + return t.accessorPropagator.Inject(sc, carrier) } return opentracing.ErrUnsupportedFormat } -func (t *tracerImpl) Join(operationName string, format interface{}, carrier interface{}) (opentracing.Span, error) { +func (t *tracerImpl) Extract(format interface{}, carrier interface{}) (opentracing.SpanContext, error) { switch format { case opentracing.TextMap: - return t.textPropagator.Join(operationName, carrier) + return t.textPropagator.Extract(carrier) case opentracing.Binary: - return t.binaryPropagator.Join(operationName, carrier) + return t.binaryPropagator.Extract(carrier) } if _, ok := format.(delegatorType); ok { - return t.accessorPropagator.Join(operationName, carrier) + return t.accessorPropagator.Extract(carrier) } return nil, opentracing.ErrUnsupportedFormat } From 9ad37ccad85c1f7129395b3c19a508d8b268322b Mon Sep 17 00:00:00 2001 From: Ben Sigelman Date: Wed, 6 Jul 2016 11:19:10 -0700 Subject: [PATCH 2/3] Fix comment indenting --- span.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/span.go b/span.go index c486525..11bf0c0 100644 --- a/span.go +++ b/span.go @@ -40,11 +40,11 @@ func (s *spanImpl) reset() { // (the recorder) needs to make sure that they're not holding on to the // baggage or logs when they return (i.e. they need to copy if they care): // - // logs, baggage := s.raw.Logs[:0], s.raw.Baggage - // for k := range baggage { - // delete(baggage, k) - // } - // s.raw.Logs, s.raw.Baggage = logs, baggage + // logs, baggage := s.raw.Logs[:0], s.raw.Baggage + // for k := range baggage { + // delete(baggage, k) + // } + // s.raw.Logs, s.raw.Baggage = logs, baggage // // That's likely too much to ask for. But there is some magic we should // be able to do with `runtime.SetFinalizer` to reclaim that memory into From ffaf3c8a1fec1be07ea3b8800e5d8bb55e9a7035 Mon Sep 17 00:00:00 2001 From: Ben Sigelman Date: Wed, 6 Jul 2016 11:42:01 -0700 Subject: [PATCH 3/3] Allocate SpanContext to deal with lifetime issues --- raw.go | 2 +- recorder_test.go | 2 +- span.go | 6 ++++-- tracer.go | 6 +++++- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/raw.go b/raw.go index 5f66015..e32c24f 100644 --- a/raw.go +++ b/raw.go @@ -10,7 +10,7 @@ import ( type RawSpan struct { // The RawSpan embeds its SpanContext. Those recording the RawSpan // should also record the contents of its SpanContext. - SpanContext + *SpanContext // The SpanID of this SpanContext's first intra-trace reference (i.e., // "parent"), or 0 if there is no parent. diff --git a/recorder_test.go b/recorder_test.go index f7a8bf2..940aa57 100644 --- a/recorder_test.go +++ b/recorder_test.go @@ -12,7 +12,7 @@ func TestInMemoryRecorderSpans(t *testing.T) { recorder := NewInMemoryRecorder() var apiRecorder SpanRecorder = recorder span := RawSpan{ - SpanContext: SpanContext{}, + SpanContext: &SpanContext{}, Operation: "test-span", Start: time.Now(), Duration: -1, diff --git a/span.go b/span.go index 11bf0c0..a4818f8 100644 --- a/span.go +++ b/span.go @@ -51,7 +51,9 @@ func (s *spanImpl) reset() { // a buffer pool when GC considers them unreachable, which should ease // some of the load. Hard to say how quickly that would be in practice // though. - s.raw = RawSpan{} + s.raw = RawSpan{ + SpanContext: &SpanContext{}, + } } func (s *spanImpl) SetOperationName(operationName string) opentracing.Span { @@ -153,7 +155,7 @@ func (s *spanImpl) Tracer() opentracing.Tracer { } func (s *spanImpl) Context() opentracing.SpanContext { - return &s.raw.SpanContext + return s.raw.SpanContext } func (s *spanImpl) Operation() string { diff --git a/tracer.go b/tracer.go index 78ec6e8..ef20a01 100644 --- a/tracer.go +++ b/tracer.go @@ -133,7 +133,11 @@ func (t *tracerImpl) getSpan() *spanImpl { sp.reset() return sp } - return &spanImpl{} + return &spanImpl{ + raw: RawSpan{ + SpanContext: &SpanContext{}, + }, + } } func (t *tracerImpl) StartSpanWithOptions(