[PR #1] Migrating all things to the span context (and add setters, and do not instantiate HashMap if it's not needed)

src/main/java/com/datadoghq/trace/impl/DDSpan.java

@@ -1,19 +1,17 @@
 package com.datadoghq.trace.impl;
 
+import com.fasterxml.jackson.annotation.JsonGetter;
+import com.fasterxml.jackson.annotation.JsonIgnore;
+import io.opentracing.Span;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
 import java.util.concurrent.TimeUnit;
 
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
-import com.fasterxml.jackson.annotation.JsonGetter;
-import com.fasterxml.jackson.annotation.JsonIgnore;
-
-import io.opentracing.Span;
-
 /**
  * Represents an in-flight span in the opentracing system.
  * <p>
@@ -22,14 +20,7 @@ import io.opentracing.Span;
  */
 public class DDSpan implements io.opentracing.Span {
 
-    /**
+
-     * Each span have an operation name describing the current span
-     */
-    private String operationName;
-    /**
-     * Tags are associated to the current span, they will not propagate to the children span
-     */
-    private Map<String, Object> tags;
     /**
      * StartTime stores the creation time of the span in milliseconds
      */
@@ -53,19 +44,13 @@ public class DDSpan implements io.opentracing.Span {
      * A simple constructor.
      * Currently, users have
      *
-     * @param operationName  the operation name associated to the span
-     * @param tags           Tags attached to the span
      * @param timestampMicro if set, use this time instead of the auto-generated time
      * @param context        the context
      */
     protected DDSpan(
-            String operationName,
-            Map<String, Object> tags,
             long timestampMicro,
             DDSpanContext context) {
 
-        this.operationName = operationName;
-        this.tags = tags;
         this.context = context;
 
         // record the start time in nano (current milli + nano delta)
@@ -79,11 +64,6 @@ public class DDSpan implements io.opentracing.Span {
         // track each span of the trace
         this.context.getTrace().add(this);
 
-        // check DD attributes required
-        // FIXME Remove IAE
-        if (this.context.getServiceName() == null) {
-            throw new IllegalArgumentException("No ServiceName provided");
-        }
     }
 
     /* (non-Javadoc)
@@ -145,34 +125,26 @@ public class DDSpan implements io.opentracing.Span {
      * @see io.opentracing.Span#setTag(java.lang.String, java.lang.String)
      */
     public Span setTag(String tag, String value) {
-        return setTag(tag, (Object) value);
+        this.context().setTag(tag, (Object) value);
+        return this;
     }
 
     /* (non-Javadoc)
      * @see io.opentracing.Span#setTag(java.lang.String, boolean)
      */
     public Span setTag(String tag, boolean value) {
-        return setTag(tag, (Object) value);
+        this.context().setTag(tag, (Object) value);
+        return this;
     }
 
     /* (non-Javadoc)
      * @see io.opentracing.Span#setTag(java.lang.String, java.lang.Number)
      */
     public Span setTag(String tag, Number value) {
-        return this.setTag(tag, (Object) value);
+         this.context().setTag(tag, (Object) value);
+         return this;
     }
 
-    /**
-     * Add a tag to the span. Tags are not propagated to the children
-     *
-     * @param tag   the tag-name
-     * @param value the value of the value
-     * @return the builder instance
-     */
-    private Span setTag(String tag, Object value) {
-        tags.put(tag, value);
-        return this;
-    }
 
     /* (non-Javadoc)
      * @see io.opentracing.Span#context()
@@ -200,11 +172,7 @@ public class DDSpan implements io.opentracing.Span {
      * @see io.opentracing.Span#setOperationName(java.lang.String)
      */
     public Span setOperationName(String operationName) {
-        // FIXME operationName is in each constructor --> always IAE
+        this.context().setOperationName(operationName);
-        if (this.operationName != null) {
-            throw new IllegalArgumentException("The operationName is already assigned.");
-        }
-        this.operationName = operationName;
         return this;
     }
 
@@ -258,15 +226,6 @@ public class DDSpan implements io.opentracing.Span {
 
 
     //Getters and JSON serialisation instructions
-    @JsonGetter("name")
-    public String getOperationName() {
-        return operationName;
-    }
-
-    @JsonIgnore
-    public Map<String, Object> getTags() {
-        return this.tags;
-    }
 
     /**
      * Meta merges baggage and tags (stringified values)
@@ -295,8 +254,8 @@ public class DDSpan implements io.opentracing.Span {
         return durationNano;
     }
 
-    @JsonGetter
+    @JsonGetter("service")
-    public String getService() {
+    public String getServiceName() {
         return context.getServiceName();
     }
 
@@ -317,9 +276,18 @@ public class DDSpan implements io.opentracing.Span {
 
     @JsonGetter("resource")
     public String getResourceName() {
-        return context.getResourceName() == null ? this.operationName : context.getResourceName();
+        return context.getResourceName() == null ? context.getOperationName() : context.getResourceName();
     }
 
+    @JsonGetter("name")
+    public String getOperationName() {
+        return this.context().getOperationName();
+    }
+
+    @JsonIgnore
+    public Map<String, Object> getTags() {
+        return this.context().getTags();
+    }
     @JsonGetter
     public String getType() {
         return context.getSpanType();
@@ -335,4 +303,19 @@ public class DDSpan implements io.opentracing.Span {
         return context.toString();
     }
 
+
+    public Span setServiceName(String serviceName) {
+        this.context().setServiceName(serviceName);
+        return this;
+    }
+
+    public Span setResourceName(String resourceName) {
+        this.context().setResourceName(resourceName);
+        return this;
+    }
+
+    public Span setType(String type) {
+        this.context().setType(type);
+        return this;
+    }
 }

src/main/java/com/datadoghq/trace/impl/DDSpanContext.java

@@ -1,10 +1,7 @@
 package com.datadoghq.trace.impl;
 
 
-import java.util.ArrayList;
+import java.util.*;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
 
 import com.fasterxml.jackson.annotation.JsonIgnore;
 
@@ -23,17 +20,17 @@ public class DDSpanContext implements io.opentracing.SpanContext {
     private final long traceId;
     private final long spanId;
     private final long parentId;
-    private final Map<String, String> baggageItems;
+    private Map<String, String> baggageItems;
 
     // DD attributes
     /**
      * The service name is required, otherwise the span are dropped by the agent
      */
-    private final String serviceName;
+    private String serviceName;
     /**
      * The resource associated to the service (server_web, database, etc.)
      */
-    private final String resourceName;
+    private String resourceName;
     /**
      * True indicates that the span reports an error
      */
@@ -41,11 +38,19 @@ public class DDSpanContext implements io.opentracing.SpanContext {
     /**
      * The type of the span. If null, the Datadog Agent will report as a custom
      */
-    private final String spanType;
+    private String spanType;
     /**
      * The collection of all span related to this one
      */
     private final List<Span> trace;
+    /**
+     * Each span have an operation name describing the current span
+     */
+    private String operationName;
+    /**
+     * Tags are associated to the current span, they will not propagate to the children span
+     */
+    private Map<String, Object> tags;
     // Others attributes
     /**
      * For technical reasons, the ref to the original tracer
@@ -57,10 +62,12 @@ public class DDSpanContext implements io.opentracing.SpanContext {
             long spanId,
             long parentId,
             String serviceName,
+            String operationName,
             String resourceName,
             Map<String, String> baggageItems,
             boolean errorFlag,
             String spanType,
+            Map<String, Object> tags,
             List<Span> trace,
             DDTracer tracer) {
 
@@ -69,15 +76,19 @@ public class DDSpanContext implements io.opentracing.SpanContext {
         this.parentId = parentId;
 
         if (baggageItems == null) {
-            this.baggageItems = new HashMap<String, String>();
+            this.baggageItems = Collections.emptyMap();
         } else {
             this.baggageItems = baggageItems;
         }
+
         this.serviceName = serviceName;
+        this.operationName = operationName;
         this.resourceName = resourceName;
         this.errorFlag = errorFlag;
         this.spanType = spanType;
 
+        this.tags = tags;
+
         if (trace == null) {
             this.trace = new ArrayList<Span>();
         } else {
@@ -117,6 +128,9 @@ public class DDSpanContext implements io.opentracing.SpanContext {
     }
 
     public void setBaggageItem(String key, String value) {
+        if (this.baggageItems.isEmpty()) {
+            this.baggageItems = new HashMap<String, String>();
+        }
         this.baggageItems.put(key, value);
     }
 
@@ -145,6 +159,20 @@ public class DDSpanContext implements io.opentracing.SpanContext {
         return this.tracer;
     }
 
+    /**
+     * Add a tag to the span. Tags are not propagated to the children
+     *
+     * @param tag   the tag-name
+     * @param value the value of the value
+     * @return the builder instance
+     */
+    public void setTag(String tag, Object value) {
+        if (this.tags.isEmpty()) {
+            this.tags = new HashMap<String, Object>();
+        }
+        this.tags.put(tag, value);
+    }
+
     @Override
     public String toString() {
         return "Span [traceId=" + traceId
@@ -152,4 +180,27 @@ public class DDSpanContext implements io.opentracing.SpanContext {
                 + ", parentId=" + parentId + "]";
     }
 
+    public void setOperationName(String operationName) {
+        this.operationName = operationName;
+    }
+
+    public String getOperationName() {
+        return operationName;
+    }
+
+    public Map<String, Object> getTags() {
+        return tags;
+    }
+
+    public void setServiceName(String serviceName) {
+        this.serviceName = serviceName;
+    }
+
+    public void setResourceName(String resourceName) {
+        this.resourceName = resourceName;
+    }
+
+    public void setType(String type) {
+        this.spanType = type;
+    }
 }

src/main/java/com/datadoghq/trace/impl/DDTracer.java

@@ -9,10 +9,7 @@ import io.opentracing.propagation.Format;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.util.Collections;
+import java.util.*;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
 
 /**
  * DDTracer makes it easy to send traces and span to DD using the OpenTracing instrumentation.
@@ -83,10 +80,10 @@ public class DDTracer implements io.opentracing.Tracer {
         /**
          * Each span must have an operationName according to the opentracing specification
          */
-        private final String operationName;
+        private String operationName;
 
         // Builder attributes
-        private Map<String, Object> tags = new HashMap<String, Object>();
+        private Map<String, Object> tags = Collections.emptyMap();
         private long timestamp;
         private SpanContext parent;
         private String serviceName;
@@ -104,7 +101,7 @@ public class DDTracer implements io.opentracing.Tracer {
 
             // build the context
             DDSpanContext context = buildSpanContext();
-            DDSpan span = new DDSpan(this.operationName, this.tags, this.timestamp, context);
+            DDSpan span = new DDSpan(this.timestamp, context);
 
             logger.debug("{} - Starting a new span.", span);
 
@@ -177,6 +174,9 @@ public class DDTracer implements io.opentracing.Tracer {
 
         // Private methods
         private DDTracer.DDSpanBuilder withTag(String tag, Object value) {
+            if (this.tags.isEmpty()){
+                this.tags = new HashMap<String, Object>();
+            }
             this.tags.put(tag, value);
             return this;
         }
@@ -195,7 +195,6 @@ public class DDTracer implements io.opentracing.Tracer {
          * @return the context
          */
         private DDSpanContext buildSpanContext() {
-
             long generatedId = generateNewId();
             DDSpanContext context;
             DDSpanContext p = this.parent != null ? (DDSpanContext) this.parent : null;
@@ -210,16 +209,20 @@ public class DDTracer implements io.opentracing.Tracer {
                 serviceName = p.getServiceName();
             }
 
+            //this.operationName, this.tags,
+
             // some attributes are inherited from the parent
             context = new DDSpanContext(
                     this.parent == null ? generatedId : p.getTraceId(),
                     generatedId,
                     this.parent == null ? 0L : p.getSpanId(),
                     serviceName,
+                    this.operationName,
                     this.resourceName,
-                    this.parent == null ? null : p.getBaggageItems(),
+                    this.parent == null ? Collections.<String, String>emptyMap() : p.getBaggageItems(),
                     errorFlag,
                     spanType,
+                    this.tags,
                     this.parent == null ? null : p.getTrace(),
                     DDTracer.this
             );

src/main/java/com/datadoghq/trace/writer/impl/DDAgentWriter.java

@@ -75,7 +75,6 @@ public class DDAgentWriter implements Writer {
      */
     public void write(List<Span> trace) {
         //Try to add a new span in the queue
-        //FIXME oldest?
         boolean proceed = tokens.tryAcquire(trace.size());
 
         if (proceed) {

src/test/java/com/datadoghq/trace/impl/DDSpanSerializerTest.java

@@ -28,16 +28,16 @@ public class DDSpanSerializerTest {
                 2L,
                 0L,
                 "service",
+                "operation",
                 "resource",
                 baggage,
                 false,
                 "type",
+                tags,
                 null,
                 null);
 
         span = new DDSpan(
-                "operation",
-                tags,
                 100L,
                 context);
 
@@ -48,12 +48,14 @@ public class DDSpanSerializerTest {
     @Test
     public void test() throws Exception {
 
+
         String expected = "{\"meta\":{\"a-baggage\":\"value\",\"k1\":\"v1\"},\"service\":\"service\",\"error\":0,\"type\":\"type\",\"name\":\"operation\",\"duration\":33000,\"resource\":\"resource\",\"start\":100000,\"span_id\":2,\"parent_id\":0,\"trace_id\":1}";
-        //FIXME attributes order is not maintained I disabled the test for now
-        //assertEquals("
-        //        , serializer.serialize(span));
         // FIXME At the moment, just compare the string sizes
-        assertThat(serializer.serialize(span).length()).isEqualTo(expected.length());
+        try {
+            assertThat(serializer.serialize(span).length()).isEqualTo(expected.length());
+        } catch (AssertionError e) {
+            assertThat(serializer.serialize(span)).isEqualTo(expected);
+        }
     }
 
 }

src/test/java/com/datadoghq/trace/impl/DDSpanTest.java

@@ -1,24 +1,54 @@
 package com.datadoghq.trace.impl;
 
-import io.opentracing.Span;
 import org.junit.Test;
 
+import java.util.Collections;
+
 import static org.assertj.core.api.Assertions.assertThat;
 
 
 public class DDSpanTest {
 
 
-    @Test(expected = IllegalArgumentException.class)
+    @Test
-    public void shouldHaveServiceName() {
+    public void testGetterSetter() {
-        new DDTracer().buildSpan("operationName").start();
+
+        DDSpanContext context = new DDSpanContext(
+                1L,
+                1L,
+                0L,
+                "fakeService",
+                "fakeOperation",
+                "fakeResource",
+                Collections.<String, String>emptyMap(),
+                false,
+                "fakeType",
+                null,
+                null,
+                null);
+
+
+        String expected;
+        DDSpan span = new DDSpan(1L, context);
+
+        expected = "service";
+        span.setServiceName(expected);
+        assertThat(span.getServiceName()).isEqualTo(expected);
+
+        expected = "operation";
+        span.setOperationName(expected);
+        assertThat(span.getOperationName()).isEqualTo(expected);
+
+        expected = "resource";
+        span.setResourceName(expected);
+        assertThat(span.getResourceName()).isEqualTo(expected);
+
+        expected = "type";
+        span.setType(expected);
+        assertThat(span.getType()).isEqualTo(expected);
+
     }
 
-    @Test(expected = IllegalArgumentException.class)
-    public void shouldOperationNameImmutable() {
-        Span span = new DDTracer().buildSpan("foo").withServiceName("foo").start();
-        span.setOperationName("boom");
-    }
 
     @Test
     public void shouldResourceNameEqualsOperationNameIfNull() {
@@ -39,4 +69,6 @@ public class DDSpanTest {
         assertThat(span.getResourceName()).isEqualTo(expectedResourceName);
 
     }
+
+
 }

src/test/java/com/datadoghq/trace/impl/RateSamplerTest.java

@@ -1,8 +1,6 @@
 package com.datadoghq.trace.impl;
 
 import com.datadoghq.trace.Sampler;
-import com.datadoghq.trace.impl.DDSpan;
-import com.datadoghq.trace.impl.RateSampler;
 import org.junit.Test;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -14,7 +12,6 @@ public class RateSamplerTest {
     @Test
     public void testRateSampler() {
 
-        //FIXME test has to be more predictable
         DDSpan mockSpan = mock(DDSpan.class);
 
         final double sampleRate = 0.35;
@@ -28,7 +25,7 @@ public class RateSamplerTest {
                 kept++;
             }
         }
-
+        //FIXME test has to be more predictable
         //assertThat(((double) kept / iterations)).isBetween(sampleRate - 0.02, sampleRate + 0.02);
 
     }