zipkin-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From adrianc...@apache.org
Subject [incubator-zipkin-brave] branch master updated: Makes Tracer.currentSpanCustomizer() lazy (#924)
Date Fri, 14 Jun 2019 05:21:20 GMT
This is an automated email from the ASF dual-hosted git repository.

adriancole pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/incubator-zipkin-brave.git


The following commit(s) were added to refs/heads/master by this push:
     new 68d05d4   Makes Tracer.currentSpanCustomizer() lazy  (#924)
68d05d4 is described below

commit 68d05d4a028dde883318a35e5244b5ace9ad6946
Author: Adrian Cole <adriancole@users.noreply.github.com>
AuthorDate: Fri Jun 14 13:21:14 2019 +0800

     Makes Tracer.currentSpanCustomizer() lazy  (#924)
    
    @lambcode noticed that calling `Tracer.currentSpan()` without using the
    result can lead to orphaned spans. The simplest way to prevent this is
    to defer overhead until data is added with the result.
---
 brave/src/main/java/brave/LazySpan.java            | 143 +++++++++++++++++++++
 brave/src/main/java/brave/RealSpan.java            |   4 +-
 ...anCustomizer.java => SpanCustomizerShield.java} |  32 ++---
 brave/src/main/java/brave/Tracer.java              |  17 ++-
 brave/src/test/java/brave/RealSpanTest.java        |   2 +-
 brave/src/test/java/brave/TracerTest.java          |   2 +-
 .../src/main/java/brave/TracerBenchmarks.java      |  20 +++
 7 files changed, 188 insertions(+), 32 deletions(-)

diff --git a/brave/src/main/java/brave/LazySpan.java b/brave/src/main/java/brave/LazySpan.java
new file mode 100644
index 0000000..b76af49
--- /dev/null
+++ b/brave/src/main/java/brave/LazySpan.java
@@ -0,0 +1,143 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package brave;
+
+import brave.handler.MutableSpan;
+import brave.propagation.TraceContext;
+
+/**
+ * This defers creation of a span until first public method call.
+ *
+ * <p>This type was created to reduce overhead for code that calls {@link Tracer#currentSpan()},
+ * but without ever using the result.
+ */
+final class LazySpan extends Span {
+  final Tracer tracer;
+  final TraceContext context;
+  Span delegate;
+
+  LazySpan(Tracer tracer, TraceContext context) {
+    this.tracer = tracer;
+    this.context = context;
+  }
+
+  @Override public boolean isNoop() {
+    return false;
+  }
+
+  @Override public TraceContext context() {
+    return context;
+  }
+
+  @Override public SpanCustomizer customizer() {
+    return new SpanCustomizerShield(this);
+  }
+
+  @Override public Span start() {
+    return span().start();
+  }
+
+  @Override public Span start(long timestamp) {
+    return span().start(timestamp);
+  }
+
+  @Override public Span name(String name) {
+    return span().name(name);
+  }
+
+  @Override public Span kind(Kind kind) {
+    return span().kind(kind);
+  }
+
+  @Override public Span annotate(String value) {
+    return span().annotate(value);
+  }
+
+  @Override public Span annotate(long timestamp, String value) {
+    return span().annotate(timestamp, value);
+  }
+
+  @Override public Span tag(String key, String value) {
+    return span().tag(key, value);
+  }
+
+  @Override public Span error(Throwable throwable) {
+    return span().error(throwable);
+  }
+
+  @Override public Span remoteServiceName(String remoteServiceName) {
+    return span().remoteServiceName(remoteServiceName);
+  }
+
+  @Override public boolean remoteIpAndPort(String remoteIp, int remotePort) {
+    return span().remoteIpAndPort(remoteIp, remotePort);
+  }
+
+  @Override public void finish() {
+    span().finish();
+  }
+
+  @Override public void finish(long timestamp) {
+    span().finish(timestamp);
+  }
+
+  @Override public void abandon() {
+    if (delegate == null) return; // prevent resurrection
+    span().abandon();
+  }
+
+  @Override public void flush() {
+    if (delegate == null) return; // prevent resurrection
+    span().flush();
+  }
+
+  @Override public String toString() {
+    return "LazySpan(" + context + ")";
+  }
+
+  /**
+   * This also matches equals against a real span. The rationale is least surprise to the
user, as
+   * code should not act differently given an instance of lazy or {@link RealSpan}.
+   */
+  @Override public boolean equals(Object o) {
+    if (o == this) return true;
+    if (o instanceof LazySpan) {
+      return context.equals(((LazySpan) o).context);
+    } else if (o instanceof RealSpan) {
+      return context.equals(((RealSpan) o).context);
+    }
+    return false;
+  }
+
+  /**
+   * This does not guard on all concurrent edge cases assigning the delegate field. That's
because
+   * this type is only used when a user calls {@link Tracer#currentSpan()}, which is unlikley
to be
+   * exposed in such a way that multiple threads end up in a race assigning the field. Finally,
+   * there is no state risk if {@link Tracer#toSpan(TraceContext)} is called concurrently.
Duplicate
+   * instances of span may occur, but they would share the same {@link MutableSpan} instance
+   * internally.
+   */
+  Span span() {
+    Span result = delegate;
+    if (result != null) return result;
+    return delegate = tracer.toSpan(context);
+  }
+
+  @Override public int hashCode() {
+    return context.hashCode();
+  }
+}
diff --git a/brave/src/main/java/brave/RealSpan.java b/brave/src/main/java/brave/RealSpan.java
index 0a900e4..99cbe00 100644
--- a/brave/src/main/java/brave/RealSpan.java
+++ b/brave/src/main/java/brave/RealSpan.java
@@ -29,7 +29,6 @@ final class RealSpan extends Span {
   final MutableSpan state;
   final Clock clock;
   final FinishedSpanHandler finishedSpanHandler;
-  final RealSpanCustomizer customizer;
 
   RealSpan(TraceContext context,
       PendingSpans pendingSpans,
@@ -41,7 +40,6 @@ final class RealSpan extends Span {
     this.pendingSpans = pendingSpans;
     this.state = state;
     this.clock = clock;
-    this.customizer = new RealSpanCustomizer(context, state, clock);
     this.finishedSpanHandler = finishedSpanHandler;
   }
 
@@ -54,7 +52,7 @@ final class RealSpan extends Span {
   }
 
   @Override public SpanCustomizer customizer() {
-    return new RealSpanCustomizer(context, state, clock);
+    return new SpanCustomizerShield(this);
   }
 
   @Override public Span start() {
diff --git a/brave/src/main/java/brave/RealSpanCustomizer.java b/brave/src/main/java/brave/SpanCustomizerShield.java
similarity index 60%
rename from brave/src/main/java/brave/RealSpanCustomizer.java
rename to brave/src/main/java/brave/SpanCustomizerShield.java
index 0cb00a5..1a51777 100644
--- a/brave/src/main/java/brave/RealSpanCustomizer.java
+++ b/brave/src/main/java/brave/SpanCustomizerShield.java
@@ -16,46 +16,32 @@
  */
 package brave;
 
-import brave.handler.MutableSpan;
-import brave.propagation.TraceContext;
+/** This reduces exposure of methods on {@link Span} to those exposed on {@link SpanCustomizer}.
*/
+final class SpanCustomizerShield implements SpanCustomizer {
 
-/** This wraps the public api and guards access to a mutable span. */
-final class RealSpanCustomizer implements SpanCustomizer {
+  final Span delegate;
 
-  final TraceContext context;
-  final MutableSpan state;
-  final Clock clock;
-
-  RealSpanCustomizer(TraceContext context, MutableSpan state, Clock clock) {
-    this.context = context;
-    this.state = state;
-    this.clock = clock;
+  SpanCustomizerShield(Span delegate) {
+    this.delegate = delegate;
   }
 
   @Override public SpanCustomizer name(String name) {
-    synchronized (state) {
-      state.name(name);
-    }
+    delegate.name(name);
     return this;
   }
 
   @Override public SpanCustomizer annotate(String value) {
-    long timestamp = clock.currentTimeMicroseconds();
-    synchronized (state) {
-      state.annotate(timestamp, value);
-    }
+    delegate.annotate(value);
     return this;
   }
 
   @Override public SpanCustomizer tag(String key, String value) {
-    synchronized (state) {
-      state.tag(key, value);
-    }
+    delegate.tag(key, value);
     return this;
   }
 
   @Override
   public String toString() {
-    return "RealSpanCustomizer(" + context + ")";
+    return "SpanCustomizer(" + delegate.customizer() + ")";
   }
 }
diff --git a/brave/src/main/java/brave/Tracer.java b/brave/src/main/java/brave/Tracer.java
index 05f5ec2..d722e96 100644
--- a/brave/src/main/java/brave/Tracer.java
+++ b/brave/src/main/java/brave/Tracer.java
@@ -348,6 +348,10 @@ public class Tracer {
 
   /** Converts the context to a Span object after decorating it for propagation */
   public Span toSpan(TraceContext context) {
+    return _toSpan(decorateExternal(context));
+  }
+
+  TraceContext decorateExternal(TraceContext context) {
     if (context == null) throw new NullPointerException("context == null");
     if (alwaysSampleLocal) {
       int flags = InternalPropagation.instance.flags(context);
@@ -356,7 +360,7 @@ public class Tracer {
       }
     }
     // decorating here addresses join, new traces or children and ad-hoc trace contexts
-    return _toSpan(propagationFactory.decorate(context));
+    return propagationFactory.decorate(context);
   }
 
   Span _toSpan(TraceContext decorated) {
@@ -426,8 +430,7 @@ public class Tracer {
     // note: we don't need to decorate the context for propagation as it is only used for
toString
     TraceContext context = currentTraceContext.get();
     if (context == null || isNoop(context)) return NoopSpanCustomizer.INSTANCE;
-    PendingSpan pendingSpan = pendingSpans.getOrCreate(context, false);
-    return new RealSpanCustomizer(context, pendingSpan.state(), pendingSpan.clock());
+    return new SpanCustomizerShield(toSpan(context));
   }
 
   /**
@@ -438,7 +441,13 @@ public class Tracer {
    */
   @Nullable public Span currentSpan() {
     TraceContext currentContext = currentTraceContext.get();
-    return currentContext != null ? toSpan(currentContext) : null;
+    if (currentContext == null) return null;
+    TraceContext decorated = decorateExternal(currentContext);
+    if (isNoop(decorated)) return new NoopSpan(decorated);
+
+    // Returns a lazy span to reduce overhead when tracer.currentSpan() is invoked just to
see if
+    // one exists, or when the result is never used.
+    return new LazySpan(this, decorated);
   }
 
   /**
diff --git a/brave/src/test/java/brave/RealSpanTest.java b/brave/src/test/java/brave/RealSpanTest.java
index 5ffd55b..6040b00 100644
--- a/brave/src/test/java/brave/RealSpanTest.java
+++ b/brave/src/test/java/brave/RealSpanTest.java
@@ -48,7 +48,7 @@ public class RealSpanTest {
   }
 
   @Test public void hasRealCustomizer() {
-    assertThat(span.customizer()).isInstanceOf(RealSpanCustomizer.class);
+    assertThat(span.customizer()).isInstanceOf(SpanCustomizerShield.class);
   }
 
   @Test public void start() {
diff --git a/brave/src/test/java/brave/TracerTest.java b/brave/src/test/java/brave/TracerTest.java
index 353f275..c204228 100644
--- a/brave/src/test/java/brave/TracerTest.java
+++ b/brave/src/test/java/brave/TracerTest.java
@@ -396,7 +396,7 @@ public class TracerTest {
 
     try {
       assertThat(tracer.currentSpanCustomizer())
-          .isInstanceOf(RealSpanCustomizer.class);
+          .isInstanceOf(SpanCustomizerShield.class);
     } finally {
       parent.finish();
     }
diff --git a/instrumentation/benchmarks/src/main/java/brave/TracerBenchmarks.java b/instrumentation/benchmarks/src/main/java/brave/TracerBenchmarks.java
index d0619d5..799f70f 100644
--- a/instrumentation/benchmarks/src/main/java/brave/TracerBenchmarks.java
+++ b/instrumentation/benchmarks/src/main/java/brave/TracerBenchmarks.java
@@ -19,6 +19,7 @@ package brave;
 import brave.handler.FinishedSpanHandler;
 import brave.handler.MutableSpan;
 import brave.propagation.B3Propagation;
+import brave.propagation.CurrentTraceContext;
 import brave.propagation.ExtraFieldPropagation;
 import brave.propagation.Propagation;
 import brave.propagation.SamplingFlags;
@@ -226,6 +227,25 @@ public class TracerBenchmarks {
     }
   }
 
+  @Benchmark public void currentSpan() {
+    currentSpan(tracer, extracted.context(), false);
+  }
+
+  @Benchmark public void currentSpan_tag() {
+    currentSpan(tracer, extracted.context(), true);
+  }
+
+  @Benchmark public void currentSpan_unsampled() {
+    currentSpan(tracer, unsampledExtracted.context(), false);
+  }
+
+  void currentSpan(Tracer tracer, TraceContext context, boolean tag) {
+    try (CurrentTraceContext.Scope scope = tracer.currentTraceContext.newScope(context))
{
+      Span span = tracer.currentSpan();
+      if (tag) span.tag("customer.id", "1234");
+    }
+  }
+
   // Convenience main entry-point
   public static void main(String[] args) throws Exception {
     Options opt = new OptionsBuilder()


Mime
View raw message