Remove the try-with-resources support for the Span. This is important because we want to discourage the usage of the Span and encourage the usage of ScopedSpan. (#127)

6 files changed, 59 insertions, 77 deletions

diff --git a/core/src/main/java/com/google/instrumentation/trace/ScopedSpanHandle.java b/core/src/main/java/com/google/instrumentation/trace/ScopedSpanHandle.java
index 1ac67a45..152e17e3 100644
--- a/core/src/main/java/com/google/instrumentation/trace/ScopedSpanHandle.java
+++ b/core/src/main/java/com/google/instrumentation/trace/ScopedSpanHandle.java
@@ -18,7 +18,7 @@ import com.google.instrumentation.common.NonThrowingCloseable;
 /**
  * Defines a scope of code where the given {@link Span} is in the current context. The scope is
  * exited when the object is closed and the previous context is restored. When the scope exits the
- * given {@link Span} will be ended using {@link Span#end}.
+ * given {@code Span} will be ended using {@link Span#end}.
  *
  * <p>Supports try-with-resource idiom.
  */
@@ -27,7 +27,7 @@ final class ScopedSpanHandle implements NonThrowingCloseable {
   private final NonThrowingCloseable withSpan;
 
   /**
-   * Creates a {@link ScopedSpanHandle}
+   * Creates a {@code ScopedSpanHandle}
    *
    * @param span The span that will be installed in the current context.
    * @param contextSpanHandler The handler that is used to interact with the current context.
@@ -38,12 +38,11 @@ final class ScopedSpanHandle implements NonThrowingCloseable {
   }
 
   /**
-   * Uninstalls the {@code Span} from the current context and ends it with default options. If the
-   * span has been already ended then {@link Span#end()} is a no-op.
+   * Uninstalls the {@code Span} from the current context and ends it by calling {@link Span#end()}.
    */
   @Override
   public void close() {
     withSpan.close();
-    span.end(EndSpanOptions.DEFAULT);
+    span.end();
   }
 }
diff --git a/core/src/main/java/com/google/instrumentation/trace/Span.java b/core/src/main/java/com/google/instrumentation/trace/Span.java
index 28ed14a7..55ab5a37 100644
--- a/core/src/main/java/com/google/instrumentation/trace/Span.java
+++ b/core/src/main/java/com/google/instrumentation/trace/Span.java
@@ -15,8 +15,6 @@ package com.google.instrumentation.trace;
 
 import static com.google.common.base.Preconditions.checkNotNull;
 
-import com.google.instrumentation.common.NonThrowingCloseable;
-
 import java.util.EnumSet;
 import java.util.Set;
 import javax.annotation.Nullable;
@@ -27,10 +25,9 @@ import javax.annotation.Nullable;
  *
  * <p>Spans are created by the {@link SpanBuilder#startSpan} method.
  *
- * <p>{@code Span} implements NonThrowingCloseable, to support try-with-resource idiom. See {@link
- * Span#close}.
+ * <p>{@code Span} <b>must</b> be ended by calling {@link #end()} or {@link #end(EndSpanOptions)}
  */
-public abstract class Span implements NonThrowingCloseable {
+public abstract class Span {
   // Contains the identifiers associated with this Span.
   private final SpanContext context;
 
@@ -146,7 +143,6 @@ public abstract class Span implements NonThrowingCloseable {
   /**
    * Ends the current {@code Span} by calling {@link #end}.
    */
-  @Override
   public final void close() {
     end(EndSpanOptions.DEFAULT);
   }
diff --git a/core/src/main/java/com/google/instrumentation/trace/SpanBuilder.java b/core/src/main/java/com/google/instrumentation/trace/SpanBuilder.java
index 25e2db56..07fcde75 100644
--- a/core/src/main/java/com/google/instrumentation/trace/SpanBuilder.java
+++ b/core/src/main/java/com/google/instrumentation/trace/SpanBuilder.java
@@ -31,11 +31,10 @@ import javax.annotation.Nullable;
  *   private static final Tracer tracer = Tracer.getTracer();
  *   void doWork(Span parent) {
  *     // Create a Span as a child of the given parent.
- *     try (Span span = tracer.spanBuilder(parent, "MyChildSpan").startSpan()) {
- *       span.addAnnotation("my annotation");
- *       doSomeWork(span); // Manually propagate the new span down the stack.
- *     }
- *     // "span" will be ended here.
+ *     Span span = tracer.spanBuilder(parent, "MyChildSpan").startSpan();
+ *     span.addAnnotation("my annotation");
+ *     doSomeWork(span); // Manually propagate the new span down the stack.
+ *     span.end();  // Manually end the span.
  *   }
  * }
  * }</pre>
@@ -187,7 +186,8 @@ public final class SpanBuilder {
   /**
    * Starts a new {@link Span}.
    *
-   * <p>If used without try-with-resources <b>must</b> manually call {@link Span#end}.
+   * <p>Users <b>must</b> manually call {@link Span#end()} or {@link Span#end(EndSpanOptions)} to
+   * end this {@code Span}.
    *
    * <p>Does not install the newly created {@code Span} to the current Context.
    *
@@ -197,30 +197,16 @@ public final class SpanBuilder {
    * class MyClass {
    *   private static final Tracer tracer = Tracer.getTracer();
    *   void DoWork() {
-   *     try (Span span = tracer.spanBuilder(null, "MyRootSpan").startSpan()) {
-   *       span.addAnnotation("We did X");
-   *     }
-   *   }
-   * }
-   * }</pre>
-   *
-   * <p>Prior to Java SE 7, you can use a finally block to ensure that a resource is closed (the
-   * {@code Span} is ended) regardless of whether the try statement completes normally or abruptly.
-   *
-   * <p>Example of usage prior to Java SE7:
-   *
-   * <pre>{@code
-   * class MyClass {
-   *   private static final Tracer tracer = Tracer.getTracer();
-   *   void DoWork() {
    *     Span span = tracer.spanBuilder(null, "MyRootSpan").startSpan();
+   *     span.addAnnotation("my annotation");
    *     try {
-   *       span.addAnnotation("We did X");
+   *       doSomeWork(span); // Manually propagate the new span down the stack.
    *     } finally {
-   *       span.close();
+   *       // To make sure we end the span even in case of an exception.
+   *       span.end();  // Manually end the span.
    *     }
    *   }
-   *  }
+   * }
    * }</pre>
    *
    * @return the newly created {@code Span}.
diff --git a/core/src/test/java/com/google/instrumentation/trace/SpanBuilderTest.java b/core/src/test/java/com/google/instrumentation/trace/SpanBuilderTest.java
index 14cc7535..56679585 100644
--- a/core/src/test/java/com/google/instrumentation/trace/SpanBuilderTest.java
+++ b/core/src/test/java/com/google/instrumentation/trace/SpanBuilderTest.java
@@ -94,9 +94,10 @@ public class SpanBuilderTest {
     when(spanFactory.startSpan(
             isNull(SpanContext.class), eq(false), same(SPAN_NAME), eq(new StartSpanOptions())))
         .thenReturn(span);
-    try (Span rootSpan = spanBuilder.becomeRoot().startSpan()) {
-      assertThat(rootSpan).isEqualTo(span);
-    }
+    Span rootSpan = spanBuilder.becomeRoot().startSpan();
+    assertThat(rootSpan).isEqualTo(span);
+    rootSpan.end();
+    verify(span).end(same(EndSpanOptions.DEFAULT));
   }
 
   @Test
@@ -107,9 +108,10 @@ public class SpanBuilderTest {
     when(spanFactory.startSpan(
             isNull(SpanContext.class), eq(false), same(SPAN_NAME), eq(new StartSpanOptions())))
         .thenReturn(span);
-    try (Span rootSpan = spanBuilder.startSpan()) {
-      assertThat(rootSpan).isEqualTo(span);
-    }
+    Span rootSpan = spanBuilder.startSpan();
+    assertThat(rootSpan).isEqualTo(span);
+    rootSpan.end();
+    verify(span).end(same(EndSpanOptions.DEFAULT));
   }
 
   @Test
@@ -121,14 +123,15 @@ public class SpanBuilderTest {
     when(spanFactory.startSpan(
             isNull(SpanContext.class), eq(false), same(SPAN_NAME), eq(startSpanOptions)))
         .thenReturn(span);
-    try (Span rootSpan =
+    Span rootSpan =
         spanBuilder
             .becomeRoot()
             .setSampler(Samplers.neverSample())
             .setParentLinks(parentList)
-            .startSpan()) {
-      assertThat(rootSpan).isEqualTo(span);
-    }
+            .startSpan();
+    assertThat(rootSpan).isEqualTo(span);
+    rootSpan.end();
+    verify(span).end(same(EndSpanOptions.DEFAULT));
   }
 
   @Test
@@ -136,9 +139,10 @@ public class SpanBuilderTest {
     when(spanFactory.startSpan(
             same(spanContext), eq(false), same(SPAN_NAME), eq(new StartSpanOptions())))
         .thenReturn(span);
-    try (Span childSpan = spanBuilder.startSpan()) {
-      assertThat(childSpan).isEqualTo(span);
-    }
+    Span childSpan = spanBuilder.startSpan();
+    assertThat(childSpan).isEqualTo(span);
+    childSpan.end();
+    verify(span).end(same(EndSpanOptions.DEFAULT));
   }
 
   @Test
@@ -148,10 +152,11 @@ public class SpanBuilderTest {
     startSpanOptions.setRecordEvents(true);
     when(spanFactory.startSpan(same(spanContext), eq(false), same(SPAN_NAME), eq(startSpanOptions)))
         .thenReturn(span);
-    try (Span childSpan =
-        spanBuilder.setSampler(Samplers.neverSample()).setRecordEvents(true).startSpan()) {
-      assertThat(childSpan).isEqualTo(span);
-    }
+    Span childSpan =
+        spanBuilder.setSampler(Samplers.neverSample()).setRecordEvents(true).startSpan();
+    assertThat(childSpan).isEqualTo(span);
+    childSpan.end();
+    verify(span).end(same(EndSpanOptions.DEFAULT));
   }
 
   @Test
@@ -162,9 +167,10 @@ public class SpanBuilderTest {
     when(spanFactory.startSpan(
             same(spanContext), eq(true), same(SPAN_NAME), eq(new StartSpanOptions())))
         .thenReturn(span);
-    try (Span remoteChildSpan = spanBuilder.startSpan()) {
-      assertThat(remoteChildSpan).isEqualTo(span);
-    }
+    Span remoteChildSpan = spanBuilder.startSpan();
+    assertThat(remoteChildSpan).isEqualTo(span);
+    remoteChildSpan.end();
+    verify(span).end(same(EndSpanOptions.DEFAULT));
   }
 
   @Test
@@ -175,8 +181,9 @@ public class SpanBuilderTest {
     when(spanFactory.startSpan(
             isNull(SpanContext.class), eq(true), same(SPAN_NAME), eq(new StartSpanOptions())))
         .thenReturn(span);
-    try (Span remoteChildSpan = spanBuilder.startSpan()) {
-      assertThat(remoteChildSpan).isEqualTo(span);
-    }
+    Span remoteChildSpan = spanBuilder.startSpan();
+    assertThat(remoteChildSpan).isEqualTo(span);
+    remoteChildSpan.end();
+    verify(span).end(same(EndSpanOptions.DEFAULT));
   }
 }
diff --git a/core/src/test/java/com/google/instrumentation/trace/SpanTest.java b/core/src/test/java/com/google/instrumentation/trace/SpanTest.java
index 8c3a36d5..0dae18eb 100644
--- a/core/src/test/java/com/google/instrumentation/trace/SpanTest.java
+++ b/core/src/test/java/com/google/instrumentation/trace/SpanTest.java
@@ -68,13 +68,6 @@ public class SpanTest {
     verify(span).end(same(EndSpanOptions.DEFAULT));
   }
 
-  @Test
-  public void closeCallsEndWithDefaultOptions() {
-    Span span = Mockito.spy(new NoopSpan(spanContext, spanOptions));
-    span.close();
-    verify(span).end(same(EndSpanOptions.DEFAULT));
-  }
-
   // No-op implementation of the Span for testing only.
   private static class NoopSpan extends Span {
     private NoopSpan(SpanContext context, EnumSet<Span.Options> options) {
diff --git a/core/src/test/java/com/google/instrumentation/trace/TracerTest.java b/core/src/test/java/com/google/instrumentation/trace/TracerTest.java
index 38fc10f8..1c338883 100644
--- a/core/src/test/java/com/google/instrumentation/trace/TracerTest.java
+++ b/core/src/test/java/com/google/instrumentation/trace/TracerTest.java
@@ -200,10 +200,10 @@ public class TracerTest {
     when(spanFactory.startSpan(
             isNull(SpanContext.class), eq(false), eq("MySpanName"), eq(new StartSpanOptions())))
         .thenReturn(span);
-    try (Span rootSpan =
-        mockTracer.spanBuilder(BlankSpan.INSTANCE, "MySpanName").becomeRoot().startSpan()) {
-      assertThat(rootSpan).isEqualTo(span);
-    }
+    Span rootSpan =
+        mockTracer.spanBuilder(BlankSpan.INSTANCE, "MySpanName").becomeRoot().startSpan();
+    assertThat(rootSpan).isEqualTo(span);
+    rootSpan.end();
     verify(span).end(same(EndSpanOptions.DEFAULT));
   }
 
@@ -216,9 +216,9 @@ public class TracerTest {
             eq("MySpanName"),
             eq(new StartSpanOptions())))
         .thenReturn(span);
-    try (Span childSpan = mockTracer.spanBuilder(BlankSpan.INSTANCE, "MySpanName").startSpan()) {
-      assertThat(childSpan).isEqualTo(span);
-    }
+    Span childSpan = mockTracer.spanBuilder(BlankSpan.INSTANCE, "MySpanName").startSpan();
+    assertThat(childSpan).isEqualTo(span);
+    childSpan.end();
     verify(span).end(same(EndSpanOptions.DEFAULT));
   }
 
@@ -234,10 +234,11 @@ public class TracerTest {
     when(spanFactory.startSpan(
             same(spanContext), eq(true), eq("MySpanName"), eq(new StartSpanOptions())))
         .thenReturn(span);
-    try (Span remoteChildSpan =
-        mockTracer.spanBuilderWithRemoteParent(spanContext, "MySpanName").startSpan()) {
-      assertThat(remoteChildSpan).isEqualTo(span);
-    }
+    Span remoteChildSpan =
+        mockTracer.spanBuilderWithRemoteParent(spanContext, "MySpanName").startSpan();
+    assertThat(remoteChildSpan).isEqualTo(span);
+    remoteChildSpan.end();
+    verify(span).end(same(EndSpanOptions.DEFAULT));
   }
 
   private Tracer newTracerWithMocks() {