Apply code review changes
MODIFY
dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java
dd-trace-ot/src/main/java/datadog/opentracing/DDSpan.java
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<String, ?> 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<String, ?> 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<String, ?> 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;
}

MODIFY
dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java
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";
+ if (logHandler != null) {
this.logHandler = logHandler;
+ }
return this;
}

MODIFY
dd-trace-ot/src/main/java/datadog/opentracing/DefaultLogHandler.java
dd-trace-ot/src/main/java/datadog/opentracing/DefaultLogHandler.java
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<String, ?> fields) {
+ public void log(Map<String, ?> fields, DDSpan span) {
+ extractError(fields, span);
log.debug("`log` method is not implemented. Doing nothing");
}
@Override
- public void log(long timestampMicroseconds, Map<String, ?> fields) {
+ public void log(long timestampMicroseconds, Map<String, ?> 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<String, ?> 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));
+ }
+ }
}

MODIFY
dd-trace-ot/src/main/java/datadog/opentracing/LogHandler.java
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<String, ?> fields);
+ void log(Map<String, ?> 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<String, ?> fields);
+ void log(long timestampMicroseconds, Map<String, ?> 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);
}

MODIFY
dd-trace-ot/src/test/groovy/datadog/opentracing/DDSpanBuilderTest.groovy
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<String, String> 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<String, ?> fields) {
- arguments = new Object[1]
+ void log(Map<String, ?> fields, DDSpan span) {
+ arguments = new Object[2]
arguments[0] = fields
+ arguments[1] = span
}
@Override
- void log(long timestampMicroseconds, Map<String, ?> fields) {
- arguments = new Object[2]
+ void log(long timestampMicroseconds, Map<String, ?> 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]
+ void log(String event, DDSpan span) {
+ arguments = new Object[2]
arguments[0] = event
+ arguments[1] = span
}
@Override
- void log(long timestampMicroseconds, String event) {
- arguments = new Object[2]
+ 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) {

ADD
dd-trace-ot/src/test/groovy/datadog/opentracing/DefaultLogHandlerTest.groovy
+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<String, ?> 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<String, ?> 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<String, ?> 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<String, ?> fields = new HashMap<>()
+ fields.put(Fields.MESSAGE, errorMessage)
+
+ when:
+ underTest.log(System.currentTimeMillis(), fields, span)
+
+ then:
+ span.getTags().get(DDTags.ERROR_MSG) == errorMessage
+ }
+}

Apply code review changes