You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@zipkin.apache.org by ad...@apache.org on 2019/06/14 05:21:20 UTC
[incubator-zipkin-brave] branch master updated: Makes
Tracer.currentSpanCustomizer() lazy (#924)
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 <ad...@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()