opentelemetry-sdk: fill every exemplar reservoir bucket before sampling - #5462
Open
henry3260 wants to merge 1 commit into
Open
opentelemetry-sdk: fill every exemplar reservoir bucket before sampling#5462henry3260 wants to merge 1 commit into
henry3260 wants to merge 1 commit into
Conversation
SimpleFixedSizeExemplarReservoir incremented _measurements_seen before comparing it against _size, so the deterministic fill phase of the reservoir sampling algorithm ran size - 1 times instead of size. The size-th measurement therefore entered the random replacement phase early. Because randrange(0, size) always yields an index below _size, that measurement overwrote an already-filled bucket with probability (size - 1) / size and left the last bucket empty. The reservoir never reached its configured capacity, and the uniformity guarantee of the algorithm, which depends on the first size measurements always being retained, no longer held. Increment the counter after the comparison so that it means "number of measurements seen before this one" at the point of the check.
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
SimpleFixedSizeExemplarReservoirimplements Algorithm R reservoir sampling, whichhas two phases: the first
sizemeasurements fill each bucket directly, and everymeasurement after that replaces a random bucket with probability
size / seen._find_bucket_indexincremented_measurements_seenbefore comparing it against_size, so at the point of the check the counter meant "measurements seen includingthis one" rather than "measurements seen before this one". The deterministic fill
phase therefore ran
size - 1times instead ofsize.on the first
sizemeasurements always being retained. Losing one of them breaks theinvariant that every measurement seen so far is retained with equal probability.
size=1was unaffected (1 < 1is false, andrandrange(0, 1)is always0).The fix moves the increment after the comparison so the counter carries the meaning the
algorithm expects at the point of the check. No public API or configuration changes.
Type of change
How Has This Been Tested?
test_fills_every_bucket_before_samplinginopentelemetry-sdk/tests/metrics/test_exemplarreservoir.py. It offers exactlysizemeasurements for
sizein(1, 2, 4, 10)and asserts that allsizebuckets arefilled with exactly those measurements. The test fails on the current code
(
AssertionError: 9 != 10forsize=10) and passes with the fix.Does This PR Require a Contrib Repo Change?
Checklist: