ZipkinTracePropagator, fixes for handling of x-b3-flags and x-b3-samp…

…led headers. If header x-b3-flags is present and has value 1, header x-b3-sampled should be ignored, sampled decision is true. Header x-b3-sampled should be read only if x-b3-flags not present. Same story for setting these two headers, if x-b3-flags is to be set to 1, x-b3-sampled should not be set as it is redundant info.

Signed-off-by: dst4096 <[email protected]>

linkerd/protocol/base-http/src/main/scala/io/buoyant/linkerd/http/ZipkinTracePropagator.scala

@@ -7,6 +7,7 @@ import com.twitter.finagle.tracing.{Flags, SpanId, TraceId, TraceId128}
 import com.twitter.util.Try
 import io.buoyant.linkerd.{TracePropagator, TracePropagatorInitializer}
 import io.buoyant.router.http.{HeadersLike, RequestLike}
+import com.twitter.logging.Logger
 
 class ZipkinTracePropagator[Req, H: HeadersLike](implicit requestLike: RequestLike[Req, H]) extends TracePropagator[Req] {
  /**
@@ -83,19 +84,47 @@ object ZipkinTrace {
  }
 
  headersLike.set(headers, ZipkinParentHeader, id.parentId.toString)
- headersLike.set(headers, ZipkinSampleHeader, (if ((id.sampled exists { _ == true })) 1 else 0).toString)
- headersLike.set(headers, ZipkinFlagsHeader, id.flags.toLong.toString)
+
+ //the only valid value for x-b3-flags is 1, which means debug
+ if (id.flags.isDebug) {
+ //when setting x-b3-flags to debug, 1, do not also set x-b3-sampled because it is a redundant info
+ headersLike.set(headers, ZipkinFlagsHeader, id.flags.toLong.toString)
+ } else {
+ id.sampled match {
+ // valid values fro x-b3-sampled are 1 and 0
+ case Some(true) => headersLike.set(headers, ZipkinSampleHeader, "1")
+ case Some(false) => headersLike.set(headers, ZipkinSampleHeader, "0")
+ case None => // do nothing
+ }
+ }
  ()
  }
 
  def getSampler[H: HeadersLike](headers: H): Option[Float] = {
  val headersLike = implicitly[HeadersLike[H]]
-
- headersLike.get(headers, ZipkinSampleHeader).flatMap { s =>
- Try(s.toFloat).toOption.map {
- case v if v < 0 => 0.0f
- case v if v > 1 => 1.0f
- case v => v
+ val samplerNone: Option[Float] = None
+ val samplerTrue: Option[Float] = Option(1.0f)
+ val samplerFalse: Option[Float] = Option(0.0f)
+
+ // first try getting x-b3-flags, flags = 1 means debug
+ val flags = caseInsensitiveGet(headers, ZipkinFlagsHeader)
+ if (flags.isEmpty) {
+ // try getting x-b3-sampled only if x-b3-flags not present
+ caseInsensitiveGet(headers, ZipkinSampleHeader).flatMap { s =>
+ Try(s.toFloat).toOption match {
+ //x-b3-sampled present, the only valid values for x-b3-sampled are 0 and 1, any other values are invalid and should be ignored
+ case Some(v) if v == 0 => samplerFalse
+ case Some(v) if v == 1 => samplerTrue
+ case _ => samplerNone
+ }
+ }
+ } else {
+ flags.flatMap { s =>
+ Try(s.toLong).toOption match {
+ //x-b3-flags present, the only valid value for x-b3-flags is 1, any other values are invalid and should be ignored
+ case Some(v) if v == 1 => samplerTrue
+ case _ => samplerNone
+ }
  }
  }
  }

linkerd/protocol/h2/src/test/scala/io/buoyant/linkerd/protocol/h2/ZipkinTracePropagatorTest.scala

@@ -39,16 +39,13 @@ class ZipkinTracePropagatorTest extends FunSuite {
  assert(tid.sampled.contains(true))
 
  // expect to get the right sampled value which is 0
- val sampler = ZipkinTrace.getSampler(req1.headers)
- assert(sampler.contains(1.0f))
+ assert(ZipkinTrace.getSampler(req1.headers).contains(1.0f))
  }}
 
  // expect to get the right sampled value which is 1
- val sampler1 = ZipkinTrace.getSampler(req1.headers)
- assert(sampler1.contains(1.0f))
+ assert(ZipkinTrace.getSampler(req1.headers).contains(1.0f))
  // expect to get the right sampled value which is 1
- val sampler2 = ZipkinTrace.getSampler(req2.headers)
- assert(sampler2.contains(1.0f))
+ assert(ZipkinTrace.getSampler(req2.headers).contains(1.0f))
  }
 
  test("get traceid from multi x-b3 headers - set/get 128bit trace, two fields") {
@@ -70,4 +67,48 @@ class ZipkinTracePropagatorTest extends FunSuite {
  assert(req2.headers.get("x-b3-spanid").contains("05e3ac9a4f6e3b90"))
  }}
  }
- }
+
+ test("multi x-b3 headers - get flags/sampled test") {
+ val ztp = new ZipkinTracePropagator()
+ val req = Request("http", Method.Get, "auf", "/", Stream.empty())
+ req.headers.add("x-b3-traceid", "80f198ee56343ba864fe8b2a57d3eff7")
+ req.headers.add("x-b3-spanid", "05e3ac9a4f6e3b90")
+
+ //flags 1, no sampled => sampler 1
+ req.headers.add("x-b3-flags", "1")
+ assert(ZipkinTrace.getSampler(req.headers).contains(1.0f))
+
+ //flags 0 (invalid value), no sampled => sampler None
+ req.headers.remove("x-b3-flags")
+ req.headers.add("x-b3-flags", "0")
+ assert(ZipkinTrace.getSampler(req.headers).contains(0.0f))
+
+ //flags asd (invalid value), no sampled = > sampler None
+ req.headers.remove("x-b3-flags")
+ req.headers.add("x-b3-flags", "asd")
+ assert(ZipkinTrace.getSampler(req.headers).isEmpty)
+
+ //flags 1, sampled 1 (redundant sampled since flags is already 1)
+ req.headers.remove("x-b3-flags")
+ req.headers.add("x-b3-flags", "1")
+ req.headers.add("x-b3-sampled", "1")
+ assert(ZipkinTrace.getSampler(req.headers).contains(1.0f))
+
+ //sampled 1, no flags
+ req.headers.remove("x-b3-flags")
+ req.headers.remove("x-b3-sampled")
+ req.headers.add("x-b3-sampled", "1")
+ assert(ZipkinTrace.getSampler(req.headers).contains(1.0f))
+
+ //sampled 0, no flags
+ req.headers.remove("x-b3-flags")
+ req.headers.remove("x-b3-sampled")
+ req.headers.add("x-b3-sampled", "0")
+ assert(ZipkinTrace.getSampler(req.headers).contains(0.0f))
+
+ //no sampled, no flags
+ req.headers.remove("x-b3-flags")
+ req.headers.remove("x-b3-sampled")
+ assert(ZipkinTrace.getSampler(req.headers).isEmpty)
+ }
+}

linkerd/protocol/http/src/test/scala/io/buoyant/linkerd/protocol/http/ZipkinTracePropagatorTest.scala

@@ -7,7 +7,7 @@ import io.buoyant.linkerd.protocol.http._
 import io.buoyant.router.HttpInstances._
 
 class ZipkinTracePropagatorTest extends FunSuite {
- test("get traceid from multi x-b3 headers, 64bit trace id, sampled, UPPER CASE header doesn't matter") {
+ test("multi x-b3 headers, 64bit trace id, sampled, UPPER CASE header doesn't matter") {
  val ztp = new ZipkinTracePropagator
 
  //x-b3 multi headers with lower case
@@ -36,21 +36,15 @@ class ZipkinTracePropagatorTest extends FunSuite {
  assert(tid.spanId.toString().equals("05e3ac9a4f6e3b90"))
  assert(tid.parentId.toString().equals("e457b5a2e4d86bd1"))
  assert(tid.sampled.contains(true))
-
- // expect to get the right sampled value which is 0
- val sampler = ZipkinTrace.getSampler(req1.headerMap)
- assert(sampler.contains(1.0f))
  }}
 
  // expect to get the right sampled value which is 1
- val sampler1 = ZipkinTrace.getSampler(req1.headerMap)
- assert(sampler1.contains(1.0f))
+ assert(ZipkinTrace.getSampler(req1.headerMap).contains(1.0f))
  // expect to get the right sampled value which is 1
- val sampler2 = ZipkinTrace.getSampler(req2.headerMap)
- assert(sampler2.contains(1.0f))
+ assert(ZipkinTrace.getSampler(req2.headerMap).contains(1.0f))
  }
 
- test("get traceid from multi x-b3 headers - set/get 128bit trace, two fields") {
+ test("multi x-b3 headers - set/get 128bit trace, two fields") {
  val ztp = new ZipkinTracePropagator()
  val req = Request()
  req.headerMap.add("x-b3-traceid", "80f198ee56343ba864fe8b2a57d3eff7")
@@ -69,4 +63,48 @@ class ZipkinTracePropagatorTest extends FunSuite {
  assert(req2.headerMap.get("x-b3-spanid").contains("05e3ac9a4f6e3b90"))
  }}
  }
+
+ test("multi x-b3 headers - get flags/sampled test") {
+ val ztp = new ZipkinTracePropagator()
+ val req = Request()
+ req.headerMap.add("x-b3-traceid", "80f198ee56343ba864fe8b2a57d3eff7")
+ req.headerMap.add("x-b3-spanid", "05e3ac9a4f6e3b90")
+
+ //flags 1, no sampled => sampler 1
+ req.headerMap.add("x-b3-flags", "1")
+ assert(ZipkinTrace.getSampler(req.headerMap).contains(1.0f))
+
+ //flags 0 (invalid value), no sampled => sampler None
+ req.headerMap.remove("x-b3-flags")
+ req.headerMap.add("x-b3-flags", "0")
+ assert(ZipkinTrace.getSampler(req.headerMap).contains(0.0f))
+
+ //flags asd (invalid value), no sampled = > sampler None
+ req.headerMap.remove("x-b3-flags")
+ req.headerMap.add("x-b3-flags", "asd")
+ assert(ZipkinTrace.getSampler(req.headerMap).isEmpty)
+
+ //flags 1, sampled 1 (redundant sampled since flags is already 1)
+ req.headerMap.remove("x-b3-flags")
+ req.headerMap.add("x-b3-flags", "1")
+ req.headerMap.add("x-b3-sampled", "1")
+ assert(ZipkinTrace.getSampler(req.headerMap).contains(1.0f))
+
+ //sampled 1, no flags
+ req.headerMap.remove("x-b3-flags")
+ req.headerMap.remove("x-b3-sampled")
+ req.headerMap.add("x-b3-sampled", "1")
+ assert(ZipkinTrace.getSampler(req.headerMap).contains(1.0f))
+
+ //sampled 0, no flags
+ req.headerMap.remove("x-b3-flags")
+ req.headerMap.remove("x-b3-sampled")
+ req.headerMap.add("x-b3-sampled", "0")
+ assert(ZipkinTrace.getSampler(req.headerMap).contains(0.0f))
+
+ //no sampled, no flags
+ req.headerMap.remove("x-b3-flags")
+ req.headerMap.remove("x-b3-sampled")
+ assert(ZipkinTrace.getSampler(req.headerMap).isEmpty)
+ }
 }