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

Fix tracing module sampling option should default to 1.0 when not set #3231

Merged
merged 1 commit into from
Jul 31, 2023

Conversation

oleiade
Copy link
Member

@oleiade oleiade commented Jul 27, 2023

What?

This Pull Request ensures that if the sampling option of the tracing module's client is not provided by the user, the Client defaults to sample 100% of the request.

		import http from "k6/http";
		import tracing from "k6/experimental/tracing";

		// We do not set the sampling option, thus the default
		// behavior should be to always sample.
		const instrumentedHTTP = new tracing.Client({
			propagator: "w3c",
		})

Why?

As noted by @Blinkuu, the current behavior of the tracing module client is to use a 0% sampling when the option is not set. It leads to effectively no HTTP request being sampled. Whereas the desired behavior would be to have all requests sampled unless instructed otherwise.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make ci-like-lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

@oleiade oleiade added the bug label Jul 27, 2023
@oleiade oleiade added this to the v0.46.0 milestone Jul 27, 2023
@oleiade oleiade self-assigned this Jul 27, 2023
@github-actions github-actions bot requested review from codebien and imiric July 27, 2023 12:44
@oleiade oleiade requested a review from Blinkuu July 27, 2023 12:46
mstoykov
mstoykov previously approved these changes Jul 27, 2023
Blinkuu
Blinkuu previously approved these changes Jul 27, 2023
Copy link
Contributor

@Blinkuu Blinkuu left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

This commit ensures that if the `sampling` option of the tracing
module's client is not provided by user, the Client defaults to
sample 100% of the request.
@oleiade oleiade dismissed stale reviews from Blinkuu and mstoykov via 9ea8a8f July 28, 2023 08:33
@oleiade oleiade force-pushed the fix/sample-rate-with-no-sampling-option branch from bb608a2 to 9ea8a8f Compare July 28, 2023 08:33
@oleiade
Copy link
Member Author

oleiade commented Jul 28, 2023

It turns out this was caused by a difference in how options are handled between the client and the options. I addressed the issue differently and ensured all the tests passed now.

@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2023

Codecov Report

Merging #3231 (4baa0e8) into master (6c31075) will decrease coverage by 0.01%.
Report is 1 commits behind head on master.
The diff coverage is 80.00%.

❗ Current head 4baa0e8 differs from pull request most recent head 9ea8a8f. Consider uploading reports for the commit 9ea8a8f to get more accurate results

@@            Coverage Diff             @@
##           master    #3231      +/-   ##
==========================================
- Coverage   73.24%   73.24%   -0.01%     
==========================================
  Files         259      259              
  Lines       19892    19899       +7     
==========================================
+ Hits        14570    14575       +5     
- Misses       4400     4401       +1     
- Partials      922      923       +1     
Flag Coverage Δ
ubuntu 73.18% <80.00%> (+<0.01%) ⬆️
windows 73.07% <80.00%> (+<0.01%) ⬆️

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

Files Changed Coverage Δ
js/modules/k6/experimental/tracing/module.go 69.09% <50.00%> (-2.61%) ⬇️
js/modules/k6/experimental/tracing/options.go 88.88% <100.00%> (ø)
log/loki.go 69.08% <100.00%> (+0.60%) ⬆️

@oleiade oleiade merged commit 7184786 into master Jul 31, 2023
@oleiade oleiade deleted the fix/sample-rate-with-no-sampling-option branch July 31, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants