Introduce ActiveSpan, ActiveSpan.Continuation, and ActiveSpanSource #115
Conversation
|
... you can see how |
|
A comment about propagating MDC: bhs#4 (comment) |
|
@bhs Think the ref counting approach is definitely better. However this approach wouldn't work for something like the camel-opentracing, where there is an event handler called at the beginning and end of a scoped unit of work, and each handler may be called in a different thread. So no guarantee to be able to activate/deactivate a Its possible that the requirements of camel-opentracing are rare, so not suggesting we change the current PR to accommodate this if not considered a general requirement - just outlining the scenario in case more widely applicable. |
README.md
Outdated
| 1. In the method/function that *allocates* the closure/`Runnable`/`Future`/etc, call `ActiveSpanHolder#captureActive()` to obtain a `ActiveSpanHolder.Continuation` | ||
| 2. In the closure/`Runnable`/`Future`/etc itself, pair calls to `ActiveSpanHolder.Continuation#activate` and `ActiveSpanHolder.Continuation#deactivate` | ||
|
|
||
| In practice, the latter is most fluently accomplished through the use of an OpenTracing-aware `ExecutorService` and/or `Runnable`/`Callable` adapter; they can factor all of the above. |
There was a problem hiding this comment.
might be useful to have an actual code example, as this is arguably one of the most critical (difficult to get right) use cases.
There was a problem hiding this comment.
@yurishkuro no argument – this is what I've done in https://github.com/bhs/opentracing-java/tree/bhs/ash_demo / https://github.com/bhs/opentracing-java/blob/bhs/ash_demo/opentracing-mdc-demo/src/main/java/io/opentracing/mdcdemo/TracedExecutorService.java
If we proceed with this direction, something like the above should be part of OT-contrib (independent of MDC, btw), and we can link to it here.
|
One structural comment - I think it would be a lot easier to comprehend this if we defined the Holder and Continuation as interfaces only. The abstract classes are, after all, implementations. And I would expect the abstract classes to use generics for the concrete types of Span, Continuation, etc. |
Is there any guarantee that the thread calling the notifier with Assuming that ^^^ is not a concern, the remaining question is how to deal with the I also saw https://camel.apache.org/maven/camel-2.15.0/camel-core/apidocs/org/apache/camel/spi/InterceptStrategy.html#wrapProcessorInInterceptors(org.apache.camel.CamelContext,%20org.apache.camel.model.ProcessorDefinition,%20org.apache.camel.Processor,%20org.apache.camel.Processor) – would that work here? At first glance it seems like it would be easier from a threading perspective. |
| * | ||
| * @return a new {@link Continuation} to {@link Continuation#activate()} at the appropriate time. | ||
| */ | ||
| public final Continuation capture() { |
There was a problem hiding this comment.
Not quite following this method. Isn't the main use case of "capture" done via tracer.holder().capture(span) which returns Continuation? Why would one need to re-capture an already captured continuation?
There was a problem hiding this comment.
The idea is to make code like this easier to write:
It's def just a convenience, though.
"Capture" is when the incRef happens (rather than activate), which is really important for async stuff to work correctly.
There was a problem hiding this comment.
I see. Need to think about this a bit more. On the surface it seems unnecessary if we minimize the number of ways to get to that point. I.e. when TracerRunnable is created, the continuation can be captured from the top, via holder.captureActive(). Otherwise I would call this method explicitly "increment reference count", because "capture" seems misleading.
I started off that way, but had these comments in the interface definitions that basically read like code... "Implementations are expected to check X, call Y, then do Z". Esp around the refcounting. YMMV. I haven't formed an opinion about the generics, but no objection assuming there's no downside (and I can't think of one off-hand). |
| * remain. | ||
| */ | ||
| final void decRef() { | ||
| if (0 == refCount.decrementAndGet()) { |
There was a problem hiding this comment.
just wondering - is there a race condition here, if another thread simultaneously re-activates this continuation?
There was a problem hiding this comment.
incRef happens at capture-time, not activation-time.
Re the race condition: I don't think so, at least in practice, as
- if deactivate() hasn't been called for any Continuation, that means the refcount is > 0
- any capture()ing must happen before deactivate() is called
- thus, the capture/incRef will always happen before the deactivate/decRef
|
there seems to be a bit of an overlap in the APIs of Holder and Continuation. Maybe we could start without helper methods and define an absolutely minimal interface first? |
This may just be a restriction of my feeble brain, but I have been mainly focused on making the calling code look and feel reasonable, and I can't do that without the helpers. I can create a branch (for diffing purposes) with helpers stripped out if you'd like. Thoughts? |
maybe it's a sign of gaps in the API. For example, I basically goes to my comment about redundancy in the two interfaces, which makes me think there should be a more explicit conceptual separation between the two. If we can define clear separation, then we could have a completely distinct set of methods (e.g. |
|
@bhs I think there are workarounds for camel, so that isn't a problem, just highlighting the scenario in case relevant more generally. |
Agree. Especially for someone like me... this is like an English word puzzle to me... That is why, for these days, I just watch, but not comment. Hope we can provide concepts and responsibilities of the |
|
I understand where you're coming from... I'm tinkering with different naming and different approaches. I am reluctant to fully abstract things into interfaces for the reason I cited earlier. I tried the pure-interface approach and the comments felt a little absurd (dictating precise refcounting semantics, e.g.), esp in a language that supports abstract classes for exactly this reason. YMMV? |
|
I like something about this refcounting approach, but it is also quite fancy. I would be happier if it was optional e.g. just another SpanHolder impl, so users could choose what is the best for them. I think an active span API should:
example 1: tracer.buildSpan("parent")
.startAndActivate()
.span()
.finish();
tracer.buildSpan("child")
.start()
.finish();
List<MockSpan> mockSpans = tracer.finishedSpans();
Assert.assertEquals(2, mockSpans.size());
Assert.assertEquals(mockSpans.get(0).context().spanId(), mockSpans.get(1).parentId());parent is referenced in children as example 2: multiple span finish: span parent = tracer.buildSpan("parent")
.start();
tracer.holder().capture(parent).deactivate();
tracer.holder().capture(parent).deactivate(); |
bhs
changed the title
|
@pavolloffay @yurishkuro @sjoerdtalsma @objectiser @wu-sheng thanks for your comments. I did a major rev of this (really could be its own PR, but having three WIP PRs for the same issue seems absurd). The diff, basically:
Here's a simple usage example from the README: https://github.com/opentracing/opentracing-java/pull/115/files#diff-04c6e90faac2675aa89e2176d2eec7d8R107 Here's the updated MDC demo code: https://github.com/bhs/opentracing-java/pull/4/files#diff-885b8e99541d8b2fee5d686273d0e199R47 And here's the core API with "real" comments removed, and quick notes about refcounting added: PTAL. |
|
@pavolloffay re your concerns from the previous rev of this:
A few thoughts...
Again, this is undefined behavior. If we had to define it, I would say that the |
|
I have a couple of suggestions that might make this API a bit more fluent.
public interface ActiveSpan extends Span, Closable {
Continuation defer();
ActiveSpan replace(Span); // instead of adopt()
void deactivate(); // == finish() when refcount==1
interface Continuation {
// invariant:
// ActiveSpan span = cont.activate();
// span == tracer.activeSpan()
ActiveSpan activate();
}
}I haven't checked all the use cases, but this seems to simplify the usage quite a bit, and does away with some methods that feel redundant, e.g. instead of |
|
@yurishkuro thanks for the suggestions. Some commentary:
If this is an "in addition to" rather than an "instead of": sure, I guess? Various people were asking that the PR restrict itself to the bare interfaces, hence the lack of helpers in the PR. That said, I actually don't envision people calling If the suggestion is truly "instead of": I wanted the
The arrangement of the inner/outer classes might be weird/bad, but IMO there really are three concepts here that warrant separate objects:
Combining the first and second into a single API seems like a weird design due to the lifetime differences... we kind of want a "static abstract" method which is of course impossible. We could restructure things like so: BTW, regarding inheriting from |
|
I meant truly "instead of". The only method on the current ActiveSpanSource that requires some notion of the "manager" is Note that my proposal does not preclude custom implementations of ActiveSpanSource, the objection is simply to remove additional objects/handles that the user needs to deal with. I.e rather than getting a Handle and then a Span from it, just get ActiveSpan that implements both. public interface ActiveSpanSource { // can be implemented by Tracer
ActiveSpan activeSpan();
ActiveSpan withActiveSpan(Span span); // aka adopt(), replace()
// helper, could be dropped
ActiveSpan startAndActivate(...); // == withActiveSpan(startSpan(...))
}
public interface ActiveSpan extends Span, Closable {
Continuation defer();
void deactivate(); // == finish() when refcount==1
interface Continuation {
// invariant:
// ActiveSpan span = cont.activate();
// span == tracer.activeSpan()
ActiveSpan activate();
}
}This ^^^ whole framework can be a completely standalone add-on, in fact, i.e. it can be implemented as a wrapper tracer that implements ActiveSpanSource, or a specific Tracer implementation can implement ActiveSpanSource interface by taking real ActiveSpanSource (although there's a bit of a circular dependency in startAndActivate - the problem with helper methods. If this makes it to OT API it won't be a problem). Another thought - if we want ActiveSpanSource to be pluggable, then it probably needs another SPI that is not directly exposed to OT user.
My assumption was that if you get ActiveSpan instance, try-with-resources would work differently on it than if it was a plain Span. Because in practice it's likely going to be a wrapper around a Span, it can override the close -> deactivate(), instead of close -> finish on the underlying Span. Anyway, I agree that inheritance may create a different mental overhead, I just like having to write |
Yes, exactly. That's why there are two interfaces: one for the manager (adopt) and one for the instance (defer and deactivate). IMO things like this "end badly" when one squints about object lifetimes and convolves these separate concepts into a single API. This is certainly not to say that this PR represents the only reasonable way forward... I would just argue against putting all three of the above into one interface. Putting "adopt" (or whatever it's called) on Per your point about how almost all of this can be built outside of the core API: I agree with that at a literal level, though the benefit for users is lost if OT callers (esp in 3rd-party instrumentation code) can't assume that To sum up: I think we both agree that the "manager" aspects could be shared with Tracer in various ways (composition, extension, etc). I think we also both agree that having ActiveSpan extend Span is plausible though has some tradeoffs. What are we actively disagreeing about at this point that "matters"? :) I just want to be sure to focus the discussion where we need to. |
Don't think we do, I like the current approach. There are some naming questions we can address later, but overall I think a separation of "active span source/manager", "active span (holder)", and "continuation" has been very useful, and the API has become very lean. |
README.md
Outdated
| 1. Start the `Span` via `Tracer.startAndActivate()` rather than via `Tracer.start()`; or, if the `Span` was already `start()`ed, call `ActiveSpanSource#adopt(span)`. Either route will yield an `ActiveSpanSource.Handle` instance that's "adopted" the `Span`. | ||
| 2. In the method/function that *allocates* the closure/`Runnable`/`Future`/etc, call `ActiveSpanSource.Handle#defer()` to obtain an `ActiveSpanSource.Continuation` | ||
| 3. In the closure/`Runnable`/`Future`/etc itself, invoke `ActiveSpanSource.Continuation#activate` to re-activate the `ActiveSpanSource.Handle`, then `deactivate()` it (or use try-with-resources for less typing). | ||
|
|
There was a problem hiding this comment.
So many verbs in a small bit of text... start, activate, adopt, allocate, defer, re-activate and deactivate ...
By the way, I think they're all much better chosen than previous iterations, but I think a reader trying to discover what OpenTracing is will get absolutely overwhelmed by these couple of bullets 😉
|
@yurishkuro 🆒. I did some refactoring and ended up with https://github.com/bhs/opentracing-java/compare/bhs/ash_demo...bhs:bhs/outer_active_span?expand=1#diff-5e4b55691403ed8b93d2209691a930d3R16 . I'll leave those commits out of this PR's branch until I get some positive feedback. Here's the important part: ... IMO we are getting closer to something I actually like. |
| * | ||
| * @return the {@link ActiveSpan active span}, or null if none could be found. | ||
| */ | ||
| ActiveSpan activeSpan(); |
There was a problem hiding this comment.
I'm not sure if this has already been discussed, but from a user's standpoint I'd prefer returning a NoopSpan over returning null so that they don't have to do null checks just to add a simple tag.
If the tag is expensive to compute, it would however be beneficial if the user could differ between proper and noop spans. But rather than checking for nulls or a global NOOP constant I propose to introduce a isSampled() method on BaseSpan. From the user's perspective, it does not really matter wether there is no active span or the active span is not sampled. They only want to avoid computing a expensive tag if not necessary.
I think this is something we should decide on before merging, or at least before releasing a new version as this would be a somewhat breaking change (although remaining binary compatibility) to introduce later (check for null vs check for isSampled()).
There was a problem hiding this comment.
Are there no valid use-cases where you need the null value? E.g. only create a span if there's no active one etc?
the general isSampled() functionality is tracked by opentracing/specification#8
There was a problem hiding this comment.
Probably there are use cases for that. But I think the default use case is to set simple tags or add logs. IMHO an API should be optimized for the default use case. So maybe add ActiveSpanSource#hasActiveSpan() and let #activeSpan() return noops?
There was a problem hiding this comment.
@felixbarny this has already been discussed, yes. Unfortunately the github thread here is so convoluted that I can't even find the relevant comment. :-(
I am 💯% happy to add an issue about this (and block release of a new version of OT-Java accordingly), but IMO it can be decoupled from this PR. Just to prep you for my point-of-view: I generally think instrumentation will either (a) create a new Span, in which case there's no need for a null check regardless, or (b) be at the layer of a logging library or similar where the verbosity of a null check is fine.
If we actually think that it's going to be commonplace to use the OT interface as the logging API (rather than a registrant of the logging API), then I would be in favor of what you propose. That seems like the essential question.
(I'm less convinced about arguments involving tags: they are almost always set at start()-time in my experience)
There was a problem hiding this comment.
+1 for the separate issue
I think this would actually be the main api end users would use.
The use case that I have in mind is when ot is used by an agent, users should still be able to add custon tags. For example, I've implemented a search analytics Kibana dashboard on top of stagemonitor and OT for a customer. It works by adding meta info about the search via tags like no of hits and the search term. For these kinds of things it would be nice if null checks could be avoided.
| * Set a key:value tag on the Span. | ||
| */ | ||
| // overloaded 3x to support the BasicType concern | ||
| BaseSpan setTag(String key, String value); |
There was a problem hiding this comment.
We could do:
public interface BaseSpan<S extends BaseSpan> {
[...]
S setTag(String key, String value);
[...]
}
public interface Span extends BaseSpan<Span> {
[...]
}That way, chaining would still be possible in a type safe way.
|
|
||
| In practice, all of this is most fluently accomplished through the use of an OpenTracing-aware `ExecutorService` and/or `Runnable`/`Callable` adapter; they can factor most of the typing. | ||
|
|
||
| #### Automatic `finish()`ing via `ActiveSpan` reference counts |
There was a problem hiding this comment.
I'm happy with ThreadLocalActiveSpan being moved to opentracing-util, but this section still suggests that automatic finishing via ref counting will happen all the time. IMHO this section should be explicit on whether all tracers must implement ref counting (via ThreadLocalActiveSpan or any other means) or if it is a optional behavior that the tracer implementations can decide to support or not.
There was a problem hiding this comment.
If the behavior of active span is different between implementations, then frameworks wouldn't know what the right way is to instrument their code. So the spec must be unambiguous that de-activating the last instance of the active span always finishes the span. Is there another way to achieve that without ref counting?
There was a problem hiding this comment.
Whether there's an AtomicInteger is an implementation detail, but I don't see how this spec could be implemented without reference counting of some sort.
|
Fair warning: I'm planning to do a thorough round of nitpick-removal and merge this sometime tomorrow. |
| /** | ||
| * Set a key:value tag on the Span. | ||
| */ | ||
| // overloaded 3x to support the BasicType concern |
| * {@code span.log(timestampMicroseconds, Map.of("event", "exception", "payload", stackTrace))} | ||
| **/ | ||
| S log(String eventName, /* @Nullable */ Object payload); | ||
| /** |
opentracing-mock/pom.xml
Outdated
| <artifactId>opentracing-noop</artifactId> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.opentracing</groupId> |
There was a problem hiding this comment.
Could this module depend only on util? We would like to use ThreadLocalActiveSpanSource in MockTracer and maybe as a default.
If you do the change then there will be a cyclic dependency because util depends on ot-mock, but that can be removed MockTracer is not used in ot-util.
There was a problem hiding this comment.
btw, to follow up on this one point @pavolloffay: ot-util has tests that depend on MockTracer.
| public void deactivate() { | ||
| if (source.tlsSnapshot.get() != this) { | ||
| // This shouldn't happen if users call methods in the expected order. Bail out. | ||
| return; |
There was a problem hiding this comment.
Should this be logged to make finding such issues?
There was a problem hiding this comment.
it's a good question... I am uneasy about adding logging statements at this layer. I could perhaps add a private boolean strict with an overloaded constructor and have it throw an exception here in strict mode?
There was a problem hiding this comment.
Not sure - needs further thought, but not a blocker.
|
|
||
| // We can't use 1.7 features like try-with-resources in this repo -- argh. | ||
| // | ||
| // F*** IT, WE'LL DO IT LIVE! |
| /** | ||
| * @deprecated use {@link #startManual} or {@link #startActive} instead. | ||
| */ | ||
| Span start(); |
opentracing-mock/pom.xml
Outdated
| <artifactId>opentracing-noop</artifactId> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>io.opentracing</groupId> |
There was a problem hiding this comment.
Could this module depend only on util? We would like to use ThreadLocalActiveSpanSource in MockTracer and maybe as a default.
If you do the change then there will be a cyclic dependency because util depends on ot-mock, but that can be removed MockTracer is not used in ot-util.
| /** | ||
| * Create a new MockTracer that passes through any calls to inject() and/or extract(). | ||
| */ | ||
| public MockTracer(Propagator propagator) { | ||
| this.propagator = propagator; | ||
| this(NoopActiveSpanSource.INSTANCE, propagator); |
There was a problem hiding this comment.
As mentioned above an actual impl would be better.
There was a problem hiding this comment.
@pavolloffay you mean instead of a noop? This runs into problems with circular dependencies since the ThreadLocalActiveSpanSourceTest depends on the MockTracer. We allow the programmer to manually specify an ActiveSpanSource via an overloaded constructor; I thought that was sufficient.
| public void makeActiveSpan() throws Exception { | ||
| Span span = mock(Span.class); | ||
|
|
||
| // We can't use 1.7 features like try-with-resources in this repo -- argh. |
There was a problem hiding this comment.
Tests can use java 8. It could be set in the root pom.
| if (this.startMicros == 0) { | ||
| this.startMicros = MockSpan.nowMicros(); | ||
| } | ||
| return new MockSpan(MockTracer.this, this.operationName, this.startMicros, initialTags, this.firstParent); | ||
| if (firstParent == null && !ignoringActiveSpan) { | ||
| firstParent = (MockSpan.MockContext)activeSpanContext(); |
There was a problem hiding this comment.
implicit parent should be derived at tracer.buildSpan() time https://github.com/opentracing/opentracing-java/pull/115/files#diff-9f484babc6e66bd62198ce009d9247dcR26
There was a problem hiding this comment.
Hm, I thought we decided to do startSpan-time?
| * or | ||
| * {@code span.log(timestampMicroseconds, Map.of("event", "exception", "payload", stackTrace))} | ||
| **/ | ||
| S log(String eventName, /* @Nullable */ Object payload); |
There was a problem hiding this comment.
add @Deprecated on the method below too.
| */ | ||
| SpanBuilder asChildOf(Span parent); | ||
| SpanBuilder asChildOf(BaseSpan parent); |
There was a problem hiding this comment.
any particular reason for using a raw type here instead of something like <T extends BaseSpan<?>> SpanBuilder asChildOf(BaseSpan<T> parent)?
There was a problem hiding this comment.
SpanBuilder asChildOf(BaseSpan<?> parent) should be good enough as the return type is independent from the actual generic type of BaseSpan.
There was a problem hiding this comment.
I have a questions regarding one usage category. To make the example concrete let's assume that there's a generic HTTP client, with own threadpool, all hidden under some abstraction layers.
Naturally the client is instrumented, creates client-side span and injects span context.
I use this client to execution operation "op" in service "Service". It's understandable that I want the client-side span created inside the client to have operation_name based on the service that it calls, e.g. "call Service#op".
How to do that - pass operation name (or even tags) "higher layer" ?
The client has access only to the active span and some http-level details. I could set it as a tag in the parent span, but that would pollute parent span. I could add the name to baggage temporarily but that would abuse baggage and requires a way to remove it from baggage later.
It seems that the only way is some explicit propagation of these parameters - operation name for the future child span, possibly also tags - and it needs to be supported by the API of the used library (http client in this case)
|
@mabn This is an interesting problem, and relevant for many of the framework integrations in the opentracing-contrib repo. However I think it would be best raised as a separate issue rather than getting mixed up with this PR. |
|
@mabn I've created an issue related to this, as a similar question came up in the okhttp framework integration repo: opentracing-contrib/meta#25 |
|
@objectiser thanks! |
|
@pavolloffay @objectiser @ivantopo @felixbarny thanks for your recent comments, I have tried to address them. I also did a full pass over the PR and cleaned up a bunch of comments/etc. |
|
Ok people... Since this PR apparently broke Github (https://gitter.im/opentracing/public?at=59146a4892217cca58669dfa) I am going to merge. I will also create an issue in this repo to track things we collectively agreed to defer until this was in. |
This is a relatively small diff against what can be found in #111
I created a separate PR since – IMO – it can be difficult to discuss two alternatives in Github PRs unless they're separate, well, PRs.
Delta from #111
About the reference-counting...
IMO the relationship in #111 between
Span.finish()andContinuation.deactivate()was borderline unacceptable. While that's addressed (syntactically) in this PR, the result is thatContinuationmanages anAtomicIntand coreopentracing-javagets further into the business of implementation than it has to date.Desired feedback
I'd love to hear about structural/API-level concerns, see structural/API-level alternatives, and perhaps learn about naming preferences.
Let's not nitpick this review in too much detail; or at least not until the "WIP" prefix is removed. All in due time. :)