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

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

Open
AdrianSchneble opened this issue Oct 16, 2019 · 4 comments
Open

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

AdrianSchneble opened this issue Oct 16, 2019 · 4 comments

Comments

@AdrianSchneble
Copy link

AdrianSchneble commented Oct 16, 2019

In an old issue (#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 (#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
Copy link

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
Copy link
Contributor

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
Copy link
Author

@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
Copy link

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();
    }

}

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

No branches or pull requests

3 participants