Remove default finish behaviour for activate() (#219)

* Do not auto finish on scope.close

* Fix review comments

* Fix review comments

* Add explanatory statement about not auto-finishing

* Define only activate(s, bool)

README.md

@@ -10,14 +10,6 @@ In order to understand the Java platform API, one must first be familiar with
 the [OpenTracing project](http://opentracing.io) and
 [terminology](http://opentracing.io/documentation/pages/spec.html) more specifically.
 
-## Status
-
-This project has a working design of interfaces for the OpenTracing API. There
-is a [MockTracer](https://github.com/opentracing/opentracing-java/tree/master/opentracing-mock)
-to facilitate unit-testing of OpenTracing Java instrumentation.
-
-Packages are deployed to Maven Central under the `io.opentracing` group.
-
 ## Usage
 
 ### Initialization
@@ -51,67 +43,48 @@ if (scope != null) {
 
 ### Starting a new Span
 
-The common case starts an `Scope` that's automatically registered for intra-process propagation via `ScopeManager`. The best practice is to use a try-with-resources pattern which handles Exceptions and early returns:
+The common case starts a `Scope` that's automatically registered for intra-process propagation via `ScopeManager`.
 
-```java
-io.opentracing.Tracer tracer = ...;
-...
-try (Scope scope = tracer.buildSpan("someWork").startActive()) {
-    // Do things.
-    //
-    // If we create async work, `scope.span()` allows us to pass the `Span` along as well.
-}
-```
-
-The above is semantically equivalent to the more explicit try-finally version:
+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.
 
 ```java
 io.opentracing.Tracer tracer = ...;
 ...
-Scope scope = tracer.buildSpan("someWork").startActive();
-try {
+Span span = tracer.buildSpan("someWork").start();
+try (Scope scope = tracer.scopeManager().activate(span, false))
     // Do things.
+} catch {
+    Tags.ERROR.set(scope.span(), true);
 } finally {
-    scope.deactivate();
+    span.finish();
 }
 ```
 
-To manually step around the `ScopeManager` registration, use `startManual()`, like this:
+The following code uses `try-with-resources` to finish the span.
 
 ```java
 io.opentracing.Tracer tracer = ...;
 ...
-Span span = tracer.buildSpan("someWork").startManual();
-try {
-    // (do things / record data to `span`)
-} finally {
-    span.finish();
+try (Scope scope = tracer.buildSpan("someWork").startActive(true)) {
+    // Do things.
+    //
+    // `scope.span()` allows us to pass the `Span` to newly created threads.
 }
 ```
 
-**If there is an `Scope`, it will act as the parent to any newly started `Span`** unless the programmer invokes `ignoreActiveSpan()` at `buildSpan()` time, like so:
+**If there is a `Scope`, it will act as the parent to any newly started `Span`** unless 
+the programmer invokes `ignoreActiveSpan()` at `buildSpan()` time or specified parent context explicitly:
 
 ```java
 io.opentracing.Tracer tracer = ...;
 ...
 Scope scope = tracer.buildSpan("someWork").ignoreActiveSpan().startActive();
 ```
 
-#### Explicit `finish()`ing via `Scope.activate(boolean)`
-
-When an `Scope` is created (either via `Tracer.SpanBuilder#startActive` or `ScopeManager#activate`), it's possible to specify whether the `Span` will be finished upon `Scope.deactivate()`, through a method overload taking a `finishSpanOnClose` parameter, which defaults to true.
-
-```java
-io.opentracing.Tracer tracer = ...;
-...
-try (Scope scope = tracer.buildSpan("someWork").startActive(false)) {
-    // false was passed to `startActive()`, so the `Span` will not be finished
-    // upon Scope deactivation
-}
-```
-
-This is specially useful when a `Span` needs to be passed to a another thread or callback, reactivated, and then passed to the next thread or callback, and only be finished when the task end is reached.
-
 ### Deferring asynchronous work
 
 Consider the case where a `Span`'s lifetime logically starts in one thread and ends in another. For instance, the Span's own internal timing breakdown might look like this:
@@ -158,16 +131,20 @@ Observe that passing `Scope` to another thread or callback is not supported. Onl
 
 In practice, all of this is most fluently accomplished through the use of an OpenTracing-aware `ExecutorService` and/or `Runnable`/`Callable` adapter; they factor out most of the typing.
 
-# Development
+## Instrumentation Tests
 
-This is a maven project, and provides a wrapper, `./mvnw` to pin a consistent
-version. For example, `./mvnw clean install`.
+This project has a working design of interfaces for the OpenTracing API. There
+is a [MockTracer](https://github.com/opentracing/opentracing-java/tree/master/opentracing-mock)
+to facilitate unit-testing of OpenTracing Java instrumentation.
 
-This wrapper was generated by `mvn -N io.takari:maven:wrapper -Dmaven=3.5.0`
+Packages are deployed to Maven Central under the `io.opentracing` group.
+
+## Development
 
-## Building
+This is a maven project, and provides a wrapper, `./mvnw` to pin a consistent
+version. Run `./mvnw clean install` to build, run tests, and create jars.
 
-Execute `./mvnw clean install` to build, run tests, and create jars.
+This wrapper was generated by `mvn -N io.takari:maven:wrapper -Dmaven=3.5.0`
 
 ## Contributing
 

opentracing-api/src/main/java/io/opentracing/ScopeManager.java

@@ -15,22 +15,13 @@
 
 /**
  * The {@link ScopeManager} interface abstracts both the activation of {@link Span} instances (via
- * {@link ScopeManager#activate(Span)}) and access to an active {@link Span}/{@link Scope}
+ * {@link ScopeManager#activate(Span, boolean)}) and access to an active {@link Span}/{@link Scope}
  * (via {@link ScopeManager#active()}).
  *
  * @see Scope
  * @see Tracer#scopeManager()
  */
 public interface ScopeManager {
-    /**
-     * Make a {@link Span} instance active.
-     *
-     * @param span the {@link Span} that should become the {@link #active()}
-     * @return a {@link Scope} instance to control the end of the active period for the {@link Span}.
-     * Span will automatically be finished when {@link Scope#close()} is called. It is a
-     * programming error to neglect to call {@link Scope#close()} on the returned instance.
-     */
-    Scope activate(Span span);
 
     /**
      * Make a {@link Span} instance active.
@@ -48,7 +39,7 @@ public interface ScopeManager {
      *
      * <p>
      * If there is an {@link Scope non-null scope}, its wrapped {@link Span} becomes an implicit parent of any
-     * newly-created {@link Span} at {@link Tracer.SpanBuilder#startActive()} time (rather than at
+     * newly-created {@link Span} at {@link Tracer.SpanBuilder#startActive(boolean)} time (rather than at
      * {@link Tracer#buildSpan(String)} time).
      *
      * @return the {@link Scope active scope}, or null if none could be found.

opentracing-api/src/main/java/io/opentracing/Tracer.java

@@ -136,7 +136,7 @@ interface SpanBuilder {
          * <li>{@link SpanBuilder#ignoreActiveSpan()} is not invoked,
          * </ul>
          * ... then an inferred {@link References#CHILD_OF} reference is created to the
-         * {@link ScopeManager#active()} {@link SpanContext} when either {@link SpanBuilder#startActive()} or
+         * {@link ScopeManager#active()} {@link SpanContext} when either {@link SpanBuilder#startActive(boolean)} or
          * {@link SpanBuilder#startManual} is invoked.
          *
          * @param referenceType the reference type, typically one of the constants defined in References
@@ -171,41 +171,11 @@ interface SpanBuilder {
          * <p>
          * The returned {@link Scope} supports try-with-resources. For example:
          * <pre><code>
-         *     try (Scope scope = tracer.buildSpan("...").startActive()) {
+         *     try (Scope scope = tracer.buildSpan("...").startActive(true)) {
          *         // (Do work)
          *         scope.span().setTag( ... );  // etc, etc
          *     }
-         *     // Span finishes automatically when the Scope is closed,
-         *     // following the default behavior of ScopeManager.activate(Span)
-         * </code></pre>
-         *
-         * <p>
-         * For detailed information, see {@link SpanBuilder#startActive(boolean)}
-         *
-         * <p>
-         * Note: {@link SpanBuilder#startActive()} is a shorthand for
-         * {@code tracer.scopeManager().activate(spanBuilder.startManual())}.
-         *
-         * @return a {@link Scope}, already registered via the {@link ScopeManager}
-         *
-         * @see ScopeManager
-         * @see Scope
-         * @see SpanBuilder#startActive(boolean)
-         */
-        Scope startActive();
-
-        /**
-         * Returns a newly started and activated {@link Scope}.
-         *
-         * <p>
-         * The returned {@link Scope} supports try-with-resources. For example:
-         * <pre><code>
-         *     try (Scope scope = tracer.buildSpan("...").startActive(false)) {
-         *         // (Do work)
-         *         scope.span().setTag( ... );  // etc, etc
-         *     }
-         *     // Span does not finish automatically when the Scope is closed as
-         *     // 'finishOnClose' is false
+         *     // Span does finishes automatically only when 'finishSpanOnClose' is true
          * </code></pre>
          *
          * <p>
@@ -241,7 +211,7 @@ interface SpanBuilder {
          * Like {@link #startActive()}, but the returned {@link Span} has not been registered via the
          * {@link ScopeManager}.
          *
-         * @see SpanBuilder#startActive()
+         * @see SpanBuilder#startActive(boolean)
          * @return the newly-started Span instance, which has *not* been automatically registered
          *         via the {@link ScopeManager}
          */

opentracing-examples/src/test/java/io/opentracing/examples/AutoFinishScopeManager.java

@@ -21,11 +21,6 @@
 public class AutoFinishScopeManager implements ScopeManager {
     final ThreadLocal<AutoFinishScope> tlsScope = new ThreadLocal<AutoFinishScope>();
 
-    @Override
-    public AutoFinishScope activate(Span span) {
-        return new AutoFinishScope(this, new AtomicInteger(1), span);
-    }
-
     @Override
     public AutoFinishScope activate(Span span, boolean finishOnClose) {
         return new AutoFinishScope(this, new AtomicInteger(1), span);

opentracing-examples/src/test/java/io/opentracing/examples/activate_deactivate/ScheduledActionsTest.java

@@ -89,7 +89,7 @@ private Thread entryThread() {
             @Override
             public void run() {
                 logger.info("Entry thread started");
-                try (Scope scope = tracer.buildSpan("parent").startActive()) {
+                try (Scope scope = tracer.buildSpan("parent").startActive(false)) {
                     Runnable action = new RunnableAction((AutoFinishScope)scope);
 
                     // Action is executed at some time and we are not able to check status
@@ -108,7 +108,7 @@ private Thread entryThreadWithTwoActions() {
             @Override
             public void run() {
                 logger.info("Entry thread 2x started");
-                try (Scope scope = tracer.buildSpan("parent").startActive()) {
+                try (Scope scope = tracer.buildSpan("parent").startActive(false)) {
                     Runnable action = new RunnableAction((AutoFinishScope)scope);
                     Runnable action2 = new RunnableAction((AutoFinishScope)scope);
 

opentracing-examples/src/test/java/io/opentracing/examples/active_span_replacement/ActiveSpanReplacementTest.java

@@ -74,16 +74,16 @@ private void submitAnotherTask(final Span initialSpan) {
             @Override
             public void run() {
                 // Create a new Span for this task
-                try (Scope taskScope = tracer.buildSpan("task").startActive()) {
+                try (Scope taskScope = tracer.buildSpan("task").startActive(true)) {
 
                     // Simulate work strictly related to the initial Span
                     // and finish it.
-                    try (Scope initialScope = tracer.scopeManager().activate(initialSpan)) {
+                    try (Scope initialScope = tracer.scopeManager().activate(initialSpan, true)) {
                         sleep(50);
                     }
 
                     // Restore the span for this task and create a subspan
-                    try (Scope subTaskScope = tracer.buildSpan("subtask").startActive()) {
+                    try (Scope subTaskScope = tracer.buildSpan("subtask").startActive(true)) {
                     }
                 }
             }

opentracing-examples/src/test/java/io/opentracing/examples/actor_propagation/Actor.java

@@ -55,7 +55,7 @@ public void run() {
                     .buildSpan("received")
                     .addReference(References.FOLLOWS_FROM, parent.context())
                     .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CONSUMER)
-                    .startActive()) {
+                    .startActive(true)) {
               phaser.arriveAndAwaitAdvance(); // child tracer started
               child.span().log("received " + message);
               phaser.arriveAndAwaitAdvance(); // assert size
@@ -79,7 +79,7 @@ public String call() throws Exception {
                         .buildSpan("received")
                         .addReference(References.FOLLOWS_FROM, parent.context())
                         .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CONSUMER)
-                        .startActive()) {
+                        .startActive(true)) {
                   phaser.arriveAndAwaitAdvance(); // child tracer started
                   phaser.arriveAndAwaitAdvance(); // assert size
                   return "received " + message;

opentracing-examples/src/test/java/io/opentracing/examples/actor_propagation/ActorPropagationTest.java

@@ -60,7 +60,7 @@ public void testActorTell() {
               .buildSpan("actorTell")
               .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_PRODUCER)
               .withTag(Tags.COMPONENT.getKey(), "example-actor")
-              .startActive()) {
+              .startActive(true)) {
         actor.tell("my message 1");
         actor.tell("my message 2");
       }
@@ -96,7 +96,7 @@ public void testActorAsk() throws ExecutionException, InterruptedException {
               .buildSpan("actorAsk")
               .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_PRODUCER)
               .withTag(Tags.COMPONENT.getKey(), "example-actor")
-              .startActive()) {
+              .startActive(true)) {
         future1 = actor.ask("my message 1");
         future2 = actor.ask("my message 2");
       }

opentracing-examples/src/test/java/io/opentracing/examples/client_server/Client.java

@@ -37,7 +37,7 @@ public void send() throws InterruptedException {
         try (Scope scope = tracer.buildSpan("send")
                 .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_CLIENT)
                 .withTag(Tags.COMPONENT.getKey(), "example-client")
-                .startActive()) {
+                .startActive(true)) {
             tracer.inject(scope.span().context(), Builtin.TEXT_MAP, new TextMapInjectAdapter(message));
             queue.put(message);
         }

opentracing-examples/src/test/java/io/opentracing/examples/client_server/Server.java

@@ -35,9 +35,10 @@ public Server(ArrayBlockingQueue<Message> queue, Tracer tracer) {
     private void process(Message message) {
         SpanContext context = tracer.extract(Builtin.TEXT_MAP, new TextMapExtractAdapter(message));
         try (Scope scope = tracer.buildSpan("receive")
-                .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_SERVER)
-                .withTag(Tags.COMPONENT.getKey(), "example-server")
-                .asChildOf(context).startActive()) {
+              .withTag(Tags.SPAN_KIND.getKey(), Tags.SPAN_KIND_SERVER)
+              .withTag(Tags.COMPONENT.getKey(), "example-server")
+              .asChildOf(context)
+              .startActive(true)) {
         }
     }
 

opentracing-examples/src/test/java/io/opentracing/examples/common_request_handler/HandlerTest.java

@@ -75,7 +75,7 @@ public void two_requests() throws Exception {
      */
     @Test
     public void parent_not_picked_up() throws Exception {
-        try (Scope parent = tracer.buildSpan("parent").startActive()) {
+        try (Scope parent = tracer.buildSpan("parent").startActive(true)) {
             String response = client.send("no_parent").get(15, TimeUnit.SECONDS);
             assertEquals("no_parent:response", response);
         }
@@ -102,7 +102,7 @@ public void parent_not_picked_up() throws Exception {
     @Test
     public void bad_solution_to_set_parent() throws Exception {
         Client client;
-        try (Scope parent = tracer.buildSpan("parent").startActive()) {
+        try (Scope parent = tracer.buildSpan("parent").startActive(true)) {
             client = new Client(new RequestHandler(tracer, parent.span().context()));
             String response = client.send("correct_parent").get(15, TimeUnit.SECONDS);
             assertEquals("correct_parent:response", response);

opentracing-examples/src/test/java/io/opentracing/examples/late_span_finish/LateSpanFinishTest.java

@@ -73,7 +73,7 @@ private void submitTasks(final Span parentSpan) {
             public void run() {
                 // Alternative to calling makeActive() is to pass it manually to asChildOf() for each created Span.
                 try (Scope scope = tracer.scopeManager().activate(parentSpan, false)) {
-                    try (Scope childScope1 = tracer.buildSpan("task1").startActive()) {
+                    try (Scope childScope1 = tracer.buildSpan("task1").startActive(true)) {
                         sleep(55);
                     }
                 }
@@ -84,7 +84,7 @@ public void run() {
             @Override
             public void run() {
                 try (Scope span = tracer.scopeManager().activate(parentSpan, false)) {
-                    try (Scope childScope2 = tracer.buildSpan("task2").startActive()) {
+                    try (Scope childScope2 = tracer.buildSpan("task2").startActive(true)) {
                         sleep(85);
                     }
                 }

opentracing-examples/src/test/java/io/opentracing/examples/multiple_callbacks/Client.java

@@ -48,7 +48,7 @@ public Object call() throws Exception {
                 logger.info("Child thread with message '{}' started", message);
 
                 try (Scope parentScope = cont.activate()) {
-                    try (Scope subtaskScope = tracer.buildSpan("subtask").startActive()) {
+                    try (Scope subtaskScope = tracer.buildSpan("subtask").startActive(false)) {
                         // Simulate work.
                         sleep(milliseconds);
                     }

opentracing-examples/src/test/java/io/opentracing/examples/multiple_callbacks/MultipleCallbacksTest.java

@@ -37,7 +37,7 @@ public class MultipleCallbacksTest {
     @Test
     public void test() throws Exception {
         Client client = new Client(tracer);
-        try (Scope scope = tracer.buildSpan("parent").startActive()) {
+        try (Scope scope = tracer.buildSpan("parent").startActive(false)) {
             client.send("task1", 300);
             client.send("task2", 200);
             client.send("task3", 100);

opentracing-examples/src/test/java/io/opentracing/examples/nested_callbacks/NestedCallbacksTest.java

@@ -78,7 +78,7 @@ public void run() {
                                 executor.submit(new Runnable() {
                                     @Override
                                     public void run() {
-                                        try (Scope scope = tracer.scopeManager().activate(span)) {
+                                        try (Scope scope = tracer.scopeManager().activate(span, true)) {
                                             span.setTag("key3", "3");
                                         }
                                     }