Adds ext/tags mechanism for adding common tags to the span #19
Conversation
| @@ -1,4 +1,4 @@ | |||
| package main | |||
| package dapperish | |||
There was a problem hiding this comment.
I had to rename the package to be able to import it into tests, since we don't have any other working implementation of the trace context source.
| @@ -37,7 +37,7 @@ func (s *standardSpan) StartChild(operationName string) opentracing.Span { | |||
| func (s *standardSpan) SetTag(key string, value interface{}) opentracing.Span { | |||
| s.lock.Lock() | |||
| defer s.lock.Unlock() | |||
| s.raw.Tags[key] = fmt.Sprint(value) | |||
| s.raw.Tags[key] = value | |||
There was a problem hiding this comment.
Removed conversion to string, as it was making the new tests fail. I think conversion should be done by the Recorder, if necessary.
| @@ -78,7 +79,7 @@ func server() { | |||
| } | |||
|
|
|||
| func main() { | |||
| opentracing.InitDefaultTracer(NewTracer("dapperish_tester")) | |||
| opentracing.InitDefaultTracer(dapperish.NewTracer("dapperish_tester")) | |||
There was a problem hiding this comment.
moved one level up to keep it in main package, while the actual dapperish implementation is in dapperish subpackage
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/opentracing/api-golang/examples/dapperish" |
There was a problem hiding this comment.
maybe we should rename examples to testing... seems weird to have anything under api-golang/opentracing/ depending on anything under api-golang/examples/, even if it's just a test.
There was a problem hiding this comment.
really it was just me cheating. I've already added testutils package, and for this test I only need minimal set of functionality from the Source implementation, so we could just copy a DaperishTraceContextSource under test utils
|
LGTM with the caveat that all of this stuff will probably have to change as we move forward with proper bindings, etc. Thanks! |
…apperish in main tests
|
Oh, per #12: can you update the |
Adds ext/tags mechanism for adding common tags to the span
@bensigelman