Merge 1bb2e14 into b9feb48

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,28 +43,34 @@ 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 an `Scope` that's automatically registered for intra-process propagation via `ScopeManager`.
+Note that the default behaviour of `startActive()` does not finish the span on `Scope.close()`.
+This was made on purpose because `try-with-resources` construct would finished the span before
+`catch` or `finally` blocks are executed, which makes logging exceptions impossible.
+This behaviour can be overridden by `startActive(finishOnClose)` which is demonstrated later in this section.
 
 ```java
 io.opentracing.Tracer tracer = ...;
 ...
-try (Scope scope = tracer.buildSpan("someWork").startActive()) {
+Span span = tracer.buildSpan("someWork").startManual();
+try (Scope scope = tracer.scopeManager().activate(span))
     // Do things.
-    //
-    // If we create async work, `scope.span()` allows us to pass the `Span` along as well.
+} catch {
+    Tags.ERROR.set(scope.span(), true);
+} finally {
+    span.finish();
 }
 ```
 
-The above is semantically equivalent to the more explicit try-finally version:
+The following code uses `try-with-resources` to finish the span.
 
 ```java
 io.opentracing.Tracer tracer = ...;
 ...
-Scope scope = tracer.buildSpan("someWork").startActive();
-try {
+try (Scope scope = tracer.buildSpan("someWork").startActive(true)) {
     // Do things.
-} finally {
-    scope.deactivate();
+    //
+    // `scope.span()` allows us to pass the `Span` to newly created threads.
 }
 ```
 
@@ -89,29 +87,15 @@ try {
 }
 ```
 
-**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 an `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 +142,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

@@ -27,7 +27,7 @@ public interface ScopeManager {
      *
      * @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
+     * Span will not 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);

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

