zipkin-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From adrianc...@apache.org
Subject [incubator-zipkin-brave] 01/01: Makes Tracer.currentSpanCustomizer() lazy
Date Wed, 12 Jun 2019 11:11:41 GMT
This is an automated email from the ASF dual-hosted git repository.

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

commit 32d2c835d7a439c3767fd1bed6acde4d6ad360f1
Author: Adrian Cole <acole@pivotal.io>
AuthorDate: Wed Jun 12 19:07:17 2019 +0800

    Makes Tracer.currentSpanCustomizer() lazy
    
    @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.
    
    Fixes #920
---
 brave/src/main/java/brave/LazySpan.java            | 126 +++++++++++++++++++++
 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      |  24 ++++
 7 files changed, 175 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..13744cc
--- /dev/null
+++ b/brave/src/main/java/brave/LazySpan.java
@@ -0,0 +1,126 @@
+/*
+ * 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.propagation.TraceContext;
+
+/** This wraps the public api and guards access to a mutable span. */
+final class LazySpan extends Span {
+
+  final Tracer tracer;
+  final TraceContext context;
+  volatile 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 + ")";
+  }
+
+  @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;
+  }
+
+  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..2ea962e 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,29 @@ public class TracerBenchmarks {
     }
   }
 
+  @Benchmark public void currentSpan() {
+    currentSpan(tracer, extracted.context(), false);
+  }
+
+  @Benchmark public void currentSpan_unsampled() {
+    currentSpan(tracer, unsampledExtracted.context(), false);
+  }
+
+  @Benchmark public void currentSpan_tag() {
+    currentSpan(tracer, extracted.context(), true);
+  }
+
+  @Benchmark public void currentSpan_tag_unsampled() {
+    currentSpan(tracer, unsampledExtracted.context(), true);
+  }
+
+  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