From 0ba91eaee7bd351fe5536c66ce664aefd4445f56 Mon Sep 17 00:00:00 2001 From: Mayur Kale Date: Mon, 15 Oct 2018 15:04:55 -0700 Subject: [PATCH 1/9] ignore negative bucket bounds --- .../implcore/stats/MutableAggregation.java | 17 ++++++++++++++--- .../implcore/stats/MutableAggregationTest.java | 12 +++++------- .../implcore/stats/RecordUtilsTest.java | 2 +- .../implcore/stats/StatsRecorderImplTest.java | 1 - 4 files changed, 20 insertions(+), 12 deletions(-) diff --git a/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java b/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java index 6e2bff1cc8..00e722bde1 100644 --- a/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java +++ b/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java @@ -287,7 +287,20 @@ private MutableDistribution(BucketBoundaries bucketBoundaries) { */ static MutableDistribution create(BucketBoundaries bucketBoundaries) { checkNotNull(bucketBoundaries, "bucketBoundaries should not be null."); - return new MutableDistribution(bucketBoundaries); + + List currentBucketBoundaries = bucketBoundaries.getBoundaries(); + int ignoreBucketBounds = 0; + for (Double bucketBound : currentBucketBoundaries) { + if (bucketBound < 0) { + ignoreBucketBounds++; + } else { + break; + } + } + List newBucketBoundaries = + currentBucketBoundaries.subList(ignoreBucketBounds, currentBucketBoundaries.size()); + + return new MutableDistribution(BucketBoundaries.create(newBucketBoundaries)); } @Override @@ -429,8 +442,6 @@ Point toPoint(Timestamp timestamp) { buckets.add(metricBucket); } - // TODO(mayurkale): Drop the first bucket when converting to metrics. - // Reason: In Stats API, bucket bounds begin with -infinity (first bucket is (-infinity, 0)). BucketOptions bucketOptions = BucketOptions.explicitOptions(bucketBoundaries.getBoundaries()); return Point.create( diff --git a/impl_core/src/test/java/io/opencensus/implcore/stats/MutableAggregationTest.java b/impl_core/src/test/java/io/opencensus/implcore/stats/MutableAggregationTest.java index a6139e53f9..ab3fa092bd 100644 --- a/impl_core/src/test/java/io/opencensus/implcore/stats/MutableAggregationTest.java +++ b/impl_core/src/test/java/io/opencensus/implcore/stats/MutableAggregationTest.java @@ -139,8 +139,7 @@ public void testAdd() { TOLERANCE); assertAggregationDataEquals( aggregations.get(4).toAggregationData(), - AggregationData.DistributionData.create( - 4.0, 5, -5.0, 20.0, 372, Arrays.asList(0L, 2L, 2L, 1L)), + AggregationData.DistributionData.create(4.0, 5, -5.0, 20.0, 372, Arrays.asList(2L, 2L, 1L)), TOLERANCE); assertAggregationDataEquals( aggregations.get(5).toAggregationData(), @@ -181,7 +180,6 @@ public void testAdd_DistributionWithExemplarAttachments() { // bucket, only the last one will be kept. List expected = Arrays.asList( - null, Exemplar.create(values.get(2), timestamps.get(2), attachmentsList.get(2)), Exemplar.create(values.get(4), timestamps.get(4), attachmentsList.get(4)), Exemplar.create(values.get(3), timestamps.get(3), attachmentsList.get(3))); @@ -258,13 +256,13 @@ public void testCombine_Distribution() { MutableDistribution combined = MutableDistribution.create(BUCKET_BOUNDARIES); combined.combine(distribution1, 1.0); // distribution1 will be combined combined.combine(distribution2, 0.6); // distribution2 will be ignored - verifyMutableDistribution(combined, 0, 2, -5, 5, 50.0, new long[] {0, 1, 1, 0}, TOLERANCE); + verifyMutableDistribution(combined, 0, 2, -5, 5, 50.0, new long[] {1, 1, 0}, TOLERANCE); combined.combine(distribution2, 1.0); // distribution2 will be combined - verifyMutableDistribution(combined, 7.5, 4, -5, 20, 325.0, new long[] {0, 1, 1, 2}, TOLERANCE); + verifyMutableDistribution(combined, 7.5, 4, -5, 20, 325.0, new long[] {1, 1, 2}, TOLERANCE); combined.combine(distribution3, 1.0); // distribution3 will be combined - verifyMutableDistribution(combined, 0, 8, -20, 20, 1500.0, new long[] {2, 2, 1, 3}, TOLERANCE); + verifyMutableDistribution(combined, 0, 8, -20, 20, 1500.0, new long[] {4, 1, 3}, TOLERANCE); } @Test @@ -281,7 +279,7 @@ public void mutableAggregation_ToAggregationData() { Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY, 0, - Arrays.asList(0L, 0L, 0L, 0L))); + Arrays.asList(0L, 0L, 0L))); assertThat(MutableLastValueDouble.create().toAggregationData()) .isEqualTo(LastValueDataDouble.create(Double.NaN)); assertThat(MutableLastValueLong.create().toAggregationData()) diff --git a/impl_core/src/test/java/io/opencensus/implcore/stats/RecordUtilsTest.java b/impl_core/src/test/java/io/opencensus/implcore/stats/RecordUtilsTest.java index 1e22a7a1e8..57fa647a1f 100644 --- a/impl_core/src/test/java/io/opencensus/implcore/stats/RecordUtilsTest.java +++ b/impl_core/src/test/java/io/opencensus/implcore/stats/RecordUtilsTest.java @@ -111,6 +111,6 @@ public void createMutableAggregation() { assertThat(mutableDistribution.getMin()).isPositiveInfinity(); assertThat(mutableDistribution.getMax()).isNegativeInfinity(); assertThat(mutableDistribution.getSumOfSquaredDeviations()).isWithin(EPSILON).of(0); - assertThat(mutableDistribution.getBucketCounts()).isEqualTo(new long[4]); + assertThat(mutableDistribution.getBucketCounts()).isEqualTo(new long[3]); } } diff --git a/impl_core/src/test/java/io/opencensus/implcore/stats/StatsRecorderImplTest.java b/impl_core/src/test/java/io/opencensus/implcore/stats/StatsRecorderImplTest.java index bd8b5b888e..ff0600dd1f 100644 --- a/impl_core/src/test/java/io/opencensus/implcore/stats/StatsRecorderImplTest.java +++ b/impl_core/src/test/java/io/opencensus/implcore/stats/StatsRecorderImplTest.java @@ -170,7 +170,6 @@ public void record_WithAttachments_Distribution() { (DistributionData) viewData.getAggregationMap().get(Collections.singletonList(VALUE)); List expected = Arrays.asList( - Exemplar.create(-20.0, Timestamp.create(4, 0), Collections.singletonMap("k3", "v1")), Exemplar.create(-5.0, Timestamp.create(5, 0), Collections.singletonMap("k3", "v3")), Exemplar.create(1.0, Timestamp.create(2, 0), Collections.singletonMap("k2", "v2")), Exemplar.create(12.0, Timestamp.create(3, 0), Collections.singletonMap("k1", "v3"))); From b3f15322f5244070b36e4ddbf3d99f7537a3d9ca Mon Sep 17 00:00:00 2001 From: Mayur Kale Date: Sun, 21 Oct 2018 22:56:21 -0700 Subject: [PATCH 2/9] drop negative values in the Record API. --- CHANGELOG.md | 2 ++ .../io/opencensus/stats/BucketBoundaries.java | 35 ++++++++++++++++--- .../stats/BucketBoundariesTest.java | 8 +++++ .../implcore/stats/MutableAggregation.java | 15 +------- 4 files changed, 42 insertions(+), 18 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b426ab2278..cbd4df2fd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,8 @@ - Add Gauges (`DoubleGauge`, `LongGauge`, `DerivedDoubleGauge`, `DerivedLongGauge`) APIs. - Update `opencensus-contrib-log-correlation-log4j2` to match the [OpenCensus log correlation spec](https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/LogCorrelation.md). +- Negative bucket boundaries are not longer supported in the Record API. In case negative values + provided, the Record API drops the negative value(s) and logs the warning. ## 0.16.1 - 2018-09-18 - Fix ClassCastException in Log4j log correlation diff --git a/api/src/main/java/io/opencensus/stats/BucketBoundaries.java b/api/src/main/java/io/opencensus/stats/BucketBoundaries.java index 61e21e6cc5..6b0c90fa2c 100644 --- a/api/src/main/java/io/opencensus/stats/BucketBoundaries.java +++ b/api/src/main/java/io/opencensus/stats/BucketBoundaries.java @@ -21,6 +21,8 @@ import java.util.ArrayList; import java.util.Collections; import java.util.List; +import java.util.logging.Level; +import java.util.logging.Logger; import javax.annotation.concurrent.Immutable; /** @@ -32,6 +34,8 @@ @AutoValue public abstract class BucketBoundaries { + private static final Logger logger = Logger.getLogger(BucketBoundaries.class.getName()); + /** * Returns a {@code BucketBoundaries} with the given buckets. * @@ -46,14 +50,37 @@ public static final BucketBoundaries create(List bucketBoundaries) { List bucketBoundariesCopy = new ArrayList(bucketBoundaries); // Deep copy. // Check if sorted. if (bucketBoundariesCopy.size() > 1) { - double lower = bucketBoundariesCopy.get(0); + double previous = 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; + Utils.checkArgument(previous < next, "Bucket boundaries not sorted."); + previous = next; } } - return new AutoValue_BucketBoundaries(Collections.unmodifiableList(bucketBoundariesCopy)); + return new AutoValue_BucketBoundaries( + Collections.unmodifiableList(dropNegativeBucketBounds(bucketBoundariesCopy))); + } + + private static List dropNegativeBucketBounds(List bucketBoundaries) { + // Negative values are currently not supported by any of the backends that OC supports. + int negativeValuesCount = 0; + for (Double value : bucketBoundaries) { + if (value < 0) { + negativeValuesCount++; + } else { + break; + } + } + + if (negativeValuesCount > 0) { + bucketBoundaries = bucketBoundaries.subList(negativeValuesCount, bucketBoundaries.size()); + logger.log( + Level.WARNING, + "Dropping " + + negativeValuesCount + + " negative bucket boundaries, the values must be strictly > 0."); + } + return bucketBoundaries; } /** diff --git a/api/src/test/java/io/opencensus/stats/BucketBoundariesTest.java b/api/src/test/java/io/opencensus/stats/BucketBoundariesTest.java index 36f2edb473..caa7e6c49a 100644 --- a/api/src/test/java/io/opencensus/stats/BucketBoundariesTest.java +++ b/api/src/test/java/io/opencensus/stats/BucketBoundariesTest.java @@ -41,6 +41,14 @@ public void testConstructBoundaries() { assertThat(bucketBoundaries.getBoundaries()).isEqualTo(buckets); } + @Test + public void testConstructBoundaries_IgnoreNegativeBounds() { + List buckets = Arrays.asList(-5.0, -1.0, 1.0, 2.0); + List expectedBuckets = Arrays.asList(1.0, 2.0); + BucketBoundaries bucketBoundaries = BucketBoundaries.create(buckets); + assertThat(bucketBoundaries.getBoundaries()).isEqualTo(expectedBuckets); + } + @Test public void testBoundariesDoesNotChangeWithOriginalList() { List original = new ArrayList(); diff --git a/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java b/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java index 00e722bde1..578b52ab26 100644 --- a/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java +++ b/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java @@ -287,20 +287,7 @@ private MutableDistribution(BucketBoundaries bucketBoundaries) { */ static MutableDistribution create(BucketBoundaries bucketBoundaries) { checkNotNull(bucketBoundaries, "bucketBoundaries should not be null."); - - List currentBucketBoundaries = bucketBoundaries.getBoundaries(); - int ignoreBucketBounds = 0; - for (Double bucketBound : currentBucketBoundaries) { - if (bucketBound < 0) { - ignoreBucketBounds++; - } else { - break; - } - } - List newBucketBoundaries = - currentBucketBoundaries.subList(ignoreBucketBounds, currentBucketBoundaries.size()); - - return new MutableDistribution(BucketBoundaries.create(newBucketBoundaries)); + return new MutableDistribution(bucketBoundaries); } @Override From aea3049e26cdc6294385fff83423ea631a8bf048 Mon Sep 17 00:00:00 2001 From: Mayur Kale Date: Mon, 22 Oct 2018 16:14:13 -0700 Subject: [PATCH 3/9] Drop negative values from histograms count and sum. --- CHANGELOG.md | 5 +++-- .../implcore/stats/MutableAggregation.java | 9 +++++++++ .../implcore/stats/MutableAggregationTest.java | 12 +++++++----- .../implcore/stats/StatsRecorderImplTest.java | 1 - 4 files changed, 19 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cbd4df2fd5..872c8230f8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,8 +12,9 @@ - Add Gauges (`DoubleGauge`, `LongGauge`, `DerivedDoubleGauge`, `DerivedLongGauge`) APIs. - Update `opencensus-contrib-log-correlation-log4j2` to match the [OpenCensus log correlation spec](https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/LogCorrelation.md). -- Negative bucket boundaries are not longer supported in the Record API. In case negative values - provided, the Record API drops the negative value(s) and logs the warning. +- The histogram bucket boundaries (`BucketBoundaries`) and values (`Count` and `Sum`) are no longer + supported for negative values. The Record API drops the negative values and logs the warning. + This could be a breaking change if you are using negative values to create or add a histogram. ## 0.16.1 - 2018-09-18 - Fix ClassCastException in Log4j log correlation diff --git a/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java b/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java index 578b52ab26..223887c235 100644 --- a/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java +++ b/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java @@ -33,10 +33,14 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.logging.Level; +import java.util.logging.Logger; /** Mutable version of {@link Aggregation} that supports adding values. */ abstract class MutableAggregation { + private static final Logger logger = Logger.getLogger(MutableAggregation.class.getName()); + private MutableAggregation() {} // Tolerance for double comparison. @@ -292,6 +296,11 @@ static MutableDistribution create(BucketBoundaries bucketBoundaries) { @Override void add(double value, Map attachments, Timestamp timestamp) { + if (value < 0) { + logger.log(Level.WARNING, "Dropping negative values for histogram count and sum."); + return; + } + sum += value; count++; diff --git a/impl_core/src/test/java/io/opencensus/implcore/stats/MutableAggregationTest.java b/impl_core/src/test/java/io/opencensus/implcore/stats/MutableAggregationTest.java index ab3fa092bd..c530ead4b9 100644 --- a/impl_core/src/test/java/io/opencensus/implcore/stats/MutableAggregationTest.java +++ b/impl_core/src/test/java/io/opencensus/implcore/stats/MutableAggregationTest.java @@ -139,7 +139,8 @@ public void testAdd() { TOLERANCE); assertAggregationDataEquals( aggregations.get(4).toAggregationData(), - AggregationData.DistributionData.create(4.0, 5, -5.0, 20.0, 372, Arrays.asList(2L, 2L, 1L)), + AggregationData.DistributionData.create( + 8.666666, 3, 1.0, 20.0, 200.666666, Arrays.asList(0L, 2L, 1L)), TOLERANCE); assertAggregationDataEquals( aggregations.get(5).toAggregationData(), @@ -180,7 +181,7 @@ public void testAdd_DistributionWithExemplarAttachments() { // bucket, only the last one will be kept. List expected = Arrays.asList( - Exemplar.create(values.get(2), timestamps.get(2), attachmentsList.get(2)), + null, Exemplar.create(values.get(4), timestamps.get(4), attachmentsList.get(4)), Exemplar.create(values.get(3), timestamps.get(3), attachmentsList.get(3))); assertThat(mutableDistribution.getExemplars()) @@ -256,13 +257,14 @@ public void testCombine_Distribution() { MutableDistribution combined = MutableDistribution.create(BUCKET_BOUNDARIES); combined.combine(distribution1, 1.0); // distribution1 will be combined combined.combine(distribution2, 0.6); // distribution2 will be ignored - verifyMutableDistribution(combined, 0, 2, -5, 5, 50.0, new long[] {1, 1, 0}, TOLERANCE); + verifyMutableDistribution(combined, 5, 1, 5, 5, 0.0, new long[] {0, 1, 0}, TOLERANCE); combined.combine(distribution2, 1.0); // distribution2 will be combined - verifyMutableDistribution(combined, 7.5, 4, -5, 20, 325.0, new long[] {1, 1, 2}, TOLERANCE); + verifyMutableDistribution( + combined, 11.666666, 3, 5, 20, 116.66666666, new long[] {0, 1, 2}, TOLERANCE); combined.combine(distribution3, 1.0); // distribution3 will be combined - verifyMutableDistribution(combined, 0, 8, -20, 20, 1500.0, new long[] {4, 1, 3}, TOLERANCE); + verifyMutableDistribution(combined, 12.5, 4, 5, 20, 125.0, new long[] {0, 1, 3}, TOLERANCE); } @Test diff --git a/impl_core/src/test/java/io/opencensus/implcore/stats/StatsRecorderImplTest.java b/impl_core/src/test/java/io/opencensus/implcore/stats/StatsRecorderImplTest.java index ff0600dd1f..f4ac9655e1 100644 --- a/impl_core/src/test/java/io/opencensus/implcore/stats/StatsRecorderImplTest.java +++ b/impl_core/src/test/java/io/opencensus/implcore/stats/StatsRecorderImplTest.java @@ -170,7 +170,6 @@ public void record_WithAttachments_Distribution() { (DistributionData) viewData.getAggregationMap().get(Collections.singletonList(VALUE)); List expected = Arrays.asList( - Exemplar.create(-5.0, Timestamp.create(5, 0), Collections.singletonMap("k3", "v3")), Exemplar.create(1.0, Timestamp.create(2, 0), Collections.singletonMap("k2", "v2")), Exemplar.create(12.0, Timestamp.create(3, 0), Collections.singletonMap("k1", "v3"))); assertThat(distributionData.getExemplars()).containsExactlyElementsIn(expected).inOrder(); From 8793afe54618b4286e86f7384a749353b3322358 Mon Sep 17 00:00:00 2001 From: Mayur Kale Date: Tue, 23 Oct 2018 14:34:56 -0700 Subject: [PATCH 4/9] report negative value --- .../java/io/opencensus/implcore/stats/MutableAggregation.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java b/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java index 223887c235..aa62b55941 100644 --- a/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java +++ b/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java @@ -297,7 +297,8 @@ static MutableDistribution create(BucketBoundaries bucketBoundaries) { @Override void add(double value, Map attachments, Timestamp timestamp) { if (value < 0) { - logger.log(Level.WARNING, "Dropping negative values for histogram count and sum."); + logger.log( + Level.WARNING, "Dropping negative value (" + value + ") from histogram count and sum."); return; } From f442d3406e538924cf85cf4bd7e0a37eddf6e944 Mon Sep 17 00:00:00 2001 From: Mayur Kale Date: Wed, 24 Oct 2018 11:37:49 -0700 Subject: [PATCH 5/9] drop negative values in the Record API --- CHANGELOG.md | 4 ++-- .../implcore/stats/MeasureMapImpl.java | 16 ++++++++++++++-- .../implcore/stats/MutableAggregation.java | 10 ---------- .../implcore/stats/MutableAggregationTest.java | 13 +++++-------- .../implcore/stats/StatsRecorderImplTest.java | 2 +- .../implcore/stats/ViewManagerImplTest.java | 14 +++++++------- 6 files changed, 29 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 872c8230f8..230e29c422 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,8 +13,8 @@ - Update `opencensus-contrib-log-correlation-log4j2` to match the [OpenCensus log correlation spec](https://github.com/census-instrumentation/opencensus-specs/blob/master/trace/LogCorrelation.md). - The histogram bucket boundaries (`BucketBoundaries`) and values (`Count` and `Sum`) are no longer - supported for negative values. The Record API drops the negative values and logs the warning. - This could be a breaking change if you are using negative values to create or add a histogram. + supported for negative values. The Record API drops the negative `value` and logs the warning. + This could be a breaking change if you are recording negative value for any `measure`. ## 0.16.1 - 2018-09-18 - Fix ClassCastException in Log4j log correlation diff --git a/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapImpl.java b/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapImpl.java index ee51796c34..d73efa2fd9 100644 --- a/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapImpl.java +++ b/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapImpl.java @@ -21,9 +21,13 @@ import io.opencensus.stats.MeasureMap; import io.opencensus.tags.TagContext; import io.opencensus.tags.unsafe.ContextUtils; +import java.util.logging.Level; +import java.util.logging.Logger; /** Implementation of {@link MeasureMap}. */ final class MeasureMapImpl extends MeasureMap { + private static final Logger logger = Logger.getLogger(MeasureMapImpl.class.getName()); + private final StatsManager statsManager; private final MeasureMapInternal.Builder builder = MeasureMapInternal.builder(); @@ -37,13 +41,21 @@ private MeasureMapImpl(StatsManager statsManager) { @Override public MeasureMapImpl put(MeasureDouble measure, double value) { - builder.put(measure, value); + if (value < 0) { + logger.log(Level.WARNING, "Dropping (" + value + "), value to record must be non-negative."); + } else { + builder.put(measure, value); + } return this; } @Override public MeasureMapImpl put(MeasureLong measure, long value) { - builder.put(measure, value); + if (value < 0) { + logger.log(Level.WARNING, "Dropping (" + value + "), value to record must be non-negative."); + } else { + builder.put(measure, value); + } return this; } diff --git a/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java b/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java index aa62b55941..578b52ab26 100644 --- a/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java +++ b/impl_core/src/main/java/io/opencensus/implcore/stats/MutableAggregation.java @@ -33,14 +33,10 @@ import java.util.ArrayList; import java.util.List; import java.util.Map; -import java.util.logging.Level; -import java.util.logging.Logger; /** Mutable version of {@link Aggregation} that supports adding values. */ abstract class MutableAggregation { - private static final Logger logger = Logger.getLogger(MutableAggregation.class.getName()); - private MutableAggregation() {} // Tolerance for double comparison. @@ -296,12 +292,6 @@ static MutableDistribution create(BucketBoundaries bucketBoundaries) { @Override void add(double value, Map attachments, Timestamp timestamp) { - if (value < 0) { - logger.log( - Level.WARNING, "Dropping negative value (" + value + ") from histogram count and sum."); - return; - } - sum += value; count++; diff --git a/impl_core/src/test/java/io/opencensus/implcore/stats/MutableAggregationTest.java b/impl_core/src/test/java/io/opencensus/implcore/stats/MutableAggregationTest.java index c530ead4b9..59cb954e5a 100644 --- a/impl_core/src/test/java/io/opencensus/implcore/stats/MutableAggregationTest.java +++ b/impl_core/src/test/java/io/opencensus/implcore/stats/MutableAggregationTest.java @@ -139,8 +139,7 @@ public void testAdd() { TOLERANCE); assertAggregationDataEquals( aggregations.get(4).toAggregationData(), - AggregationData.DistributionData.create( - 8.666666, 3, 1.0, 20.0, 200.666666, Arrays.asList(0L, 2L, 1L)), + AggregationData.DistributionData.create(4.0, 5, -5.0, 20.0, 372, Arrays.asList(2L, 2L, 1L)), TOLERANCE); assertAggregationDataEquals( aggregations.get(5).toAggregationData(), @@ -181,7 +180,7 @@ public void testAdd_DistributionWithExemplarAttachments() { // bucket, only the last one will be kept. List expected = Arrays.asList( - null, + Exemplar.create(values.get(2), timestamps.get(2), attachmentsList.get(2)), Exemplar.create(values.get(4), timestamps.get(4), attachmentsList.get(4)), Exemplar.create(values.get(3), timestamps.get(3), attachmentsList.get(3))); assertThat(mutableDistribution.getExemplars()) @@ -257,14 +256,12 @@ public void testCombine_Distribution() { MutableDistribution combined = MutableDistribution.create(BUCKET_BOUNDARIES); combined.combine(distribution1, 1.0); // distribution1 will be combined combined.combine(distribution2, 0.6); // distribution2 will be ignored - verifyMutableDistribution(combined, 5, 1, 5, 5, 0.0, new long[] {0, 1, 0}, TOLERANCE); + verifyMutableDistribution(combined, 0, 2, -5, 5, 50.0, new long[] {1, 1, 0}, TOLERANCE); combined.combine(distribution2, 1.0); // distribution2 will be combined - verifyMutableDistribution( - combined, 11.666666, 3, 5, 20, 116.66666666, new long[] {0, 1, 2}, TOLERANCE); - + verifyMutableDistribution(combined, 7.5, 4, -5, 20, 325.0, new long[] {1, 1, 2}, TOLERANCE); combined.combine(distribution3, 1.0); // distribution3 will be combined - verifyMutableDistribution(combined, 12.5, 4, 5, 20, 125.0, new long[] {0, 1, 3}, TOLERANCE); + verifyMutableDistribution(combined, 0, 8, -20, 20, 1500.0, new long[] {4, 1, 3}, TOLERANCE); } @Test diff --git a/impl_core/src/test/java/io/opencensus/implcore/stats/StatsRecorderImplTest.java b/impl_core/src/test/java/io/opencensus/implcore/stats/StatsRecorderImplTest.java index f4ac9655e1..5c67489896 100644 --- a/impl_core/src/test/java/io/opencensus/implcore/stats/StatsRecorderImplTest.java +++ b/impl_core/src/test/java/io/opencensus/implcore/stats/StatsRecorderImplTest.java @@ -207,7 +207,7 @@ public void record_WithAttachments_Count() { CountData countData = (CountData) viewData.getAggregationMap().get(Collections.singletonList(VALUE)); // Recording exemplar does not affect views with an aggregation other than distribution. - assertThat(countData.getCount()).isEqualTo(6L); + assertThat(countData.getCount()).isEqualTo(2L); } private void recordWithAttachments() { diff --git a/impl_core/src/test/java/io/opencensus/implcore/stats/ViewManagerImplTest.java b/impl_core/src/test/java/io/opencensus/implcore/stats/ViewManagerImplTest.java index a4018b7988..c5531c3a70 100644 --- a/impl_core/src/test/java/io/opencensus/implcore/stats/ViewManagerImplTest.java +++ b/impl_core/src/test/java/io/opencensus/implcore/stats/ViewManagerImplTest.java @@ -324,10 +324,10 @@ public void testRecordDouble_mean_interval() { testRecordInterval( MEASURE_DOUBLE, MEAN, - new double[] {20.0, -1.0, 1.0, -5.0, 5.0}, + new double[] {20.0, 1.0, 1.0, 5.0, 5.0}, 9.0, 30.0, - MeanData.create((19 * 0.6 + 1) / 4, 4), + MeanData.create((21 * 0.6 + 11) / 4, 4), MeanData.create(0.2 * 5 + 9, 1), MeanData.create(30.0, 1)); } @@ -341,7 +341,7 @@ public void testRecordLong_mean_interval() { -5000, 30, MeanData.create((3000 * 0.6 + 12000) / 4, 4), - MeanData.create(-4000, 1), + MeanData.create(0, 1), MeanData.create(30, 1)); } @@ -350,10 +350,10 @@ public void testRecordDouble_sum_interval() { testRecordInterval( MEASURE_DOUBLE, SUM, - new double[] {20.0, -1.0, 1.0, -5.0, 5.0}, + new double[] {20.0, 1.0, 1.0, 5.0, 5.0}, 9.0, 30.0, - SumDataDouble.create(19 * 0.6 + 1), + SumDataDouble.create(21 * 0.6 + 11), SumDataDouble.create(0.2 * 5 + 9), SumDataDouble.create(30.0)); } @@ -367,7 +367,7 @@ public void testRecordLong_sum_interval() { -50, 30, SumDataLong.create(Math.round(34 * 0.6 + 120)), - SumDataLong.create(-40), + SumDataLong.create(10), SumDataLong.create(30)); } @@ -393,7 +393,7 @@ public void testRecordLong_lastvalue_interval() { -5000, 30, LastValueDataLong.create(5000), - LastValueDataLong.create(-5000), + LastValueDataLong.create(0), LastValueDataLong.create(30)); } From a3d31ece2884897dc29b4b2f0535781dd2a6febd Mon Sep 17 00:00:00 2001 From: Mayur Kale Date: Thu, 25 Oct 2018 15:22:17 -0700 Subject: [PATCH 6/9] Add check and warning in noop implementation, Ignore 0 bucket bounds --- .../io/opencensus/stats/BucketBoundaries.java | 22 ++++++++++++------- .../java/io/opencensus/stats/NoopStats.java | 16 ++++++++++++++ .../io/opencensus/stats/AggregationTest.java | 7 +++--- .../stats/BucketBoundariesTest.java | 12 ++++++++-- .../io/opencensus/stats/NoopStatsTest.java | 5 +++++ .../http/util/HttpViewConstantsTest.java | 7 +++--- .../zpages/StatszZPageHandlerTest.java | 2 +- .../implcore/stats/MeasureMapImpl.java | 18 ++++++++++----- .../implcore/stats/MeasureMapInternal.java | 8 +++++++ .../stats/MeasureMapInternalTest.java | 11 ++++++++++ .../stats/MutableAggregationTest.java | 19 +++++----------- .../implcore/stats/RecordUtilsTest.java | 2 +- 12 files changed, 91 insertions(+), 38 deletions(-) diff --git a/api/src/main/java/io/opencensus/stats/BucketBoundaries.java b/api/src/main/java/io/opencensus/stats/BucketBoundaries.java index 6b0c90fa2c..258dbfd63f 100644 --- a/api/src/main/java/io/opencensus/stats/BucketBoundaries.java +++ b/api/src/main/java/io/opencensus/stats/BucketBoundaries.java @@ -62,25 +62,31 @@ public static final BucketBoundaries create(List bucketBoundaries) { } private static List dropNegativeBucketBounds(List bucketBoundaries) { - // Negative values are currently not supported by any of the backends that OC supports. - int negativeValuesCount = 0; + // Negative values (BucketBounds) are currently not supported by any of the backends + // that OC supports. + int negativeBucketBounds = 0; + int zeroBucketBounds = 0; for (Double value : bucketBoundaries) { - if (value < 0) { - negativeValuesCount++; + if (value <= 0) { + if (value == 0) { + zeroBucketBounds++; + } else { + negativeBucketBounds++; + } } else { break; } } - if (negativeValuesCount > 0) { - bucketBoundaries = bucketBoundaries.subList(negativeValuesCount, bucketBoundaries.size()); + if (negativeBucketBounds > 0) { logger.log( Level.WARNING, "Dropping " - + negativeValuesCount + + negativeBucketBounds + " negative bucket boundaries, the values must be strictly > 0."); } - return bucketBoundaries; + return bucketBoundaries.subList( + negativeBucketBounds + zeroBucketBounds, bucketBoundaries.size()); } /** diff --git a/api/src/main/java/io/opencensus/stats/NoopStats.java b/api/src/main/java/io/opencensus/stats/NoopStats.java index e7e94a3828..cc8d09e1f2 100644 --- a/api/src/main/java/io/opencensus/stats/NoopStats.java +++ b/api/src/main/java/io/opencensus/stats/NoopStats.java @@ -30,6 +30,8 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.logging.Level; +import java.util.logging.Logger; import javax.annotation.concurrent.GuardedBy; import javax.annotation.concurrent.Immutable; import javax.annotation.concurrent.ThreadSafe; @@ -122,15 +124,23 @@ public MeasureMap newMeasureMap() { @Immutable private static final class NoopMeasureMap extends MeasureMap { + private static final Logger logger = Logger.getLogger(NoopMeasureMap.class.getName()); static final MeasureMap INSTANCE = new NoopMeasureMap(); + private volatile boolean hasUnsupportedValues; @Override public MeasureMap put(MeasureDouble measure, double value) { + if (value < 0) { + hasUnsupportedValues = true; + } return this; } @Override public MeasureMap put(MeasureLong measure, long value) { + if (value < 0) { + hasUnsupportedValues = true; + } return this; } @@ -140,6 +150,12 @@ public void record() {} @Override public void record(TagContext tags) { Utils.checkNotNull(tags, "tags"); + + if (hasUnsupportedValues) { + // drop all the recorded values + logger.log(Level.WARNING, "Dropping values, value to record must be non-negative."); + hasUnsupportedValues = false; + } } } diff --git a/api/src/test/java/io/opencensus/stats/AggregationTest.java b/api/src/test/java/io/opencensus/stats/AggregationTest.java index cf33703039..11606dad44 100644 --- a/api/src/test/java/io/opencensus/stats/AggregationTest.java +++ b/api/src/test/java/io/opencensus/stats/AggregationTest.java @@ -61,10 +61,11 @@ public void testEquals() { .addEqualityGroup(Count.create(), Count.create()) .addEqualityGroup( Distribution.create(BucketBoundaries.create(Arrays.asList(-10.0, 1.0, 5.0))), - Distribution.create(BucketBoundaries.create(Arrays.asList(-10.0, 1.0, 5.0)))) - .addEqualityGroup( - Distribution.create(BucketBoundaries.create(Arrays.asList(0.0, 1.0, 5.0))), + Distribution.create(BucketBoundaries.create(Arrays.asList(-10.0, 1.0, 5.0))), Distribution.create(BucketBoundaries.create(Arrays.asList(0.0, 1.0, 5.0)))) + .addEqualityGroup( + Distribution.create(BucketBoundaries.create(Arrays.asList(1.0, 2.0, 5.0))), + Distribution.create(BucketBoundaries.create(Arrays.asList(1.0, 2.0, 5.0)))) .addEqualityGroup(Mean.create(), Mean.create()) .addEqualityGroup(LastValue.create(), LastValue.create()) .testEquals(); diff --git a/api/src/test/java/io/opencensus/stats/BucketBoundariesTest.java b/api/src/test/java/io/opencensus/stats/BucketBoundariesTest.java index caa7e6c49a..f5b0f29d63 100644 --- a/api/src/test/java/io/opencensus/stats/BucketBoundariesTest.java +++ b/api/src/test/java/io/opencensus/stats/BucketBoundariesTest.java @@ -37,8 +37,9 @@ public class BucketBoundariesTest { @Test public void testConstructBoundaries() { List buckets = Arrays.asList(0.0, 1.0, 2.0); + List expectedBuckets = Arrays.asList(1.0, 2.0); BucketBoundaries bucketBoundaries = BucketBoundaries.create(buckets); - assertThat(bucketBoundaries.getBoundaries()).isEqualTo(buckets); + assertThat(bucketBoundaries.getBoundaries()).isEqualTo(expectedBuckets); } @Test @@ -49,6 +50,13 @@ public void testConstructBoundaries_IgnoreNegativeBounds() { assertThat(bucketBoundaries.getBoundaries()).isEqualTo(expectedBuckets); } + @Test + public void testConstructBoundaries_IgnoreZeroAndNegativeBounds() { + List buckets = Arrays.asList(-5.0, -2.0, -1.0, 0.0); + BucketBoundaries bucketBoundaries = BucketBoundaries.create(buckets); + assertThat(bucketBoundaries.getBoundaries()).isEmpty(); + } + @Test public void testBoundariesDoesNotChangeWithOriginalList() { List original = new ArrayList(); @@ -58,7 +66,7 @@ public void testBoundariesDoesNotChangeWithOriginalList() { BucketBoundaries bucketBoundaries = BucketBoundaries.create(original); original.set(2, 3.0); original.add(4.0); - List expected = Arrays.asList(0.0, 1.0, 2.0); + List expected = Arrays.asList(1.0, 2.0); assertThat(bucketBoundaries.getBoundaries()).isNotEqualTo(original); assertThat(bucketBoundaries.getBoundaries()).isEqualTo(expected); } diff --git a/api/src/test/java/io/opencensus/stats/NoopStatsTest.java b/api/src/test/java/io/opencensus/stats/NoopStatsTest.java index 4bae14a68f..d84321c1ad 100644 --- a/api/src/test/java/io/opencensus/stats/NoopStatsTest.java +++ b/api/src/test/java/io/opencensus/stats/NoopStatsTest.java @@ -109,6 +109,11 @@ public void noopStatsRecorder_PutAttachmentNullValue() { measureMap.putAttachment("key", null); } + @Test + public void noopStatsRecorder_PutNegativeValue() { + NoopStats.getNoopStatsRecorder().newMeasureMap().put(MEASURE, -5).record(tagContext); + } + // The NoopStatsRecorder should do nothing, so this test just checks that record doesn't throw an // exception. @Test diff --git a/contrib/http_util/src/test/java/io/opencensus/contrib/http/util/HttpViewConstantsTest.java b/contrib/http_util/src/test/java/io/opencensus/contrib/http/util/HttpViewConstantsTest.java index d008348ea6..5708f4a68b 100644 --- a/contrib/http_util/src/test/java/io/opencensus/contrib/http/util/HttpViewConstantsTest.java +++ b/contrib/http_util/src/test/java/io/opencensus/contrib/http/util/HttpViewConstantsTest.java @@ -38,7 +38,6 @@ public void constants() { .getBucketBoundaries() .getBoundaries()) .containsExactly( - 0.0, 1024.0, 2048.0, 4096.0, @@ -59,9 +58,9 @@ public void constants() { .getBucketBoundaries() .getBoundaries()) .containsExactly( - 0.0, 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 8.0, 10.0, 13.0, 16.0, 20.0, 25.0, 30.0, 40.0, 50.0, - 65.0, 80.0, 100.0, 130.0, 160.0, 200.0, 250.0, 300.0, 400.0, 500.0, 650.0, 800.0, - 1000.0, 2000.0, 5000.0, 10000.0, 20000.0, 50000.0, 100000.0) + 1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 8.0, 10.0, 13.0, 16.0, 20.0, 25.0, 30.0, 40.0, 50.0, 65.0, + 80.0, 100.0, 130.0, 160.0, 200.0, 250.0, 300.0, 400.0, 500.0, 650.0, 800.0, 1000.0, + 2000.0, 5000.0, 10000.0, 20000.0, 50000.0, 100000.0) .inOrder(); // Test views. diff --git a/contrib/zpages/src/test/java/io/opencensus/contrib/zpages/StatszZPageHandlerTest.java b/contrib/zpages/src/test/java/io/opencensus/contrib/zpages/StatszZPageHandlerTest.java index 81e64a6499..32fd92f01e 100644 --- a/contrib/zpages/src/test/java/io/opencensus/contrib/zpages/StatszZPageHandlerTest.java +++ b/contrib/zpages/src/test/java/io/opencensus/contrib/zpages/StatszZPageHandlerTest.java @@ -82,7 +82,7 @@ public class StatszZPageHandlerTest { 0.2, 16.3, 234.56, - Arrays.asList(0L, 1L, 1L, 2L, 1L, 0L, 0L, 0L, 0L, 0L, 0L, 0L, 0L, 0L, 0L)); + Arrays.asList(1L, 1L, 2L, 1L, 0L, 0L, 0L, 0L, 0L, 0L, 0L, 0L, 0L, 0L)); private static final AggregationData.DistributionData DISTRIBUTION_DATA_2 = AggregationData.DistributionData.create( 7.9, diff --git a/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapImpl.java b/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapImpl.java index d73efa2fd9..ed8efc80d0 100644 --- a/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapImpl.java +++ b/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapImpl.java @@ -30,6 +30,7 @@ final class MeasureMapImpl extends MeasureMap { private final StatsManager statsManager; private final MeasureMapInternal.Builder builder = MeasureMapInternal.builder(); + private volatile boolean hasUnsupportedValues; static MeasureMapImpl create(StatsManager statsManager) { return new MeasureMapImpl(statsManager); @@ -42,20 +43,18 @@ private MeasureMapImpl(StatsManager statsManager) { @Override public MeasureMapImpl put(MeasureDouble measure, double value) { if (value < 0) { - logger.log(Level.WARNING, "Dropping (" + value + "), value to record must be non-negative."); - } else { - builder.put(measure, value); + hasUnsupportedValues = true; } + builder.put(measure, value); return this; } @Override public MeasureMapImpl put(MeasureLong measure, long value) { if (value < 0) { - logger.log(Level.WARNING, "Dropping (" + value + "), value to record must be non-negative."); - } else { - builder.put(measure, value); + hasUnsupportedValues = true; } + builder.put(measure, value); return this; } @@ -73,6 +72,13 @@ public void record() { @Override public void record(TagContext tags) { + if (hasUnsupportedValues) { + // drop all the recorded values + builder.clear(); + logger.log(Level.WARNING, "Dropping values, value to record must be non-negative."); + hasUnsupportedValues = false; + return; + } statsManager.record(tags, builder.build()); } } diff --git a/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapInternal.java b/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapInternal.java index d867b3428d..266d8d84a0 100644 --- a/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapInternal.java +++ b/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapInternal.java @@ -85,6 +85,14 @@ Builder put(MeasureLong measure, long value) { return this; } + /** + * Removes all of the measurements from this {@code measurements}. The list will be empty after + * this call returns. + */ + void clear() { + measurements.clear(); + } + Builder putAttachment(String key, String value) { this.attachments.put(key, value); return this; diff --git a/impl_core/src/test/java/io/opencensus/implcore/stats/MeasureMapInternalTest.java b/impl_core/src/test/java/io/opencensus/implcore/stats/MeasureMapInternalTest.java index 19e8a6c546..239fea23ac 100644 --- a/impl_core/src/test/java/io/opencensus/implcore/stats/MeasureMapInternalTest.java +++ b/impl_core/src/test/java/io/opencensus/implcore/stats/MeasureMapInternalTest.java @@ -81,6 +81,17 @@ public void testBuilderEmpty() { assertContains(metrics); } + @Test + public void testBuilderClear() { + MeasureMapInternal.Builder builder = MeasureMapInternal.builder(); + MeasureMapInternal metrics = builder.put(M1, -44.4).build(); + assertContains(metrics, MeasurementDouble.create(M1, -44.4)); + builder.clear(); + assertContains(metrics); + MeasureMapInternal metrics1 = builder.put(M2, 66.6).build(); + assertContains(metrics1, MeasurementDouble.create(M2, 66.6)); + } + @Test public void testBuilder() { ArrayList expected = new ArrayList(10); diff --git a/impl_core/src/test/java/io/opencensus/implcore/stats/MutableAggregationTest.java b/impl_core/src/test/java/io/opencensus/implcore/stats/MutableAggregationTest.java index 59cb954e5a..6d94fcb50b 100644 --- a/impl_core/src/test/java/io/opencensus/implcore/stats/MutableAggregationTest.java +++ b/impl_core/src/test/java/io/opencensus/implcore/stats/MutableAggregationTest.java @@ -139,7 +139,7 @@ public void testAdd() { TOLERANCE); assertAggregationDataEquals( aggregations.get(4).toAggregationData(), - AggregationData.DistributionData.create(4.0, 5, -5.0, 20.0, 372, Arrays.asList(2L, 2L, 1L)), + AggregationData.DistributionData.create(4.0, 5, -5.0, 20.0, 372, Arrays.asList(4L, 1L)), TOLERANCE); assertAggregationDataEquals( aggregations.get(5).toAggregationData(), @@ -180,7 +180,6 @@ public void testAdd_DistributionWithExemplarAttachments() { // bucket, only the last one will be kept. List expected = Arrays.asList( - Exemplar.create(values.get(2), timestamps.get(2), attachmentsList.get(2)), Exemplar.create(values.get(4), timestamps.get(4), attachmentsList.get(4)), Exemplar.create(values.get(3), timestamps.get(3), attachmentsList.get(3))); assertThat(mutableDistribution.getExemplars()) @@ -256,12 +255,12 @@ public void testCombine_Distribution() { MutableDistribution combined = MutableDistribution.create(BUCKET_BOUNDARIES); combined.combine(distribution1, 1.0); // distribution1 will be combined combined.combine(distribution2, 0.6); // distribution2 will be ignored - verifyMutableDistribution(combined, 0, 2, -5, 5, 50.0, new long[] {1, 1, 0}, TOLERANCE); + verifyMutableDistribution(combined, 0, 2, -5, 5, 50.0, new long[] {2, 0}, TOLERANCE); combined.combine(distribution2, 1.0); // distribution2 will be combined - verifyMutableDistribution(combined, 7.5, 4, -5, 20, 325.0, new long[] {1, 1, 2}, TOLERANCE); + verifyMutableDistribution(combined, 7.5, 4, -5, 20, 325.0, new long[] {2, 2}, TOLERANCE); combined.combine(distribution3, 1.0); // distribution3 will be combined - verifyMutableDistribution(combined, 0, 8, -20, 20, 1500.0, new long[] {4, 1, 3}, TOLERANCE); + verifyMutableDistribution(combined, 0, 8, -20, 20, 1500.0, new long[] {5, 3}, TOLERANCE); } @Test @@ -278,7 +277,7 @@ public void mutableAggregation_ToAggregationData() { Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY, 0, - Arrays.asList(0L, 0L, 0L))); + Arrays.asList(0L, 0L))); assertThat(MutableLastValueDouble.create().toAggregationData()) .isEqualTo(LastValueDataDouble.create(Double.NaN)); assertThat(MutableLastValueLong.create().toAggregationData()) @@ -296,8 +295,6 @@ public void mutableAggregation_ToPoint() { assertThat(MutableMean.create().toPoint(TIMESTAMP)) .isEqualTo(Point.create(Value.doubleValue(0), TIMESTAMP)); - thrown.expect(IllegalArgumentException.class); - thrown.expectMessage("bucket boundary should be > 0"); assertThat(MutableDistribution.create(BUCKET_BOUNDARIES).toPoint(TIMESTAMP)) .isEqualTo( Point.create( @@ -307,11 +304,7 @@ public void mutableAggregation_ToPoint() { 0, 0, BucketOptions.explicitOptions(BUCKET_BOUNDARIES.getBoundaries()), - Arrays.asList( - Bucket.create(0), - Bucket.create(0), - Bucket.create(0), - Bucket.create(0)))), + Arrays.asList(Bucket.create(0), Bucket.create(0)))), TIMESTAMP)); } diff --git a/impl_core/src/test/java/io/opencensus/implcore/stats/RecordUtilsTest.java b/impl_core/src/test/java/io/opencensus/implcore/stats/RecordUtilsTest.java index 57fa647a1f..85a7a0526a 100644 --- a/impl_core/src/test/java/io/opencensus/implcore/stats/RecordUtilsTest.java +++ b/impl_core/src/test/java/io/opencensus/implcore/stats/RecordUtilsTest.java @@ -111,6 +111,6 @@ public void createMutableAggregation() { assertThat(mutableDistribution.getMin()).isPositiveInfinity(); assertThat(mutableDistribution.getMax()).isNegativeInfinity(); assertThat(mutableDistribution.getSumOfSquaredDeviations()).isWithin(EPSILON).of(0); - assertThat(mutableDistribution.getBucketCounts()).isEqualTo(new long[3]); + assertThat(mutableDistribution.getBucketCounts()).isEqualTo(new long[2]); } } From 4ed1e64378376295d0ff4b48361e8448986ba84b Mon Sep 17 00:00:00 2001 From: Mayur Kale Date: Tue, 30 Oct 2018 13:09:43 -0700 Subject: [PATCH 7/9] Fix reviews --- api/src/main/java/io/opencensus/stats/NoopStats.java | 1 - .../io/opencensus/implcore/stats/MeasureMapImpl.java | 2 -- .../opencensus/implcore/stats/MeasureMapInternal.java | 8 -------- .../implcore/stats/MeasureMapInternalTest.java | 11 ----------- 4 files changed, 22 deletions(-) diff --git a/api/src/main/java/io/opencensus/stats/NoopStats.java b/api/src/main/java/io/opencensus/stats/NoopStats.java index cc8d09e1f2..de6b31926d 100644 --- a/api/src/main/java/io/opencensus/stats/NoopStats.java +++ b/api/src/main/java/io/opencensus/stats/NoopStats.java @@ -154,7 +154,6 @@ public void record(TagContext tags) { if (hasUnsupportedValues) { // drop all the recorded values logger.log(Level.WARNING, "Dropping values, value to record must be non-negative."); - hasUnsupportedValues = false; } } } diff --git a/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapImpl.java b/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapImpl.java index ed8efc80d0..8bad7ef5a9 100644 --- a/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapImpl.java +++ b/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapImpl.java @@ -74,9 +74,7 @@ public void record() { public void record(TagContext tags) { if (hasUnsupportedValues) { // drop all the recorded values - builder.clear(); logger.log(Level.WARNING, "Dropping values, value to record must be non-negative."); - hasUnsupportedValues = false; return; } statsManager.record(tags, builder.build()); diff --git a/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapInternal.java b/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapInternal.java index 266d8d84a0..d867b3428d 100644 --- a/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapInternal.java +++ b/impl_core/src/main/java/io/opencensus/implcore/stats/MeasureMapInternal.java @@ -85,14 +85,6 @@ Builder put(MeasureLong measure, long value) { return this; } - /** - * Removes all of the measurements from this {@code measurements}. The list will be empty after - * this call returns. - */ - void clear() { - measurements.clear(); - } - Builder putAttachment(String key, String value) { this.attachments.put(key, value); return this; diff --git a/impl_core/src/test/java/io/opencensus/implcore/stats/MeasureMapInternalTest.java b/impl_core/src/test/java/io/opencensus/implcore/stats/MeasureMapInternalTest.java index 239fea23ac..19e8a6c546 100644 --- a/impl_core/src/test/java/io/opencensus/implcore/stats/MeasureMapInternalTest.java +++ b/impl_core/src/test/java/io/opencensus/implcore/stats/MeasureMapInternalTest.java @@ -81,17 +81,6 @@ public void testBuilderEmpty() { assertContains(metrics); } - @Test - public void testBuilderClear() { - MeasureMapInternal.Builder builder = MeasureMapInternal.builder(); - MeasureMapInternal metrics = builder.put(M1, -44.4).build(); - assertContains(metrics, MeasurementDouble.create(M1, -44.4)); - builder.clear(); - assertContains(metrics); - MeasureMapInternal metrics1 = builder.put(M2, 66.6).build(); - assertContains(metrics1, MeasurementDouble.create(M2, 66.6)); - } - @Test public void testBuilder() { ArrayList expected = new ArrayList(10); From 8e1e82786f30812c7965a4788a8ecef5f2a0fd09 Mon Sep 17 00:00:00 2001 From: Mayur Kale Date: Wed, 31 Oct 2018 13:40:59 -0700 Subject: [PATCH 8/9] Remove singleton from NoopMeasureMap --- api/src/main/java/io/opencensus/stats/NoopStats.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/api/src/main/java/io/opencensus/stats/NoopStats.java b/api/src/main/java/io/opencensus/stats/NoopStats.java index de6b31926d..1f56a34302 100644 --- a/api/src/main/java/io/opencensus/stats/NoopStats.java +++ b/api/src/main/java/io/opencensus/stats/NoopStats.java @@ -68,8 +68,8 @@ static StatsRecorder getNoopStatsRecorder() { * * @return a {@code MeasureMap} that ignores all calls to {@code MeasureMap#put}. */ - static MeasureMap getNoopMeasureMap() { - return NoopMeasureMap.INSTANCE; + static MeasureMap newNoopMeasureMap() { + return new NoopMeasureMap(); } /** @@ -118,14 +118,13 @@ private static final class NoopStatsRecorder extends StatsRecorder { @Override public MeasureMap newMeasureMap() { - return getNoopMeasureMap(); + return newNoopMeasureMap(); } } @Immutable private static final class NoopMeasureMap extends MeasureMap { private static final Logger logger = Logger.getLogger(NoopMeasureMap.class.getName()); - static final MeasureMap INSTANCE = new NoopMeasureMap(); private volatile boolean hasUnsupportedValues; @Override From 4ff5f41ef70dcea560f988946400d6d92c9a8ec8 Mon Sep 17 00:00:00 2001 From: Mayur Kale Date: Thu, 1 Nov 2018 10:28:59 -0700 Subject: [PATCH 9/9] Remove volatile variable --- api/src/main/java/io/opencensus/stats/NoopStats.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/api/src/main/java/io/opencensus/stats/NoopStats.java b/api/src/main/java/io/opencensus/stats/NoopStats.java index 1f56a34302..ee7b97a9be 100644 --- a/api/src/main/java/io/opencensus/stats/NoopStats.java +++ b/api/src/main/java/io/opencensus/stats/NoopStats.java @@ -122,10 +122,9 @@ public MeasureMap newMeasureMap() { } } - @Immutable private static final class NoopMeasureMap extends MeasureMap { private static final Logger logger = Logger.getLogger(NoopMeasureMap.class.getName()); - private volatile boolean hasUnsupportedValues; + private boolean hasUnsupportedValues; @Override public MeasureMap put(MeasureDouble measure, double value) {