From 3d6f699b5e9f12b35ce506f32215ac3dd8dd80c0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabian=20St=C3=A4ber?= Date: Sun, 30 Jan 2022 21:38:06 +0100 Subject: [PATCH] CKMS Quantiles: Fix test and disallow corner cases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Fabian Stäber --- .../io/prometheus/client/CKMSQuantiles.java | 12 ++-- .../java/io/prometheus/client/Summary.java | 4 +- .../prometheus/client/CKMSQuantilesTest.java | 57 ++++++------------- 3 files changed, 22 insertions(+), 51 deletions(-) diff --git a/simpleclient/src/main/java/io/prometheus/client/CKMSQuantiles.java b/simpleclient/src/main/java/io/prometheus/client/CKMSQuantiles.java index 7fe6cff3b..140c41696 100644 --- a/simpleclient/src/main/java/io/prometheus/client/CKMSQuantiles.java +++ b/simpleclient/src/main/java/io/prometheus/client/CKMSQuantiles.java @@ -129,14 +129,10 @@ final class CKMSQuantiles { * @param quantiles The targeted quantiles, can be empty. */ CKMSQuantiles(Quantile[] quantiles) { - // hard-coded epsilon of 0.1% to determine the batch size, and default epsilon in case of empty quantiles - double pointOnePercent = 0.001; if (quantiles.length == 0) { // we need at least one for this algorithm to work - this.quantiles = new Quantile[1]; - this.quantiles[0] = new Quantile(0.5, pointOnePercent / 2); - } else { - this.quantiles = quantiles; + throw new IllegalArgumentException("quantiles cannot be empty"); } + this.quantiles = quantiles; // section 5.1 Methods - Batch. // This is hardcoded to 500, which corresponds to an epsilon of 0.1%. @@ -431,8 +427,8 @@ static class Quantile { * @param epsilon the desired error for this quantile, between 0 and 1. */ Quantile(double quantile, double epsilon) { - if (quantile < 0 || quantile > 1.0) throw new IllegalArgumentException("Quantile must be between 0 and 1"); - if (epsilon < 0 || epsilon > 1.0) throw new IllegalArgumentException("Epsilon must be between 0 and 1"); + if (quantile <= 0 || quantile >= 1.0) throw new IllegalArgumentException("Quantile must be between 0 and 1"); + if (epsilon <= 0 || epsilon >= 1.0) throw new IllegalArgumentException("Epsilon must be between 0 and 1"); this.quantile = quantile; this.epsilon = epsilon; diff --git a/simpleclient/src/main/java/io/prometheus/client/Summary.java b/simpleclient/src/main/java/io/prometheus/client/Summary.java index 34b123ba0..886358cd9 100644 --- a/simpleclient/src/main/java/io/prometheus/client/Summary.java +++ b/simpleclient/src/main/java/io/prometheus/client/Summary.java @@ -99,10 +99,10 @@ public static class Builder extends SimpleCollector.Builder { private int ageBuckets = 5; public Builder quantile(double quantile, double error) { - if (quantile < 0.0 || quantile > 1.0) { + if (quantile <= 0.0 || quantile >= 1.0) { throw new IllegalArgumentException("Quantile " + quantile + " invalid: Expected number between 0.0 and 1.0."); } - if (error < 0.0 || error > 1.0) { + if (error <= 0.0 || error >= 1.0) { throw new IllegalArgumentException("Error " + error + " invalid: Expected number between 0.0 and 1.0."); } quantiles.add(new Quantile(quantile, error)); diff --git a/simpleclient/src/test/java/io/prometheus/client/CKMSQuantilesTest.java b/simpleclient/src/test/java/io/prometheus/client/CKMSQuantilesTest.java index 63a281007..7e5f273c9 100644 --- a/simpleclient/src/test/java/io/prometheus/client/CKMSQuantilesTest.java +++ b/simpleclient/src/test/java/io/prometheus/client/CKMSQuantilesTest.java @@ -25,37 +25,6 @@ public void testGetOnEmptyValues() { assertEquals(Double.NaN, ckms.get(0), 0); } - @Test - public void testGetWhenNoQuantilesAreDefined() { - CKMSQuantiles ckms = new CKMSQuantiles(new Quantile[]{}); - assertEquals(Double.NaN, ckms.get(0), 0); - } - - @Test - public void testInsertWhenNoQuantilesAreDefined() { - CKMSQuantiles ckms = new CKMSQuantiles(new Quantile[]{}); - ckms.insert(1.0); - ckms.insert(2.0); - ckms.insert(3.0); - assertEquals(1.0, ckms.get(0), 0); - assertEquals(2.0, ckms.get(0.5), 0); - assertEquals(3.0, ckms.get(1), 0); - } - - @Test - public void testCompressWhenBufferSize500Reached() { - CKMSQuantiles ckms = new CKMSQuantiles(new Quantile[]{}); - List input = makeSequence(1, 499); - - for (double v : input) { - ckms.insert(v); - } - assertEquals("No compress should be triggered", 0, ckms.samples.size()); - - ckms.insert(500); - assertEquals(500, ckms.samples.size()); - } - @Test public void testGet() { List quantiles = new ArrayList(); @@ -80,20 +49,19 @@ public void testGet() { @Test public void testGetWithAMillionElements() { List quantiles = new ArrayList(); - quantiles.add(new Quantile(0.0, 0.01)); + quantiles.add(new Quantile(0.01, 0.001)); quantiles.add(new Quantile(0.10, 0.01)); quantiles.add(new Quantile(0.90, 0.001)); quantiles.add(new Quantile(0.95, 0.02)); quantiles.add(new Quantile(0.99, 0.001)); final int elemCount = 1000000; - double[] shuffle = new double[elemCount]; - for (int i = 0; i < shuffle.length; i++) { - shuffle[i] = i + 1; + List shuffle = new ArrayList(elemCount); + for (int i = 0; i < elemCount; i++) { + shuffle.add(i+1.0); } Random rand = new Random(0); - - Collections.shuffle(Arrays.asList(shuffle), rand); + Collections.shuffle(shuffle, rand); CKMSQuantiles ckms = new CKMSQuantiles( quantiles.toArray(new Quantile[]{})); @@ -102,14 +70,21 @@ public void testGetWithAMillionElements() { ckms.insert(v); } // given the linear distribution, we set the delta equal to the εn value for this quantile - assertEquals(0.1 * elemCount, ckms.get(0.1), 0.01 * elemCount); - assertEquals(0.9 * elemCount, ckms.get(0.9), 0.001 * elemCount); - assertEquals(0.95 * elemCount, ckms.get(0.95), 0.02 * elemCount); - assertEquals(0.99 * elemCount, ckms.get(0.99), 0.001 * elemCount); + assertRank(elemCount, ckms.get(0.01), 0.01, 0.001); + assertRank(elemCount, ckms.get(0.1), 0.1, 0.01); + assertRank(elemCount, ckms.get(0.9), 0.9, 0.001); + assertRank(elemCount, ckms.get(0.95), 0.95, 0.02); + assertRank(elemCount, ckms.get(0.99), 0.99, 0.001); assertTrue("sample size should be way below 1_000_000", ckms.samples.size() < 1000); } + private void assertRank(int elemCount, double actual, double quantile, double epsilon) { + double lowerBound = elemCount * (quantile - epsilon); + double upperBound = elemCount * (quantile + epsilon); + assertTrue("quantile=" + quantile + ", actual=" + actual + ", lowerBound=" + lowerBound, actual >= lowerBound); + assertTrue("quantile=" + quantile + ", actual=" + actual + ", upperBound=" + upperBound, actual <= upperBound); + } @Test public void testGetGaussian() {