@@ -165,11 +165,16 @@ interface SpanBuilder {
          * <p>
          * The returned {@link Scope} supports try-with-resources. For example:
          * <pre><code>
-         *     try (Scope scope = tracer.buildSpan("...").startActive()) {
+         *     Span span = tracer.buildSpan("...").startManual();
+         *     try (Scope scope = tracer.scopeManager().activate(span))
          *         // (Do work)
          *         scope.span().setTag( ... );  // etc, etc
+         *     } catch(Exception ex) {
+         *       Tags.ERROR.set(span, true)'
+         *     } finally {
+         *       span.finish();
          *     }
-         *     // Span finishes automatically when the Scope is closed,
+         *     // Span is not automatically finished when the Scope is closed,
          *     // following the default behavior of ScopeManager.activate(Span)
          * </code></pre>
          *
@@ -194,12 +199,11 @@ interface SpanBuilder {
          * <p>
          * The returned {@link Scope} supports try-with-resources. For example:
          * <pre><code>
-         *     try (Scope scope = tracer.buildSpan("...").startActive(false)) {
+         *     try (Scope scope = tracer.buildSpan("...").startActive(true)) {
          *         // (Do work)
          *         scope.span().setTag( ... );  // etc, etc
          *     }
-         *     // Span does not finish automatically when the Scope is closed as
-         *     // 'finishOnClose' is false
+         *     // Span does finish automatically only when 'finishSpanOnClose' is true
          * </code></pre>
          *
          * <p>

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

@@ -37,7 +37,8 @@ private void process(Message 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()) {
+                .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

@@ -72,8 +72,8 @@ private void submitTasks(final Span parentSpan) {
             @Override
             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 scope = tracer.scopeManager().activate(parentSpan)) {
+                    try (Scope childScope1 = tracer.buildSpan("task1").startActive(true)) {
                         sleep(55);
                     }
                 }
@@ -83,8 +83,8 @@ public void run() {
         executor.submit(new Runnable() {
             @Override
             public void run() {
-                try (Scope span = tracer.scopeManager().activate(parentSpan, false)) {
-                    try (Scope childScope2 = tracer.buildSpan("task2").startActive()) {
+                try (Scope span = tracer.scopeManager().activate(parentSpan)) {
+                    try (Scope childScope2 = tracer.buildSpan("task2").startActive(true)) {
                         sleep(85);
                     }
                 }

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

@@ -42,7 +42,7 @@ public class NestedCallbacksTest {
     @Test
     public void test() throws Exception {
 
-        try (Scope scope = tracer.buildSpan("one").startActive(false)) {
+        try (Scope scope = tracer.buildSpan("one").startActive()) {
             submitCallbacks(scope.span());
         }
 
@@ -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");
                                         }
                                     }

opentracing-examples/src/test/java/io/opentracing/examples/promise_propagation/Promise.java

@@ -56,7 +56,7 @@ public void run() {
                         .buildSpan("success")
                             .addReference(References.FOLLOWS_FROM, parentScope.span().context())
                         .withTag(Tags.COMPONENT.getKey(), "success")
-                        .startActive()) {
+                        .startActive(true)) {
                   callback.accept(result);
                 }
               context.getPhaser().arriveAndAwaitAdvance(); // trace reported
@@ -76,7 +76,7 @@ public void run() {
                         .buildSpan("error")
                             .addReference(References.FOLLOWS_FROM, parentScope.span().context())
                         .withTag(Tags.COMPONENT.getKey(), "error")
-                        .startActive()) {
+                        .startActive(true)) {
                   callback.accept(error);
                 }
               context.getPhaser().arriveAndAwaitAdvance(); // trace reported

opentracing-examples/src/test/java/io/opentracing/examples/promise_propagation/PromisePropagationTest.java

@@ -60,7 +60,7 @@ public void testPromiseCallback() {
           tracer
               .buildSpan("promises")
               .withTag(Tags.COMPONENT.getKey(), "example-promises")
-              .startActive()) {
+              .startActive(true)) {
 
         Promise<String> successPromise = new Promise<>(context, tracer);
 

opentracing-util/src/main/java/io/opentracing/util/ThreadLocalScopeManager.java

@@ -27,7 +27,7 @@ public class ThreadLocalScopeManager implements ScopeManager {
 
     @Override
     public Scope activate(Span span) {
-        return new ThreadLocalScope(this, span, true);
+        return new ThreadLocalScope(this, span, false);
     }
 
     @Override

opentracing-util/src/test/java/io/opentracing/util/ThreadLocalScopeManagerTest.java

@@ -39,7 +39,7 @@ public void missingActiveScope() throws Exception {
     }
 
     @Test
-    public void activateSpan() throws Exception {
+    public void defaultActivate() throws Exception {
         Span span = mock(Span.class);
 
         // We can't use 1.7 features like try-with-resources in this repo without meddling with pom details for tests.
@@ -53,15 +53,15 @@ public void activateSpan() throws Exception {
         }
 
         // Make sure the Span got finish()ed.
-        verify(span, times(1)).finish();
+        verify(span, times(0)).finish();
 
         // And now it's gone:
         Scope missingScope = source.active();
         assertNull(missingScope);
     }
 
     @Test
-    public void activateSpanClose() throws Exception {
+    public void finishSpanClose() throws Exception {
         Span span = mock(Span.class);
 
         // We can't use 1.7 features like try-with-resources in this repo without meddling with pom details for tests.
@@ -81,7 +81,7 @@ public void activateSpanClose() throws Exception {
     }
 
     @Test
-    public void activateSpanNoClose() throws Exception {
+    public void dontFinishSpanNoClose() throws Exception {
         Span span = mock(Span.class);
 
         // We can't use 1.7 features like try-with-resources in this repo without meddling with pom details for tests.

opentracing-util/src/test/java/io/opentracing/util/ThreadLocalScopeTest.java

@@ -17,7 +17,6 @@
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.mockito.Mockito.mock;
-import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
 
@@ -40,12 +39,12 @@ public void implicitSpanStack() throws Exception {
         Span foregroundSpan = mock(Span.class);
 
         // Quasi try-with-resources (this is 1.6).
-        Scope backgroundActive = scopeManager.activate(backgroundSpan);
+        Scope backgroundActive = scopeManager.activate(backgroundSpan, true);
         try {
             assertNotNull(backgroundActive);
 
             // Activate a new Scope on top of the background one.
-            Scope foregroundActive = scopeManager.activate(foregroundSpan);
+            Scope foregroundActive = scopeManager.activate(foregroundSpan, true);
             try {
                 Scope shouldBeForeground = scopeManager.active();
                 assertEquals(foregroundActive, shouldBeForeground);