Skip to content

Commit 6466c7d

Browse files
authored
fix bug for p100 percentile approximation (#1181)
In some cases the floating point math would result in it going to the end of the buckets and treating the max value as `Long.MAX_VALUE` rather than the end boundary for the last bucket with a non-zero count.
1 parent 75ee7ae commit 6466c7d

File tree

2 files changed

+30
-6
lines changed

2 files changed

+30
-6
lines changed

spectator-api/src/main/java/com/netflix/spectator/api/histogram/PercentileBuckets.java

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2014-2024 Netflix, Inc.
2+
* Copyright 2014-2025 Netflix, Inc.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -185,18 +185,22 @@ public static void percentiles(double[] counts, double[] pcts, double[] results)
185185
Preconditions.checkArg(pcts.length == results.length,
186186
"pcts is not the same size as results array");
187187

188+
int lastNonZeroIdx = 0;
188189
double total = 0.0;
189-
for (double c : counts) {
190-
if (c > 0.0 && Double.isFinite(c))
190+
for (int i = 0; i < counts.length; ++i) {
191+
double c = counts[i];
192+
if (c > 0.0 && Double.isFinite(c)) {
191193
total += c;
194+
lastNonZeroIdx = i;
195+
}
192196
}
193197

194198
int pctIdx = 0;
195199

196200
double prev = 0.0;
197201
double prevP = 0.0;
198202
long prevB = 0;
199-
for (int i = 0; i < BUCKET_VALUES.length; ++i) {
203+
for (int i = 0; i <= lastNonZeroIdx; ++i) {
200204
double next = prev + counts[i];
201205
double nextP = 100.0 * next / total;
202206
long nextB = BUCKET_VALUES[i];
@@ -215,7 +219,7 @@ public static void percentiles(double[] counts, double[] pcts, double[] results)
215219
}
216220

217221
double nextP = 100.0;
218-
long nextB = Long.MAX_VALUE;
222+
long nextB = BUCKET_VALUES[lastNonZeroIdx];
219223
while (pctIdx < pcts.length) {
220224
double f = (pcts[pctIdx] - prevP) / (nextP - prevP);
221225
results[pctIdx] = f * (nextB - prevB) + prevB;

spectator-api/src/test/java/com/netflix/spectator/api/histogram/PercentileBucketsTest.java

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2014-2024 Netflix, Inc.
2+
* Copyright 2014-2025 Netflix, Inc.
33
*
44
* Licensed under the Apache License, Version 2.0 (the "License");
55
* you may not use this file except in compliance with the License.
@@ -166,4 +166,24 @@ public void percentileDouble() {
166166
Assertions.assertEquals(expected, PercentileBuckets.percentile(counts, pct), threshold);
167167
}
168168
}
169+
170+
@Test
171+
public void maxLongForP100() {
172+
double[] counts = new double[PercentileBuckets.length()];
173+
counts[128] = 0.03416666826233268;
174+
counts[129] = 0.031666668225079776;
175+
counts[130] = 0.008333333767950535;
176+
counts[131] = 0.005833333637565375;
177+
counts[132] = 0.013333334028720856;
178+
counts[133] = 0.015000000689178707;
179+
counts[134] = 0.005000000260770321;
180+
counts[135] = 0.006666667014360428;
181+
counts[136] = 0.0008333333767950535;
182+
counts[137] = 0.0025000001303851606;
183+
counts[138] = 0.0;
184+
counts[139] = 0.0008333333767950535;
185+
double v = PercentileBuckets.percentile(counts, 100.0) / 1e9;
186+
Assertions.assertTrue(v < 3.94);
187+
Assertions.assertTrue(v > 3.93);
188+
}
169189
}

0 commit comments

Comments
 (0)