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: Consistent probability sampler init #1447

Merged
merged 4 commits into from
May 15, 2023
Merged

Conversation

fbogsany
Copy link
Contributor

In the consistent probability sampler, we cache the exponent of the next highest and next lowest negative power-of-two values, and the probability of the highest. E.g. for a probability of 0.1, the next highest negative power-of-two is 2**-3 (0.125) and the next lowest is 2**-4 (0.0625), and the probability of the higher value is the fraction of difference between the 0.0625 and 0.125 that is 0.1 - i.e. 0.1 - 0.0625 / 0.0625 = 0.6.

Unfortunately, we calculated the ceiling incorrectly in the current implementation. Because these are negative power-of-two exponents, the smaller value is the upper bound and the larger value is the lower bound. This corrects that error.

Relevant song: https://www.youtube.com/watch?v=Esdnbq4hxHo

Copy link
Contributor

@plantfansam plantfansam left a comment

Choose a reason for hiding this comment

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

👍 Wouldn't mind if we added a test for sampler being initialized with 0.0 (see opentelemetry-go-contrib's testing of the same general stuff). I initially thought this was covered in 62.downto(0) but actually that gets passed to the initializer as 2**-62, which isn't the same thing!

Not essential, but I also think adding a test case where the sampling probability is almost a power of 2 (0.12, 0.48, that kind of thing) can be illuminating to the reader, since you can see a p_ceil_probability that's very apparently skewing towards a given value of p.

Copy link
Contributor

@plantfansam plantfansam left a comment

Choose a reason for hiding this comment

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

👍 Wouldn't mind if we added a test for sampler being initialized with 0.0 (see opentelemetry-go-contrib's testing of the same general stuff). I initially thought this was covered in 62.downto(0) but actually that gets passed to the initializer as 2**-62, which isn't the same thing!

Not essential, but I also think adding a test case where the sampling probability is almost a power of 2 (0.12, 0.48, that kind of thing) can be illuminating to the reader, since you can see a p_ceil_probability that's very apparently skewing towards a given value of p.

fbogsany and others added 2 commits May 15, 2023 12:01
…ent_probability_based_test.rb

Co-authored-by: Sam <370182+plantfansam@users.noreply.github.com>
@fbogsany fbogsany merged commit f3e5f87 into main May 15, 2023
45 of 46 checks passed
@plantfansam plantfansam deleted the her-floor-is-my-ceiling branch May 15, 2023 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants