From 901c965534d403d8db054d923b76ba3675a8b140 Mon Sep 17 00:00:00 2001 From: legendecas Date: Thu, 9 Jul 2020 18:10:06 +0800 Subject: [PATCH 1/3] feat: remove HistogramAggregator.reset --- .../test/common/transformMetrics.test.ts | 2 -- .../{histogram.ts => Histogram.ts} | 28 +++++++------------ .../src/export/aggregators/index.ts | 2 +- .../{histogram.test.ts => Histogram.test.ts} | 7 ----- 4 files changed, 11 insertions(+), 28 deletions(-) rename packages/opentelemetry-metrics/src/export/aggregators/{histogram.ts => Histogram.ts} (71%) rename packages/opentelemetry-metrics/test/export/aggregators/{histogram.test.ts => Histogram.test.ts} (96%) diff --git a/packages/opentelemetry-exporter-collector/test/common/transformMetrics.test.ts b/packages/opentelemetry-exporter-collector/test/common/transformMetrics.test.ts index 6e4563e6b6..f017075e0c 100644 --- a/packages/opentelemetry-exporter-collector/test/common/transformMetrics.test.ts +++ b/packages/opentelemetry-exporter-collector/test/common/transformMetrics.test.ts @@ -29,7 +29,6 @@ import { ensureValueRecorderIsCorrect, mockValueRecorder, } from '../helper'; -import { HistogramAggregator } from '@opentelemetry/metrics'; import { hrTimeToNanoseconds } from '@opentelemetry/core'; describe('transformMetrics', () => { describe('toCollectorMetric', () => { @@ -46,7 +45,6 @@ describe('transformMetrics', () => { ); mockHistogram.aggregator.update(7); mockHistogram.aggregator.update(14); - (mockHistogram.aggregator as HistogramAggregator).reset(); ensureHistogramIsCorrect( transform.toCollectorMetric(mockHistogram, 1592602232694000000), hrTimeToNanoseconds(mockHistogram.aggregator.toPoint().timestamp) diff --git a/packages/opentelemetry-metrics/src/export/aggregators/histogram.ts b/packages/opentelemetry-metrics/src/export/aggregators/Histogram.ts similarity index 71% rename from packages/opentelemetry-metrics/src/export/aggregators/histogram.ts rename to packages/opentelemetry-metrics/src/export/aggregators/Histogram.ts index 655afd8ff7..11cd33ee8f 100644 --- a/packages/opentelemetry-metrics/src/export/aggregators/histogram.ts +++ b/packages/opentelemetry-metrics/src/export/aggregators/Histogram.ts @@ -23,9 +23,8 @@ import { hrTime } from '@opentelemetry/core'; * and provides the total sum and count of all observations. */ export class HistogramAggregator implements Aggregator { - private _lastCheckpoint: Histogram; - private _currentCheckpoint: Histogram; - private _lastCheckpointTime: HrTime; + private _current: Histogram; + private _lastUpdateTime: HrTime; private readonly _boundaries: number[]; constructor(boundaries: number[]) { @@ -35,36 +34,29 @@ export class HistogramAggregator implements Aggregator { // we need to an ordered set to be able to correctly compute count for each // boundary since we'll iterate on each in order. this._boundaries = boundaries.sort(); - this._lastCheckpoint = this._newEmptyCheckpoint(); - this._lastCheckpointTime = hrTime(); - this._currentCheckpoint = this._newEmptyCheckpoint(); + this._current = this._newEmptyCheckpoint(); + this._lastUpdateTime = hrTime(); } update(value: number): void { - this._currentCheckpoint.count += 1; - this._currentCheckpoint.sum += value; + this._current.count += 1; + this._current.sum += value; for (let i = 0; i < this._boundaries.length; i++) { if (value < this._boundaries[i]) { - this._currentCheckpoint.buckets.counts[i] += 1; + this._current.buckets.counts[i] += 1; return; } } // value is above all observed boundaries - this._currentCheckpoint.buckets.counts[this._boundaries.length] += 1; - } - - reset(): void { - this._lastCheckpointTime = hrTime(); - this._lastCheckpoint = this._currentCheckpoint; - this._currentCheckpoint = this._newEmptyCheckpoint(); + this._current.buckets.counts[this._boundaries.length] += 1; } toPoint(): Point { return { - value: this._lastCheckpoint, - timestamp: this._lastCheckpointTime, + value: this._current, + timestamp: this._lastUpdateTime, }; } diff --git a/packages/opentelemetry-metrics/src/export/aggregators/index.ts b/packages/opentelemetry-metrics/src/export/aggregators/index.ts index 93001f73d7..9ff9f904ea 100644 --- a/packages/opentelemetry-metrics/src/export/aggregators/index.ts +++ b/packages/opentelemetry-metrics/src/export/aggregators/index.ts @@ -14,6 +14,6 @@ * limitations under the License. */ -export * from './histogram'; +export * from './Histogram'; export * from './MinMaxLastSumCount'; export * from './Sum'; diff --git a/packages/opentelemetry-metrics/test/export/aggregators/histogram.test.ts b/packages/opentelemetry-metrics/test/export/aggregators/Histogram.test.ts similarity index 96% rename from packages/opentelemetry-metrics/test/export/aggregators/histogram.test.ts rename to packages/opentelemetry-metrics/test/export/aggregators/Histogram.test.ts index 45f6fe80f6..19e0283be3 100644 --- a/packages/opentelemetry-metrics/test/export/aggregators/histogram.test.ts +++ b/packages/opentelemetry-metrics/test/export/aggregators/Histogram.test.ts @@ -51,7 +51,6 @@ describe('HistogramAggregator', () => { it('should update the second bucket', () => { const aggregator = new HistogramAggregator([100, 200]); aggregator.update(150); - aggregator.reset(); const point = aggregator.toPoint().value as Histogram; assert.equal(point.count, 1); assert.equal(point.sum, 150); @@ -63,7 +62,6 @@ describe('HistogramAggregator', () => { it('should update the second bucket', () => { const aggregator = new HistogramAggregator([100, 200]); aggregator.update(50); - aggregator.reset(); const point = aggregator.toPoint().value as Histogram; assert.equal(point.count, 1); assert.equal(point.sum, 50); @@ -75,7 +73,6 @@ describe('HistogramAggregator', () => { it('should update the third bucket since value is above all boundaries', () => { const aggregator = new HistogramAggregator([100, 200]); aggregator.update(250); - aggregator.reset(); const point = aggregator.toPoint().value as Histogram; assert.equal(point.count, 1); assert.equal(point.sum, 250); @@ -91,7 +88,6 @@ describe('HistogramAggregator', () => { let point = aggregator.toPoint().value as Histogram; assert.equal(point.count, point.count); aggregator.update(10); - aggregator.reset(); point = aggregator.toPoint().value as Histogram; assert.equal(point.count, 1); assert.equal(point.count, point.count); @@ -104,7 +100,6 @@ describe('HistogramAggregator', () => { let point = aggregator.toPoint().value as Histogram; assert.equal(point.sum, point.sum); aggregator.update(10); - aggregator.reset(); point = aggregator.toPoint().value as Histogram; assert.equal(point.sum, 10); }); @@ -126,7 +121,6 @@ describe('HistogramAggregator', () => { it('should update checkpoint', () => { const aggregator = new HistogramAggregator([100]); aggregator.update(10); - aggregator.reset(); const point = aggregator.toPoint().value as Histogram; assert.equal(point.count, 1); assert.equal(point.sum, 10); @@ -147,7 +141,6 @@ describe('HistogramAggregator', () => { it('should return last checkpoint if updated', () => { const aggregator = new HistogramAggregator([100]); aggregator.update(100); - aggregator.reset(); assert( aggregator .toPoint() From 5293f13655481e53053c82196969c6e012df0a9e Mon Sep 17 00:00:00 2001 From: legendecas Date: Mon, 13 Jul 2020 16:47:33 +0800 Subject: [PATCH 2/3] test: remove outdated case --- .../test/export/aggregators/Histogram.test.ts | 8 -------- 1 file changed, 8 deletions(-) diff --git a/packages/opentelemetry-metrics/test/export/aggregators/Histogram.test.ts b/packages/opentelemetry-metrics/test/export/aggregators/Histogram.test.ts index 19e0283be3..9a2b43938c 100644 --- a/packages/opentelemetry-metrics/test/export/aggregators/Histogram.test.ts +++ b/packages/opentelemetry-metrics/test/export/aggregators/Histogram.test.ts @@ -40,14 +40,6 @@ describe('HistogramAggregator', () => { }); describe('.update()', () => { - it('should not update checkpoint', () => { - const aggregator = new HistogramAggregator([100, 200]); - aggregator.update(150); - const point = aggregator.toPoint().value as Histogram; - assert.equal(point.count, 0); - assert.equal(point.sum, 0); - }); - it('should update the second bucket', () => { const aggregator = new HistogramAggregator([100, 200]); aggregator.update(150); From b2ea823d5b34dea3d28978f0615e6a9295a8bfd8 Mon Sep 17 00:00:00 2001 From: legendecas Date: Tue, 21 Jul 2020 17:59:19 +0800 Subject: [PATCH 3/3] fix: reset histogram aggregator after each run --- .../test/common/transformMetrics.test.ts | 2 -- packages/opentelemetry-exporter-collector/test/helper.ts | 6 +++++- .../test/node/CollectorMetricExporter.test.ts | 6 +++--- .../test/node/CollectorMetricExporterWithJson.test.ts | 6 +++--- 4 files changed, 11 insertions(+), 9 deletions(-) diff --git a/packages/opentelemetry-exporter-collector/test/common/transformMetrics.test.ts b/packages/opentelemetry-exporter-collector/test/common/transformMetrics.test.ts index 5d74cd9cf9..8cae6cc2cc 100644 --- a/packages/opentelemetry-exporter-collector/test/common/transformMetrics.test.ts +++ b/packages/opentelemetry-exporter-collector/test/common/transformMetrics.test.ts @@ -42,7 +42,6 @@ describe('transformMetrics', () => { // Histogram mockHistogram.aggregator.update(7); mockHistogram.aggregator.update(14); - (mockHistogram.aggregator as HistogramAggregator).reset(); // ValueRecorder mockValueRecorder.aggregator.update(5); @@ -50,7 +49,6 @@ describe('transformMetrics', () => { afterEach(() => { mockCounter.aggregator.update(-1); // Reset counter - (mockHistogram.aggregator as HistogramAggregator).reset(); }); it('should convert metric', () => { ensureCounterIsCorrect( diff --git a/packages/opentelemetry-exporter-collector/test/helper.ts b/packages/opentelemetry-exporter-collector/test/helper.ts index d7733c2c60..7437460b42 100644 --- a/packages/opentelemetry-exporter-collector/test/helper.ts +++ b/packages/opentelemetry-exporter-collector/test/helper.ts @@ -38,6 +38,10 @@ if (typeof Buffer === 'undefined') { }; } +type Mutable = { + -readonly [P in keyof T]: T[P]; +}; + const traceIdArr = [ 31, 16, @@ -113,7 +117,7 @@ export const mockValueRecorder: MetricRecord = { instrumentationLibrary: { name: 'default', version: '0.0.1' }, }; -export const mockHistogram: MetricRecord = { +export const mockHistogram: Mutable = { descriptor: { name: 'test-hist', description: 'sample observer description', diff --git a/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts b/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts index 7f75512654..abdd1d774c 100644 --- a/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts +++ b/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts @@ -146,14 +146,14 @@ const testCollectorMetricExporter = (params: TestParams) => metrics[1].aggregator.update(10); metrics[2].aggregator.update(7); metrics[2].aggregator.update(14); - (metrics[2].aggregator as HistogramAggregator).reset(); metrics[3].aggregator.update(5); done(); }); afterEach(() => { - metrics[0].aggregator.update(-1); // Aggregator is not deep-copied - (metrics[2].aggregator as HistogramAggregator).reset(); + // Aggregator is not deep-copied + metrics[0].aggregator.update(-1); + mockHistogram.aggregator = new HistogramAggregator([10, 20]); exportedData = undefined; reqMetadata = undefined; }); diff --git a/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporterWithJson.test.ts b/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporterWithJson.test.ts index 9e5155d2cd..a3e61d42f8 100644 --- a/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporterWithJson.test.ts +++ b/packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporterWithJson.test.ts @@ -85,12 +85,12 @@ describe('CollectorMetricExporter - node with json over http', () => { metrics[1].aggregator.update(10); metrics[2].aggregator.update(7); metrics[2].aggregator.update(14); - (metrics[2].aggregator as HistogramAggregator).reset(); metrics[3].aggregator.update(5); }); afterEach(() => { - metrics[0].aggregator.update(-1); // Aggregator is not deep-copied - (metrics[2].aggregator as HistogramAggregator).reset(); + // Aggregator is not deep-copied + metrics[0].aggregator.update(-1); + mockHistogram.aggregator = new HistogramAggregator([10, 20]); spyRequest.restore(); spyWrite.restore(); });