tracing: use opentracing.GlobalTracer instead of globalTracer #635
Conversation
redloaf
requested review from
fishy,
kylelemons,
konradreiche and
marcoferrer
and removed request for
a team
redloaf
force-pushed
the
use-global-tracer
branch
from
1e10652 to
3f88945
Compare
tracing/trace.go
Outdated
| @@ -159,7 +181,10 @@ func (t *trace) publish(ctx context.Context) error { | |||
| if !t.shouldSample() || t.tracer == nil { | |||
| return nil | |||
| } | |||
| return t.tracer.Record(ctx, t.toZipkinSpan()) | |||
| if tracer := t.getTracer(); tracer != nil { | |||
There was a problem hiding this comment.
This means if you override the global tracer with one that's not a *Tracer, no spans are ever published.
There was a problem hiding this comment.
aren't we actually trying to override it with a different tracer implementation? isn't it going to break a lot of things?
can you provide a high level explanation on how the library would work with the new tracer implementation?
There was a problem hiding this comment.
This is actually the behavior we desire. If you provide a tracer that is not a *Tracer, then tracer.Record is never called. However, presumably whatever tracer you do provide would record its spans and export them. Applying these changes makes this library compatible with other opentracing tracers, such as the opentracing bridge for opentelemetry, which will not be a *Tracer but would likely record and ship spans using an OTLP trace exporter.
| s.trace.tracer.logger.Log(ctx, msg+err.Error()) | ||
| if tracer := s.trace.internalTracer(); tracer != nil { | ||
| tracer.logger.Log(ctx, msg+err.Error()) | ||
| } |
There was a problem hiding this comment.
in the interest of fallbacks, should this also do something if internalTracer isn't found? like fall back to the global logger?
Why does this inject its own logger anyway? 🤔
| if tracer, ok := t.tracer.(*Tracer); ok && tracer != nil { | ||
| return tracer | ||
| } | ||
| return nil |
There was a problem hiding this comment.
don't return nil in-band, that's a recipe for nil pointer dereferences. Return *Tracer, bool
| @@ -91,8 +118,8 @@ func (t *trace) toZipkinSpan() ZipkinSpan { | |||
| zs.Duration = timebp.DurationMicrosecond(end.Sub(t.start)) | |||
|
|
|||
| var endpoint ZipkinEndpointInfo | |||
| if t.tracer != nil { | |||
| endpoint = t.tracer.endpoint | |||
| if tracer := t.internalTracer(); tracer != nil { | |||
There was a problem hiding this comment.
| if tracer := t.internalTracer(); tracer != nil { | |
| if tracer, ok := t.internalTracer(); ok { |
(see comment above about returning a bool too)
| // This function expects a *tracing.Tracer, but the global opentracing.Tracer may not be one. | ||
| // That's why it's relying on the globalTracer: even if the opentracing.GlobalTracer() has been overridden, | ||
| // the IDs generated by baseplate will still conform to the specified configuration. | ||
| traceID = globalTracer.newTraceID() |
There was a problem hiding this comment.
if we're not using globalTracer as a tracer, can we shield it somehow so that it only exposes the methods we want to be used on it, so it can't accidentally get missed again?
There was a problem hiding this comment.
I think we want to take a step back here.
AFAIK we no longer have any infra to handle the zipkin tracing data any more, so the whole tracing package is not working. trying to keep it "working" isn't really worth the effort IMHO.
If the intention here is "let's make the tracing package interop with the new tracing infra and works to emit new traces", then we should redesign the API around how new tracing system works (with opentelemetry or whatever), and just mark whatever old API that no longer making sense as deprecated, instead of trying to "keep them working".
💸 TL;DR
There are some places where
globalTraceris referenced instead of theopentracing.GlobalTracer(). If these ever differ (which is entirely possible), this breaks assumptions about how this library integrates with opentracing.📜 Details
If
nilis passed as thetracertonewTrace(which is incredibly common), use opentracing's global tracer instead of baseplate's.🧪 Testing Steps / Validation
✅ Checks