Apply code review changes

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<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, this);
-    logHandler.log(map);
     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, this);
-    logHandler.log(l, map);
     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;
   }
 

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;
+        this.logHandler = logHandler;
+      }
       return this;
     }
 

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<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));
+    }
+  }
 }

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);
 }

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) {
+    void log(Map<String, ?> fields, DDSpan span) {
-      arguments = new Object[1]
+      arguments = new Object[2]
       arguments[0] = fields
+      arguments[1] = span
     }
 
     @Override
-    void log(long timestampMicroseconds, Map<String, ?> fields) {
+    void log(long timestampMicroseconds, Map<String, ?> fields, DDSpan span) {
-      arguments = new Object[2]
+      arguments = new Object[3]
       arguments[0] = timestampMicroseconds
       arguments[1] = fields
+      arguments[2] = span
     }
 
     @Override
-    void log(String event) {
+    void log(String event, DDSpan span) {
-      arguments = new Object[1]
-      arguments[0] = event
-    }
-
-    @Override
-    void log(long timestampMicroseconds, String event) {
       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) {

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<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
+  }
+}