Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add sampling capabilities to the tracing module #2886

Merged
merged 7 commits into from
Mar 10, 2023

Conversation

oleiade
Copy link
Member

@oleiade oleiade commented Jan 31, 2023

About

This Pull Request adds probabilistic sampling support to the tracing module. Using it, users can specify the percentage of their instrumented requests which should have their trace context header's sampled field set to true.

From a user-interface perspective, this feature is introduced as an option meant to be set during either the instantiation of a tracing.Client, or during a call to instrumentHTTP. It consists of a single integer field of the tracing options, whose value, within the 0 <= n <= 100 bounds, is meant to represent a percentage.

// The sampling option is available when instantiating a Client
const sampledHTTP = new tracing.Client({
	propagator: "w3c",
	
	// Up to 55% of the requests emitted using the client instance's
	// methods will have their trace context header's sampled field
	// set to true.
	sampling: 55,
})

// It is also available when using the instrumentHTTP function
tracing.InstrumentHTTP({
	propagator: "w3c",
	
	// Up to 27% of the requests made with the HTTP module's functions
	// will have their trace context header's sampled field set to true
	// from this point onward 
	sampling: 27,
})

This feature is implemented using probabilistic sampling. Meaning that over an extensive series of iterations, the number of requests with their sampled flag set to true will tend to converge to the selected sampling rate value. It was a design choice without explicit constraints, preferring randomness over accuracy: each trace context generation evaluates its chance of being sampled independently. If you find an implementation emphasizing accuracy more suitable, I'm open to exploring that route; just let me know.

Note that regardless of the sampled header flag's value, the trace_id metadata will be attached to each output sample.

@oleiade oleiade added this to the v0.43.0 milestone Jan 31, 2023
@oleiade oleiade self-assigned this Jan 31, 2023
@oleiade oleiade force-pushed the experimental/tracing-with-sampling branch 2 times, most recently from de9c14f to 12f68d0 Compare January 31, 2023 10:52
@codecov-commenter
Copy link

codecov-commenter commented Jan 31, 2023

Codecov Report

Merging #2886 (5cd1416) into master (37cd20b) will increase coverage by 0.11%.
The diff coverage is 82.05%.

❗ Current head 5cd1416 differs from pull request most recent head 1e9926b. Consider uploading reports for the commit 1e9926b to get more accurate results

@@            Coverage Diff             @@
##           master    #2886      +/-   ##
==========================================
+ Coverage   76.83%   76.94%   +0.11%     
==========================================
  Files         229      230       +1     
  Lines       16923    16978      +55     
==========================================
+ Hits        13002    13063      +61     
+ Misses       3078     3076       -2     
+ Partials      843      839       -4     
Flag Coverage Δ
ubuntu 76.85% <82.05%> (+0.08%) ⬆️
windows 76.76% <82.05%> (+0.12%) ⬆️

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

Impacted Files Coverage Δ
cmd/tests/tests.go 46.15% <0.00%> (+1.70%) ⬆️
log/loki.go 30.70% <0.00%> (-6.36%) ⬇️
js/modules/k6/experimental/tracing/encoding.go 66.66% <40.00%> (-33.34%) ⬇️
js/modules/k6/experimental/tracing/sampling.go 50.00% <50.00%> (ø)
log/file.go 79.66% <50.00%> (-1.59%) ⬇️
js/modules/k6/experimental/tracing/module.go 71.69% <66.66%> (+0.54%) ⬆️
js/modules/k6/experimental/tracing/options.go 88.88% <77.77%> (+25.25%) ⬆️
lib/testutils/httpmultibin/httpmultibin.go 92.64% <89.18%> (+2.69%) ⬆️
cmd/root.go 95.33% <94.54%> (+13.51%) ⬆️
js/modules/k6/experimental/tracing/client.go 69.72% <100.00%> (+0.85%) ⬆️
... and 3 more

... and 3 files with indirect coverage changes

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

@oleiade
Copy link
Member Author

