opentracing/basictracer-go

Basic implementation of the OpenTracing API for Go. 🛑 This library is DEPRECATED!

basictracer-go's Issues

Make Context.Sampled an int32

@bensigelman mentioned in a comment the idea of making (Context).Sampled an int32.

This would allow detailed sampling information to be stored, such as the sampling rate, whether the trace was forced sampled, etc.

by default, don't panic unless there is no alternative

Monitoring should always be "first, do no harm".

Though it's formally undefined behavior in OpenTracing, I propose that the basictracer (and thus basictracer-go) default should be to never-panic unless there is truly no alternative; even if that means dropping data.

(Loud logging messages are fine, though)

cc: @bg451 @yurishkuro @jmacd @tschottdorf

add option to limit number of log events

We had an issue where we were doing endless retries and the memory inside basictracer grew indefinitely. It would be useful to add an option to limit the number of events in a span.

If adding this option is ok, I can work on the PR.

correctly implement the HTTPHeaders format

At the moment the baggage keys are passed through unmodified as HTTP header suffixes; this is not sufficient.

basictracer requires child.Start() to be called before parent.Finish()

e.g.
parent := StartSpan("p")
parent.Finish()
child := StartChildSpan(parent, "c") // <- panics

The semantic specification doesn't currently require StartChildSpan to be called before Finish. One use case is where the parent enqueues some future work and then calls Finish; the work is later removed from the queue and a child span is started.

On the other hand, perhaps this is not a true parent-child relationship...? The spec is not specific about what constitutes "parent-child and casual relationships." Perhaps intentionally? I'd argue that we should let the user define what 'parent-child' means.

Why do we use signed int64 in the Context?

cc @bcronin @bensigelman

I recently changed our in-house tracer to use uint64 because it ran into compatibility issues with other traces, because it was serializing large IDs as negative hex strings. I don't see a lot of benefits in keeping these fields signed.

Semantics of SplitText carrier

@bensigelman @tschottdorf I was looking at the implementation of split-text codecs in the basictracer, and noticed a big difference from our own implementation, specifically in how it handles the baggage items by simply copying the internal k/v map of the span into the carrier.Baggage map. The way I always imagined using the split-text carrier was:

func call_out(span Span, request Request) {
    // Imagine Request represents the headers as a map[string]string (unlike Go http.Request).
    carrier := &SplitTextCarrier { TracerState: request.headers, Baggage: request.headers }
    span.Tracer().Inject(span, SpitTextFormat, carrier)
    execute(request)
}

Notice that we can use the headers map directly as the target of injection. It's also quite possible that the same map already contained some other application- or protocol-specific headers. Which means when we decode a similar map back into the Span, the end-user code would also use a single map

func handle_incoming(request Request) {
    carrier := &SplitTextCarrier { TracerState: request.headers, Baggage: request.headers }
    span, _ := tracer.Join("op name", SpitTextFormat, carrier)
    ...
}

And that's where the problem with basictracer implementation comes in - it expects that Carrier.Baggage is the exact map of baggage items with exact key names provided by the end user originally. I don't see how it can work in an RPC protocol that supports custom application headers, unless the RPC frameworks takes an additional step of encoding keys/values of baggage items after they are injected into carrier (which is non-starter since there may be a different RPC framework on the receiving end).

In contrast, our previous implementation was always using a fixed prefix when converting baggage key into text carrier key. The use of the prefix helps find the baggage items on the receiving side by scanning through all app headers and looking for keys that start with that prefix.

What are your thoughts? I feel that we need to clarify the semantics of the SplitTextCarrier a bit more, as at least in my mind there were more expectations about it than what basictracer has implemented.

Releases + Tags

Hey there.

Is it possible to do releases or tags here?

Love, someone having to maintain a Gopkg.toml

Context(), Tracer() aren't taking the lock

func (s *spanImpl) Tracer() opentracing.Tracer {
    return s.tracer
}

func (s *spanImpl) Context() opentracing.SpanContext {
    return s.raw.Context
}

These functions are not taking their locks. Context() could happen concurrently to e.g. a SetBaggageItem call. For Tracer() it's less clear if it should take the lock, as it is immutable except for the DebugAssertUseAfterFinish case:

    if s.tracer.options.DebugAssertUseAfterFinish {
        // This makes it much more likely to catch a panic on any subsequent
        // operation since s.tracer is accessed on every call to `Lock`.
        // We don't call `reset()` here to preserve the logs in the Span
        // which are printed when the assertion triggers.
        s.tracer = nil
    }

It's a question of whether we want data races to show up in that case (in addition to panics). BTW I don't understand the "s.tracer is accessed on every call to Lock" part in that comment, it doesn't seem true.
@tschottdorf

Build is broken

After opentracing/opentracing-go#85, basictracer-go fails with

# github.com/opentracing/basictracer-go
./propagation.go:31: impossible type assertion:
        *spanImpl does not implement opentracing.Span (missing ForeachBaggageItem method)
./propagation_ot.go:36: impossible type assertion:
        *spanImpl does not implement opentracing.Span (missing ForeachBaggageItem method)
./propagation_ot.go:130: impossible type assertion:
        *spanImpl does not implement opentracing.Span (missing ForeachBaggageItem method)
./span.go:65: cannot use s (type *spanImpl) as type opentracing.Span in return argument:
        *spanImpl does not implement opentracing.Span (missing ForeachBaggageItem method)
./span.go:78: cannot use s (type *spanImpl) as type opentracing.Span in return argument:
        *spanImpl does not implement opentracing.Span (missing ForeachBaggageItem method)
./span.go:81: cannot use s (type *spanImpl) as type opentracing.Span in return argument:
        *spanImpl does not implement opentracing.Span (missing ForeachBaggageItem method)
./span.go:88: cannot use s (type *spanImpl) as type opentracing.Span in return argument:
        *spanImpl does not implement opentracing.Span (missing ForeachBaggageItem method)
./span.go:163: cannot use s (type *spanImpl) as type opentracing.Span in return argument:
        *spanImpl does not implement opentracing.Span (missing ForeachBaggageItem method)
./span.go:170: cannot use s (type *spanImpl) as type opentracing.Span in return argument:
        *spanImpl does not implement opentracing.Span (missing ForeachBaggageItem method)
./tracer.go:157: impossible type assertion:
        *spanImpl does not implement opentracing.Span (missing ForeachBaggageItem method)
./tracer.go:157: too many errors

Detected in Go kit; link to Circle.

/cc @yurishkuro

CVE-2021-3121 Vulnerability new version of gogo-protobuf required

The abovementioned high-severity CVE is fixed in gogo-protobuf v1.3.2; this should be the required version, instead of v1.3.2.

Remove Go 1.5 support

Travis build is failing in Go 1.5: https://travis-ci.org/opentracing/basictracer-go/builds/176048351

Does anyone still need building for that version?

@bensigelman @bg451 @slimsag @tschottdorf @RaduBerinde @nimeshksingh