Remove finishSpanOnClose flag

[felixbarny]

This is a follow up on https://github.com/opentracing/opentracing-java/issues/267. The underlying issue of that one is that you can't remember the finishSpanOnClose flag when implementing a bridge for ScopeManager#active().

The point of this issue is to remove the finishSpanOnClose flag altogether, as it is only useful in try-with-resources constructs which are a bad practice because you can't can't log exceptions as the span is closed before the catch block. To cite the main README.md:

Note that startActive(true) finishes the span on Scope.close(). Use it carefully because the try-with-resources construct finishes the span before the catch or finally blocks are executed, which makes logging exceptions and setting tags impossible. It is recommended to start the span and activate it later in try-with-resources. This makes the span available in catch and finally blocks.

So we are basically officially admitting that finishSpanOnClose is a bad practice.

You could argue that try-with-resources is valid if you don't want to log exceptions. But what if a user then later decides that they do want to log exceptions? Do they still remember that they can't use try-with-resources + a catch block then? I would most likely forget about that.

The finishSpanOnClose just enables some syntactic sugar after all which turns out to be problematic. As flag essentially advocates a bad practice, I suggest removing it.

Transition: Of course, we can't just remove the flag which would introduce a breaking change.

Instead, I propose adding the method io.opentracing.Tracer.SpanBuilder#startActive() without arguments, which has the same semantics like calling spanBuilder.startActive(false). Also, deprecate io.opentracing.Tracer.SpanBuilder#startActive(boolean finishSpanOnClose). This method can then be removed on a later release (for example 1.0.0-RC1)

The Scope interface can still extend from Closeable, because logging an exception is still valid to do after the scope is closed.

I think, even after doing this, https://github.com/opentracing/opentracing-java/issues/267 (Deprecate or remove ScopeManager.active()) is still relevant, but maybe less severe.

[felixbarny]

See also this brave issue for more information about why try-with-resources is considered an anti-pattern: https://github.com/openzipkin/brave/issues/676

[codefromthecrypt]

note this antipattern is also well documented in wingtips too. There's significant documentation burden to explain how to overcome it https://github.com/Nike-Inc/wingtips#warning-about-error-handling-when-using-try-with-resources-to-autoclose-spans

[sjoerdtalsma]

@adriancole is an explicit inner-try not a relatively easy fix? I agree there's a documentation burden though. (example: https://github.com/talsma-ict/context-propagation/blob/115439cab5e01882f43b5d41b3fa7012799822a1/context-propagation-java8/src/main/java/nl/talsmasoftware/context/functions/FunctionWithContext.java#L51)

[felixbarny]

The problem is that it requires people to actually read (and remember) the documentation because it's counter-intuitive and thus error-prone.

[codefromthecrypt]

@sjoerdtalsma I think what you are doing is clever, but yeah I agree with @felixbarny.

[carlosalberto]

So trying to get this one moving again - I played a little bit with updating some instrumentation with this change, and it feels it can definitely make things clearer when you need to add logs on unhandled Exceptions (opposed to the case where you didn't care at all about such errors).

This way we could have something like:

Span span = tracer.buildSpan("foo").start();
try (Scope scope = tracer.scopeManager().activate(span)) {
} catch (Exception exc) {
  Tags.ERROR.set(span, true);
  span.log(ImmutableMap.Builder<String, Object>()
                   .put("event", "error")
                   .put("error.object", exc)
                   .build());
} finally {
  span.finish(); // Optional
}

Or

Scope scope = null;
try {
  scope = tracer.buildSpan("foo").startActive();
} catch (Exception exc) {
  Tags.ERROR.set(scope.span(), true);
  scope.span().log(ImmutableMap.Builder<String, Object>()
                   .put("event", "error")
                   .put("error.object", exc)
                   .build());
} finally {
  if (scope != null) {
    scope.close();
    scope.span.finish(); // Optional
  }
}

And the simplest case of simply activating/deactivating without closing it and without reporting errors (when you don't own the Span) would be kept too.

On the other side, I think that, as Felix mentioned, #267 would be less severe - and I'd like to keep that one after removing the finishSpanOnClose flag, to cover up the case when the user has no context to keep the Scope around (with proper/better documentation, of course).

Thoughts? @felixbarny @yurishkuro @sjoerdtalsma @adriancole

PS - if that sounds feasible, I can definitely champion to update the testbed examples once a PR is created for this, and probably adding one more to showcase this case too.

[felixbarny]

Sounds good to me

[carlosalberto]

Hey @felixbarny ! as we merged #301, shall we close this?