Experimental tracing module (3/3): define and expose a instrumentHTTP function

[oleiade]

About

To keep Pull Requests as small as possible and reduce the review burden, this PR is the third one in a chain of three implementing the decided API design for the experimental tracing module.

warning ✋🏻 This PR is part of a PR chain, and is based on #2854

This second PR builds upon #2853 and #2854 and adds an instrumentHTTP function to k6/experimental/tracing.

Using this function in their k6 scripts, once, users can automatically have all their calls to the http module's methods (http.{del,get,head,options,patch,post,put,request}) automatically instrumented with tracing information, in the same fashion as described in #2854. Indeed, this function under the hood remaps the http module's method to use the Client's methods instead.

The instrumentHTTP takes the same option set as the Client as input but does not allow for reconfiguration, nor does it allow to perform non instrumented calls. As it is the intended behavior, once called, the http module's methods are replaced with the instrumented ones.

In action

import http from "k6/http";
import { check } from "k6";
import tracing from "k6/experimental/tracing";

// instrumentHTTP will ensure that all requests made by the http module
// will be traced. The first argument is a configuration object that
// can be used to configure the tracer.
//
// Currently supported HTTP methods are: get, post, put, patch, head,
// del, options, and request.
tracing.instrumentHTTP({
	propagator: "w3c",
});

export default () => {
	let res = http.get("http://httpbin.org/get", {
		headers: {
			"X-Example-Header": "instrumented/get",
		},
	});
	check(res, {
		"status is 200": (r) => r.status === 200,
	});
};

To observe this example in action, I recommend running it with the --http-debug too. That way, you'll be able to observe the Traceparent header being added (note that when using the b3 or jaeger propagators, the header is named differently.

go run go.k6.io/k6 run --http-debug samples/experimental/tracing.js

Review

⚠️ Although this PR has >1000 lines addition, a considerable chunk of it is testing. I believe the amount of actual logic is somewhat limited ⚠️

The commits have been organized to be progressive and make sense as a sequence; thus, you should review them in order.

References

Previous: #2853 #2854

[codecov-commenter]

Codecov Report

Merging #2855 (b9183b4) into experimental/tracing-module-client (c2648ac) will increase coverage by 0.07%.
The diff coverage is 78.12%.

❗ Current head b9183b4 differs from pull request most recent head 7187e12. Consider uploading reports for the commit 7187e12 to get more accurate results

@@                          Coverage Diff                           @@
##           experimental/tracing-module-client    #2855      +/-   ##
======================================================================
+ Coverage                               76.60%   76.67%   +0.07%     
======================================================================
  Files                                     222      222              
  Lines                                   16765    16796      +31     
======================================================================
+ Hits                                    12843    12879      +36     
+ Misses                                   3094     3085       -9     
- Partials                                  828      832       +4     

Flag	Coverage Δ
ubuntu	76.67% <78.12%> (+0.07%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
js/modules/k6/experimental/tracing/module.go	75.55% <78.12%> (+4.12%)	⬆️
metrics/value_type.go	62.50% <0.00%> (-6.25%)	⬇️
core/engine.go	93.29% <0.00%> (-1.22%)	⬇️
js/modules/k6/experimental/tracing/client.go	61.11% <0.00%> (+2.22%)	⬆️
api/v1/status_routes.go	48.97% <0.00%> (+18.36%)	⬆️
js/modules/k6/experimental/tracing/propagator.go	100.00% <0.00%> (+45.45%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[oleiade]

@codebien indeed, sorry for the noise... 🙇🏻 It should be fixed now 😸 (once CI completes)

[codebien]

LGTM

[mstoykov]

I kind of prefer if those are not integration tests as they are more expensive and in this case there is no need for them IMO.

The functionality tested is pretty common :

1. exception in an iteration does not abort the whole test
2. an exception in the init context - does.

[oleiade]

Let me see if I can easily turn them into standard unit tests indeed. I've struggled with instrumentHTTP depending on require in the past, but with the changes with made, it might not be such an issue anymore 👍🏻

[mstoykov]

LGTM, but if possible I would like to move some of the integration tests to not integration ones 🙇

[mstoykov]

I guess we do not have an integration test that instrumenting the API in one file will instrtument it across teh whole k6.

This likely is the most interesting integration test, so I think it is worth doing before merging

[oleiade]

@mstoykov thanks a lot for your feedback 🙇🏻 I've replaced the relevant integration tests with unit tests. I've also added an integration test demonstrating instrumentHTTP instruments across the whole of k6.

[mstoykov]

Kind of the same comment about this not checking that all of those have tracing ids.

Maybe assertHasTraceIDMetadata can have a number argument that is how many it expects 🤷.

Or to take the expected url(s) (as vararg not required argument) and check that all of those have trace id metadata sample.

[mstoykov]

I have left one comment that is not blocking IMO given that it is very unlikely breakage in that case (IMO)

[oleiade]

Thanks a lot for the reviews folks 🙇🏻 👏🏻 much appreciated!

For the sake of getting a sense of achievement, I'll go ahead and merge this. I think your point on the assertHasTraceIDMetadata is valid @mstoykov and I will try to address it in a further PR 🦕

🚀 🚀 🚀