Merge pull request #59 from ExpediaDotCom/parentid-bug-fix

keshavpeswani · web-flow · commit ae8ce3728caf · 2018-07-01T20:48:06.000-07:00
set parentSpanId as null in place of initializing it to UUID.zero
diff --git a/core/src/main/java/com/expedia/www/haystack/client/Tracer.java b/core/src/main/java/com/expedia/www/haystack/client/Tracer.java
@@ -267,8 +267,7 @@ public Scope startActive(boolean finishSpanOnClose) {
 
         protected SpanContext createNewContext() {
             UUID randomId = UUID.randomUUID();
-            UUID zero = new UUID(0l, 0l);
-            return createContext(randomId, randomId, zero, Collections.<String, String>emptyMap());
+            return createContext(randomId, randomId, null, Collections.emptyMap());
         }
 
         protected SpanContext createContext(UUID traceId, UUID spanId, UUID parentId, Map<String, String> baggage) {
diff --git a/core/src/main/java/com/expedia/www/haystack/client/dispatchers/formats/ProtoBufFormat.java b/core/src/main/java/com/expedia/www/haystack/client/dispatchers/formats/ProtoBufFormat.java
@@ -16,21 +16,20 @@
  */
 package com.expedia.www.haystack.client.dispatchers.formats;
 
-import java.io.ByteArrayOutputStream;
-import java.io.IOException;
-import java.io.ObjectOutputStream;
-import java.util.stream.Collectors;
-
-import org.slf4j.Logger;
-import org.slf4j.LoggerFactory;
-
 import com.expedia.open.tracing.Log;
 import com.expedia.open.tracing.Tag;
 import com.expedia.open.tracing.Tag.TagType;
 import com.expedia.www.haystack.client.LogData;
 import com.expedia.www.haystack.client.Span;
 import com.expedia.www.haystack.client.SpanContext;
 import com.google.protobuf.ByteString;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.ObjectOutputStream;
+import java.util.stream.Collectors;
 
 public class ProtoBufFormat implements Format<com.expedia.open.tracing.Span> {
     private static final Logger LOGGER = LoggerFactory.getLogger(ProtoBufFormat.class);
@@ -41,36 +40,35 @@ public com.expedia.open.tracing.Span format(Span span) {
 
         SpanContext context = span.context();
         builder.setTraceId(context.getTraceId().toString())
-            .setSpanId(context.getSpanId().toString())
-            .setParentSpanId(context.getParentId().toString());
+                .setSpanId(context.getSpanId().toString());
 
-        builder.setServiceName(span.getServiceName())
-            .setOperationName(span.getOperatioName());
+        if (context.getParentId() != null) {
+            builder.setParentSpanId(context.getParentId().toString());
+        }
 
-        builder.setStartTime(span.getStartTime());
+        builder.setServiceName(span.getServiceName())
+                .setOperationName(span.getOperatioName())
+                .setStartTime(span.getStartTime());
 
         if (span.getDuration() != null) {
             builder.setDuration(span.getDuration());
         }
 
         builder.addAllLogs(span.getLogs().stream()
-                           .map(l -> buildLog(l))
-                           .collect(Collectors.toList()));
-
-        builder.addAllTags(span.getTags().entrySet().stream()
+                           .map(this::buildLog)
+                           .collect(Collectors.toList()))
+                .addAllTags(span.getTags().entrySet().stream()
                            .map(e -> buildTag(e.getKey(), e.getValue()))
-                           .collect(Collectors.toList()));
-
-        // add the baggage items as tags for now
-        builder.addAllTags(context.getBaggage().entrySet().stream()
+                           .collect(Collectors.toList()))
+                .addAllTags(context.getBaggage().entrySet().stream() // add the baggage items as tags for now
                            .map(e -> buildTag(e.getKey(), e.getValue()))
                            .collect(Collectors.toList()));
 
         return builder.build();
     }
 
-    protected Log buildLog(LogData log) {
-        Log.Builder builder = Log.newBuilder()
+    protected Log buildLog(final LogData log) {
+        final Log.Builder builder = Log.newBuilder()
             .setTimestamp(log.getTimestamp());
 
         if (log.getFields() != null ) {
@@ -87,7 +85,7 @@ protected Tag buildTag(String key, Object value) {
         if (value == null) {
             // just a message collected; adding an empty payload
             builder.setType(TagType.STRING);
-            builder.setVStr(new String());
+            builder.setVStr("");
         } else if (value instanceof String) {
             builder.setType(TagType.STRING);
             builder.setVStr((String) value);
@@ -103,8 +101,8 @@ protected Tag buildTag(String key, Object value) {
         } else {
             builder.setType(TagType.BINARY);
 
-            try (ByteArrayOutputStream out = new ByteArrayOutputStream()) {
-                try (ObjectOutputStream os = new ObjectOutputStream(out)) {
+            try (final ByteArrayOutputStream out = new ByteArrayOutputStream()) {
+                try (final ObjectOutputStream os = new ObjectOutputStream(out)) {
                     os.writeObject(value);
                     builder.setVBytes(ByteString.copyFrom(out.toByteArray()));
                 }
diff --git a/core/src/main/java/com/expedia/www/haystack/client/propagation/TextMapCodex.java b/core/src/main/java/com/expedia/www/haystack/client/propagation/TextMapCodex.java
@@ -23,11 +23,11 @@ public class TextMapCodex implements Codex<String,Object> {
 
     @Override
     public String encode(Object value) {
-        return value.toString();
+        return value == null ? null : value.toString();
     }
 
     @Override
     public String decode(Object value) {
-        return value.toString();
+        return  value == null ? null : value.toString();
     }
 }
diff --git a/core/src/main/java/com/expedia/www/haystack/client/propagation/TextMapPropagator.java b/core/src/main/java/com/expedia/www/haystack/client/propagation/TextMapPropagator.java
@@ -106,7 +106,7 @@ public SpanContext extract(TextMap carrier) {
 
         SpanContext context = new SpanContext(UUID.fromString(traceId),
                                               UUID.fromString(spanId),
-                                              UUID.fromString(parentId));
+                                              parentId == null ? null : UUID.fromString(parentId));
         return context.addBaggage(baggage);
     }
 
diff --git a/core/src/test/java/com/expedia/www/haystack/client/SpanPropagationTest.java b/core/src/test/java/com/expedia/www/haystack/client/SpanPropagationTest.java
@@ -90,7 +90,7 @@ public void testActiveSpanIgnorePropagation() {
         Assert.assertEquals(0, secondSpan.getReferences().size());
         Assert.assertEquals(0, firstSpan.getReferences().size());
         Assert.assertNotEquals(firstSpan.context().getTraceId(), secondSpan.context().getTraceId());
-        Assert.assertEquals(new UUID(0l, 0l), secondSpan.context().getParentId());
+        Assert.assertNull(secondSpan.context().getParentId());
     }
 
     @Test
@@ -114,9 +114,9 @@ public void testActiveSpanPreSeeded() {
         Assert.assertEquals(parent.context().getSpanId(), child.context().getParentId());
 
         Assert.assertTrue(parent.getReferences().isEmpty());
-        Assert.assertEquals(new UUID(0l, 0l), parent.context().getParentId());
+        Assert.assertNull(parent.context().getParentId());
         Assert.assertTrue(active.getReferences().isEmpty());
-        Assert.assertEquals(new UUID(0l, 0l), active.context().getParentId());
+        Assert.assertNull(active.context().getParentId());
     }
 
     @Test
diff --git a/core/src/test/java/com/expedia/www/haystack/client/dispatchers/formats/ProtBufFormatTest.java b/core/src/test/java/com/expedia/www/haystack/client/dispatchers/formats/ProtBufFormatTest.java
@@ -16,20 +16,19 @@
  */
 package com.expedia.www.haystack.client.dispatchers.formats;
 
-import static org.junit.Assert.assertEquals;
-import static org.junit.Assert.assertNotNull;
-
-import org.junit.Assert;
-import org.junit.Before;
-import org.junit.Test;
-
 import com.expedia.open.tracing.Log;
 import com.expedia.open.tracing.Tag;
 import com.expedia.www.haystack.client.Span;
 import com.expedia.www.haystack.client.Tracer;
 import com.expedia.www.haystack.client.dispatchers.InMemoryDispatcher;
 import com.expedia.www.haystack.client.metrics.NoopMetricsRegistry;
 import com.google.common.collect.ImmutableMap;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotNull;
 
 public class ProtBufFormatTest {
     private Tracer tracer;
@@ -156,5 +155,4 @@ public void testSpanConversion() {
         // Tags + Baggage for now.
         assertEquals(2, protoSpan.getTagsCount());
     }
-
 }

Original file line number	Diff line number	Diff line change
`@@ -267,8 +267,7 @@ public Scope startActive(boolean finishSpanOnClose) {`
`267`	`267`
`268`	`268`	`protected SpanContext createNewContext() {`
`269`	`269`	`UUID randomId = UUID.randomUUID();`
`270`		`- UUID zero = new UUID(0l, 0l);`
`271`		`- return createContext(randomId, randomId, zero, Collections.<String, String>emptyMap());`
	`270`	`+ return createContext(randomId, randomId, null, Collections.emptyMap());`
`272`	`271`	`}`
`273`	`272`
`274`	`273`	`protected SpanContext createContext(UUID traceId, UUID spanId, UUID parentId, Map<String, String> baggage) {`
Original file line number	Diff line number	Diff line change
`@@ -23,11 +23,11 @@ public class TextMapCodex implements Codex<String,Object> {`
`23`	`23`
`24`	`24`	`@Override`
`25`	`25`	`public String encode(Object value) {`
`26`		`- return value.toString();`
	`26`	`+ return value == null ? null : value.toString();`
`27`	`27`	`}`
`28`	`28`
`29`	`29`	`@Override`
`30`	`30`	`public String decode(Object value) {`
`31`		`- return value.toString();`
	`31`	`+ return value == null ? null : value.toString();`
`32`	`32`	`}`
`33`	`33`	`}`
Original file line number	Diff line number	Diff line change
`@@ -106,7 +106,7 @@ public SpanContext extract(TextMap carrier) {`
`106`	`106`
`107`	`107`	`SpanContext context = new SpanContext(UUID.fromString(traceId),`
`108`	`108`	`UUID.fromString(spanId),`
`109`		`- UUID.fromString(parentId));`
	`109`	`+ parentId == null ? null : UUID.fromString(parentId));`
`110`	`110`	`return context.addBaggage(baggage);`
`111`	`111`	`}`
`112`	`112`