Fix for #2114 Support b3 trace header #2329
Conversation
|
one thing to spot check in the implementation is if "b3" is consumed or not. I know some proxies pass headers, which could result in a stale "b3" value sent out on the other side. Probably not an issue here, but worth asking |
There was a problem hiding this comment.
Nice work @dtacalau! Left a few comments, mainly around code style, etc. What I am wondering however is whether this same logic + tests should be implemented for the h2 version of the code.
linkerd/protocol/http/src/main/scala/io/buoyant/linkerd/protocol/ZipkinTracePropagator.scala
Outdated
Show resolved
Hide resolved
linkerd/protocol/http/src/main/scala/io/buoyant/linkerd/protocol/ZipkinTracePropagator.scala
Outdated
Show resolved
Hide resolved
linkerd/protocol/http/src/main/scala/io/buoyant/linkerd/protocol/ZipkinTracePropagator.scala
Outdated
Show resolved
Hide resolved
linkerd/protocol/http/src/main/scala/io/buoyant/linkerd/protocol/ZipkinTracePropagator.scala
Outdated
Show resolved
Hide resolved
linkerd/protocol/http/src/main/scala/io/buoyant/linkerd/protocol/ZipkinTracePropagator.scala
Outdated
Show resolved
Hide resolved
...rotocol/http/src/test/scala/io/buoyant/linkerd/protocol/http/ZipkinTracePropagatorTest.scala
Outdated
Show resolved
Hide resolved
...rotocol/http/src/test/scala/io/buoyant/linkerd/protocol/http/ZipkinTracePropagatorTest.scala
Outdated
Show resolved
Hide resolved
...rotocol/http/src/test/scala/io/buoyant/linkerd/protocol/http/ZipkinTracePropagatorTest.scala
Outdated
Show resolved
Hide resolved
...rotocol/http/src/test/scala/io/buoyant/linkerd/protocol/http/ZipkinTracePropagatorTest.scala
Outdated
Show resolved
Hide resolved
...rotocol/http/src/test/scala/io/buoyant/linkerd/protocol/http/ZipkinTracePropagatorTest.scala
Outdated
Show resolved
Hide resolved
f1cbbb8
to
27b7198
Compare
…ill propagate b3 single header the same way as x-b3- multi headers. If a request contains "b3" single header and "X-B3-" multi headers, the trace context from "b3" is preferred and "x-b3-" will be ignored. Added support for 128bit traceId. Signed-off-by: dst4096 <[email protected]>
27b7198
to
9995780
Compare
Done, added support for b3 in h2 implementation, also I've updated unit test for h2. |
|
PS as timing is interesting sometimes, a user of a gateway project just found what appears to be a propagation bug relating to my earlier comment. Basically verify that this product won't end up passing through either header choice unaffected, and it will save users from lengthy troubleshooting later spring-cloud/spring-cloud-sleuth#1452 |
linkerd/protocol/h2/src/main/scala/io/buoyant/linkerd/protocol/h2/ZipkinTracePropagator.scala
Outdated
Show resolved
Hide resolved
linkerd/protocol/http/src/main/scala/io/buoyant/linkerd/protocol/ZipkinTracePropagator.scala
Outdated
Show resolved
Hide resolved
...rotocol/http/src/test/scala/io/buoyant/linkerd/protocol/http/ZipkinTracePropagatorTest.scala
Outdated
Show resolved
Hide resolved
...rotocol/http/src/test/scala/io/buoyant/linkerd/protocol/http/ZipkinTracePropagatorTest.scala
Outdated
Show resolved
Hide resolved
linkerd/protocol/http/src/main/scala/io/buoyant/linkerd/protocol/ZipkinTracePropagator.scala
Outdated
Show resolved
Hide resolved
| req.headerMap.add("x-b3-sampled", "0") | ||
|
|
||
| val trace = ztp.traceId(req) | ||
| trace match { |
There was a problem hiding this comment.
Again, making use of the combinators would probably make sense here.
linkerd/protocol/h2/src/main/scala/io/buoyant/linkerd/protocol/h2/ZipkinTracePropagator.scala
Outdated
Show resolved
Hide resolved
| convertB3SingleHeaderToMultiHeaders(headers) | ||
|
|
||
| // expect to read a 128bit traceid field, b3 single header supports 128bit traceids | ||
| val trace128Bit = caseInsensitiveGet(headers, ZipkinTraceHeader) match { |
There was a problem hiding this comment.
The diff is complicated by the fact that we're adding support for the b3 header AND adding support for 128 bit trace ids at the same time. Can these be split into two separate PRs?
linkerd/protocol/h2/src/main/scala/io/buoyant/linkerd/protocol/h2/ZipkinTracePropagator.scala
Outdated
Show resolved
Hide resolved
linkerd/protocol/http/src/main/scala/io/buoyant/linkerd/protocol/ZipkinTracePropagator.scala
Outdated
Show resolved
Hide resolved
…. Moved implementation of http and h2 ZipkinTracePropagator to a new common place base-http. Fixed setting of sampled/flags, computation of sampled. Updated tests. Support for 128bit trace. Signed-off-by: dst4096 <[email protected]>
|
The code from this PR has been rewritten and I've created another PR |
With this fix Linkerd will propagate b3 single header the same way it propagates x-b3- multi headers. If a request contains "b3" single header and "X-B3-" multi headers, the trace context from "b3" is preferred and "x-b3-" will be ignored. "b3" is handled the same way as finagle: extract the traceid from "b3" header, write back "X-B3-*" multi headers, and let the flow continue as if it has received "x-b3-".
No config changes were needed, "b3" is not written on the way out, not sure if it's a good idea since finagle does not write "b3".
Also added support for 128bit traceId has been implemented with this fix because "b3" supports 128bit traceIds.
This has been tested in two ways:
Signed-off-by: dst4096 [email protected]