From 6cd2d9f79b7848d9e7873eb19e917d4e6233a935 Mon Sep 17 00:00:00 2001 From: Marius Constantin Date: Fri, 14 Feb 2020 18:32:16 +0100 Subject: [PATCH 1/3] Introduce the LogsHandler --- .../main/java/datadog/opentracing/DDSpan.java | 35 ++++- .../java/datadog/opentracing/DDTracer.java | 8 +- .../java/datadog/opentracing/LogsHandler.java | 40 +++++ .../opentracing/DDSpanBuilderTest.groovy | 140 +++++++++++++++++- 4 files changed, 217 insertions(+), 6 deletions(-) create mode 100644 dd-trace-ot/src/main/java/datadog/opentracing/LogsHandler.java diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java b/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java index 502a28a263..0c6cffabed 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java @@ -50,6 +50,9 @@ public class DDSpan implements Span, MutableSpan { */ private final AtomicLong durationNano = new AtomicLong(); + /** Delegates to for handling the logs if present. */ + private final LogsHandler logsHandler; + /** Implementation detail. Stores the weak reference to this span. Used by TraceCollection. */ volatile WeakReference ref; @@ -60,7 +63,19 @@ public class DDSpan implements Span, MutableSpan { * @param context the context used for the span */ DDSpan(final long timestampMicro, final DDSpanContext context) { + this(timestampMicro, context, null); + } + + /** + * Spans should be constructed using the builder, not by calling the constructor directly. + * + * @param timestampMicro if greater than zero, use this time instead of the current time + * @param context the context used for the span + * @param logsHandler as the handler where to delegate the log actions + */ + DDSpan(final long timestampMicro, final DDSpanContext context, final LogsHandler logsHandler) { this.context = context; + this.logsHandler = logsHandler; if (timestampMicro <= 0L) { // record the start time @@ -227,7 +242,9 @@ public class DDSpan implements Span, MutableSpan { */ @Override public final DDSpan log(final Map map) { - if (!extractError(map)) { + if (!extractError(map) && logsHandler != null) { + logsHandler.log(map); + } else { log.debug("`log` method is not implemented. Doing nothing"); } return this; @@ -238,7 +255,9 @@ public class DDSpan implements Span, MutableSpan { */ @Override public final DDSpan log(final long l, final Map map) { - if (!extractError(map)) { + if (!extractError(map) && logsHandler != null) { + logsHandler.log(l, map); + } else { log.debug("`log` method is not implemented. Doing nothing"); } return this; @@ -249,7 +268,11 @@ public class DDSpan implements Span, MutableSpan { */ @Override public final DDSpan log(final String s) { - log.debug("`log` method is not implemented. Provided log: {}", s); + if (logsHandler != null) { + logsHandler.log(s); + } else { + log.debug("`log` method is not implemented. Provided log: {}", s); + } return this; } @@ -258,7 +281,11 @@ public class DDSpan implements Span, MutableSpan { */ @Override public final DDSpan log(final long l, final String s) { - log.debug("`log` method is not implemented. Provided log: {}", s); + if (logsHandler != null) { + logsHandler.log(l, s); + } else { + log.debug("`log` method is not implemented. Provided log: {}", s); + } return this; } 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 0c2782a1db..f5ccdbdd5a 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java @@ -580,6 +580,7 @@ public class DDTracer implements io.opentracing.Tracer, Closeable, datadog.trace private boolean errorFlag; private String spanType; private boolean ignoreScope = false; + private LogsHandler logsHandler = null; public DDSpanBuilder(final String operationName, final ScopeManager scopeManager) { this.operationName = operationName; @@ -593,7 +594,7 @@ public class DDTracer implements io.opentracing.Tracer, Closeable, datadog.trace } private Span startSpan() { - return new DDSpan(timestampMicro, buildSpanContext()); + return new DDSpan(timestampMicro, buildSpanContext(), logsHandler); } @Override @@ -670,6 +671,11 @@ public class DDTracer implements io.opentracing.Tracer, Closeable, datadog.trace return parent.baggageItems(); } + public DDSpanBuilder withLogsHandler(LogsHandler logsHandler) { + this.logsHandler = logsHandler; + return this; + } + @Override public DDSpanBuilder asChildOf(final Span span) { return asChildOf(span == null ? null : span.context()); diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/LogsHandler.java b/dd-trace-ot/src/main/java/datadog/opentracing/LogsHandler.java new file mode 100644 index 0000000000..a6069980eb --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/opentracing/LogsHandler.java @@ -0,0 +1,40 @@ +package datadog.opentracing; + +import java.util.Map; + +public interface LogsHandler { + + /** + * Handles the log implementation in the Span. + * + * @param fields key:value log fields. Tracer implementations should support String, numeric, and + * boolean values; some may also support arbitrary Objects. + */ + void log(Map fields); + + /** + * Handles the log implementation in the Span. + * + * @param timestampMicroseconds The explicit timestamp for the log record. Must be greater than or + * equal to the Span's start timestamp. + * @param fields key:value log fields. Tracer implementations should support String, numeric, and + * boolean values; some may also support arbitrary Objects. + */ + void log(long timestampMicroseconds, Map fields); + + /** + * Handles the log implementation in the Span.. + * + * @param event the event value; often a stable identifier for a moment in the Span lifecycle + */ + void log(String event); + + /** + * Handles the log implementation in the Span. + * + * @param timestampMicroseconds The explicit timestamp for the log record. Must be greater than or + * equal to the Span's start timestamp. + * @param event the event value; often a stable identifier for a moment in the Span lifecycle + */ + void log(long timestampMicroseconds, String event); +} diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy index 8c753ce8f1..e68a756647 100644 --- a/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy @@ -8,7 +8,6 @@ import datadog.trace.common.writer.ListWriter import datadog.trace.util.test.DDSpecification import io.opentracing.Scope import io.opentracing.noop.NoopSpan - import static datadog.opentracing.DDSpanContext.ORIGIN_KEY import static java.util.concurrent.TimeUnit.MILLISECONDS @@ -480,4 +479,143 @@ class DDSpanBuilderTest extends DDSpecification { "a:a,a:b,a:c" | [a: "c"] "a:1,b-c:d" | [a: "1", "b-c": "d"] } + + def "sanity test for logs if logsHandler is null"() { + setup: + final String expectedName = "fakeName" + + final DDSpan span = + tracer + .buildSpan(expectedName) + .withServiceName("foo") + .start() + final String expectedLogEvent = "fakeEvent" + final timeStamp = System.currentTimeMillis() + final Map fieldsMap = new HashMap<>() + + span.log(expectedLogEvent) + span.log(timeStamp, expectedLogEvent) + span.log(fieldsMap) + span.log(timeStamp, fieldsMap) + } + + + def "should delegate simple logs to logHandler"() { + setup: + final LogsHandler logsHandler = new TestLogsHandler() + final String expectedName = "fakeName" + + final DDSpan span = + tracer + .buildSpan(expectedName) + .withLogsHandler(logsHandler) + .withServiceName("foo") + .start() + final String expectedLogEvent = "fakeEvent" + final timeStamp = System.currentTimeMillis() + span.log(timeStamp, expectedLogEvent) + + expect: + logsHandler.assertLogCalledWithArgs(timeStamp, expectedLogEvent) + } + + def "should delegate simple logs with timestamp to logHandler"() { + setup: + final LogsHandler logsHandler = new TestLogsHandler() + final String expectedName = "fakeName" + + final DDSpan span = + tracer + .buildSpan(expectedName) + .withLogsHandler(logsHandler) + .withServiceName("foo") + .start() + final String expectedLogEvent = "fakeEvent" + span.log(expectedLogEvent) + + expect: + logsHandler.assertLogCalledWithArgs(expectedLogEvent) + + } + + def "should delegate logs with fields to logHandler"() { + setup: + final LogsHandler logsHandler = new TestLogsHandler() + final String expectedName = "fakeName" + + final DDSpan span = + tracer + .buildSpan(expectedName) + .withLogsHandler(logsHandler) + .withServiceName("foo") + .start() + final Map fieldsMap = new HashMap<>() + span.log(fieldsMap) + + expect: + logsHandler.assertLogCalledWithArgs(fieldsMap) + + } + + def "should delegate logs with fields and timestamp to logHandler"() { + setup: + final LogsHandler logsHandler = new TestLogsHandler() + final String expectedName = "fakeName" + + final DDSpan span = + tracer + .buildSpan(expectedName) + .withLogsHandler(logsHandler) + .withServiceName("foo") + .start() + final Map fieldsMap = new HashMap<>() + final timeStamp = System.currentTimeMillis() + span.log(timeStamp, fieldsMap) + + expect: + logsHandler.assertLogCalledWithArgs(timeStamp, fieldsMap) + + } + + private static class TestLogsHandler implements LogsHandler { + Object[] arguments = null + + @Override + void log(Map fields) { + arguments = new Object[1] + arguments[0] = fields + } + + @Override + void log(long timestampMicroseconds, Map fields) { + arguments = new Object[2] + arguments[0] = timestampMicroseconds + arguments[1] = fields + } + + @Override + void log(String event) { + arguments = new Object[1] + arguments[0] = event + } + + @Override + void log(long timestampMicroseconds, String event) { + arguments = new Object[2] + arguments[0] = timestampMicroseconds + arguments[1] = event + } + + boolean assertLogCalledWithArgs(Object... args) { + if (arguments.size() != args.size()) { + return false + } + for (int i = 0; i < args.size(); i++) { + if (arguments[i] != args[i]) { + return false + } + } + return true + } + } } From b24ae8d6387868c7a69ed601a79277e1d9688d63 Mon Sep 17 00:00:00 2001 From: Marius Constantin Date: Thu, 27 Feb 2020 11:04:52 +0100 Subject: [PATCH 2/3] Add the DefaultLogHandler implementation --- .../main/java/datadog/opentracing/DDSpan.java | 36 +++++----------- .../java/datadog/opentracing/DDTracer.java | 9 ++-- .../opentracing/DefaultLogHandler.java | 28 +++++++++++++ .../{LogsHandler.java => LogHandler.java} | 2 +- .../opentracing/DDSpanBuilderTest.groovy | 41 ++++++++++++------- 5 files changed, 72 insertions(+), 44 deletions(-) create mode 100644 dd-trace-ot/src/main/java/datadog/opentracing/DefaultLogHandler.java rename dd-trace-ot/src/main/java/datadog/opentracing/{LogsHandler.java => LogHandler.java} (97%) diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java b/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java index 0c6cffabed..8f38606ee5 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java @@ -51,7 +51,7 @@ public class DDSpan implements Span, MutableSpan { private final AtomicLong durationNano = new AtomicLong(); /** Delegates to for handling the logs if present. */ - private final LogsHandler logsHandler; + private final LogHandler logHandler; /** Implementation detail. Stores the weak reference to this span. Used by TraceCollection. */ volatile WeakReference ref; @@ -63,7 +63,7 @@ public class DDSpan implements Span, MutableSpan { * @param context the context used for the span */ DDSpan(final long timestampMicro, final DDSpanContext context) { - this(timestampMicro, context, null); + this(timestampMicro, context, new DefaultLogHandler()); } /** @@ -71,11 +71,11 @@ public class DDSpan implements Span, MutableSpan { * * @param timestampMicro if greater than zero, use this time instead of the current time * @param context the context used for the span - * @param logsHandler as the handler where to delegate the log actions + * @param logHandler as the handler where to delegate the log actions */ - DDSpan(final long timestampMicro, final DDSpanContext context, final LogsHandler logsHandler) { + DDSpan(final long timestampMicro, final DDSpanContext context, final LogHandler logHandler) { this.context = context; - this.logsHandler = logsHandler; + this.logHandler = logHandler; if (timestampMicro <= 0L) { // record the start time @@ -242,11 +242,8 @@ public class DDSpan implements Span, MutableSpan { */ @Override public final DDSpan log(final Map map) { - if (!extractError(map) && logsHandler != null) { - logsHandler.log(map); - } else { - log.debug("`log` method is not implemented. Doing nothing"); - } + extractError(map); + logHandler.log(map); return this; } @@ -255,11 +252,8 @@ public class DDSpan implements Span, MutableSpan { */ @Override public final DDSpan log(final long l, final Map map) { - if (!extractError(map) && logsHandler != null) { - logsHandler.log(l, map); - } else { - log.debug("`log` method is not implemented. Doing nothing"); - } + extractError(map); + logHandler.log(l, map); return this; } @@ -268,11 +262,7 @@ public class DDSpan implements Span, MutableSpan { */ @Override public final DDSpan log(final String s) { - if (logsHandler != null) { - logsHandler.log(s); - } else { - log.debug("`log` method is not implemented. Provided log: {}", s); - } + logHandler.log(s); return this; } @@ -281,11 +271,7 @@ public class DDSpan implements Span, MutableSpan { */ @Override public final DDSpan log(final long l, final String s) { - if (logsHandler != null) { - logsHandler.log(l, s); - } else { - log.debug("`log` method is not implemented. Provided log: {}", s); - } + logHandler.log(l, s); return this; } 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 f5ccdbdd5a..8f24842df4 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java @@ -580,7 +580,7 @@ public class DDTracer implements io.opentracing.Tracer, Closeable, datadog.trace private boolean errorFlag; private String spanType; private boolean ignoreScope = false; - private LogsHandler logsHandler = null; + private LogHandler logHandler = new DefaultLogHandler(); public DDSpanBuilder(final String operationName, final ScopeManager scopeManager) { this.operationName = operationName; @@ -594,7 +594,7 @@ public class DDTracer implements io.opentracing.Tracer, Closeable, datadog.trace } private Span startSpan() { - return new DDSpan(timestampMicro, buildSpanContext(), logsHandler); + return new DDSpan(timestampMicro, buildSpanContext(), logHandler); } @Override @@ -671,8 +671,9 @@ public class DDTracer implements io.opentracing.Tracer, Closeable, datadog.trace return parent.baggageItems(); } - public DDSpanBuilder withLogsHandler(LogsHandler logsHandler) { - this.logsHandler = logsHandler; + public DDSpanBuilder withLogHandler(final LogHandler logHandler) { + assert logHandler != null : "LogHandler must not be null"; + this.logHandler = logHandler; return this; } diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DefaultLogHandler.java b/dd-trace-ot/src/main/java/datadog/opentracing/DefaultLogHandler.java new file mode 100644 index 0000000000..10d403d6e8 --- /dev/null +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DefaultLogHandler.java @@ -0,0 +1,28 @@ +package datadog.opentracing; + +import java.util.Map; +import lombok.extern.slf4j.Slf4j; + +/** The default implementation of the LogHandler. */ +@Slf4j +public class DefaultLogHandler implements LogHandler { + @Override + public void log(Map fields) { + log.debug("`log` method is not implemented. Doing nothing"); + } + + @Override + public void log(long timestampMicroseconds, Map fields) { + log.debug("`log` method is not implemented. Doing nothing"); + } + + @Override + public void log(String event) { + log.debug("`log` method is not implemented. Provided log: {}", event); + } + + @Override + public void log(long timestampMicroseconds, String event) { + log.debug("`log` method is not implemented. Provided log: {}", event); + } +} diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/LogsHandler.java b/dd-trace-ot/src/main/java/datadog/opentracing/LogHandler.java similarity index 97% rename from dd-trace-ot/src/main/java/datadog/opentracing/LogsHandler.java rename to dd-trace-ot/src/main/java/datadog/opentracing/LogHandler.java index a6069980eb..f1e5d43b68 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/LogsHandler.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/LogHandler.java @@ -2,7 +2,7 @@ package datadog.opentracing; import java.util.Map; -public interface LogsHandler { +public interface LogHandler { /** * Handles the log implementation in the Span. diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy index e68a756647..56b795999f 100644 --- a/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy @@ -480,7 +480,7 @@ class DDSpanBuilderTest extends DDSpecification { "a:1,b-c:d" | [a: "1", "b-c": "d"] } - def "sanity test for logs if logsHandler is null"() { + def "sanity test for logs if logHandler is null"() { setup: final String expectedName = "fakeName" @@ -502,13 +502,13 @@ class DDSpanBuilderTest extends DDSpecification { def "should delegate simple logs to logHandler"() { setup: - final LogsHandler logsHandler = new TestLogsHandler() + final LogHandler logHandler = new TestLogHandler() final String expectedName = "fakeName" final DDSpan span = tracer .buildSpan(expectedName) - .withLogsHandler(logsHandler) + .withLogHandler(logHandler) .withServiceName("foo") .start() final String expectedLogEvent = "fakeEvent" @@ -516,56 +516,56 @@ class DDSpanBuilderTest extends DDSpecification { span.log(timeStamp, expectedLogEvent) expect: - logsHandler.assertLogCalledWithArgs(timeStamp, expectedLogEvent) + logHandler.assertLogCalledWithArgs(timeStamp, expectedLogEvent) } def "should delegate simple logs with timestamp to logHandler"() { setup: - final LogsHandler logsHandler = new TestLogsHandler() + final LogHandler logHandler = new TestLogHandler() final String expectedName = "fakeName" final DDSpan span = tracer .buildSpan(expectedName) - .withLogsHandler(logsHandler) + .withLogHandler(logHandler) .withServiceName("foo") .start() final String expectedLogEvent = "fakeEvent" span.log(expectedLogEvent) expect: - logsHandler.assertLogCalledWithArgs(expectedLogEvent) + logHandler.assertLogCalledWithArgs(expectedLogEvent) } def "should delegate logs with fields to logHandler"() { setup: - final LogsHandler logsHandler = new TestLogsHandler() + final LogHandler logHandler = new TestLogHandler() final String expectedName = "fakeName" final DDSpan span = tracer .buildSpan(expectedName) - .withLogsHandler(logsHandler) + .withLogHandler(logHandler) .withServiceName("foo") .start() final Map fieldsMap = new HashMap<>() span.log(fieldsMap) expect: - logsHandler.assertLogCalledWithArgs(fieldsMap) + logHandler.assertLogCalledWithArgs(fieldsMap) } def "should delegate logs with fields and timestamp to logHandler"() { setup: - final LogsHandler logsHandler = new TestLogsHandler() + final LogHandler logHandler = new TestLogHandler() final String expectedName = "fakeName" final DDSpan span = tracer .buildSpan(expectedName) - .withLogsHandler(logsHandler) + .withLogHandler(logHandler) .withServiceName("foo") .start() final Map fieldsMap = new HashMap<>() @@ -573,11 +573,24 @@ class DDSpanBuilderTest extends DDSpecification { span.log(timeStamp, fieldsMap) expect: - logsHandler.assertLogCalledWithArgs(timeStamp, fieldsMap) + logHandler.assertLogCalledWithArgs(timeStamp, fieldsMap) } - private static class TestLogsHandler implements LogsHandler { + def "test assertion not null LogHandler"() { + setup: + final String expectedName = "fakeName" + + when: + tracer + .buildSpan(expectedName) + .withLogHandler(null) + + then: + thrown(AssertionError) + } + + private static class TestLogHandler implements LogHandler { Object[] arguments = null @Override From d36201c9dc8e63c1e5e066c1d71aa8ac3545a87f Mon Sep 17 00:00:00 2001 From: Marius Constantin Date: Fri, 28 Feb 2020 08:55:35 +0100 Subject: [PATCH 3/3] Apply code review changes --- .../main/java/datadog/opentracing/DDSpan.java | 25 +----- .../java/datadog/opentracing/DDTracer.java | 5 +- .../opentracing/DefaultLogHandler.java | 23 +++++- .../java/datadog/opentracing/LogHandler.java | 13 +-- .../opentracing/DDSpanBuilderTest.groovy | 64 ++++++++------- .../opentracing/DefaultLogHandlerTest.groovy | 80 +++++++++++++++++++ 6 files changed, 150 insertions(+), 60 deletions(-) create mode 100644 dd-trace-ot/src/test/groovy/datadog/opentracing/DefaultLogHandlerTest.groovy diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java b/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java index 8f38606ee5..1380b2753c 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java @@ -1,8 +1,5 @@ package datadog.opentracing; -import static io.opentracing.log.Fields.ERROR_OBJECT; -import static io.opentracing.log.Fields.MESSAGE; - import datadog.trace.api.DDTags; import datadog.trace.api.interceptor.MutableSpan; import datadog.trace.api.sampling.PrioritySampling; @@ -158,18 +155,6 @@ public class DDSpan implements Span, MutableSpan { setTag(DDTags.ERROR_STACK, errorString.toString()); } - private boolean extractError(final Map map) { - if (map.get(ERROR_OBJECT) instanceof Throwable) { - final Throwable error = (Throwable) map.get(ERROR_OBJECT); - setErrorMeta(error); - return true; - } else if (map.get(MESSAGE) instanceof String) { - setTag(DDTags.ERROR_MSG, (String) map.get(MESSAGE)); - return true; - } - return false; - } - /* (non-Javadoc) * @see io.opentracing.BaseSpan#setTag(java.lang.String, java.lang.String) */ @@ -242,8 +227,7 @@ public class DDSpan implements Span, MutableSpan { */ @Override public final DDSpan log(final Map map) { - extractError(map); - logHandler.log(map); + logHandler.log(map, this); return this; } @@ -252,8 +236,7 @@ public class DDSpan implements Span, MutableSpan { */ @Override public final DDSpan log(final long l, final Map map) { - extractError(map); - logHandler.log(l, map); + logHandler.log(l, map, this); return this; } @@ -262,7 +245,7 @@ public class DDSpan implements Span, MutableSpan { */ @Override public final DDSpan log(final String s) { - logHandler.log(s); + logHandler.log(s, this); return this; } @@ -271,7 +254,7 @@ public class DDSpan implements Span, MutableSpan { */ @Override public final DDSpan log(final long l, final String s) { - logHandler.log(l, s); + logHandler.log(l, s, this); return this; } 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 8f24842df4..76ccb4b32f 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java @@ -672,8 +672,9 @@ public class DDTracer implements io.opentracing.Tracer, Closeable, datadog.trace } public DDSpanBuilder withLogHandler(final LogHandler logHandler) { - assert logHandler != null : "LogHandler must not be null"; - this.logHandler = logHandler; + if (logHandler != null) { + this.logHandler = logHandler; + } return this; } diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/DefaultLogHandler.java b/dd-trace-ot/src/main/java/datadog/opentracing/DefaultLogHandler.java index 10d403d6e8..0f7cf2ba63 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/DefaultLogHandler.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/DefaultLogHandler.java @@ -1,5 +1,9 @@ package datadog.opentracing; +import static io.opentracing.log.Fields.ERROR_OBJECT; +import static io.opentracing.log.Fields.MESSAGE; + +import datadog.trace.api.DDTags; import java.util.Map; import lombok.extern.slf4j.Slf4j; @@ -7,22 +11,33 @@ import lombok.extern.slf4j.Slf4j; @Slf4j public class DefaultLogHandler implements LogHandler { @Override - public void log(Map fields) { + public void log(Map fields, DDSpan span) { + extractError(fields, span); log.debug("`log` method is not implemented. Doing nothing"); } @Override - public void log(long timestampMicroseconds, Map fields) { + public void log(long timestampMicroseconds, Map fields, DDSpan span) { + extractError(fields, span); log.debug("`log` method is not implemented. Doing nothing"); } @Override - public void log(String event) { + public void log(String event, DDSpan span) { log.debug("`log` method is not implemented. Provided log: {}", event); } @Override - public void log(long timestampMicroseconds, String event) { + public void log(long timestampMicroseconds, String event, DDSpan span) { log.debug("`log` method is not implemented. Provided log: {}", event); } + + private void extractError(final Map map, DDSpan span) { + if (map.get(ERROR_OBJECT) instanceof Throwable) { + final Throwable error = (Throwable) map.get(ERROR_OBJECT); + span.setErrorMeta(error); + } else if (map.get(MESSAGE) instanceof String) { + span.setTag(DDTags.ERROR_MSG, (String) map.get(MESSAGE)); + } + } } diff --git a/dd-trace-ot/src/main/java/datadog/opentracing/LogHandler.java b/dd-trace-ot/src/main/java/datadog/opentracing/LogHandler.java index f1e5d43b68..03bc25029e 100644 --- a/dd-trace-ot/src/main/java/datadog/opentracing/LogHandler.java +++ b/dd-trace-ot/src/main/java/datadog/opentracing/LogHandler.java @@ -9,8 +9,9 @@ public interface LogHandler { * * @param fields key:value log fields. Tracer implementations should support String, numeric, and * boolean values; some may also support arbitrary Objects. + * @param span from which the call was made */ - void log(Map fields); + void log(Map fields, DDSpan span); /** * Handles the log implementation in the Span. @@ -18,16 +19,17 @@ public interface LogHandler { * @param timestampMicroseconds The explicit timestamp for the log record. Must be greater than or * equal to the Span's start timestamp. * @param fields key:value log fields. Tracer implementations should support String, numeric, and - * boolean values; some may also support arbitrary Objects. + * @param span from which the call was made */ - void log(long timestampMicroseconds, Map fields); + void log(long timestampMicroseconds, Map fields, DDSpan span); /** * Handles the log implementation in the Span.. * * @param event the event value; often a stable identifier for a moment in the Span lifecycle + * @param span from which the call was made */ - void log(String event); + void log(String event, DDSpan span); /** * Handles the log implementation in the Span. @@ -35,6 +37,7 @@ public interface LogHandler { * @param timestampMicroseconds The explicit timestamp for the log record. Must be greater than or * equal to the Span's start timestamp. * @param event the event value; often a stable identifier for a moment in the Span lifecycle + * @param span from which the call was made */ - void log(long timestampMicroseconds, String event); + void log(long timestampMicroseconds, String event, DDSpan span); } diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy index 56b795999f..ad1ce537ad 100644 --- a/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy +++ b/dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy @@ -499,6 +499,23 @@ class DDSpanBuilderTest extends DDSpecification { span.log(timeStamp, fieldsMap) } + def "sanity test when passed log handler is null"() { + setup: + final String expectedName = "fakeName" + final DDSpan span = tracer + .buildSpan(expectedName) + .withLogHandler(null) + .start() + final String expectedLogEvent = "fakeEvent" + final timeStamp = System.currentTimeMillis() + final Map fieldsMap = new HashMap<>() + + span.log(expectedLogEvent) + span.log(timeStamp, expectedLogEvent) + span.log(fieldsMap) + span.log(timeStamp, fieldsMap) + } + def "should delegate simple logs to logHandler"() { setup: @@ -516,7 +533,7 @@ class DDSpanBuilderTest extends DDSpecification { span.log(timeStamp, expectedLogEvent) expect: - logHandler.assertLogCalledWithArgs(timeStamp, expectedLogEvent) + logHandler.assertLogCalledWithArgs(timeStamp, expectedLogEvent, span) } def "should delegate simple logs with timestamp to logHandler"() { @@ -534,7 +551,7 @@ class DDSpanBuilderTest extends DDSpecification { span.log(expectedLogEvent) expect: - logHandler.assertLogCalledWithArgs(expectedLogEvent) + logHandler.assertLogCalledWithArgs(expectedLogEvent, span) } @@ -553,7 +570,7 @@ class DDSpanBuilderTest extends DDSpecification { span.log(fieldsMap) expect: - logHandler.assertLogCalledWithArgs(fieldsMap) + logHandler.assertLogCalledWithArgs(fieldsMap, span) } @@ -573,50 +590,41 @@ class DDSpanBuilderTest extends DDSpecification { span.log(timeStamp, fieldsMap) expect: - logHandler.assertLogCalledWithArgs(timeStamp, fieldsMap) + logHandler.assertLogCalledWithArgs(timeStamp, fieldsMap, span) } - def "test assertion not null LogHandler"() { - setup: - final String expectedName = "fakeName" - - when: - tracer - .buildSpan(expectedName) - .withLogHandler(null) - - then: - thrown(AssertionError) - } - private static class TestLogHandler implements LogHandler { Object[] arguments = null @Override - void log(Map fields) { - arguments = new Object[1] + void log(Map fields, DDSpan span) { + arguments = new Object[2] arguments[0] = fields + arguments[1] = span } @Override - void log(long timestampMicroseconds, Map fields) { - arguments = new Object[2] + void log(long timestampMicroseconds, Map fields, DDSpan span) { + arguments = new Object[3] arguments[0] = timestampMicroseconds arguments[1] = fields + arguments[2] = span } @Override - void log(String event) { - arguments = new Object[1] - arguments[0] = event - } - - @Override - void log(long timestampMicroseconds, String event) { + void log(String event, DDSpan span) { arguments = new Object[2] + arguments[0] = event + arguments[1] = span + } + + @Override + void log(long timestampMicroseconds, String event, DDSpan span) { + arguments = new Object[3] arguments[0] = timestampMicroseconds arguments[1] = event + arguments[2] = span } boolean assertLogCalledWithArgs(Object... args) { diff --git a/dd-trace-ot/src/test/groovy/datadog/opentracing/DefaultLogHandlerTest.groovy b/dd-trace-ot/src/test/groovy/datadog/opentracing/DefaultLogHandlerTest.groovy new file mode 100644 index 0000000000..4241ffd645 --- /dev/null +++ b/dd-trace-ot/src/test/groovy/datadog/opentracing/DefaultLogHandlerTest.groovy @@ -0,0 +1,80 @@ +package datadog.opentracing + +import datadog.trace.api.DDTags +import datadog.trace.common.writer.ListWriter +import io.opentracing.log.Fields +import datadog.trace.util.test.DDSpecification + + +class DefaultLogHandlerTest extends DDSpecification { + def writer = new ListWriter() + def tracer = DDTracer.builder().writer(writer).build() + + def "handles correctly the error passed in the fields"() { + setup: + final LogHandler underTest = new DefaultLogHandler() + final DDSpan span = tracer.buildSpan("op name").withServiceName("foo").start() + final String errorMessage = "errorMessage" + final String differentMessage = "differentMessage" + final Throwable throwable = new Throwable(errorMessage) + final Map fields = new HashMap<>() + fields.put(Fields.ERROR_OBJECT, throwable) + fields.put(Fields.MESSAGE, differentMessage) + + when: + underTest.log(fields, span) + + then: + span.getTags().get(DDTags.ERROR_MSG) == throwable.getMessage() + span.getTags().get(DDTags.ERROR_TYPE) == throwable.getClass().getName() + } + + def "handles correctly the error passed in the fields when called with timestamp"() { + setup: + final LogHandler underTest = new DefaultLogHandler() + final DDSpan span = tracer.buildSpan("op name").withServiceName("foo").start() + final String errorMessage = "errorMessage" + final String differentMessage = "differentMessage" + final Throwable throwable = new Throwable(errorMessage) + final Map fields = new HashMap<>() + fields.put(Fields.ERROR_OBJECT, throwable) + fields.put(Fields.MESSAGE, differentMessage) + + when: + underTest.log(System.currentTimeMillis(), fields, span) + + then: + span.getTags().get(DDTags.ERROR_MSG) == throwable.getMessage() + span.getTags().get(DDTags.ERROR_TYPE) == throwable.getClass().getName() + } + + def "handles correctly the message passed in the fields"() { + setup: + final LogHandler underTest = new DefaultLogHandler() + final DDSpan span = tracer.buildSpan("op name").withServiceName("foo").start() + final String errorMessage = "errorMessage" + final Map fields = new HashMap<>() + fields.put(Fields.MESSAGE, errorMessage) + + when: + underTest.log(fields, span) + + then: + span.getTags().get(DDTags.ERROR_MSG) == errorMessage + } + + def "handles correctly the message passed in the fields when called with timestamp"() { + setup: + final LogHandler underTest = new DefaultLogHandler() + final DDSpan span = tracer.buildSpan("op name").withServiceName("foo").start() + final String errorMessage = "errorMessage" + final Map fields = new HashMap<>() + fields.put(Fields.MESSAGE, errorMessage) + + when: + underTest.log(System.currentTimeMillis(), fields, span) + + then: + span.getTags().get(DDTags.ERROR_MSG) == errorMessage + } +}