Fix compiled distribution sampling#422
Merged
pedro-stanaka merged 1 commit intoJun 23, 2026
Merged
Conversation
pedro-stanaka
approved these changes
Jun 23, 2026
pedro-stanaka
left a comment
pedro-stanaka
left a comment
Contributor
There was a problem hiding this comment.
LG - Apart from the comment, it would be nice if the sampling test was probabilistic since the beginning. It would have covered this.
I think the mocked test is enough for now, you may want to bump the version already to avoid a PR just for the release.
CompiledMetric::Distribution already samples in its generated latency handler so sampled-out calls can skip timing block overhead. The precompiled distribution emit path sampled again, making sampled distributions emit at rate^2 while still advertising @Rate. Route pre-sampled compiled distributions to a dedicated emit helper so each call makes one sampling decision. Bump the patch version to 3.11.1 for release. Benchmark (Ruby 3.4.9, temporary compiled distribution benchmark, main -> this branch): - value static rate=1: 2.351M -> 2.579M i/s (+9.7%) - value dynamic tags rate=1: 1.437M -> 1.509M i/s (+5.0%) - value static rate=0.5 sampled-in: 2.055M -> 2.364M i/s (+15.0%) - block static rate=1: 1.669M -> 1.746M i/s (+4.6%) Tests: - full test suite: 390 runs, 1098 assertions, 0 failures - rubocop changed files: no offenses
maxveldink
force-pushed
the
fix-compiled-distribution-sampling
branch
from
cd9ae2c to
e44cacd
Compare
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
2 participants
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.
✅ What
Fixes
CompiledMetric::Distributionso sampled metrics only make one sampling decision.CompiledMetric::Distributionalready samples in the generated latency/block handler. The precompiled distribution client emit path then sampled again, which made sampled compiled distributions emit with probabilitysample_rate²while still sending datagrams marked with only|@sample_rate.This PR routes pre-sampled compiled distributions to a dedicated private emit helper so the generated method's sampling decision is reused for both direct emission and aggregation.
Also bumps the patch version to
3.11.1for release.Fixes #421.
🤔 Why
A compiled distribution with
sample_rate: 0.001should emit about 1 in 1000 calls and let the backend scale by 1000. Instead it emitted about 1 in 1,000,000 calls and still told the backend to scale by only 1000, under-reporting by roughly the sample rate.Counters and gauges were not affected because they do not use the generated latency/block handler.
👩🔬 How to validate
Tests added coverage that fails on
mainbecausesample?is called twice:Validation run locally:
Benchmark
Temporary benchmark script based on
benchmark/send-metrics-to-dev-null-log, focused on compiled distributions. Higheri/sis better.Environment: Ruby 3.4.9, arm64-darwin24.
main→ this branch:Checklist