From 532f4af28f3160335e78a3a6ac05b4a7664d9cd0 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Thu, 7 Feb 2019 15:54:37 -0800 Subject: [PATCH 1/5] New API: Add shutdown callback to shutdown the Tracer Also add getMeta() to Span interface. --- .../main/java/datadog/trace/tracer/Span.java | 10 +++++++ .../java/datadog/trace/tracer/SpanImpl.java | 2 ++ .../java/datadog/trace/tracer/Tracer.java | 28 +++++++++++++++++-- 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/dd-trace/src/main/java/datadog/trace/tracer/Span.java b/dd-trace/src/main/java/datadog/trace/tracer/Span.java index 78f4e9f3d6..454927cf52 100644 --- a/dd-trace/src/main/java/datadog/trace/tracer/Span.java +++ b/dd-trace/src/main/java/datadog/trace/tracer/Span.java @@ -1,5 +1,7 @@ package datadog.trace.tracer; +import java.util.Map; + /** * A single measurement of time with arbitrary key-value attributes. * @@ -94,6 +96,13 @@ public interface Span { */ void attachThrowable(Throwable throwable); + /** + * Get all the metadata attached to this span. + * + * @return + */ + Map getMeta(); + /** * Get a meta value on a span. * @@ -127,5 +136,6 @@ public interface Span { * * @param finishTimestampNanoseconds Epoch time in nanoseconds. */ + // FIXME: This should take a Timestamp object instead. void finish(long finishTimestampNanoseconds); } diff --git a/dd-trace/src/main/java/datadog/trace/tracer/SpanImpl.java b/dd-trace/src/main/java/datadog/trace/tracer/SpanImpl.java index 23a8386ede..7ffd5a2e53 100644 --- a/dd-trace/src/main/java/datadog/trace/tracer/SpanImpl.java +++ b/dd-trace/src/main/java/datadog/trace/tracer/SpanImpl.java @@ -212,6 +212,7 @@ class SpanImpl implements Span { } } + @Override @JsonGetter("meta") synchronized Map getMeta() { final Map result = new HashMap<>(meta.size()); @@ -264,6 +265,7 @@ class SpanImpl implements Span { } } + // FIXME: This should take a Timestamp object instead. @Override public synchronized void finish(final long finishTimestampNanoseconds) { if (isFinished()) { diff --git a/dd-trace/src/main/java/datadog/trace/tracer/Tracer.java b/dd-trace/src/main/java/datadog/trace/tracer/Tracer.java index 9f153b274e..1f01deb4ee 100644 --- a/dd-trace/src/main/java/datadog/trace/tracer/Tracer.java +++ b/dd-trace/src/main/java/datadog/trace/tracer/Tracer.java @@ -5,7 +5,6 @@ import datadog.trace.tracer.sampling.AllSampler; import datadog.trace.tracer.sampling.Sampler; import datadog.trace.tracer.writer.Writer; import java.io.Closeable; -import java.io.IOException; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -29,6 +28,12 @@ public class Tracer implements Closeable { /** Interceptors to be called on certain trace and span events */ private final List interceptors; + /** + * JVM shutdown callback, keeping a reference to it to remove this if DDTracer gets destroyed + * earlier + */ + private final Thread shutdownCallback; + @Builder private Tracer( final Config config, @@ -45,6 +50,19 @@ public class Tracer implements Closeable { interceptors != null ? Collections.unmodifiableList(new ArrayList<>(interceptors)) : Collections.emptyList(); + + shutdownCallback = + new Thread() { + @Override + public void run() { + close(); + } + }; + try { + Runtime.getRuntime().addShutdownHook(shutdownCallback); + } catch (final IllegalStateException ex) { + // The JVM is already shutting down. + } } /** @return {@link Writer} used by this tracer */ @@ -124,7 +142,13 @@ public class Tracer implements Closeable { } @Override - public void close() throws IOException { + public void finalize() { + Runtime.getRuntime().removeShutdownHook(shutdownCallback); + shutdownCallback.run(); + } + + @Override + public void close() { writer.close(); } } From 65add99a7bb907fa8050d0568763cf13148a0d31 Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Fri, 8 Feb 2019 10:44:59 -0800 Subject: [PATCH 2/5] Make shutdown callback weakly reference the tracer. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This should allow finalize to actually be called. (Also assumes the weak reference isn’t cleared until after finalize is called.) --- .../java/datadog/opentracing/DDTracer.java | 25 ++++++++++++----- .../java/datadog/trace/tracer/SpanImpl.java | 4 +-- .../java/datadog/trace/tracer/Tracer.java | 27 +++++++++++++------ 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java index 84805f2ffa..4f4c067fdc 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java @@ -25,6 +25,7 @@ import io.opentracing.SpanContext; import io.opentracing.propagation.Format; import io.opentracing.propagation.TextMap; import java.io.Closeable; +import java.lang.ref.WeakReference; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -227,13 +228,7 @@ public class DDTracer implements io.opentracing.Tracer, Closeable, datadog.trace this.serviceNameMappings = serviceNameMappings; this.maxTraceSizeBeforePartialFlush = maxTraceSizeBeforePartialFlush; - shutdownCallback = - new Thread() { - @Override - public void run() { - close(); - } - }; + shutdownCallback = new ShutdownHook(this); try { Runtime.getRuntime().addShutdownHook(shutdownCallback); } catch (final IllegalStateException ex) { @@ -724,4 +719,20 @@ public class DDTracer implements io.opentracing.Tracer, Closeable, datadog.trace return context; } } + + private static class ShutdownHook extends Thread { + private final WeakReference reference; + + private ShutdownHook(final DDTracer tracer) { + reference = new WeakReference<>(tracer); + } + + @Override + public void run() { + final DDTracer tracer = reference.get(); + if (tracer != null) { + tracer.close(); + } + } + } } diff --git a/dd-trace/src/main/java/datadog/trace/tracer/SpanImpl.java b/dd-trace/src/main/java/datadog/trace/tracer/SpanImpl.java index 7ffd5a2e53..54854a755a 100644 --- a/dd-trace/src/main/java/datadog/trace/tracer/SpanImpl.java +++ b/dd-trace/src/main/java/datadog/trace/tracer/SpanImpl.java @@ -214,8 +214,8 @@ class SpanImpl implements Span { @Override @JsonGetter("meta") - synchronized Map getMeta() { - final Map result = new HashMap<>(meta.size()); + public synchronized Map getMeta() { + final Map result = new HashMap<>(meta.size()); for (final Map.Entry entry : meta.entrySet()) { result.put(entry.getKey(), String.valueOf(entry.getValue())); } diff --git a/dd-trace/src/main/java/datadog/trace/tracer/Tracer.java b/dd-trace/src/main/java/datadog/trace/tracer/Tracer.java index 1f01deb4ee..f309f0a0f2 100644 --- a/dd-trace/src/main/java/datadog/trace/tracer/Tracer.java +++ b/dd-trace/src/main/java/datadog/trace/tracer/Tracer.java @@ -5,6 +5,7 @@ import datadog.trace.tracer.sampling.AllSampler; import datadog.trace.tracer.sampling.Sampler; import datadog.trace.tracer.writer.Writer; import java.io.Closeable; +import java.lang.ref.WeakReference; import java.util.ArrayList; import java.util.Collections; import java.util.List; @@ -29,7 +30,7 @@ public class Tracer implements Closeable { private final List interceptors; /** - * JVM shutdown callback, keeping a reference to it to remove this if DDTracer gets destroyed + * JVM shutdown callback, keeping a reference to it to remove this if Tracer gets destroyed * earlier */ private final Thread shutdownCallback; @@ -51,13 +52,7 @@ public class Tracer implements Closeable { ? Collections.unmodifiableList(new ArrayList<>(interceptors)) : Collections.emptyList(); - shutdownCallback = - new Thread() { - @Override - public void run() { - close(); - } - }; + shutdownCallback = new ShutdownHook(this); try { Runtime.getRuntime().addShutdownHook(shutdownCallback); } catch (final IllegalStateException ex) { @@ -151,4 +146,20 @@ public class Tracer implements Closeable { public void close() { writer.close(); } + + private static class ShutdownHook extends Thread { + private final WeakReference reference; + + private ShutdownHook(final Tracer tracer) { + reference = new WeakReference<>(tracer); + } + + @Override + public void run() { + final Tracer tracer = reference.get(); + if (tracer != null) { + tracer.close(); + } + } + } } From 0e1dce868c92be4ec04bde2e083a23f9537f2d3b Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Fri, 8 Feb 2019 11:09:24 -0800 Subject: [PATCH 3/5] Log exceptions from Tracer.finalize() --- .../src/main/java/datadog/opentracing/DDTracer.java | 8 ++++++-- dd-trace/src/main/java/datadog/trace/tracer/Tracer.java | 8 ++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java index 4f4c067fdc..c795dfe4bf 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java @@ -261,8 +261,12 @@ public class DDTracer implements io.opentracing.Tracer, Closeable, datadog.trace @Override public void finalize() { - Runtime.getRuntime().removeShutdownHook(shutdownCallback); - shutdownCallback.run(); + try { + Runtime.getRuntime().removeShutdownHook(shutdownCallback); + shutdownCallback.run(); + } catch (final Exception e) { + log.error("Error while finalizing DDTracer.", e); + } } /** diff --git a/dd-trace/src/main/java/datadog/trace/tracer/Tracer.java b/dd-trace/src/main/java/datadog/trace/tracer/Tracer.java index f309f0a0f2..4aaf25bb09 100644 --- a/dd-trace/src/main/java/datadog/trace/tracer/Tracer.java +++ b/dd-trace/src/main/java/datadog/trace/tracer/Tracer.java @@ -138,8 +138,12 @@ public class Tracer implements Closeable { @Override public void finalize() { - Runtime.getRuntime().removeShutdownHook(shutdownCallback); - shutdownCallback.run(); + try { + Runtime.getRuntime().removeShutdownHook(shutdownCallback); + shutdownCallback.run(); + } catch (final Exception e) { + log.error("Error while finalizing Tracer.", e); + } } @Override From a59eec223f0a63ca3761debacbd5cbd0da6872af Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Fri, 8 Feb 2019 11:52:37 -0800 Subject: [PATCH 4/5] Span metadata must be string values. --- .../src/main/java/datadog/trace/tracer/Span.java | 2 +- .../main/java/datadog/trace/tracer/SpanImpl.java | 16 ++++++++++++++-- .../groovy/datadog/trace/tracer/JsonSpan.groovy | 4 ++-- .../datadog/trace/tracer/SpanImplTest.groovy | 5 +++-- 4 files changed, 20 insertions(+), 7 deletions(-) diff --git a/dd-trace/src/main/java/datadog/trace/tracer/Span.java b/dd-trace/src/main/java/datadog/trace/tracer/Span.java index 454927cf52..04f9630e88 100644 --- a/dd-trace/src/main/java/datadog/trace/tracer/Span.java +++ b/dd-trace/src/main/java/datadog/trace/tracer/Span.java @@ -99,7 +99,7 @@ public interface Span { /** * Get all the metadata attached to this span. * - * @return + * @return immutable map of span metadata. */ Map getMeta(); diff --git a/dd-trace/src/main/java/datadog/trace/tracer/SpanImpl.java b/dd-trace/src/main/java/datadog/trace/tracer/SpanImpl.java index 54854a755a..0ee412afa9 100644 --- a/dd-trace/src/main/java/datadog/trace/tracer/SpanImpl.java +++ b/dd-trace/src/main/java/datadog/trace/tracer/SpanImpl.java @@ -4,6 +4,7 @@ import com.fasterxml.jackson.annotation.JsonAutoDetect; import com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility; import com.fasterxml.jackson.annotation.JsonFormat; import com.fasterxml.jackson.annotation.JsonGetter; +import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.core.JsonGenerator; import com.fasterxml.jackson.databind.SerializerProvider; import com.fasterxml.jackson.databind.annotation.JsonSerialize; @@ -13,6 +14,7 @@ import java.io.IOException; import java.io.PrintWriter; import java.io.StringWriter; import java.math.BigInteger; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -213,9 +215,19 @@ class SpanImpl implements Span { } @Override - @JsonGetter("meta") + @JsonIgnore public synchronized Map getMeta() { - final Map result = new HashMap<>(meta.size()); + return Collections.unmodifiableMap(meta); + } + + /** + * The agent expects meta's values to be strings. + * + * @return a copy of meta with all values converted to strings. + */ + @JsonGetter("meta") + synchronized Map getMetaString() { + final Map result = new HashMap<>(meta.size()); for (final Map.Entry entry : meta.entrySet()) { result.put(entry.getKey(), String.valueOf(entry.getValue())); } diff --git a/dd-trace/src/test/groovy/datadog/trace/tracer/JsonSpan.groovy b/dd-trace/src/test/groovy/datadog/trace/tracer/JsonSpan.groovy index 4731ba6f82..b569ff3035 100644 --- a/dd-trace/src/test/groovy/datadog/trace/tracer/JsonSpan.groovy +++ b/dd-trace/src/test/groovy/datadog/trace/tracer/JsonSpan.groovy @@ -43,7 +43,7 @@ class JsonSpan { @JsonCreator JsonSpan() {} - JsonSpan(Span span) { + JsonSpan(SpanImpl span) { traceId = new BigInteger(span.getContext().getTraceId()) parentId = new BigInteger(span.getContext().getParentId()) spanId = new BigInteger(span.getContext().getSpanId()) @@ -58,6 +58,6 @@ class JsonSpan { error = span.isErrored() ? 1 : 0 - meta = span.getMeta() + meta = span.getMetaString() } } diff --git a/dd-trace/src/test/groovy/datadog/trace/tracer/SpanImplTest.groovy b/dd-trace/src/test/groovy/datadog/trace/tracer/SpanImplTest.groovy index 235f8627de..269bd59a8e 100644 --- a/dd-trace/src/test/groovy/datadog/trace/tracer/SpanImplTest.groovy +++ b/dd-trace/src/test/groovy/datadog/trace/tracer/SpanImplTest.groovy @@ -129,7 +129,8 @@ class SpanImplTest extends Specification { span.setMeta("boolean.key", true) then: - span.getMeta() == ["number.key": "123", "string.key": "meta string", "boolean.key": "true"] + span.getMeta() == ["number.key": 123, "string.key": "meta string", "boolean.key": true] + span.getMetaString() == ["number.key": "123", "string.key": "meta string", "boolean.key": "true"] } def "test meta setter on finished span for #key"() { @@ -227,7 +228,7 @@ class SpanImplTest extends Specification { when: "finish/finalize span" span."$method"(*methodArgs) - + then: "interceptors called" interceptors.reverseEach({ interceptor -> then: From 8f3568c894bf5d33d0d45c4e14c2c32bd4158b7e Mon Sep 17 00:00:00 2001 From: Tyler Benson Date: Fri, 8 Feb 2019 12:38:13 -0800 Subject: [PATCH 5/5] Review fixes --- .../java/datadog/trace/tracer/SpanImpl.java | 2 -- .../java/datadog/trace/tracer/Tracer.java | 2 ++ .../datadog/trace/tracer/TracerTest.groovy | 20 +++++++++++++++++++ 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/dd-trace/src/main/java/datadog/trace/tracer/SpanImpl.java b/dd-trace/src/main/java/datadog/trace/tracer/SpanImpl.java index 0ee412afa9..a311aa11d0 100644 --- a/dd-trace/src/main/java/datadog/trace/tracer/SpanImpl.java +++ b/dd-trace/src/main/java/datadog/trace/tracer/SpanImpl.java @@ -4,7 +4,6 @@ import com.fasterxml.jackson.annotation.JsonAutoDetect; import com.fasterxml.jackson.annotation.JsonAutoDetect.Visibility; import com.fasterxml.jackson.annotation.JsonFormat; import com.fasterxml.jackson.annotation.JsonGetter; -import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.core.JsonGenerator; import com.fasterxml.jackson.databind.SerializerProvider; import com.fasterxml.jackson.databind.annotation.JsonSerialize; @@ -215,7 +214,6 @@ class SpanImpl implements Span { } @Override - @JsonIgnore public synchronized Map getMeta() { return Collections.unmodifiableMap(meta); } diff --git a/dd-trace/src/main/java/datadog/trace/tracer/Tracer.java b/dd-trace/src/main/java/datadog/trace/tracer/Tracer.java index 4aaf25bb09..0a5c9a5274 100644 --- a/dd-trace/src/main/java/datadog/trace/tracer/Tracer.java +++ b/dd-trace/src/main/java/datadog/trace/tracer/Tracer.java @@ -148,6 +148,8 @@ public class Tracer implements Closeable { @Override public void close() { + // FIXME: Handle the possibility of close being called more than once or not at all. + // FIXME: Depends on order of execution between finalize, GC, and the shutdown hook. writer.close(); } diff --git a/dd-trace/src/test/groovy/datadog/trace/tracer/TracerTest.groovy b/dd-trace/src/test/groovy/datadog/trace/tracer/TracerTest.groovy index 99fed2be57..ac2a203f57 100644 --- a/dd-trace/src/test/groovy/datadog/trace/tracer/TracerTest.groovy +++ b/dd-trace/src/test/groovy/datadog/trace/tracer/TracerTest.groovy @@ -45,6 +45,26 @@ class TracerTest extends Specification { 0 * _ // don't allow any other interaction } + def "finalize closes writer"() { + setup: + def writer = Mock(Writer) + def tracer = Tracer.builder().writer(writer).build() + + when: + tracer.finalize() + + then: "closed writer" + 1 * writer.close() + 0 * _ // don't allow any other interaction + + when: + tracer.finalize() + + then: "thrown error swallowed" + 1 * writer.close() >> { throw new Exception("test error") } + 0 * _ // don't allow any other interaction + } + def "test create current timestamp"() { setup: def tracer = Tracer.builder().config(config).build()