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 <[email protected]> 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()
