From d78f4a5e6c8e6eefeb4c68de6117f7e4a8b4c2b3 Mon Sep 17 00:00:00 2001 From: Bartlomiej Obecny Date: Fri, 17 Jul 2020 17:41:11 +0200 Subject: [PATCH 1/3] chore: refactoring aggregators so that it correctly supports kind and toPoint type --- .../export/aggregators/MinMaxLastSumCount.ts | 5 ++-- .../src/export/aggregators/Sum.ts | 5 ++-- .../src/export/aggregators/histogram.ts | 5 ++-- .../opentelemetry-metrics/src/export/types.ts | 18 ++++++++++--- .../test/export/aggregators/histogram.test.ts | 25 +++++++++---------- 5 files changed, 36 insertions(+), 22 deletions(-) diff --git a/packages/opentelemetry-metrics/src/export/aggregators/MinMaxLastSumCount.ts b/packages/opentelemetry-metrics/src/export/aggregators/MinMaxLastSumCount.ts index 7f5a9df431..36dba451b5 100644 --- a/packages/opentelemetry-metrics/src/export/aggregators/MinMaxLastSumCount.ts +++ b/packages/opentelemetry-metrics/src/export/aggregators/MinMaxLastSumCount.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { Aggregator, Point } from '../types'; +import { Aggregator, AggregatorKind, Point } from '../types'; import { HrTime } from '@opentelemetry/api'; import { hrTime } from '@opentelemetry/core'; import { Distribution } from '../types'; @@ -23,6 +23,7 @@ import { Distribution } from '../types'; * Basic aggregator keeping all raw values (events, sum, max, last and min). */ export class MinMaxLastSumCountAggregator implements Aggregator { + public kind = AggregatorKind.MIN_MAX_LAST_SUM_COUNT; private _distribution: Distribution; private _lastUpdateTime: HrTime = [0, 0]; @@ -45,7 +46,7 @@ export class MinMaxLastSumCountAggregator implements Aggregator { this._lastUpdateTime = hrTime(); } - toPoint(): Point { + toPoint(): Point { return { value: this._distribution, timestamp: this._lastUpdateTime, diff --git a/packages/opentelemetry-metrics/src/export/aggregators/Sum.ts b/packages/opentelemetry-metrics/src/export/aggregators/Sum.ts index 0475320e5c..4e1f101ae1 100644 --- a/packages/opentelemetry-metrics/src/export/aggregators/Sum.ts +++ b/packages/opentelemetry-metrics/src/export/aggregators/Sum.ts @@ -14,12 +14,13 @@ * limitations under the License. */ -import { Aggregator, Point } from '../types'; +import { Aggregator, AggregatorKind, Point, Sum } from '../types'; import { HrTime } from '@opentelemetry/api'; import { hrTime } from '@opentelemetry/core'; /** Basic aggregator which calculates a Sum from individual measurements. */ export class SumAggregator implements Aggregator { + public kind = AggregatorKind.SUM; private _current: number = 0; private _lastUpdateTime: HrTime = [0, 0]; @@ -28,7 +29,7 @@ export class SumAggregator implements Aggregator { this._lastUpdateTime = hrTime(); } - toPoint(): Point { + toPoint(): Point { return { value: this._current, timestamp: this._lastUpdateTime, diff --git a/packages/opentelemetry-metrics/src/export/aggregators/histogram.ts b/packages/opentelemetry-metrics/src/export/aggregators/histogram.ts index 655afd8ff7..5b2bad00c0 100644 --- a/packages/opentelemetry-metrics/src/export/aggregators/histogram.ts +++ b/packages/opentelemetry-metrics/src/export/aggregators/histogram.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { Aggregator, Point, Histogram } from '../types'; +import { Aggregator, Point, Histogram, AggregatorKind } from '../types'; import { HrTime } from '@opentelemetry/api'; import { hrTime } from '@opentelemetry/core'; @@ -23,6 +23,7 @@ import { hrTime } from '@opentelemetry/core'; * and provides the total sum and count of all observations. */ export class HistogramAggregator implements Aggregator { + public kind = AggregatorKind.HISTOGRAM; private _lastCheckpoint: Histogram; private _currentCheckpoint: Histogram; private _lastCheckpointTime: HrTime; @@ -61,7 +62,7 @@ export class HistogramAggregator implements Aggregator { this._currentCheckpoint = this._newEmptyCheckpoint(); } - toPoint(): Point { + toPoint(): Point { return { value: this._lastCheckpoint, timestamp: this._lastCheckpointTime, diff --git a/packages/opentelemetry-metrics/src/export/types.ts b/packages/opentelemetry-metrics/src/export/types.ts index 66c1262e37..a946311785 100644 --- a/packages/opentelemetry-metrics/src/export/types.ts +++ b/packages/opentelemetry-metrics/src/export/types.ts @@ -42,6 +42,15 @@ export interface Distribution { sum: number; } +export enum AggregatorKind { + SUM = ' SUM', + MIN_MAX_LAST_SUM_COUNT = 'MIN_MAX_LAST_SUM_COUNT', + DISTRIBUTION = 'DISTRIBUTION', + HISTOGRAM = 'HISTOGRAM', +} + +export type AggregatorTypes = Sum | LastValue | Distribution | Histogram; + export interface Histogram { /** * Buckets are implemented using two different array: @@ -104,14 +113,17 @@ export interface MetricExporter { * aggregated values and taking a snapshot of these values upon export. */ export interface Aggregator { + /** Kind of aggregator. */ + kind: AggregatorKind; + /** Updates the current with the new value. */ update(value: number): void; /** Returns snapshot of the current point (value with timestamp). */ - toPoint(): Point; + toPoint(): Point; } -export interface Point { - value: Sum | LastValue | Distribution | Histogram; +export interface Point { + value: T; timestamp: HrTime; } diff --git a/packages/opentelemetry-metrics/test/export/aggregators/histogram.test.ts b/packages/opentelemetry-metrics/test/export/aggregators/histogram.test.ts index 45f6fe80f6..3910823c5d 100644 --- a/packages/opentelemetry-metrics/test/export/aggregators/histogram.test.ts +++ b/packages/opentelemetry-metrics/test/export/aggregators/histogram.test.ts @@ -16,7 +16,6 @@ import * as assert from 'assert'; import { HistogramAggregator } from '../../../src/export/aggregators'; -import { Histogram } from '../../../src'; describe('HistogramAggregator', () => { describe('constructor()', () => { @@ -28,7 +27,7 @@ describe('HistogramAggregator', () => { it('should sort boundaries', () => { const aggregator = new HistogramAggregator([500, 300, 700]); - const point = aggregator.toPoint().value as Histogram; + const point = aggregator.toPoint().value; assert.deepEqual(point.buckets.boundaries, [300, 500, 700]); }); @@ -43,7 +42,7 @@ describe('HistogramAggregator', () => { it('should not update checkpoint', () => { const aggregator = new HistogramAggregator([100, 200]); aggregator.update(150); - const point = aggregator.toPoint().value as Histogram; + const point = aggregator.toPoint().value; assert.equal(point.count, 0); assert.equal(point.sum, 0); }); @@ -52,7 +51,7 @@ describe('HistogramAggregator', () => { const aggregator = new HistogramAggregator([100, 200]); aggregator.update(150); aggregator.reset(); - const point = aggregator.toPoint().value as Histogram; + const point = aggregator.toPoint().value; assert.equal(point.count, 1); assert.equal(point.sum, 150); assert.equal(point.buckets.counts[0], 0); @@ -64,7 +63,7 @@ describe('HistogramAggregator', () => { const aggregator = new HistogramAggregator([100, 200]); aggregator.update(50); aggregator.reset(); - const point = aggregator.toPoint().value as Histogram; + const point = aggregator.toPoint().value; assert.equal(point.count, 1); assert.equal(point.sum, 50); assert.equal(point.buckets.counts[0], 1); @@ -76,7 +75,7 @@ describe('HistogramAggregator', () => { const aggregator = new HistogramAggregator([100, 200]); aggregator.update(250); aggregator.reset(); - const point = aggregator.toPoint().value as Histogram; + const point = aggregator.toPoint().value; assert.equal(point.count, 1); assert.equal(point.sum, 250); assert.equal(point.buckets.counts[0], 0); @@ -88,11 +87,11 @@ describe('HistogramAggregator', () => { describe('.count', () => { it('should return last checkpoint count', () => { const aggregator = new HistogramAggregator([100]); - let point = aggregator.toPoint().value as Histogram; + let point = aggregator.toPoint().value; assert.equal(point.count, point.count); aggregator.update(10); aggregator.reset(); - point = aggregator.toPoint().value as Histogram; + point = aggregator.toPoint().value; assert.equal(point.count, 1); assert.equal(point.count, point.count); }); @@ -101,11 +100,11 @@ describe('HistogramAggregator', () => { describe('.sum', () => { it('should return last checkpoint sum', () => { const aggregator = new HistogramAggregator([100]); - let point = aggregator.toPoint().value as Histogram; + let point = aggregator.toPoint().value; assert.equal(point.sum, point.sum); aggregator.update(10); aggregator.reset(); - point = aggregator.toPoint().value as Histogram; + point = aggregator.toPoint().value; assert.equal(point.sum, 10); }); }); @@ -113,7 +112,7 @@ describe('HistogramAggregator', () => { describe('.reset()', () => { it('should create a empty checkoint by default', () => { const aggregator = new HistogramAggregator([100]); - const point = aggregator.toPoint().value as Histogram; + const point = aggregator.toPoint().value; assert.deepEqual(point.buckets.boundaries, [100]); assert(point.buckets.counts.every(count => count === 0)); // should contains one bucket for each boundary + one for values outside of the largest boundary @@ -127,7 +126,7 @@ describe('HistogramAggregator', () => { const aggregator = new HistogramAggregator([100]); aggregator.update(10); aggregator.reset(); - const point = aggregator.toPoint().value as Histogram; + const point = aggregator.toPoint().value; assert.equal(point.count, 1); assert.equal(point.sum, 10); assert.deepEqual(point.buckets.boundaries, [100]); @@ -139,7 +138,7 @@ describe('HistogramAggregator', () => { describe('.toPoint()', () => { it('should return default checkpoint', () => { const aggregator = new HistogramAggregator([100]); - const point = aggregator.toPoint().value as Histogram; + const point = aggregator.toPoint().value; assert.deepEqual(aggregator.toPoint().value, point); assert(aggregator.toPoint().timestamp.every(nbr => nbr > 0)); }); From 937785b507fa8de814b00aff01cf15a254d595c0 Mon Sep 17 00:00:00 2001 From: Bartlomiej Obecny Date: Fri, 17 Jul 2020 17:48:41 +0200 Subject: [PATCH 2/3] chore: removing unneeded cast from tests --- .../opentelemetry-metrics/test/Meter.test.ts | 77 ++++++++----------- 1 file changed, 32 insertions(+), 45 deletions(-) diff --git a/packages/opentelemetry-metrics/test/Meter.test.ts b/packages/opentelemetry-metrics/test/Meter.test.ts index eb6f3d3d0a..019b43195c 100644 --- a/packages/opentelemetry-metrics/test/Meter.test.ts +++ b/packages/opentelemetry-metrics/test/Meter.test.ts @@ -21,7 +21,6 @@ import { Metric, CounterMetric, MetricKind, - Sum, MeterProvider, ValueRecorderMetric, ValueObserverMetric, @@ -566,16 +565,13 @@ describe('Meter', () => { await meter.collect(); const [record1] = meter.getBatcher().checkPointSet(); - assert.deepStrictEqual( - record1.aggregator.toPoint().value as Distribution, - { - count: 0, - last: 0, - max: -Infinity, - min: Infinity, - sum: 0, - } - ); + assert.deepStrictEqual(record1.aggregator.toPoint().value, { + count: 0, + last: 0, + max: -Infinity, + min: Infinity, + sum: 0, + }); }); it('should not set the instrument data when disabled', async () => { @@ -587,16 +583,13 @@ describe('Meter', () => { await meter.collect(); const [record1] = meter.getBatcher().checkPointSet(); - assert.deepStrictEqual( - record1.aggregator.toPoint().value as Distribution, - { - count: 0, - last: 0, - max: -Infinity, - min: Infinity, - sum: 0, - } - ); + assert.deepStrictEqual(record1.aggregator.toPoint().value, { + count: 0, + last: 0, + max: -Infinity, + min: Infinity, + sum: 0, + }); }); it( @@ -612,16 +605,13 @@ describe('Meter', () => { await meter.collect(); const [record1] = meter.getBatcher().checkPointSet(); - assert.deepStrictEqual( - record1.aggregator.toPoint().value as Distribution, - { - count: 2, - last: 50, - max: 50, - min: -10, - sum: 40, - } - ); + assert.deepStrictEqual(record1.aggregator.toPoint().value, { + count: 2, + last: 50, + max: 50, + min: -10, + sum: 40, + }); assert.ok( hrTimeToNanoseconds(record1.aggregator.toPoint().timestamp) > hrTimeToNanoseconds(performanceTimeOrigin) @@ -639,16 +629,13 @@ describe('Meter', () => { boundValueRecorder2.record(100); await meter.collect(); const [record1] = meter.getBatcher().checkPointSet(); - assert.deepStrictEqual( - record1.aggregator.toPoint().value as Distribution, - { - count: 2, - last: 100, - max: 100, - min: 10, - sum: 110, - } - ); + assert.deepStrictEqual(record1.aggregator.toPoint().value, { + count: 2, + last: 100, + max: 100, + min: 10, + sum: 110, + }); assert.strictEqual(boundValueRecorder1, boundValueRecorder2); }); }); @@ -1234,7 +1221,7 @@ describe('Meter', () => { const value = cpuUsageMetric .bind({ foo: 'bar' }) .getAggregator() - .toPoint().value as Distribution; + .toPoint().value; assert.deepStrictEqual(value, { count: 0, @@ -1294,7 +1281,7 @@ describe('Meter', () => { valueType: api.ValueType.DOUBLE, }); assert.strictEqual(record[0].labels, labels); - const value = record[0].aggregator.toPoint().value as Sum; + const value = record[0].aggregator.toPoint().value; assert.strictEqual(value, 10.45); }); @@ -1320,7 +1307,7 @@ describe('Meter', () => { valueType: api.ValueType.INT, }); assert.strictEqual(record[0].labels, labels); - const value = record[0].aggregator.toPoint().value as Sum; + const value = record[0].aggregator.toPoint().value; assert.strictEqual(value, 10); }); }); @@ -1351,7 +1338,7 @@ function ensureMetric( value?: Distribution ) { assert.ok(metric.aggregator instanceof MinMaxLastSumCountAggregator); - const distribution = metric.aggregator.toPoint().value as Distribution; + const distribution = metric.aggregator.toPoint().value; if (value) { assert.deepStrictEqual(distribution, value); } else { From b5aa987fb1a92f430ad8bb2ce87180c6a69a2f37 Mon Sep 17 00:00:00 2001 From: Bartlomiej Obecny Date: Fri, 17 Jul 2020 18:09:40 +0200 Subject: [PATCH 3/3] chore: fixing failing tests --- .../test/prometheus.test.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts b/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts index ba499de3f5..12acad6669 100644 --- a/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts +++ b/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts @@ -21,6 +21,7 @@ import { Meter, MeterProvider, Point, + Sum, } from '@opentelemetry/metrics'; import * as assert from 'assert'; import * as http from 'http'; @@ -30,10 +31,10 @@ const mockedHrTime: HrTime = [1586347902211, 0]; const mockedTimeMS = 1586347902211000; describe('PrometheusExporter', () => { - let toPoint: () => Point; + let toPoint: () => Point; before(() => { toPoint = SumAggregator.prototype.toPoint; - SumAggregator.prototype.toPoint = function (): Point { + SumAggregator.prototype.toPoint = function (): Point { const point = toPoint.apply(this); point.timestamp = mockedHrTime; return point;