oleiade commented Jan 31, 2023

For your information, 377abf9 incorporates improvements to the assertHasTraceIDMetadata function as suggested by @mstoykov in previous PRs 👍🏻

@codebien
Copy link
Contributor

codebien commented Feb 1, 2023

It could be good to address #2889 here

@oleiade
Copy link
Member Author

oleiade commented Feb 1, 2023

@codebien I'll give it shot, if it takes more than an hour or two, I would address it during cooldown in a separate PR 👍🏻

@oleiade
Copy link
Member Author

oleiade commented Feb 3, 2023

@codebien #2889 was addressed in a separate PR #2894 🚀

@oleiade oleiade modified the milestones: v0.43.0, v0.44.0 Feb 7, 2023
cmd/tests/tracing_module_test.go Outdated Show resolved Hide resolved
js/modules/k6/experimental/tracing/encoding.go Outdated Show resolved Hide resolved
js/modules/k6/experimental/tracing/module.go Outdated Show resolved Hide resolved
js/modules/k6/experimental/tracing/options_test.go Outdated Show resolved Hide resolved
js/modules/k6/experimental/tracing/propagator.go Outdated Show resolved Hide resolved
js/modules/k6/experimental/tracing/propagator.go Outdated Show resolved Hide resolved
js/modules/k6/experimental/tracing/client.go Outdated Show resolved Hide resolved
@oleiade oleiade force-pushed the experimental/tracing-with-sampling branch from 12f68d0 to 24a3d13 Compare March 7, 2023 11:40
@oleiade oleiade force-pushed the experimental/tracing-with-sampling branch from 4cd3f1c to c691b5c Compare March 7, 2023 14:38
@oleiade oleiade force-pushed the experimental/tracing-with-sampling branch from f1a8666 to 4caeffe Compare March 9, 2023 13:50
 This commit adds a Sampler interface defining a contract for
 implementing support for sampling. This means that any client of a
 sampler could go through the interface to decide whether or not to
 perform an action or keep some piece of data.

 This commit adds a ProbabilisticSampler implementation of Sampler,
 meant to be implemented by propagators to decide whether they should
 set their sampling flag to true or false.

 This commit also adds a chance function which returns true with a
 `percentage` of chance, to support the ProbabilisticSampler implem.
This commit defines a SamplerPropagator interface, which established
the contract for a propagator which bases its sampling flag on a
Sampler implementation.

It also makes the concrete implementations of Propagators implement
the Sampler interface, and ensure the flags field of the generated
trace context headers is set based on the Sampler's result.
@oleiade oleiade force-pushed the experimental/tracing-with-sampling branch from 4caeffe to 1545df1 Compare March 9, 2023 15:28
@oleiade oleiade force-pushed the experimental/tracing-with-sampling branch from 1545df1 to e6080c2 Compare March 9, 2023 15:30
@oleiade oleiade requested a review from Blinkuu March 9, 2023 15:35
@oleiade oleiade requested a review from mstoykov March 9, 2023 15:35
mstoykov
mstoykov previously approved these changes Mar 9, 2023
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

A nitpick was that it seems to be we can just inline the chance function, but I am okay either way

Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, has some minor comments. Really well done, I'm out from tracing domain but it has been easy to understand 👏

js/modules/k6/experimental/tracing/sampling.go Outdated Show resolved Hide resolved
js/modules/k6/experimental/tracing/propagator.go Outdated Show resolved Hide resolved
js/modules/k6/experimental/tracing/propagator.go Outdated Show resolved Hide resolved
@oleiade
Copy link
Member Author

oleiade commented Mar 10, 2023

Addressed all comments, looking forward to your 👍🏻 👏🏻 🙇🏻

Copy link
Contributor

@codebien codebien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I restarted the failed checks

@oleiade oleiade merged commit 314d9d5 into master Mar 10, 2023
@oleiade oleiade deleted the experimental/tracing-with-sampling branch March 10, 2023 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants