[Prototype] Move Context interaction to Tracer #1753
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1753 +/- ##
============================================
- Coverage 85.55% 85.24% -0.32%
+ Complexity 1372 1369 -3
============================================
Files 163 164 +1
Lines 5316 5319 +3
Branches 554 554
============================================
- Hits 4548 4534 -14
- Misses 567 584 +17
Partials 201 201
Continue to review full report at Codecov.
|
| default Span getCurrentSpan(Context context) { | ||
| return TracingContextUtils.getSpan(context); | ||
| } |
There was a problem hiding this comment.
Why not static methods for all of them?
There was a problem hiding this comment.
That was the alternative, yes. @mwear mentioned it feels better on the actual Tracer instance, but static methods would be an option.
There was a problem hiding this comment.
By requiring to be on the instance you really need that global tracer if static the need of the global tracer is limited
There was a problem hiding this comment.
One general thought is I would expect the "job" of managing the active span to be in the TracerProvider - that is the concept of tracing in OpenTelemetry, so it needs to make sure the active span is managed properly. Having Tracer shortcut to it for convenience sounds nice though.
Also, for static I'd expect Span.current to be simpler than requiring a global tracer, it could shortcut to the global TracerProvider, and no need to introduce the "global tracer". That doesn't mean we may not want an anonymous tracer in the API for users to use in their app, that discussion has come up, but it should be orthogonal to context management.
There was a problem hiding this comment.
Given that Spans are handled by Tracer, I'm more inclined to put it there directly, instead of being a shortcut.
Making this a static set of members in Tracer (e.g. Tracer.getCurrentSpan()) sounds fine by me (if we decide to go this route).
There was a problem hiding this comment.
+1 for Span.current(), which nicely parallels Context.current()
| @@ -17,7 +17,7 @@ | |||
| * @since 0.1.0 | |||
| */ | |||
| @Immutable | |||
| public final class TracingContextUtils { | |||
| final class TracingContextUtils { | |||
There was a problem hiding this comment.
This class can be moved to the Tracer class. No need to have it as a separate class
There was a problem hiding this comment.
Agreed. For the sake of simplicity I only made TracingContextUtils non-public but if we were to go down this road, we could entirely remove it (and have the keys and all that in Tracer).
|
Closing this as we won't go this route for Java. |
Prototype PR for open-telemetry/opentelemetry-specification#1019, specifically inspired by @mwear's proposal - mostly as an alternative presented by @bogdandrutu.
Changes are:
TracingContextUtilsis hidden, and remains as an implementation specific detail.Tracer(and it would happen throughBaggageManagerfor thebaggagepart). This means we end up with the following default methods inTracer:Span Tracer.getCurrentSpan()Span Tracer.getCurrentSpan(Context context)Scope Tracer.withSpan(Span span)Context Tracer.contextWithSpan(Span span)DefaultTracerclass) - that is,OpenTelemetry.getTracer()which gets aTracerwith a default name.Under this case, most of the interaction would happen through such default tracer:
Which is slightly odd, as any code that calls this will automatically force the loading of tracer/meter/baggage, even if they didn't intend to (for example, a
Propagatorhaving its inject/extract facilities could run into this). Not sure that's an actual problem, but wanted to mention it anyway.For your consideration ;)