opentracing / opentracing-java

OpenTracing API for Java. 🛑 This library is DEPRECATED! https://github.com/opentracing/specification/issues/163
http://opentracing.io
Apache License 2.0
1.68k stars 344 forks source link

Why doesn't Span support try-with-resources? #361

Open AdrianSchneble opened 4 years ago

AdrianSchneble commented 4 years ago

In an old issue (https://github.com/opentracing/opentracing-java/issues/102), it was stated that the Span interface does not extend AutoCloseable for java 1.6 compatibility. This change seems to have been requested in yet another (obviously older) issue (https://github.com/opentracing/opentracing-java/issues/75). So far no problem - if one is not using Java 6, Closeable extends AutoCloseable, so there should be no problem here.

However, as of the most current version (master > Span.java), the Span interface extends neither of those interfaces. This is quite unfortunate, as it means that the following is not possible:

try (Span span = ...) { // etc

Instead, something like the following is required to do the same thing:

Span span = // initialization
try {
    // stuff
} finally {
    span.finish();
}

By combing through the history of the Span interface, it turns out that this commit, created by @bhs, removed the Closeable interface from the Span. Simultaneously, the ActiveSpan "gained" the Closeable interface.

Now, I'm not sure if that change was intentional or not (or what the ActiveSpan interface was even meant to do), but there does not seem to be an ActiveSpan interface at all in the current version. I can't find something else that extends or implements Closeable either.

Now, as I'm merely a beginner in simply using OpenTracing (let alone developing it!), I may be missing something. Either way, I fail to see why the the Closeable interface is no longer implemented by Span. Can anyone elaborate on this?

(Note: I am aware that OpenTracing and OpenCensus are merging into OpenTelemetry. If this problem is solved with OpenTelemetry, that's fine with me.)

//edit: The docs still use try-with-resources-based code. If this functionality won't be re-implemented, the docs should be changed to reflect the latest version.

whiskeysierra commented 4 years ago

Scope is what ActiveSpan probably was meant to be. It represents the currently active span, usually bound to the current thread. I have to say that I don't use the AutoCloseable feature myself very often because I tend to instrument code within libraries where the opening and closing part are often spread, e.g. when having an interceptor or callback mechanism.

Having said that, since Scope is AutoCloseable I'd say for Span it wouldn't hurt to have it as well.

sjoerdtalsma commented 4 years ago

As far as I recall it was an intentional decision to remove (Auto-)Closeable from the Span interface because it invites the try-with-resources paradigm which actually makes it easy to forget logging caught Exceptions to the span, as per the following example from the readme:

Span span = tracer.buildSpan("someWork").start();
try (Scope scope = tracer.scopeManager().activate(span)) {
    // Do things.
} catch(Exception ex) {
    Tags.ERROR.set(span, true);
    span.log(Map.of(Fields.EVENT, "error", Fields.ERROR_OBJECT, ex, Fields.MESSAGE, ex.getMessage()));
} finally {
    span.finish();
}

Using the span variable as a resource here makes the span.log(..) statement in the catch block impossible. Adding the finally block was deemed more understandable than outer- and inner-try blocks if I recall correctly.

AdrianSchneble commented 4 years ago

@sjoerdtalsma that sounds like a fair point. I've only been experimenting with OpenTracing so far, and I haven't gone into detail concerning error logging, so I haven't had that issue yet.

Either way, the docs (https://opentracing.io/docs/getting-started/), which still use try-with-resources, need to be adapted to reflect the latest version. When I opened the issue, I couldn't find that problem right away, so I left it out at first. I'll edit it in now.

whiskeysierra commented 4 years ago

One could always define their own custom wrapper that combines the Span and AutoCloseable interfaces into one:

try (final AutoCloseableSpan span = autoclosing(tracer.buildSpan("test").start())) {
    // TODO use
}

Sample implementation (using lombok because I'm lazy):

@AllArgsConstructor
final class SimpleAutoCloseableSpan implements Span, AutoCloseable {

    @Delegate
    private final Span span;

    @Override
    public void close() {
        finish();
    }

}