Mechanism to understand when ActiveSpan has been finished

[objectiser]

Currently, if writing a wrapper tracer, there is no way to know when the ActiveSpan has actually been finished, as the application (or framework integrations) simply call deactivate, but as this is ref counted, any call to this method could potentially finish the span.

Options are:
a) The wrapper tracer would also need to reference count
b) The deactivate method could return a boolean indicating whether the associated ActiveSpan has been finished
c) The BaseSpan could support an isFinished method which could be checked after deactivate
d) Implement some active span lifecycle support

[wu-sheng]

b) +1, IMO, the return value should be enough.

[felixbarny]

Very important issue for me as it seems my current wrapper implementation would not work with OT 0.30.0.

[yurishkuro]

The wrapper implementations are becoming more and more difficult with the growing API. I think an Observer facility would make more sense (Cf. opentracing/opentracing-go#135)

[bhs]

(sorry for my delay getting to this)

IMO we should not change core APIs for the convenience of wrappers. If there's a non-theoretical performance problem or something important is impossible, those would be different stories altogether.

I think I'm basically agreeing with @yurishkuro on this: since we can factor this sort of thing into a dedicated TracerObserver, that seems like the safest (i.e., most minimal) place to start.

[sjoerdtalsma]

I like an Observer as it is clearer on how to extend an existing tracer.
However, we need to make very clear that observer implementations may degrade performance of the whole system if not carefully applied.

[pavolloffay]

+1 on observable for span events. Something like finish event should do the job here.

Currently, if writing a wrapper tracer, there is no way to know when the ActiveSpan has actually been finished, as the application (or framework integrations) simply call deactivate, but as this is ref counted, any call to this method could potentially finish the span.

ActiveSpan#deactivate https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/ActiveSpan.java#L48 should call finish. So wrapping span#finish should be enough to know when span has been finished. Or am I missing something?

[pavolloffay]

Currently, if writing a wrapper tracer, there is no way to know when the ActiveSpan has actually been finished, as the application (or framework integrations) simply call deactivate, but as this is ref counted, any call to this method could potentially finish the span.

I think this can be done at the moment by simply doing this:

      // span builder wrapper code..
        @Override
        public ActiveSpan startActive() {
            Span span = startManual();  
            return wrappedTracer.makeActive(span);
        }

and you don't have to wrap any continuation or active span. Then ActiveSpan#deactivate calls span#finish https://github.com/opentracing/opentracing-java/blob/master/opentracing-api/src/main/java/io/opentracing/ActiveSpan.java#L48

[pavolloffay]

Documentation note about I approach^^^ I proposed. The implementation shouldn't do something like this:
https://github.com/openzipkin/brave-opentracing/pull/43/files#diff-ec734b127c956fb921d0d3c12254a659R56. Though, it's still only a PR.

[felixbarny]

Maybe BaseSpan should have a unwrap method similar to java.sql.Wrapper#unwrap?

[pavolloffay]

@felixbarny it makes more sense on ActiveSpan no? to return Span

[felixbarny]

I'm not too sure about that. When its on BaseSpan, its available on Span and ActiveSpan. Do you mean we don't need it on Span?

[pavolloffay]

I don't think it's needed on Span. Do you have a concrete example?

[felixbarny]

No, I don't have a concrete use case. I'm not too familiar with the new api tbh. Why do you think Spans should not (need to) be unwrapped?

[carlosalberto]

Hey all,

Wondering if we should rename this issue to something such as "Observer API"? (which, I think, many people would like to have eventually).

(Also, the unwrap proposal has been mentioned in a few other issues already ;) )

[yurishkuro]

I would recommend a new issue with such title and refer to this one. In Go there is already an observer API (in contrib only), which only applies to the span life cycle, not the Scope life cycle, so it's a decision whether those should be merged or not. Also, it has been often mentioned that observers would work better if there was a stable span ID exposed from the SpanContext, so we can also link to that issue/PR as a dependency.