diff --git a/CHANGELOG.md b/CHANGELOG.md index b426ab2278..230e29c422 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +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). +- The histogram bucket boundaries (`BucketBoundaries`) and values (`Count` and `Sum`) are no longer + 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/api/src/main/java/io/opencensus/stats/BucketBoundaries.java b/api/src/main/java/io/opencensus/stats/BucketBoundaries.java index 61e21e6cc5..258dbfd63f 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,43 @@ 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 (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) { + if (value == 0) { + zeroBucketBounds++; + } else { + negativeBucketBounds++; + } + } else { + break; + } + } + + if (negativeBucketBounds > 0) { + logger.log( + Level.WARNING, + "Dropping " + + negativeBucketBounds + + " negative bucket boundaries, the values must be strictly > 0."); + } + 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..ee7b97a9be 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; @@ -66,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(); } /** @@ -116,21 +118,27 @@ private static final class NoopStatsRecorder extends StatsRecorder { @Override public MeasureMap newMeasureMap() { - return getNoopMeasureMap(); + return newNoopMeasureMap(); } } - @Immutable private static final class NoopMeasureMap extends MeasureMap { - static final MeasureMap INSTANCE = new NoopMeasureMap(); + private static final Logger logger = Logger.getLogger(NoopMeasureMap.class.getName()); + private 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 +148,11 @@ 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."); + } } } 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 36f2edb473..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,24 @@ 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 + 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 testConstructBoundaries_IgnoreZeroAndNegativeBounds() { + List buckets = Arrays.asList(-5.0, -2.0, -1.0, 0.0); + BucketBoundaries bucketBoundaries = BucketBoundaries.create(buckets); + assertThat(bucketBoundaries.getBoundaries()).isEmpty(); } @Test @@ -50,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 ee51796c34..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 @@ -21,11 +21,16 @@ 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(); + private volatile boolean hasUnsupportedValues; static MeasureMapImpl create(StatsManager statsManager) { return new MeasureMapImpl(statsManager); @@ -37,12 +42,18 @@ private MeasureMapImpl(StatsManager statsManager) { @Override public MeasureMapImpl put(MeasureDouble measure, double value) { + if (value < 0) { + hasUnsupportedValues = true; + } builder.put(measure, value); return this; } @Override public MeasureMapImpl put(MeasureLong measure, long value) { + if (value < 0) { + hasUnsupportedValues = true; + } builder.put(measure, value); return this; } @@ -61,6 +72,11 @@ public void record() { @Override 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."); + return; + } statsManager.record(tags, builder.build()); } } 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..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 @@ -429,8 +429,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..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,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(4L, 1L)), TOLERANCE); assertAggregationDataEquals( aggregations.get(5).toAggregationData(), @@ -181,8 +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))); assertThat(mutableDistribution.getExemplars()) @@ -258,13 +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[] {0, 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[] {0, 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[] {2, 2, 1, 3}, TOLERANCE); + verifyMutableDistribution(combined, 0, 8, -20, 20, 1500.0, new long[] {5, 3}, TOLERANCE); } @Test @@ -281,7 +277,7 @@ public void mutableAggregation_ToAggregationData() { Double.POSITIVE_INFINITY, Double.NEGATIVE_INFINITY, 0, - Arrays.asList(0L, 0L, 0L, 0L))); + Arrays.asList(0L, 0L))); assertThat(MutableLastValueDouble.create().toAggregationData()) .isEqualTo(LastValueDataDouble.create(Double.NaN)); assertThat(MutableLastValueLong.create().toAggregationData()) @@ -299,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( @@ -310,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 1e22a7a1e8..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[4]); + assertThat(mutableDistribution.getBucketCounts()).isEqualTo(new long[2]); } } 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..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 @@ -170,8 +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"))); assertThat(distributionData.getExemplars()).containsExactlyElementsIn(expected).inOrder(); @@ -209,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)); }