-
Notifications
You must be signed in to change notification settings - Fork 344
WIP: in-process propagation resolves opentracing/specification#23 #117
Conversation
@pavolloffay I like the idea of being able to use a common mechanism for sharing spans and span contexts (for cases where we want to setup Somehow there needs to be a decision at the original execution context that it wants to share the span or span context, so that the receiving execution contexts knows what type of relationship they should be setting up. |
@objectiser currently there is only one https://github.com/opentracing/specification/blob/master/specification.md#references-between-spans reference type ( Web servlet instrumentation decides to hide span e.g. (does not allow to add any data to it when span is retrieved using SpanManager). Then users in order to trace business logic has to create a child local span which in current impl would be we could add to Visibility something like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few comments inline... my high-level feedback:
- thank you for creating another POC... the more of these the better
- It's nice how this PR hews closer to the syntax / spirit of existing OT-Java... it feels independent
- The drawback of the above is that there's a lot of bookkeeping for the caller; and in the MDCDemo I see more code than I'd like regarding on/off/finish (compare against https://github.com/bhs/opentracing-java/pull/4/files#diff-885b8e99541d8b2fee5d686273d0e199R47, I guess)
- It may just be a naming thing, but I am having trouble coming up with an "intuitive" way of separating the Visibility and VisibilityContext concepts in my brain
Put another way, IMO #115 is less intrusive in the example code, but I have a feeling (??) that #117 might be more flexible. Do you have examples in mind that would demonstrate the benefits of that flexibility? Or do you disagree with my premise?
@Override | ||
public void run() { | ||
final Span childSpan = tracer.buildSpan("child_" + j).start(); | ||
childSpan.visibility().capture().on(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is there no call to childSpan.visibility().off()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here it is missing it should be when this span is finished.
// Continuation mechanism. | ||
Span grandchild = tracer.buildSpan("grandchild_" + j).start(); | ||
grandchild.finish(); | ||
childSpan.finish(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's a lot (too much?) to ask users to keep track of both Span lifetimes and visibility/active windows via (procedural) calls like these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree I wanted to keep interfaces as simple as possible (KISS). Clossable with ref count could be added to make this to look like in #115.
@bhs thanks for review. I commented my concerns about #115 there. This PR is basically trying to solve that.
I don't have any concrete examples, though I think auto finish with ref count should be well understood and can we expect that by developers using framwework integrations with possibly low knowledge of tracing API? |
That's the big question, I guess. I agree that there is risk here, but it's hard to reason about. I have a few ideas which I will illustrate in #115 this AM. Thanks again for all of your work and feedback on this thing... it does feel like there's collective forward progress. |
Here is my minimal POC for in-process propagation #23.
It is a permutation of #111 and #115 with slightly different ideas:
span.finish()
What is added/changed:
SpanBuilder
automatically adds parent span,childOf
if span is not finished,followsFrom
if active span is finished/hidden.Span.finish()
hides span from spanManagerSpan
is associated to object calledVisibility
and vice versa e.g.span.visibility()
andvisibility.span()
VisibilityContext vc = Visibility.capture()
andVisibility v = VisibilityContext.visibility()
more in codeWorking implementation is done only for
MockTracer
along with some examples/tests including MDC demo from @bhs.It is a minimal implementation with no helper methods like
SpanBuilder.startAndActivate()
,Span.finishAndDeactivate()
. Please focus on the concept itself. Anyways we should think of better names forVisibility
andVisibilityContext
...cc @yurishkuro @tedsuo @adriancole @mabn @michaelsembwever @objectiser @wu-sheng @sjoerdtalsma