diff --git a/src/main/java/com/datadoghq/trace/impl/DDSpan.java b/src/main/java/com/datadoghq/trace/impl/DDSpan.java index e8cb1b70dd..3b3a3596d5 100644 --- a/src/main/java/com/datadoghq/trace/impl/DDSpan.java +++ b/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. *

@@ -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 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 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 - if (this.operationName != null) { - throw new IllegalArgumentException("The operationName is already assigned."); - } - this.operationName = operationName; + this.context().setOperationName(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 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 - public String getService() { + @JsonGetter("service") + 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 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; + } } diff --git a/src/main/java/com/datadoghq/trace/impl/DDSpanContext.java b/src/main/java/com/datadoghq/trace/impl/DDSpanContext.java index 4d19109a39..de2249e3bb 100644 --- a/src/main/java/com/datadoghq/trace/impl/DDSpanContext.java +++ b/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.HashMap; -import java.util.List; -import java.util.Map; +import java.util.*; 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 baggageItems; + private Map 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 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 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 baggageItems, boolean errorFlag, String spanType, + Map tags, List trace, DDTracer tracer) { @@ -69,15 +76,19 @@ public class DDSpanContext implements io.opentracing.SpanContext { this.parentId = parentId; if (baggageItems == null) { - this.baggageItems = new HashMap(); + 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(); } 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(); + } 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(); + } + 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 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; + } } diff --git a/src/main/java/com/datadoghq/trace/impl/DDTracer.java b/src/main/java/com/datadoghq/trace/impl/DDTracer.java index 5d6733fa6d..187b977563 100644 --- a/src/main/java/com/datadoghq/trace/impl/DDTracer.java +++ b/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.HashMap; -import java.util.List; -import java.util.Map; +import java.util.*; /** * 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 tags = new HashMap(); + private Map 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(); + } 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.emptyMap() : p.getBaggageItems(), errorFlag, spanType, + this.tags, this.parent == null ? null : p.getTrace(), DDTracer.this ); diff --git a/src/main/java/com/datadoghq/trace/writer/impl/DDAgentWriter.java b/src/main/java/com/datadoghq/trace/writer/impl/DDAgentWriter.java index 9df88b485a..817dd0b2eb 100644 --- a/src/main/java/com/datadoghq/trace/writer/impl/DDAgentWriter.java +++ b/src/main/java/com/datadoghq/trace/writer/impl/DDAgentWriter.java @@ -75,7 +75,6 @@ public class DDAgentWriter implements Writer { */ public void write(List trace) { //Try to add a new span in the queue - //FIXME oldest? boolean proceed = tokens.tryAcquire(trace.size()); if (proceed) { diff --git a/src/test/java/com/datadoghq/trace/impl/DDSpanSerializerTest.java b/src/test/java/com/datadoghq/trace/impl/DDSpanSerializerTest.java index b3fa50acfa..eb35b3770a 100644 --- a/src/test/java/com/datadoghq/trace/impl/DDSpanSerializerTest.java +++ b/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); + } } } diff --git a/src/test/java/com/datadoghq/trace/impl/DDSpanTest.java b/src/test/java/com/datadoghq/trace/impl/DDSpanTest.java index 67510a730f..b97a8e5cf8 100644 --- a/src/test/java/com/datadoghq/trace/impl/DDSpanTest.java +++ b/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) - public void shouldHaveServiceName() { - new DDTracer().buildSpan("operationName").start(); + @Test + public void testGetterSetter() { + + DDSpanContext context = new DDSpanContext( + 1L, + 1L, + 0L, + "fakeService", + "fakeOperation", + "fakeResource", + Collections.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); } + + } \ No newline at end of file diff --git a/src/test/java/com/datadoghq/trace/impl/RateSamplerTest.java b/src/test/java/com/datadoghq/trace/impl/RateSamplerTest.java index 99668033f6..811f363df9 100644 --- a/src/test/java/com/datadoghq/trace/impl/RateSamplerTest.java +++ b/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); }