From 4aa3f0c4a17a183c43b793eb5e946c34fdefd4bf Mon Sep 17 00:00:00 2001 From: Mayur Kale Date: Sat, 13 Oct 2018 12:14:59 -0700 Subject: Add BucketOptions for DistributionValue (#1484) * Add BucketOptions for DistributionValue * Fix reviews --- .../io/opencensus/metrics/export/Distribution.java | 161 +++++++++++++++------ .../metrics/export/DistributionTest.java | 146 +++++++++++++++---- .../io/opencensus/metrics/export/PointTest.java | 3 +- .../io/opencensus/metrics/export/ValueTest.java | 21 ++- 4 files changed, 255 insertions(+), 76 deletions(-) (limited to 'api') diff --git a/api/src/main/java/io/opencensus/metrics/export/Distribution.java b/api/src/main/java/io/opencensus/metrics/export/Distribution.java index dc9fa9e9..eb0add86 100644 --- a/api/src/main/java/io/opencensus/metrics/export/Distribution.java +++ b/api/src/main/java/io/opencensus/metrics/export/Distribution.java @@ -18,6 +18,7 @@ package io.opencensus.metrics.export; import com.google.auto.value.AutoValue; import io.opencensus.common.ExperimentalApi; +import io.opencensus.common.Function; import io.opencensus.common.Timestamp; import io.opencensus.internal.Utils; import java.util.ArrayList; @@ -48,7 +49,7 @@ public abstract class Distribution { * @param count the count of the population values. * @param sum the sum of the population values. * @param sumOfSquaredDeviations the sum of squared deviations of the population values. - * @param bucketBoundaries bucket boundaries of a histogram. + * @param bucketOptions the bucket options used to create a histogram for the distribution. * @param buckets {@link Bucket}s of a histogram. * @return a {@code Distribution}. * @since 0.17 @@ -57,7 +58,7 @@ public abstract class Distribution { long count, double sum, double sumOfSquaredDeviations, - List bucketBoundaries, + BucketOptions bucketOptions, List buckets) { Utils.checkArgument(count >= 0, "count should be non-negative."); Utils.checkArgument( @@ -67,35 +68,16 @@ public abstract class Distribution { Utils.checkArgument( sumOfSquaredDeviations == 0, "sum of squared deviations should be 0 if count is 0."); } - return new AutoValue_Distribution( - count, - sum, - sumOfSquaredDeviations, - copyBucketBounds(bucketBoundaries), - copyBucketCount(buckets)); - } + Utils.checkNotNull(bucketOptions, "bucketOptions"); - private static List copyBucketBounds(List bucketBoundaries) { - Utils.checkNotNull(bucketBoundaries, "bucketBoundaries list should not be null."); - List bucketBoundariesCopy = new ArrayList(bucketBoundaries); // Deep copy. - // Check if sorted. - if (bucketBoundariesCopy.size() > 1) { - double lower = bucketBoundariesCopy.get(0); - for (int i = 1; i < bucketBoundariesCopy.size(); i++) { - double next = bucketBoundariesCopy.get(i); - Utils.checkArgument(lower < next, "bucket boundaries not sorted."); - lower = next; - } - } - return Collections.unmodifiableList(bucketBoundariesCopy); + return new AutoValue_Distribution( + count, sum, sumOfSquaredDeviations, bucketOptions, copyBucketCount(buckets)); } private static List copyBucketCount(List buckets) { Utils.checkNotNull(buckets, "bucket list should not be null."); List bucketsCopy = new ArrayList(buckets); - for (Bucket bucket : bucketsCopy) { - Utils.checkNotNull(bucket, "bucket should not be null."); - } + Utils.checkListElementNotNull(bucketsCopy, "bucket should not be null."); return Collections.unmodifiableList(bucketsCopy); } @@ -131,29 +113,14 @@ public abstract class Distribution { public abstract double getSumOfSquaredDeviations(); /** - * Returns the bucket boundaries of this distribution. + * Returns bucket options used to create a histogram for the distribution. * - *

The bucket boundaries for that histogram are described by bucket_bounds. This defines - * size(bucket_bounds) + 1 (= N) buckets. The boundaries for bucket index i are: - * - *

    - *
  • {@code (-infinity, bucket_bounds[i]) for i == 0} - *
  • {@code [bucket_bounds[i-1], bucket_bounds[i]) for 0 < i < N-2} - *
  • {@code [bucket_bounds[i-1], +infinity) for i == N-1} - *
- * - *

i.e. an underflow bucket (number 0), zero or more finite buckets (1 through N - 2, and an - * overflow bucket (N - 1), with inclusive lower bounds and exclusive upper bounds. - * - *

If bucket_bounds has no elements (zero size), then there is no histogram associated with the - * Distribution. If bucket_bounds has only one element, there are no finite buckets, and that - * single element is the common boundary of the overflow and underflow buckets. The values must be - * monotonically increasing. - * - * @return the bucket boundaries of this distribution. + * @return the {@code BucketOptions} associated with the {@code Distribution}, or {@code null} if + * there isn't one. * @since 0.17 */ - public abstract List getBucketBoundaries(); + @Nullable + public abstract BucketOptions getBucketOptions(); /** * Returns the aggregated histogram {@link Bucket}s. @@ -163,6 +130,110 @@ public abstract class Distribution { */ public abstract List getBuckets(); + /** + * The bucket options used to create a histogram for the distribution. + * + * @since 0.17 + */ + @Immutable + public abstract static class BucketOptions { + + private BucketOptions() {} + + /** + * Returns a {@link ExplicitOptions}. + * + *

The bucket boundaries for that histogram are described by bucket_bounds. This defines + * size(bucket_bounds) + 1 (= N) buckets. The boundaries for bucket index i are: + * + *

    + *
  • {@code [0, bucket_bounds[i]) for i == 0} + *
  • {@code [bucket_bounds[i-1], bucket_bounds[i]) for 0 < i < N-1} + *
  • {@code [bucket_bounds[i-1], +infinity) for i == N-1} + *
+ * + *

If bucket_bounds has no elements (zero size), then there is no histogram associated with + * the Distribution. If bucket_bounds has only one element, there are no finite buckets, and + * that single element is the common boundary of the overflow and underflow buckets. The values + * must be monotonically increasing. + * + * @param bucketBoundaries the bucket boundaries of a distribution (given explicitly). The + * values must be strictly increasing and should be positive values. + * @return a {@code ExplicitOptions} {@code BucketOptions}. + * @since 0.17 + */ + public static BucketOptions explicitOptions(List bucketBoundaries) { + return ExplicitOptions.create(bucketBoundaries); + } + + /** + * Applies the given match function to the underlying BucketOptions. + * + * @param explicitFunction the function that should be applied if the BucketOptions has type + * {@code ExplicitOptions}. + * @param defaultFunction the function that should be applied if the BucketOptions has a type + * that was added after this {@code match} method was added to the API. See {@link + * io.opencensus.common.Functions} for some common functions for handling unknown types. + * @return the result of the function applied to the underlying BucketOptions. + * @since 0.17 + */ + public abstract T match( + Function explicitFunction, + Function defaultFunction); + + /** A Bucket with explicit bounds {@link BucketOptions}. */ + @AutoValue + @Immutable + public abstract static class ExplicitOptions extends BucketOptions { + + ExplicitOptions() {} + + @Override + public final T match( + Function explicitFunction, + Function defaultFunction) { + return explicitFunction.apply(this); + } + + /** + * Creates a {@link ExplicitOptions}. + * + * @param bucketBoundaries the bucket boundaries of a distribution (given explicitly). The + * values must be strictly increasing and should be positive. + * @return a {@code ExplicitOptions}. + * @since 0.17 + */ + private static ExplicitOptions create(List bucketBoundaries) { + Utils.checkNotNull(bucketBoundaries, "bucketBoundaries list should not be null."); + return new AutoValue_Distribution_BucketOptions_ExplicitOptions( + checkBucketBoundsAreSorted(bucketBoundaries)); + } + + private static List checkBucketBoundsAreSorted(List bucketBoundaries) { + List bucketBoundariesCopy = new ArrayList(bucketBoundaries); // Deep copy. + // Check if sorted. + if (bucketBoundariesCopy.size() >= 1) { + double previous = bucketBoundariesCopy.get(0); + Utils.checkArgument(previous > 0, "bucket boundaries should be > 0"); + for (int i = 1; i < bucketBoundariesCopy.size(); i++) { + double next = bucketBoundariesCopy.get(i); + Utils.checkArgument(previous < next, "bucket boundaries not sorted."); + previous = next; + } + } + return Collections.unmodifiableList(bucketBoundariesCopy); + } + + /** + * Returns the bucket boundaries of this distribution. + * + * @return the bucket boundaries of this distribution. + * @since 0.17 + */ + public abstract List getBucketBoundaries(); + } + } + /** * The histogram bucket of the population values. * diff --git a/api/src/test/java/io/opencensus/metrics/export/DistributionTest.java b/api/src/test/java/io/opencensus/metrics/export/DistributionTest.java index 0be4b5d1..ad89d338 100644 --- a/api/src/test/java/io/opencensus/metrics/export/DistributionTest.java +++ b/api/src/test/java/io/opencensus/metrics/export/DistributionTest.java @@ -19,9 +19,14 @@ package io.opencensus.metrics.export; import static com.google.common.truth.Truth.assertThat; import com.google.common.testing.EqualsTester; +import io.opencensus.common.Function; +import io.opencensus.common.Functions; import io.opencensus.common.Timestamp; import io.opencensus.metrics.export.Distribution.Bucket; +import io.opencensus.metrics.export.Distribution.BucketOptions; +import io.opencensus.metrics.export.Distribution.BucketOptions.ExplicitOptions; import io.opencensus.metrics.export.Distribution.Exemplar; +import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.List; @@ -72,20 +77,100 @@ public class DistributionTest { assertThat(exemplar.getAttachments()).isEqualTo(ATTACHMENTS); } + @Test + public void createAndGet_ExplicitBuckets() { + List bucketBounds = Arrays.asList(1.0, 2.0, 3.0); + + BucketOptions bucketOptions = BucketOptions.explicitOptions(bucketBounds); + final List actual = new ArrayList(); + bucketOptions.match( + new Function() { + @Override + public Object apply(ExplicitOptions arg) { + actual.addAll(arg.getBucketBoundaries()); + return null; + } + }, + Functions.throwAssertionError()); + + assertThat(actual).containsExactlyElementsIn(bucketBounds).inOrder(); + } + + @Test + public void createAndGet_ExplicitBucketsNegativeBounds() { + List bucketBounds = Arrays.asList(-1.0); + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("bucket boundaries should be > 0"); + BucketOptions.explicitOptions(bucketBounds); + } + + @Test + public void createAndGet_PreventNullExplicitBuckets() { + thrown.expect(NullPointerException.class); + BucketOptions.explicitOptions(Arrays.asList(1.0, null, 3.0)); + } + + @Test + public void createAndGet_ExplicitBucketsEmptyBounds() { + List bucketBounds = new ArrayList(); + BucketOptions bucketOptions = BucketOptions.explicitOptions(bucketBounds); + + final List actual = new ArrayList(); + bucketOptions.match( + new Function() { + @Override + public Object apply(ExplicitOptions arg) { + actual.addAll(arg.getBucketBoundaries()); + return null; + } + }, + Functions.throwAssertionError()); + + assertThat(actual).isEmpty(); + } + + @Test + public void createBucketOptions_UnorderedBucketBounds() { + List bucketBounds = Arrays.asList(1.0, 5.0, 2.0); + thrown.expect(IllegalArgumentException.class); + thrown.expectMessage("bucket boundaries not sorted."); + BucketOptions.explicitOptions(bucketBounds); + } + + @Test + public void createAndGet_PreventNullBucketOptions() { + thrown.expect(NullPointerException.class); + BucketOptions.explicitOptions(null); + } + @Test public void createAndGet_Distribution() { Exemplar exemplar = Exemplar.create(15.0, TIMESTAMP, ATTACHMENTS); - List bucketBounds = Arrays.asList(-1.0, 0.0, 1.0); + List bucketBounds = Arrays.asList(1.0, 2.0, 5.0); + BucketOptions bucketOptions = BucketOptions.explicitOptions(bucketBounds); List buckets = Arrays.asList( Bucket.create(3), Bucket.create(1), Bucket.create(2), Bucket.create(4, exemplar)); - Distribution distribution = Distribution.create(10, 6.6, 678.54, bucketBounds, buckets); + Distribution distribution = Distribution.create(10, 6.6, 678.54, bucketOptions, buckets); assertThat(distribution.getCount()).isEqualTo(10); assertThat(distribution.getSum()).isWithin(TOLERANCE).of(6.6); assertThat(distribution.getSumOfSquaredDeviations()).isWithin(TOLERANCE).of(678.54); - assertThat(distribution.getBucketBoundaries()) - .containsExactlyElementsIn(bucketBounds) - .inOrder(); + + final List actual = new ArrayList(); + distribution + .getBucketOptions() + .match( + new Function() { + @Override + public Object apply(ExplicitOptions arg) { + actual.addAll(arg.getBucketBoundaries()); + return null; + } + }, + Functions.throwAssertionError()); + + assertThat(actual).containsExactlyElementsIn(bucketBounds).inOrder(); + assertThat(distribution.getBuckets()).containsExactlyElementsIn(buckets).inOrder(); } @@ -121,42 +206,49 @@ public class DistributionTest { @Test public void createDistribution_NegativeCount() { - List bucketBounds = Arrays.asList(-1.0, 0.0, 1.0); + List bucketBounds = Arrays.asList(1.0, 2.0, 5.0); + BucketOptions bucketOptions = BucketOptions.explicitOptions(bucketBounds); + List buckets = Arrays.asList(Bucket.create(3), Bucket.create(1), Bucket.create(2), Bucket.create(4)); thrown.expect(IllegalArgumentException.class); thrown.expectMessage("count should be non-negative."); - Distribution.create(-10, 6.6, 678.54, bucketBounds, buckets); + Distribution.create(-10, 6.6, 678.54, bucketOptions, buckets); } @Test public void createDistribution_NegativeSumOfSquaredDeviations() { - List bucketBounds = Arrays.asList(-1.0, 0.0, 1.0); + List bucketBounds = Arrays.asList(1.0, 2.0, 5.0); + BucketOptions bucketOptions = BucketOptions.explicitOptions(bucketBounds); + List buckets = Arrays.asList(Bucket.create(0), Bucket.create(0), Bucket.create(0), Bucket.create(0)); thrown.expect(IllegalArgumentException.class); thrown.expectMessage("sum of squared deviations should be non-negative."); - Distribution.create(0, 6.6, -678.54, bucketBounds, buckets); + Distribution.create(0, 6.6, -678.54, bucketOptions, buckets); } @Test public void createDistribution_ZeroCountAndPositiveMean() { - List bucketBounds = Arrays.asList(-1.0, 0.0, 1.0); + List bucketBounds = Arrays.asList(1.0, 2.0, 5.0); + BucketOptions bucketOptions = BucketOptions.explicitOptions(bucketBounds); + List buckets = Arrays.asList(Bucket.create(0), Bucket.create(0), Bucket.create(0), Bucket.create(0)); thrown.expect(IllegalArgumentException.class); thrown.expectMessage("sum should be 0 if count is 0."); - Distribution.create(0, 6.6, 0, bucketBounds, buckets); + Distribution.create(0, 6.6, 0, bucketOptions, buckets); } @Test public void createDistribution_ZeroCountAndSumOfSquaredDeviations() { - List bucketBounds = Arrays.asList(-1.0, 0.0, 1.0); + List bucketBounds = Arrays.asList(1.0, 2.0, 5.0); + BucketOptions bucketOptions = BucketOptions.explicitOptions(bucketBounds); List buckets = Arrays.asList(Bucket.create(0), Bucket.create(0), Bucket.create(0), Bucket.create(0)); thrown.expect(IllegalArgumentException.class); thrown.expectMessage("sum of squared deviations should be 0 if count is 0."); - Distribution.create(0, 0, 678.54, bucketBounds, buckets); + Distribution.create(0, 0, 678.54, bucketOptions, buckets); } @Test @@ -165,53 +257,55 @@ public class DistributionTest { Arrays.asList(Bucket.create(3), Bucket.create(1), Bucket.create(2), Bucket.create(4)); thrown.expect(NullPointerException.class); thrown.expectMessage("bucketBoundaries list should not be null."); - Distribution.create(10, 6.6, 678.54, null, buckets); + Distribution.create(10, 6.6, 678.54, BucketOptions.explicitOptions(null), buckets); } @Test - public void createDistribution_UnorderedBucketBounds() { - List bucketBounds = Arrays.asList(0.0, -1.0, 1.0); + public void createDistribution_NullBucketOptions() { List buckets = Arrays.asList(Bucket.create(3), Bucket.create(1), Bucket.create(2), Bucket.create(4)); - thrown.expect(IllegalArgumentException.class); - thrown.expectMessage("bucket boundaries not sorted."); - Distribution.create(10, 6.6, 678.54, bucketBounds, buckets); + thrown.expect(NullPointerException.class); + thrown.expectMessage("bucketOptions"); + Distribution.create(10, 6.6, 678.54, null, buckets); } @Test public void createDistribution_NullBucketList() { - List bucketBounds = Arrays.asList(-1.0, 0.0, 1.0); + List bucketBounds = Arrays.asList(1.0, 2.0, 5.0); + BucketOptions bucketOptions = BucketOptions.explicitOptions(bucketBounds); thrown.expect(NullPointerException.class); thrown.expectMessage("bucket list should not be null."); - Distribution.create(10, 6.6, 678.54, bucketBounds, null); + Distribution.create(10, 6.6, 678.54, bucketOptions, null); } @Test public void createDistribution_NullBucket() { - List bucketBounds = Arrays.asList(-1.0, 0.0, 1.0); + List bucketBounds = Arrays.asList(1.0, 2.0, 5.0); + BucketOptions bucketOptions = BucketOptions.explicitOptions(bucketBounds); List buckets = Arrays.asList(Bucket.create(3), Bucket.create(1), null, Bucket.create(4)); thrown.expect(NullPointerException.class); thrown.expectMessage("bucket should not be null."); - Distribution.create(10, 6.6, 678.54, bucketBounds, buckets); + Distribution.create(10, 6.6, 678.54, bucketOptions, buckets); } @Test public void testEquals() { + List bucketBounds = Arrays.asList(1.0, 2.0, 2.5); new EqualsTester() .addEqualityGroup( Distribution.create( 10, 10, 1, - Arrays.asList(-5.0, 0.0, 5.0), + BucketOptions.explicitOptions(bucketBounds), Arrays.asList( Bucket.create(3), Bucket.create(1), Bucket.create(2), Bucket.create(4))), Distribution.create( 10, 10, 1, - Arrays.asList(-5.0, 0.0, 5.0), + BucketOptions.explicitOptions(bucketBounds), Arrays.asList( Bucket.create(3), Bucket.create(1), Bucket.create(2), Bucket.create(4)))) .addEqualityGroup( @@ -219,7 +313,7 @@ public class DistributionTest { 7, 10, 23.456, - Arrays.asList(-5.0, 0.0, 5.0), + BucketOptions.explicitOptions(bucketBounds), Arrays.asList( Bucket.create(3), Bucket.create(1), Bucket.create(2), Bucket.create(4)))) .testEquals(); diff --git a/api/src/test/java/io/opencensus/metrics/export/PointTest.java b/api/src/test/java/io/opencensus/metrics/export/PointTest.java index da5b83dc..cdfc7792 100644 --- a/api/src/test/java/io/opencensus/metrics/export/PointTest.java +++ b/api/src/test/java/io/opencensus/metrics/export/PointTest.java @@ -21,6 +21,7 @@ import static com.google.common.truth.Truth.assertThat; import com.google.common.testing.EqualsTester; import io.opencensus.common.Timestamp; import io.opencensus.metrics.export.Distribution.Bucket; +import io.opencensus.metrics.export.Distribution.BucketOptions; import java.util.Arrays; import org.junit.Test; import org.junit.runner.RunWith; @@ -38,7 +39,7 @@ public class PointTest { 10, 6.6, 678.54, - Arrays.asList(-1.0, 0.0, 1.0), + BucketOptions.explicitOptions(Arrays.asList(1.0, 2.0, 5.0)), Arrays.asList( Bucket.create(3), Bucket.create(1), Bucket.create(2), Bucket.create(4)))); private static final Timestamp TIMESTAMP_1 = Timestamp.create(1, 2); diff --git a/api/src/test/java/io/opencensus/metrics/export/ValueTest.java b/api/src/test/java/io/opencensus/metrics/export/ValueTest.java index 3758ed2d..bf947692 100644 --- a/api/src/test/java/io/opencensus/metrics/export/ValueTest.java +++ b/api/src/test/java/io/opencensus/metrics/export/ValueTest.java @@ -22,6 +22,8 @@ import com.google.common.testing.EqualsTester; import io.opencensus.common.Function; import io.opencensus.common.Functions; import io.opencensus.metrics.export.Distribution.Bucket; +import io.opencensus.metrics.export.Distribution.BucketOptions; +import io.opencensus.metrics.export.Distribution.BucketOptions.ExplicitOptions; import io.opencensus.metrics.export.Summary.Snapshot; import io.opencensus.metrics.export.Summary.Snapshot.ValueAtPercentile; import io.opencensus.metrics.export.Value.ValueDistribution; @@ -46,7 +48,7 @@ public class ValueTest { 10, 10, 1, - Arrays.asList(-5.0, 0.0, 5.0), + BucketOptions.explicitOptions(Arrays.asList(1.0, 2.0, 5.0)), Arrays.asList(Bucket.create(3), Bucket.create(1), Bucket.create(2), Bucket.create(4))); private static final Summary SUMMARY = Summary.create( @@ -96,7 +98,7 @@ public class ValueTest { 7, 10, 23.456, - Arrays.asList(-5.0, 0.0, 5.0), + BucketOptions.explicitOptions(Arrays.asList(1.0, 2.0, 5.0)), Arrays.asList( Bucket.create(3), Bucket.create(1), Bucket.create(2), Bucket.create(4))))) .testEquals(); @@ -111,7 +113,7 @@ public class ValueTest { ValueDistribution.create(DISTRIBUTION), ValueSummary.create(SUMMARY)); List expected = - Arrays.asList(1.0, -1L, 10.0, 10L, 1.0, -5.0, 0.0, 5.0, 3L, 1L, 2L, 4L); + Arrays.asList(1.0, -1L, 10.0, 10L, 1.0, 1.0, 2.0, 5.0, 3L, 1L, 2L, 4L); final List actual = new ArrayList(); for (Value value : values) { value.match( @@ -135,7 +137,18 @@ public class ValueTest { actual.add(arg.getSum()); actual.add(arg.getCount()); actual.add(arg.getSumOfSquaredDeviations()); - actual.addAll(arg.getBucketBoundaries()); + + arg.getBucketOptions() + .match( + new Function() { + @Override + public Object apply(ExplicitOptions arg) { + actual.addAll(arg.getBucketBoundaries()); + return null; + } + }, + Functions.throwAssertionError()); + for (Bucket bucket : arg.getBuckets()) { actual.add(bucket.getCount()); } -- cgit v1.2.3