Drops negative bucket bounds and negative values in record API (#175)

#172
* Ignores negative bucket bounds
* Drops negative values in record API

Author: isaikevych

packages/opencensus-core/src/stats/stats.ts

@@ -14,6 +14,8 @@
  * limitations under the License.
  */
 
+import * as defaultLogger from '../common/console-logger';
+import * as loggerTypes from '../common/types';
 import {StatsEventListener} from '../exporters/types';
 
 import {AggregationType, Measure, Measurement, MeasureType, MeasureUnit, View} from './types';
@@ -24,8 +26,16 @@ export class Stats {
   private statsEventListeners: StatsEventListener[] = [];
   /** A map of Measures (name) to their corresponding Views */
   private registeredViews: {[key: string]: View[]} = {};
+  /** An object to log information to */
+  private logger: loggerTypes.Logger;
 
-  constructor() {}
+  /**
+   * Creates stats
+   * @param logger
+   */
+  constructor(logger = defaultLogger) {
+    this.logger = logger.logger();
+  }
 
   /**
    * Registers a view to listen to new measurements in its measure. Prefer using
@@ -110,6 +120,12 @@ export class Stats {
    */
   record(...measurements: Measurement[]) {
     for (const measurement of measurements) {
+      if (measurement.value < 0) {
+        this.logger.warn(`Dropping value ${
+            measurement.value}, value to record must be non-negative.`);
+        break;
+      }
+
       const views = this.registeredViews[measurement.measure.name];
       if (!views) {
         break;

packages/opencensus-core/src/stats/view.ts

@@ -14,6 +14,9 @@
  * limitations under the License.
  */
 
+import * as defaultLogger from '../common/console-logger';
+import * as loggerTypes from '../common/types';
+
 import {Recorder} from './recorder';
 import {AggregationData, AggregationMetadata, AggregationType, Bucket, CountData, DistributionData, LastValueData, Measure, Measurement, MeasureType, SumData, Tags, View} from './types';
 
@@ -59,6 +62,8 @@ export class BaseView implements View {
   endTime: number;
   /** true if the view was registered */
   registered = false;
+  /** An object to log information to */
+  private logger: loggerTypes.Logger;
 
   /**
    * Creates a new View instance. This constructor is used by Stats. User should
@@ -70,13 +75,16 @@ export class BaseView implements View {
    * @param description The view description
    * @param bucketBoundaries The view bucket boundaries for a distribution
    * aggregation type
+   * @param logger
    */
   constructor(
       name: string, measure: Measure, aggregation: AggregationType,
-      tagsKeys: string[], description: string, bucketBoundaries?: number[]) {
+      tagsKeys: string[], description: string, bucketBoundaries?: number[],
+      logger = defaultLogger) {
     if (aggregation === AggregationType.DISTRIBUTION && !bucketBoundaries) {
       throw new Error('No bucketBoundaries specified');
     }
+    this.logger = logger.logger();
     this.name = name;
     this.description = description;
     this.measure = measure;
@@ -115,6 +123,7 @@ export class BaseView implements View {
     if (!this.rows[encodedTags]) {
       this.rows[encodedTags] = this.createAggregationData(measurement.tags);
     }
+
     Recorder.addMeasurement(this.rows[encodedTags], measurement);
   }
 
@@ -181,15 +190,57 @@ export class BaseView implements View {
    * @param bucketBoundaries a list with the bucket boundaries
    */
   private createBuckets(bucketBoundaries: number[]): Bucket[] {
-    return bucketBoundaries.map((boundary, boundaryIndex) => {
-      return {
-        count: 0,
-        lowBoundary: boundaryIndex ? boundary : -Infinity,
-        highBoundary: (boundaryIndex === bucketBoundaries.length - 1) ?
-            Infinity :
-            bucketBoundaries[boundaryIndex + 1]
-      };
-    });
+    let negative = 0;
+    const result = bucketBoundaries.reduce((accumulator, boundary, index) => {
+      if (boundary >= 0) {
+        const nextBoundary = bucketBoundaries[index + 1];
+        this.validateBoundary(boundary, nextBoundary);
+        const len = bucketBoundaries.length - negative;
+        const position = index - negative;
+        const bucket = this.createBucket(boundary, nextBoundary, position, len);
+        accumulator.push(bucket);
+      } else {
+        negative++;
+      }
+      return accumulator;
+    }, []);
+    if (negative) {
+      this.logger.warn(`Dropping ${
+          negative} negative bucket boundaries, the values must be strictly > 0.`);
+    }
+    return result;
+  }
+
+  /**
+   * Checks boundaries order and duplicates
+   * @param current Boundary
+   * @param next Next boundary
+   */
+  private validateBoundary(current: number, next: number) {
+    if (next) {
+      if (current > next) {
+        this.logger.error('Bucket boundaries not sorted.');
+      }
+      if (current === next) {
+        this.logger.error('Bucket boundaries not unique.');
+      }
+    }
+  }
+
+  /**
+   * Creates empty bucket boundary.
+   * @param current Current boundary
+   * @param next Next boundary
+   * @param position Index of boundary
+   * @param max Maximum length of boundaries
+   */
+  private createBucket(
+      current: number, next: number, position: number, max: number): Bucket {
+    return {
+      count: 0,
+      lowBoundary: position ? current : -Infinity,
+      highBoundary: (position === max - 1) ? Infinity : next
+    };
   }
 
   /**

packages/opencensus-core/test/test-stats.ts

@@ -135,49 +135,57 @@ describe('Stats', () => {
   describe('record()', () => {
     let measure: Measure;
     const testExporter = new TestExporter();
-
+    let view;
+    let aggregationData: LastValueData;
     before(() => {
       measure = stats.createMeasureInt64(measureName, measureUnit);
     });
 
     beforeEach(() => {
       testExporter.clean();
+      stats.registerExporter(testExporter);
+      view = stats.createView(
+          viewName, measure, AggregationType.LAST_VALUE, tagKeys, description);
     });
 
     it('should record a single measurement', () => {
-      stats.registerExporter(testExporter);
-      const view = stats.createView(
-          viewName, measure, AggregationType.LAST_VALUE, tagKeys, description);
       const measurement = {measure, tags, value: 1};
-
       assert.strictEqual(testExporter.recordedMeasurements.length, 0);
-
       stats.record(measurement);
-      const aggregationData =
-          testExporter.registeredViews[0].getSnapshot(tags) as LastValueData;
-
       assert.strictEqual(testExporter.recordedMeasurements.length, 1);
       assert.deepEqual(testExporter.recordedMeasurements[0], measurement);
+      aggregationData =
+          testExporter.registeredViews[0].getSnapshot(tags) as LastValueData;
       assert.strictEqual(aggregationData.value, measurement.value);
     });
 
-    it('should record multiple measurements', () => {
+    it('should not record a single negative measurement', () => {
       stats.registerExporter(testExporter);
-      const view = stats.createView(
-          viewName, measure, AggregationType.LAST_VALUE, tagKeys, description);
+      const measurement = {measure, tags, value: -1};
+      stats.record(measurement);
+      assert.strictEqual(testExporter.recordedMeasurements.length, 0);
+    });
+
+    it('should record multiple measurements', () => {
       const measurement1 = {measure, tags, value: 1};
       const measurement2 = {measure, tags, value: 1};
-
       assert.strictEqual(testExporter.recordedMeasurements.length, 0);
-
       stats.record(measurement1, measurement2);
-      const aggregationData =
-          testExporter.registeredViews[0].getSnapshot(tags) as LastValueData;
-
       assert.strictEqual(testExporter.recordedMeasurements.length, 2);
       assert.deepEqual(testExporter.recordedMeasurements[0], measurement1);
       assert.deepEqual(testExporter.recordedMeasurements[1], measurement2);
+      aggregationData =
+          testExporter.registeredViews[0].getSnapshot(tags) as LastValueData;
       assert.strictEqual(aggregationData.value, measurement2.value);
     });
+
+    it('should stop record on negative value of multiple measurement', () => {
+      const measurments = [
+        {measure, tags, value: 1}, {measure, tags, value: -1},
+        {measure, tags, value: 1}
+      ];
+      stats.record(...measurments);
+      assert.equal(testExporter.recordedMeasurements.length, 1);
+    });
   });
 });

packages/opencensus-core/test/test-view.ts

@@ -78,7 +78,6 @@ function assertView(
     aggregationType: AggregationType) {
   assert.strictEqual(view.aggregation, aggregationType);
   const aggregationData = view.getSnapshot(measurement.tags);
-
   switch (aggregationData.type) {
     case AggregationType.SUM:
       const acc = recordedValues.reduce((acc, cur) => acc + cur);
@@ -125,19 +124,18 @@ describe('BaseView', () => {
   });
 
   describe('recordMeasurement()', () => {
-    const measurementValues = [1.1, -2.3, 3.2, -4.3, 5.2];
+    const measurementValues = [1.1, 2.3, 3.2, 4.3, 5.2];
     const bucketBoundaries = [0, 2, 4, 6];
     const emptyAggregation = {};
+    const tags: Tags = {testKey1: 'testValue', testKey2: 'testValue'};
 
     for (const aggregationTestCase of aggregationTestCases) {
-      const tags: Tags = {testKey1: 'testValue', testKey2: 'testValue'};
-      const view = new BaseView(
-          'test/view/name', measure, aggregationTestCase.aggregationType,
-          ['testKey1', 'testKey2'], 'description test', bucketBoundaries);
-
       it(`should record measurements on a View with ${
              aggregationTestCase.description} Aggregation Data type`,
          () => {
+           const view = new BaseView(
+               'test/view/name', measure, aggregationTestCase.aggregationType,
+               ['testKey1', 'testKey2'], 'description test', bucketBoundaries);
            const recordedValues = [];
            for (const value of measurementValues) {
              recordedValues.push(value);
@@ -150,6 +148,31 @@ describe('BaseView', () => {
          });
     }
 
+    it('should ignore negative bucket bounds', () => {
+      const negativeBucketBoundaries = [-Infinity, -4, -2, 0, 2, 4, 6];
+      const view = new BaseView(
+          'test/view/name', measure, AggregationType.DISTRIBUTION,
+          ['testKey1', 'testKey2'], 'description test',
+          negativeBucketBoundaries);
+      const recordedValues = [];
+      for (const value of measurementValues) {
+        recordedValues.push(value);
+        const measurement = {measure, tags, value};
+        view.recordMeasurement(measurement);
+      }
+      const data = view.getSnapshot(tags) as DistributionData;
+      const expectedBuckets = [
+        {count: 1, lowBoundary: -Infinity, highBoundary: 2},
+        {count: 2, lowBoundary: 2, highBoundary: 4},
+        {count: 2, lowBoundary: 4, highBoundary: 6},
+        {count: 0, lowBoundary: 6, highBoundary: Infinity}
+      ];
+      assert.equal(data.buckets.length, expectedBuckets.length);
+      expectedBuckets.forEach((bucket, index) => {
+        assert.deepStrictEqual(data.buckets[index], bucket);
+      });
+    });
+
     const view = new BaseView(
         'test/view/name', measure, AggregationType.LAST_VALUE,
         ['testKey1', 'testKey2'], 'description test');