Add support for jaeger style uber-trace-id propagation

[nvx]

Extending on the header_type config option, I've added support for jaeger-style uber-trace-id propagation. This implements #22.

I'm not overly familiar with unit testing in Lua however so I haven't made unit tests for the new functionality, but it has been tested manually and seems to work as intended including inter-op with downstream http services using the golang Jaeger library.

As an aside, I noticed setting header_type to eg w3c doesn't prevent the generation of b3 headers to the downstream service if no headers were found in the incoming request. It doesn't stop the configured header_type from being generated as well so not nothing breaks, but did seem a little odd?

[seh]

I noticed setting header_type to eg w3c doesn't prevent the generation of b3 headers to the downstream service if no headers were found in the incoming request. It doesn't stop the configured header_type from being generated as well so not nothing breaks, but did seem a little odd?

See the discussion starting here: #67 (comment).

[nvx]

Cheers for the heads up.

I also just noticed there was a merge conflict due to changes in master that I've just resolved now too. Any ETA on getting this merged @Tieske / @kikito. Happy to do more work on this if anything else is needed.

[CLAassistant]

All committers have signed the CLA.

[nvx]

I've accepted those two changes you suggested @greut, resolved the merge conflict with master, and signed the CLA (I assume the CLA is new as I don't remember it flagging up previously).

[Tieske]

there still seem to be conflicts, but more importantly it seems to miss tests?

[nvx]

GitHub says there are no conflicts with the base branch?

As for the tests, I mentioned in the original post of the PR that I'm not familiar with unit testing in Lua. If there's a quick guide you can point me to for how to run the test suite I'd be more than happy to write up some tests for the new functionality.

[Tieske]

try this:

# install Pongo
git clone https://github.com/kong/kong-pongo
cd kong-pongo
make install
# adjust PATH if you have to here
cd ..

# clone plugin
git clone https://github.com/kong/kong-plugin-zipkin
cd kong-plugin-zipkin

# run tests
pongo run

Additional info in the README.md.

[nvx]

Unit tests added and a couple of issues fixed.

As an aside, I noticed a couple of issues with parse_w3c_trace_context_headers (which I based the jaeger parse function on) while implementing unit tests for the jaeger headers functionality as they caused my unit tests to fail (the W3C tests pass coincidentally due to other factors).

The first issue is that it returns 4 items on error, but only 3 when no error is returned.

kong-plugin-zipkin/kong/plugins/zipkin/tracing_headers.lua

Line 162 in b3a9807

return nil, nil, nil, should_sample

vs

kong-plugin-zipkin/kong/plugins/zipkin/tracing_headers.lua

Line 202 in b3a9807

return trace_id, parent_id, should_sample

I suspect returning just nil 3 times and not also returning should_sample is the correct behaviour as that triggers the should sample logic. This is effectively the current behaviour anyway as the 4th item returned isn't referenced anywhere.

kong-plugin-zipkin/kong/plugins/zipkin/handler.lua

Lines 109 to 111 in 4825b09

	if should_sample == nil then
	should_sample = math_random() < conf.sample_ratio
	end

If returning should_sample rather than nil (which would be false in most cases) is considered the right option instead, I can adjust the Jaeger parser to match that as well.

The same function also doesn't return in one of the error cases, but is then caught by the version check just afterwards. I suspect this is just a case of a missing return line:

kong-plugin-zipkin/kong/plugins/zipkin/tracing_headers.lua

Lines 168 to 176 in b3a9807

	if version == nil or trace_id == nil or parent_id == nil or trace_flags == nil then
	warn("invalid W3C traceparent header; ignoring.")
	end
	-- Only support version 00 of the W3C Trace Context spec.
	if version ~= "00" then
	warn("invalid W3C Trace Context version; ignoring.")
	return nil, nil, nil, should_sample
	end

Both are pretty trivial to fix, but I didn't want to clutter this PR with those changes too.

[Tieske]

@kikito mind having a look, since you did most of the maintenance on this repo iirc?

[kikito]

Hello, thank you for sending this PR and apologies for the time it took us to merge it.

I merged this on another pr (#101) after rebasing over master.

I noticed a couple of issues with parse_w3c_trace_context_headers

Thanks for this. I have done the relevant changes in #100