Merged
Conversation
5a46076 to
ac1d56e
Compare
2880947 to
b44d739
Compare
18cdd6d to
e9843eb
Compare
cbeauchesne
reviewed
Aug 21, 2023
e9843eb to
1bd195e
Compare
cbeauchesne
approved these changes
Aug 22, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add a new test
test_sample_rate_functionwhich checks that weblog takes the exact same sampling decision as the one precomputed in an auxiliary .csv file.This test is broader and stricter than the previous tests
test_sampling_decision,test_sampling_decision_addedandtest_sampling_decision_added(if any of them fail, the new one would fail too). In a later PR, I'll propose to consolidate it all.Motivation
The purpose of this approach is to use a portable language agnostic set of
(trace_id, sampling_rate, sampling_decision)to test the implementation of the sampling function. The given CSV can be copied to be sure in libraries' unit tests. I plan on using it on to test our backend implementation too.The CSV is meant to include cases for "tricky" values (ex: small, large, at the edge of 2^63) which would who typically fail if the function were badly implemented (ex: offset by one, doing a 2^63 modulo instead of 2^64).
Workflow
Once your PR is reviewed, you can merge it! ❤️
Reviewer checklist
run-parametric-scenario,run-profiling-scenario...). If this PR modifies any system-tests internal, then add therun-all-scenarioslabel (more info).build-some-imagelabel is present