Exposes Tracer.currentSpanCustomizer() and Span.customizer() for safety

Eventhough we made a safe user type `SpanCustomizer`, we didn't provide
a safe alternative to `Tracer.currentSpan()`. This led to routine
leaking of `brave.Span` to users (even if they need to cast to discover
this). While we did have `CurrentSpanCustomizer`, that relied on current
context to work. Particularly the new servlet instrumentation uses
explicit references, which are cheaper.

This adds a couple utilities and uses them:
`Tracer.currentSpanCustomizer()` - safer than `currentSpan()`
`Span.customizer()` - safely masks lifecycle methods from parsers

These are safer as they don't have the ability to abend the span, and
have a chance of holding less state (ex NoopSpanCustomizer is constant).

Future work will introduce `UnsafeSpan`, which can rely on these methods
to ensure higher performance in single-threaded scenarios such as
synchronous calls.

brave/README.md

@@ -478,9 +478,16 @@ noot cache the result. Instead, look them up each time you need them.
 ## Current Span
 
 Brave supports a "current span" concept which represents the in-flight
-operation. `Tracer.currentSpan()` can be used to add custom tags to a
-span and `Tracer.nextSpan()` can be used to create a child of whatever
-is in-flight.
+operation.
+
+`Tracer.currentSpanCustomizer()` never returns null and `SpanCustomizer`
+is generally a safe object to expose to third-party code to add tags.
+
+`Tracer.currentSpan()` should be reserved for framework code that cannot
+reference the span explicitly which to close or abandon an operation.
+
+`Tracer.nextSpan()` uses the "current span" to determine a parent. This
+creates a child of whatever is in-flight.
 
 ### Setting a span in scope via custom executors
 
@@ -504,8 +511,8 @@ c.setExecutorService(currentTraceContext.executorService(realExecutorService));
 
 When writing new instrumentation, it is important to place a span you
 created in scope as the current span. Not only does this allow users to
-access it with `Tracer.currentSpan()`, but it also allows customizations
-like SLF4J MDC to see the current trace IDs.
+access it with `Tracer.currentSpanCustomizer()`, but it also allows
+customizations like SLF4J MDC to see the current trace IDs.
 
 `Tracer.withSpanInScope(Span)` facilitates this and is most conveniently
 employed via the try-with-resources idiom. Whenever external code might
