From 0c21da75a07ed45c615b949ddef7db2e2b5242e4 Mon Sep 17 00:00:00 2001
From: Jim Schubert <james.schubert@gmail.com>
Date: Tue, 26 Sep 2017 23:34:57 -0400
Subject: [PATCH] Coerce traceIdHigh=0L to None, include tests

---
 .../com/twitter/finagle/tracing/Id.scala      | 10 +++--
 .../com/twitter/finagle/tracing/Trace.scala   |  4 +-
 .../com/twitter/finagle/tracing/IdTest.scala  | 42 ++++++++++++++++++-
 3 files changed, 49 insertions(+), 7 deletions(-)

diff --git a/finagle-core/src/main/scala/com/twitter/finagle/tracing/Id.scala b/finagle-core/src/main/scala/com/twitter/finagle/tracing/Id.scala
index e685e51e915..8408fc95ad2 100644
--- a/finagle-core/src/main/scala/com/twitter/finagle/tracing/Id.scala
+++ b/finagle-core/src/main/scala/com/twitter/finagle/tracing/Id.scala
@@ -119,7 +119,7 @@ object TraceId {
       val trace64 = ByteArrays.get64be(bytes, 16)
       val flags64 = ByteArrays.get64be(bytes, 24)
 
-      val traceIdHigh = if(bytes.length == 40) ByteArrays.get64be(bytes, 32) else -1L
+      val traceIdHigh = if(bytes.length == 40) ByteArrays.get64be(bytes, 32) else 0L
 
       val flags = Flags(flags64)
       val sampled = if (flags.isFlagSet(Flags.SamplingKnown)) {
@@ -132,7 +132,7 @@ object TraceId {
         SpanId(span64),
         sampled,
         flags,
-        if(traceIdHigh == -1L) None else Some(SpanId(traceIdHigh))
+        Some(SpanId(traceIdHigh))
       )
       Return(traceId)
     }
@@ -223,7 +223,11 @@ final case class TraceId(
     case Some(id) => id
   }
 
-  def traceIdHigh: Option[SpanId] = _traceIdHigh
+  def traceIdHigh: Option[SpanId] = _traceIdHigh match {
+    case None => None
+    case Some(id) if id.self == 0L => None
+    case tid@Some(_) => tid
+  }
 
   /**
    * Override [[_sampled]] to Some(true) if the debug flag is set.
diff --git a/finagle-core/src/main/scala/com/twitter/finagle/tracing/Trace.scala b/finagle-core/src/main/scala/com/twitter/finagle/tracing/Trace.scala
index dd49253dca8..7535aff9df3 100644
--- a/finagle-core/src/main/scala/com/twitter/finagle/tracing/Trace.scala
+++ b/finagle-core/src/main/scala/com/twitter/finagle/tracing/Trace.scala
@@ -72,7 +72,7 @@ object Trace {
       val trace64 = ByteArrays.get64be(bytes, 16)
       val flags64 = ByteArrays.get64be(bytes, 24)
 
-      val traceIdHigh = if(body.length == 40) ByteArrays.get64be(bytes, 32) else -1L
+      val traceIdHigh = if(body.length == 40) ByteArrays.get64be(bytes, 32) else 0L
 
       val flags = Flags(flags64)
       val sampled = if (flags.isFlagSet(Flags.SamplingKnown)) {
@@ -85,7 +85,7 @@ object Trace {
         SpanId(span64),
         sampled,
         flags,
-        if(traceIdHigh == -1L) None else Some(SpanId(traceIdHigh))
+        Some(SpanId(traceIdHigh))
       )
 
       Return(traceId)
diff --git a/finagle-core/src/test/scala/com/twitter/finagle/tracing/IdTest.scala b/finagle-core/src/test/scala/com/twitter/finagle/tracing/IdTest.scala
index d048fbd4762..1d2c31ce58d 100644
--- a/finagle-core/src/test/scala/com/twitter/finagle/tracing/IdTest.scala
+++ b/finagle-core/src/test/scala/com/twitter/finagle/tracing/IdTest.scala
@@ -12,10 +12,22 @@ class IdTest extends FunSuite {
     assert(TraceId(None, None, SpanId(0L), None) != TraceId(None, None, SpanId(1L), None))
   }
 
+  test("compare unequal ids (128bit)") {
+    val first = TraceId(Some(SpanId(1L)), None, SpanId(2L), None, Flags(), Some(SpanId(3L)))
+    val second = TraceId(Some(SpanId(1L)), None, SpanId(2L), None, Flags(), Some(SpanId(4L)))
+    assert(first != second)
+  }
+
   test("compare equal ids") {
     assert(TraceId(None, None, SpanId(0L), None) == TraceId(None, None, SpanId(0L), None))
   }
 
+  test("compare equal ids (128bit)") {
+    val first = TraceId(Some(SpanId(1L)), None, SpanId(2L), None, Flags(), Some(SpanId(3L)))
+    val second = TraceId(Some(SpanId(1L)), None, SpanId(2L), None, Flags(), Some(SpanId(3L)))
+    assert(first == second)
+  }
+
   test("compare synthesized parentId") {
     assert(
       TraceId(None, Some(SpanId(1L)), SpanId(1L), None) ==
@@ -48,6 +60,32 @@ class IdTest extends FunSuite {
 
   def hex(l: Long) = new RichU64Long(l).toU64HexString
 
+  test("extract 64bit only ids") {
+    val low = 5208512171318403364L
+    val spanId = hex(low)
+    val (highBits, lowBits) = TraceId.mk128BitTraceId(spanId)
+
+    assert(highBits.isDefined == false)
+    assert(lowBits.isDefined)
+    assert(lowBits.get.self == low)
+  }
+
+  test("extract 128bit ids") {
+    val low = 5208512171318403364L
+    val high = 5060571933882717101L
+    val spanId =  hex(high) + hex(low)
+    val (highBits, lowBits) = TraceId.mk128BitTraceId(spanId)
+
+    assert(highBits.isDefined)
+    assert(highBits.get.self == high)
+    assert(lowBits.isDefined)
+    assert(lowBits.get.self == low)
+  }
+
+  test("drops invalid traceIdHigh = 0L") {
+    assert(TraceId(Some(SpanId(1L)), None, SpanId(2L), None, Flags(), Some(SpanId(0L))).traceIdHigh.isEmpty)
+  }
+
   test("SpanId.toString: each bit must be correct") {
     for (b <- 0 until 64)
       assert(hex(1 << b) == SpanId(1 << b).toString)
@@ -63,8 +101,8 @@ class IdTest extends FunSuite {
 
   test("hashCode only accounts for id fields") {
     assert(
-      TraceId(Some(SpanId(1L)), Some(SpanId(2L)), SpanId(3L), Some(true)).hashCode ==
-        TraceId(Some(SpanId(1L)), Some(SpanId(2L)), SpanId(3L), Some(false)).hashCode
+      TraceId(Some(SpanId(1L)), Some(SpanId(2L)), SpanId(3L), Some(true), Flags(), Some(SpanId(4L))).hashCode ==
+        TraceId(Some(SpanId(1L)), Some(SpanId(2L)), SpanId(3L), Some(false), Flags(Flags.Debug), Some(SpanId(4L))).hashCode
     )
   }
 }