Experimental tracing module (2/3): define and expose a tracing.Client

[oleiade]

About

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

⚠️ ✋🏻 This PR is part of a PR chain, and is based on #2853

This second PR builds upon #2853 and adds a Client type to k6/experimental/tracing. This type mimics the relevant part of the HTTP module to allow users to perform HTTP requests embedding a trace context while also attaching the produced trace id as output metadata on the HTTP-related samples. In the larger context of the module's goal, the Client should hold the underlying logic for the instrumentHTTP function that's to come in the third PR. Still, it also allows users to selectively trace requests and reconfigure their tracing configuration on the fly if they wish.

To achieve that, it is built upon the HTTP module's request function, which wraps with hooking logic to ensure the expected headers are produced and present and emit the output metadata.

The Client is configured using an options object that lets the user select the propagation format they use. Other options, such as sampling rate, and baggage, have been added for forward compatibility and convenience but will lead to "not implemented" errors if used.

Example of it in action

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

// Explicitly instantiating a tracing client allows to distringuish
// instrumented from non-instrumented HTTP calls, by keeping APIs separate.
// It also allows for finer-grained configuration control, by letting
// users changing the tracing configuration on the fly during their
// script's execution.
let instrumentedHTTP = new tracing.Client({
	propagator: "w3c",
});

const testData = { name: "Bert" };

export default () => {
	// Using the tracing client instance, HTTP calls will have
	// their trace context headers set.
	let res = instrumentedHTTP.request("GET", "http://httpbin.org/get", null, {
		headers: {
			"X-Example-Header": "instrumented/request",
		},
	});
	check(res, {
		"status is 200": (r) => r.status === 200,
	});

	// The tracing client offers more flexibility over
	// the `instrumentHTTP` function, as it leaves the
	// imported standard http module untouched. Thus,
	// one can still perform non-instrumented HTTP calls
	// using it.
	res = http.post("http://httpbin.org/post", JSON.stringify(testData), {
		headers: { "X-Example-Header": "noninstrumented/post" },
	});
	check(res, {
		"status is 200": (r) => r.status === 200,
	});

	res = instrumentedHTTP.del("http://httpbin.org/delete", null, {
		headers: { "X-Example-Header": "instrumented/delete" },
	});
	check(res, {
		"status is 200": (r) => r.status === 200,
	});
};

In order to observe this example in action, I recommend to run 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-client.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 PR: #2853

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (experimental/tracing-module-propagators@3889364). Click here to learn what that means.
The diff coverage is n/a.

@@                            Coverage Diff                             @@
##             experimental/tracing-module-propagators    #2854   +/-   ##
==========================================================================
  Coverage                                           ?   76.68%           
==========================================================================
  Files                                              ?      222           
  Lines                                              ?    16796           
  Branches                                           ?        0           
==========================================================================
  Hits                                               ?    12880           
  Misses                                             ?     3086           
  Partials                                           ?      830           

Flag	Coverage Δ
ubuntu	76.68% <0.00%> (?)

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

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

[oleiade]

Heads-up @olegbespalov, I removed you from reviewers to ensure consistency in the PR-chain. This PR is indeed part of a chain, and the bot has automatically selected @imiric and @codebien for the first and last one. I aligned the reviewers for this one as I believe it will make the review easier for reviewers if they review the three of them.

If you wish to review this module, and have time for it, feel free to add yourself back or ping me to do; I'll gladly do so 🙇🏻 👍🏻

[codebien]

@oleiade can you change the base branch to experimental/tracing-module-propagators, please? Otherwise, we see the changes from 1/3 here also.

[oleiade]

@codebien done ✅ thanks for pointing it out 🙇🏻

[mstoykov]

Looks good in general but I have a bunch of comments

[mstoykov]

Is there a reason we just don't do the equal comparision directly in the if sttatement?

[oleiade]

It's a style recommendation I discovered in the Google style guide, which, I find, helps with readability, as the if statement ends up much shorter and reads almost like a sentence. I'm not hung onto this though, if you have a strong opinion against that, I'm happy to drop it 🙇🏻

[codebien]

@oleiade Added a few comments.

It should already be possible to introduce an integration test here, so I would add it. WDYT?

[oleiade]

@codebien I've rebased the PR chain on the latest state of master, and updated the integration tests accordingly 👍🏻

[codebien]

LGTM 👏

[mstoykov]

Left a nitpick about style with common.Throw, but LGTM even with it

[mstoykov]

nit: In general I prefer to user require as it will stop the execution directly which helps in two ways:

1. I won't get a wall of text when i break somethign listing 20 things that went wrong, when the first thing that went wrong is likely to tell me what I neeed to know to fix it.
2. In this particular case assert.NotNil failing means that hte test will panic with nil exception. Arguably you can mix assert and require, but I have rarely been in a case taht this makes sense 🤷

[oleiade]

In general, I try to use require when it's a requirement for the test, as in: "this is required even to perform the test in the first place", and use assert for stuff that's tested. I guess I approach it in a rather semantic way, "this is required to perform the test", and "this asserts the behavior of the tested code is correct".

It's also how I understood it to be used in the tests I've been exposed.

Do you have a strong opinion on this, or would it be fine if I keep it like this? I'm okay with either way.

[codebien]

Blocking so we remember to rebase the fixups before merging

[mstoykov]

Blocking so we remember to rebase the fixups before merging

Won't this just be "squashing" it in the previours PR ?

[oleiade]

Indeed, I would expect to squash this in the 1/3 PR, which would obfuscate the fixups. If you have a strong opinion on this I'm happy to rebase them anyways though 👍🏻

[codebien]

Indeed, I would expect to squash this in the 1/3 PR

I thought we would keep the history, if we are going to squash then okay for me.