Skip to content
This repository has been archived by the owner on May 23, 2023. It is now read-only.

WIP: in-process propagation resolves opentracing/specification#23 #117

Closed
wants to merge 1 commit into from

Conversation

pavolloffay
Copy link
Member

Here is my minimal POC for in-process propagation #23.

It is a permutation of #111 and #115 with slightly different ideas:

  • span should be only finished with span.finish()
  • span manager should not expose finished span, spanContext always, if there is any..
  • span can be activated but "hidden" and only exposed spanContext

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 spanManager
  • Span is associated to object called Visibility and vice versa e.g. span.visibility() and visibility.span()
  • VisibilityContext vc = Visibility.capture() and Visibility v = VisibilityContext.visibility() more in code :octocat:

Working 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 for Visibility and VisibilityContext...

cc @yurishkuro @tedsuo @adriancole @mabn @michaelsembwever @objectiser @wu-sheng @sjoerdtalsma

@objectiser
Copy link
Contributor

@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 followsFrom relationships) - however I am not sure it should be based on just whether the span is finished, as this could lead to non-deterministic results.

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.

@pavolloffay
Copy link
Member Author

@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 followsFrom relationships) - however I am not sure it should be based on just whether the span is finished, as this could lead to non-deterministic results.

@objectiser currently there is only one https://github.com/opentracing/specification/blob/master/specification.md#references-between-spans reference type (followsFrom) which should be used when a parent is finished. However, Visibility allows users to hide span even if the span is not finished. Example:

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 followsFrom (if they just called tracer.buildSpan(), they could use SpanManager and specify reference type explicitly..) I guess you see my concern here..

we could add to Visibility something like .finish and keep also hide if it would be useful.

Copy link
Contributor

@bhs bhs left a 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();
Copy link
Contributor

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()?

Copy link
Member Author

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();
Copy link
Contributor

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

Copy link
Member Author

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.

@pavolloffay
Copy link
Member Author

@bhs thanks for review. I commented my concerns about #115 there. This PR is basically trying to solve that.

Do you have examples in mind that would demonstrate the benefits of that flexibility? Or do you disagree with my premise?

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?

@bhs
Copy link
Contributor

bhs commented Apr 7, 2017

@pavolloffay

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants