opentracing / basictracer-go

Basic implementation of the OpenTracing API for Go. 🛑 This library is DEPRECATED!
Apache License 2.0
81 stars 27 forks source link

Update to support opentracing.SpanContext (and References) #29

Closed bhs closed 8 years ago

bhs commented 8 years ago

See https://github.com/opentracing/opentracing-go/pull/82 .

This change is included to show some of the ramifications of SpanContext for implementations and OT callers (via the examples and tests here). I'll add a few notes inline.

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).

bhs commented 8 years ago

... this PR has also been updated per https://github.com/opentracing/opentracing-go/pull/82.

bhs commented 8 years ago

This has been updated per https://github.com/opentracing/opentracing.github.io/pull/100 and #32

bhs commented 8 years ago

I will merge this (along with https://github.com/opentracing/opentracing-go/pull/82) tomorrow if there are no objections.

bg451 commented 8 years ago

LGTM! 🚢

bhs commented 8 years ago

Well, go vet did a good job finding some actual problems here... namely that it's bad to copy structs by value that have Mutexes inside.

The Mutex was there to ensure that the Baggage accesses are concurrency-safe. Since the SpanContext may now outlive the Span, the easy+safe fix is to make it a pointer within Span. Sadly this does mean that there will be more allocations.

We could create a pool for these (and that would make sense), but for now I would rather get this change in and unbreak builds that depend on basictracer-go and opentracing-go.