Repository navigation
Count zero amounts in percentile summary batches - #1303
Conversation
`PercentileDistributionSummary.record(long[], int)` only updated the percentile buckets for amounts greater than zero, while `record(long)` updates them for any amount greater than or equal to zero. The base summary counts the zero amounts in both cases, so a batch that contains zeros left the bucket counts short and inflated the estimated percentiles. With a range set, the zeros are also missing from the bucket for the range minimum. Use the same check as `record(long)` so batched and single updates produce the same percentiles.
| @@ -254,7 +254,7 @@ private long restrict(long amount) { | |||
|
|
|||
| final int limit = Math.min(n, amounts.length); | |||
There was a problem hiding this comment.
Negative amounts still diverge between the two paths. record(long) only calls summary.record when amount >= 0, but the batch path passes the whole array to summary.record(amounts, n) unfiltered. On Atlas, AtlasDistributionSummary counts negatives, so record(new long[]{-5, 100}, 2) adds 2 to count() and 1 to the buckets, while record(-5); record(100) adds 1 to count().
| for (int i = 0; i < limit; i++) { | ||
| if (amounts[i] > 0) { | ||
| if (amounts[i] >= 0) { | ||
| counterFor(PercentileBuckets.indexOf(restrict(amounts[i]))).increment(); |
There was a problem hiding this comment.
Minor: zeros now do one atomic increment() each on the same bucket counter, so a mostly-zero batch adds a lot of contended increments. Counting zeros (or runs that land in the same bucket) and calling increment(k) once would avoid that. Probably fine to leave as-is.
| Registry r = newRegistry(); | ||
| PercentileDistributionSummary t1 = PercentileDistributionSummary.builder(r) | ||
| .withName("single") | ||
| .withRange(10, 1000) |
There was a problem hiding this comment.
With withRange(10, 1000), zero gets clamped to 10. Could we also cover the default range (min=0, so zero goes to bucket 0), a partial n < amounts.length, and the batchUpdater() path mentioned in the description?
| } | ||
| t2.record(amounts, amounts.length); | ||
|
|
||
| Assertions.assertEquals(t1.count(), t2.count()); |
There was a problem hiding this comment.
This assertion passes even without the fix, because count() comes from the base summary, which already counted zeros on both paths. Adding up the per-bucket counters and comparing the total to count() would test the change directly.
Verify bucket counts sum to count(), and cover the default range, partial batches, and the batchUpdater path.
Fixes #1302
PercentileDistributionSummary.record(long[], int)updates the percentile buckets only for amounts greater than zero.record(long)updates them for any amount greater than or equal to zero, and the base summary counts zero amounts in both paths. A batch that contains zeros therefore leaves the bucket counts short, andpercentile()and the published percentile counters overestimate the distribution. WithwithRange(min, max), the zeros are also missing from the bucket formin.batchUpdater()flushes throughrecord(long[], int), so it has the same problem.Changes
amounts[i] >= 0in the batch path, matchingrecord(long).batchRecordMatchesSingleRecordtoPercentileDistributionSummaryTest. It records{0, 0, 0, 500}once withrecord(long)and once withrecord(long[], int)and checks that the counts and percentiles 0 through 100 match.Testing
expected: <10.013333333333334> but was: <426.85>)../gradlew :spectator-api:buildand./gradlew licensepass.