ignore negative bucket bounds (#1499)

* ignore negative bucket bounds

* drop negative values in the Record API.

* Drop negative values from histograms count and sum.

* report negative value

* drop negative values in the Record API

* Add check and warning in noop implementation, Ignore 0 bucket bounds

* Fix reviews

* Remove singleton from NoopMeasureMap

* Remove volatile variable

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

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<Double> bucketBoundaries) {
     List<Double> bucketBoundariesCopy = new ArrayList<Double>(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<Double> dropNegativeBucketBounds(List<Double> 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());
   }
 
   /**

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.");
+      }
     }
   }
 

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();

api/src/test/java/io/opencensus/stats/BucketBoundariesTest.java

@@ -37,8 +37,24 @@ public class BucketBoundariesTest {
   @Test
   public void testConstructBoundaries() {
     List<Double> buckets = Arrays.asList(0.0, 1.0, 2.0);
+    List<Double> 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<Double> buckets = Arrays.asList(-5.0, -1.0, 1.0, 2.0);
+    List<Double> expectedBuckets = Arrays.asList(1.0, 2.0);
+    BucketBoundaries bucketBoundaries = BucketBoundaries.create(buckets);
+    assertThat(bucketBoundaries.getBoundaries()).isEqualTo(expectedBuckets);
+  }
+
+  @Test
+  public void testConstructBoundaries_IgnoreZeroAndNegativeBounds() {
+    List<Double> 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<Double> expected = Arrays.asList(0.0, 1.0, 2.0);
+    List<Double> expected = Arrays.asList(1.0, 2.0);
     assertThat(bucketBoundaries.getBoundaries()).isNotEqualTo(original);
     assertThat(bucketBoundaries.getBoundaries()).isEqualTo(expected);
   }

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

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.

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,

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());
   }
 }

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(

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<Exemplar> expected =
         Arrays.<Exemplar>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));
   }
 

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]);
   }
 }

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<Exemplar> 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() {