Add support for W3C Trace Context #69
Conversation
Add tests. Update header set to be lowercase. Optimize the pattern matching of the header value. Refactor parsing function for ease of reading. Fix linting.
Update W3C header parsing
There was a problem hiding this comment.
LGTM, thanks Victor,
I am not a maintainer of this repo, but I think as far as the configurability,
it can be done later in another PR.
The only obvious drawback I see is the added network cost of around 100 bytes per proxied request,
I'll leave the maintainers be judge of that.
| @@ -136,6 +136,13 @@ if subsystem == "http" then | |||
| -- Want to send headers to upstream | |||
| local proxy_span = zipkin.proxy_span | |||
| local set_header = kong.service.request.set_header | |||
|
|
|||
| -- Set the W3C Trace Context header | |||
| -- TODO: Should W3C traceparent header be set even it wasn't on the incoming request? | |||
There was a problem hiding this comment.
I asked myself a very similar question when implementing the b3 single header (#66).
In order to minimize the amount of configuration involved, I decided to take a salomonical approach: The plugin understands b3 single headers, but it only produces "regular" headers (such as x-b3-traceid).
It seemed an ok solution for the time but since we are adding a third kind of header I think we should control it via a new config file. A possible design would be:
options.header_types is a new config option with 3 possible values:
preserve: the plugin produces whatever format it receives on each requestb3-single: always use b3 singleb3: always use the "regular zipkin" headers, such asx-b3-traceidw3c-trace-context: use the format proposed on this PR
When options.header_types is set to anything other than preserve, and the trace headers don't match (for example the config is b3-single but we receive a w3c-trace-context) then we should send both kinds of headers. We should also log a warning about the inconsistency.
If you want me to take over adding this, please let me know.
There was a problem hiding this comment.
@victorNewRelic, do you intend to continue with this patch?
|
@victorNewRelic @ecourreges-orange @seh I've added the features I mentioned in #75, so I will be closing this PR in favor of that one. Thanks a lot! |
|
@kikito - Great!! I'll test the new PR. |
Accepts W3C Trace Context header and prioritizes the W3C Trace Context when present.
Emits W3C Trace Context header whether it was present on the incoming request or not. Would like feedback on whether this behavior should be configurable.