WIP: Separate SpanContext from SpanBuilder #68
Conversation
SpanBuilder should not extend SpanContext - these types have different responsibilities. In particular SpanBuilder does not need identifiers like trace_id or span_id. The code using SpanContext was updated to use AbstractSpan instead (which also implements SpanContext). In addition Extractor now extract SpanContext instead of SpanBuilder.
- AbstractTracer now creates AbstractSpanContext based on the trace state - Introduced NoopSpanContext with the assumption that it will be reworked during some cleanup of opentracing-noop module - Removed assert from AbstractSpan.setBaggageItem - a better place for this check will be inside AbstractSpanContext once it allows adding baggage items
Instead SpanContext is an attribute of AbstractSpan Unfortunately there's a mess with NoopSpanContexts - there are two and both are used.
Tip: prepend "WIP - " to the title, and remove when ready for review/merge |
| return new AbstractSpanContext(traceState, baggage, tracer); | ||
| } | ||
|
|
||
| public AbstractSpanContext setBaggageItem(String key, String value) { |
There was a problem hiding this comment.
Just an FYI - in basictracer implementations in other languages there is only withBaggage(key, value) method, which creates a new immutable ctx.
| @@ -35,6 +34,8 @@ protected AbstractTracer() { | |||
| } | |||
|
|
|||
| abstract AbstractSpanBuilder createSpanBuilder(String operationName); | |||
|
|
|||
| abstract AbstractSpanContext createSpanContext(Map<String, Object> traceState); | |||
There was a problem hiding this comment.
If I were designing this abstract implementation, I would've done it as:
abstract class AbstractTracer<SPAN extends AbstractSpan, SCTX extends AbstractSpanContext> ... {
abstract SPAN createSpanContext(Map<String, Object> traceState);
}
The implementations then would deal with their own concrete types instead of abstract superclass, and the abstract tracer would take care of the necessary downcasting (as long as it gets a concrete Class in the constructor)
This is intentional. Each implementation might want to override (re-implement) the noops. |
These are used, ref https://github.com/openzipkin/brave-opentracing/blob/master/src/main/java/io/opentracing/impl/BraveSpanBuilder.java#L102 I think you'll answer a lot of these questions and changes if you can work on the brave-opentracing PR in parallel. |
michaelsembwever
changed the title
* master: [maven-release-plugin] prepare for next development iteration [maven-release-plugin] prepare release 0.20.2 revert deletion of developer section as it broke Maven sync [maven-release-plugin] prepare for next development iteration [maven-release-plugin] prepare release 0.20.1 Resets tag to HEAD [maven-release-plugin] prepare for next development iteration [maven-release-plugin] prepare release 0.20.0 travis: use container infrastructure Propagate baggage to child spans # Conflicts: # opentracing-impl/src/test/java/io/opentracing/impl/AbstractTracerTest.java
|
I created a PR (openzipkin-contrib/brave-opentracing#9) in brave-opentracing, but I think that the direction it's going does't look good enough. I'm starting to think that the abstract implementation does too much and it would be better if it was limited to some basic methods like the whole family of |
Seems solvable to me, e.g.:
To me that feels more pure. I'm curious why this would not work? |
|
It wouldn't work because:
2 is based on what @michaelsembwever wrote earlier, but I don't really like this approach - it's a very surprising behaviour. It's one thing to have a noop tracer which does nothing, but in this case receiving request without trace context extracts a All in all I wouldn't use the abstract implementation anymore, it introduces more problems than it solves. Little changes in the abstract implementation led to lots of changes e.g. in the mentioned brave-opentracing. IMO it would be more useful if it provided implementation of all simple "non-factory" methods - |
+1000 to this. I could not agree more. I also think it "would be nice" to move |
|
(I'm going to close this since #115 deals with the underlying problem and this has fallen out of date) |
This is an attempt to remove SpanContext interface from SpanBuilder. See #55
Apparently it has various repercussions...
There are two places where SpanContext should be created:
One option would be to create AbstractSpanContext and then AbstractSpanContextBuilder which would have methods like withTraceState or withBaggageItem, but to keep it simple I think it’s ok to reuse just stick to immutable AbstractSpanContext (where mutations create new instances).
Still, there is a need for some way to instantiate concrete subclass - that's why I've added createSpanContext(traceState) to AbstractTracer.
I did few other changes:
But I've run into problems with Noop implementation.
Right now there are two - one in -impl, the other in -noop. AbstractSpanBuilder asChildOf(Span parent) - requires NoopSpanBuilder to extends AbstractSpanBuilder but on the other hand AbstractSpanBuilder asChildOf tests against NoopSpanContext from opentracing-noop
which cannot depend on AbstractSpanContext (because circular dependency).
I think that it needs to be cleaned up first, especially:
(Note - the this is not ready to merge)