@@ -549,7 +556,8 @@ class MyFilter extends Filter {
     // Assume you have code to start the span and add relevant tags...
 
     // We now set the span in scope so that any code between here and
-    // the end of the request can see it with Tracer.currentSpan()
+    // the end of the request can modify it with Tracer.currentSpan()
+    // or Tracer.currentSpanCustomizer()
     SpanInScope spanInScope = tracer.withSpanInScope(span);
 
     // We don't want to leak the scope, so we place it somewhere we can

brave/src/main/java/brave/CurrentSpanCustomizer.java

@@ -20,37 +20,21 @@ public static CurrentSpanCustomizer create(Tracing tracing) {
 
   /** {@inheritDoc} */
   @Override public SpanCustomizer name(String name) {
-    Span currentSpan = tracer.currentSpan();
-    if (currentSpan != null) {
-      currentSpan.name(name);
-    }
-    return this;
+    return tracer.currentSpanCustomizer().name(name);
   }
 
   /** {@inheritDoc} */
   @Override public SpanCustomizer tag(String key, String value) {
-    Span currentSpan = tracer.currentSpan();
-    if (currentSpan != null) {
-      currentSpan.tag(key, value);
-    }
-    return this;
+    return tracer.currentSpanCustomizer().tag(key, value);
   }
 
   /** {@inheritDoc} */
   @Override public SpanCustomizer annotate(String value) {
-    Span currentSpan = tracer.currentSpan();
-    if (currentSpan != null) {
-      currentSpan.annotate(value);
-    }
-    return this;
+    return tracer.currentSpanCustomizer().annotate(value);
   }
 
   /** {@inheritDoc} */
   @Override public SpanCustomizer annotate(long timestamp, String value) {
-    Span currentSpan = tracer.currentSpan();
-    if (currentSpan != null) {
-      currentSpan.annotate(timestamp, value);
-    }
-    return this;
+    return tracer.currentSpanCustomizer().annotate(timestamp, value);
   }
 }

brave/src/main/java/brave/NoopSpan.java

@@ -11,6 +11,10 @@ static NoopSpan create(TraceContext context) {
     return new AutoValue_NoopSpan(context);
   }
 
+  @Override public SpanCustomizer customizer() {
+    return NoopSpanCustomizer.INSTANCE;
+  }
+
   @Override public boolean isNoop() {
     return true;
   }

brave/src/main/java/brave/NoopSpanCustomizer.java

@@ -0,0 +1,22 @@
+package brave;
+
+// Preferred to a constant NOOP in SpanCustomizer as the latter ends up in a hierachy including Span
+enum NoopSpanCustomizer implements SpanCustomizer {
+  INSTANCE;
+
+  @Override public SpanCustomizer name(String name) {
+    return this;
+  }
+
+  @Override public SpanCustomizer tag(String key, String value) {
+    return this;
+  }
+
+  @Override public SpanCustomizer annotate(String value) {
+    return this;
+  }
+
+  @Override public SpanCustomizer annotate(long timestamp, String value) {
+    return this;
+  }
+}

brave/src/main/java/brave/RealSpan.java

@@ -12,7 +12,7 @@ abstract class RealSpan extends Span {
   abstract Recorder recorder();
 
   static RealSpan create(TraceContext context, Recorder recorder) {
-    return new AutoValue_RealSpan(context, recorder);
+    return new AutoValue_RealSpan(context, RealSpanCustomizer.create(context, recorder), recorder);
   }
 
   @Override public boolean isNoop() {

brave/src/main/java/brave/RealSpanCustomizer.java

@@ -0,0 +1,42 @@
+package brave;
+
+import brave.internal.recorder.Recorder;
+import brave.propagation.TraceContext;
+import com.google.auto.value.AutoValue;
+
+/** This wraps the public api and guards access to a mutable span. */
+@AutoValue
+abstract class RealSpanCustomizer implements SpanCustomizer {
+
+  abstract TraceContext context();
+  abstract Recorder recorder();
+
+  static RealSpanCustomizer create(TraceContext context, Recorder recorder) {
+    return new AutoValue_RealSpanCustomizer(context, recorder);
+  }
+
+  @Override public SpanCustomizer name(String name) {
+    recorder().name(context(), name);
+    return this;
+  }
+
+  @Override public SpanCustomizer annotate(String value) {
+    recorder().annotate(context(), value);
+    return this;
+  }
+
+  @Override public SpanCustomizer annotate(long timestamp, String value) {
+    recorder().annotate(context(), timestamp, value);
+    return this;
+  }
+
+  @Override public SpanCustomizer tag(String key, String value) {
+    recorder().tag(context(), key, value);
+    return this;
+  }
+
+  @Override
+  public String toString() {
+    return "RealSpanCustomizer(" + context() + ")";
+  }
+}

brave/src/main/java/brave/Span.java

@@ -54,6 +54,9 @@ public enum Kind {
 
   public abstract TraceContext context();
 
+  /** Returns a customizer appropriate for the current span. Prefer this when invoking user code */
+  public abstract SpanCustomizer customizer();
+
   /**
    * Starts the span with an implicit timestamp.
    *

brave/src/main/java/brave/Tracer.java

@@ -48,7 +48,6 @@
  * @see Propagation
  */
 public final class Tracer {
-
   /** @deprecated Please use {@link Tracing#newBuilder()} */
   @Deprecated public static Builder newBuilder() {
     return new Builder();
@@ -331,7 +330,8 @@ long nextId() {
 
   /**
    * Makes the given span the "current span" and returns an object that exits that scope on close.
-   * The span provided will be returned by {@link #currentSpan()} until the return value is closed.
+   * Calls to {@link #currentSpan()} and {@link #currentSpanCustomizer()} will affect this span
+   * until the return value is closed.
    *
    * <p>The most convenient way to use this method is via the try-with-resources idiom.
    *
@@ -344,8 +344,7 @@ long nextId() {
    *   span.finish();
    * }
    *
-   * // An unrelated framework interceptor can now lookup the correct parent for an outbound
-   * request
+   * // An unrelated framework interceptor can now lookup the correct parent for outbound requests
    * Span parent = tracer.currentSpan()
    * Span span = tracer.nextSpan().name("outbound").start(); // parent is implicitly looked up
    * try (SpanInScope ws = tracer.withSpanInScope(span)) {
@@ -366,7 +365,25 @@ public SpanInScope withSpanInScope(@Nullable Span span) {
     return new SpanInScope(currentTraceContext.newScope(span != null ? span.context() : null));
   }
 
-  /** Returns the current span in scope or null if there isn't one. */
+  /**
+   * Returns a customizer for current span in scope or noop if there isn't one.
+   *
+   * <p>Unlike {@link CurrentSpanCustomizer}, this represents a single span. Accordingly, this
+   * reference should not be saved as a field. That said, it is more efficient to save this result
+   * as a method-local variable vs repeated calls to {@link #currentSpanCustomizer()}.
+   */
+  public SpanCustomizer currentSpanCustomizer() {
+    TraceContext currentContext = currentTraceContext.get();
+    return currentContext != null
+        ? RealSpanCustomizer.create(currentContext, recorder) : NoopSpanCustomizer.INSTANCE;
+  }
+
+  /**
+   * Returns the current span in scope or null if there isn't one.
+   *
+   * <p>When entering user code, prefer {@link #currentSpanCustomizer()} as it is a stable type and
+   * will never return null.
+   */
   @Nullable public Span currentSpan() {
     TraceContext currentContext = currentTraceContext.get();
     return currentContext != null ? toSpan(currentContext) : null;

brave/src/main/java/brave/Tracing.java

@@ -230,9 +230,9 @@ public Builder sampler(Sampler sampler) {
     }
 
     /**
-     * Responsible for implementing {@link Tracer#currentSpan()} and {@link
-     * Tracer#withSpanInScope(Span)}. By default a simple thread-local is used. Override to support
-     * other mechanisms or to synchronize with other mechanisms such as SLF4J's MDC.
+     * Responsible for implementing {@link Tracer#currentSpanCustomizer()}, {@link Tracer#currentSpan()}
+     * and {@link Tracer#withSpanInScope(Span)}. By default a simple thread-local is used. Override
+     * to support other mechanisms or to synchronize with other mechanisms such as SLF4J's MDC.
      */
     public Builder currentTraceContext(CurrentTraceContext currentTraceContext) {
       if (currentTraceContext == null) throw new NullPointerException("currentTraceContext == null");

brave/src/test/java/brave/NoopSpanTest.java

@@ -31,6 +31,10 @@ public class NoopSpanTest {
     assertThat(span.context().spanId()).isNotZero();
   }
 
+  @Test public void hasNoopCustomizer() {
+    assertThat(span.customizer()).isSameAs(NoopSpanCustomizer.INSTANCE);
+  }
+
   @Test public void doesNothing() {
     // Since our clock and spanReporter throw, we know this is doing nothing
     span.start();

brave/src/test/java/brave/RealSpanCustomizerTest.java

@@ -0,0 +1,54 @@
+package brave;
+
+import java.util.ArrayList;
+import java.util.List;
+import org.junit.After;
+import org.junit.Test;
+import zipkin2.Annotation;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.entry;
+
+public class RealSpanCustomizerTest {
+  List<zipkin2.Span> spans = new ArrayList();
+  Tracer tracer = Tracing.newBuilder().spanReporter(spans::add).build().tracer();
+  Span span = tracer.newTrace();
+  SpanCustomizer spanCustomizer = span.customizer();
+
+  @After public void close() {
+    Tracing.current().close();
+  }
+
+  @Test public void name() {
+    spanCustomizer.name("foo");
+    span.flush();
+
+    assertThat(spans).extracting(zipkin2.Span::name)
+        .containsExactly("foo");
+  }
+
+  @Test public void annotate() {
+    spanCustomizer.annotate("foo");
+    span.flush();
+
+    assertThat(spans).flatExtracting(zipkin2.Span::annotations)
+        .extracting(Annotation::value)
+        .containsExactly("foo");
+  }
+
+  @Test public void annotate_timestamp() {
+    spanCustomizer.annotate(2, "foo");
+    span.flush();
+
+    assertThat(spans).flatExtracting(zipkin2.Span::annotations)
+        .containsExactly(Annotation.create(2L, "foo"));
+  }
+
+  @Test public void tag() {
+    spanCustomizer.tag("foo", "bar");
+    span.flush();
+
+    assertThat(spans).flatExtracting(s -> s.tags().entrySet())
+        .containsExactly(entry("foo", "bar"));
+  }
+}

brave/src/test/java/brave/RealSpanTest.java

@@ -26,6 +26,10 @@ public class RealSpanTest {
     assertThat(span.context().spanId()).isNotZero();
   }
 
+  @Test public void hasRealCustomizer() {
+    assertThat(span.customizer()).isInstanceOf(RealSpanCustomizer.class);
+  }
+
   @Test public void start() {
     span.start();
     span.flush();

brave/src/test/java/brave/TracerTest.java

@@ -270,6 +270,11 @@ public class TracerTest {
         .isInstanceOf(NoopSpan.class);
   }
 
+  @Test public void currentSpanCustomizer_defaultsToNoop() {
+    assertThat(tracer.currentSpanCustomizer())
+        .isSameAs(NoopSpanCustomizer.INSTANCE);
+  }
+
   @Test public void currentSpan_defaultsToNull() {
     assertThat(tracer.currentSpan()).isNull();
   }
@@ -378,6 +383,9 @@ public class TracerTest {
     try (Tracer.SpanInScope ws = tracer.withSpanInScope(current)) {
       assertThat(tracer.currentSpan())
           .isEqualTo(current);
+      assertThat(tracer.currentSpanCustomizer())
+          .isNotEqualTo(current)
+          .isNotEqualTo(NoopSpanCustomizer.INSTANCE);
     }
 
     // context was cleared
@@ -441,6 +449,8 @@ public class TracerTest {
       try (Tracer.SpanInScope clearScope = tracer.withSpanInScope(null)) {
         assertThat(tracer.currentSpan())
             .isNull();
+        assertThat(tracer.currentSpanCustomizer())
+            .isEqualTo(NoopSpanCustomizer.INSTANCE);
       }
 
       // old parent reverted

instrumentation/dubbo-rpc/src/main/java/brave/dubbo/rpc/TracingFilter.java

@@ -83,7 +83,7 @@ public Result invoke(Invoker<?> invoker, Invocation invocation) throws RpcExcept
     try (Tracer.SpanInScope scope = tracer.withSpanInScope(span)) {
       Result result = invoker.invoke(invocation);
       if (result.hasException()) {
-        onError(result.getException(), span);
+        onError(result.getException(), span.customizer());
       }
       isOneway = RpcUtils.isOneway(invoker.getUrl(), invocation);
       Future<Object> future = rpcContext.getFuture(); // the case on async client invocation
@@ -93,7 +93,7 @@ public Result invoke(Invoker<?> invoker, Invocation invocation) throws RpcExcept
       }
       return result;
     } catch (Error | RuntimeException e) {
-      onError(e, span);
+      onError(e, span.customizer());
       throw e;
     } finally {
       if (isOneway) {