feat: Deterministic Sampler #51
Conversation
Co-Authored-By: Purvi Kanal <8810222+pkanal@users.noreply.github.com>
| @@ -296,7 +301,8 @@ def __init__( | |||
|
|
|||
| self.sample_rate = parse_int( | |||
| SAMPLE_RATE, | |||
| (sample_rate or DEFAULT_SAMPLE_RATE), | |||
There was a problem hiding this comment.
I had to edit the way we do this here a lil, since 0 is falsey but it's still a valid sample rate parameter.
| @@ -177,6 +177,14 @@ def test_can_set_sample_rate_with_param(): | |||
| options = HoneycombOptions(sample_rate=123) | |||
| assert options.sample_rate == 123 | |||
|
|
|||
| def test_can_set_sample_rate_of_zero_param(): | |||
There was a problem hiding this comment.
added these tests to ensure setting 0 (a falsey value in python) still makes it through and set as a sample rate!
|
|
||
| class DeterministicSampler(Sampler): | ||
| """ | ||
| Custom samplers can be created by subclassing Sampler and implementing |
There was a problem hiding this comment.
OPE! I forgot to change this filler docstring. I'll get it!
| ) | ||
|
|
||
|
|
||
| def test_sample_with_undefined_rate_defaults_to_DEFAULT_ON_and_recorded(): |
There was a problem hiding this comment.
TY for these detailed tests! ✨
| undefined_rate_sampler = configure_sampler() | ||
| # test the `DEFAULT_SAMPLE_RATE` is applied (i.e. 1) | ||
| assert undefined_rate_sampler.rate == DEFAULT_SAMPLE_RATE | ||
| # test the inner DeterministicSampler choice |
There was a problem hiding this comment.
For these subtest cases, is it possible to do sub tests in python? or perhaps make them their own test cases?
There was a problem hiding this comment.
Oh good call! I don't know offhand but I'll look into it as we build out more features and tests.
| self.rate = rate | ||
|
|
||
| if self.rate <= 0: | ||
| # Sampler that respects its parent span's sampling decision, |
There was a problem hiding this comment.
These comments are super helpful 👍🏽
| ratio = 1.0 / self.rate | ||
| self._sampler = ParentBasedTraceIdRatio(ratio) | ||
|
|
||
| # pylint: disable=too-many-arguments |
There was a problem hiding this comment.
Are too-many-arguments out of our control in this case because we're using the should_sample args from OTel?
There was a problem hiding this comment.
Yep, we gotta match upstream 👍
Which problem is this PR solving?
Short description of the changes
How to verify that this has the expected result
Try playing around with the example (/examples/hello-world-flask) and Honeycomb Usage Mode.
HONEYCOMB_API_KEY=[redacted] OTEL_SERVICE_NAME=“hello-world-flask” SAMPLE_RATE=3 DEBUG=TRUE poetry run opentelemetry-instrument flask runandcurl localhost:5000