Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: remove HistogramAggregator.reset #1292

Merged
merged 8 commits into from
Jul 21, 2020
Merged

feat: remove HistogramAggregator.reset #1292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
901c965
feat: remove HistogramAggregator.reset
legendecas Jul 9, 2020
5293f13
test: remove outdated case
legendecas Jul 13, 2020
88e670c
Merge branch 'master' into histogram
legendecas Jul 14, 2020
bd006b5
Merge branch 'master' into histogram
legendecas Jul 14, 2020
58befff
Merge branch 'master' into histogram
legendecas Jul 17, 2020
42f9257
Merge branch 'master' into histogram
legendecas Jul 19, 2020
57de38c
Merge remote-tracking branch 'upstream/master' into histogram
legendecas Jul 21, 2020
b2ea823
fix: reset histogram aggregator after each run
legendecas Jul 21, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions packages/opentelemetry-exporter-collector/test/common/transformMetrics.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import {
ensureValueRecorderIsCorrect,
mockValueRecorder,
} from '../helper';
import { HistogramAggregator } from '@opentelemetry/metrics';
import { hrTimeToNanoseconds } from '@opentelemetry/core';
describe('transformMetrics', () => {
describe('toCollectorMetric', () => {
Expand All @@ -43,15 +42,13 @@ describe('transformMetrics', () => {
// Histogram
mockHistogram.aggregator.update(7);
mockHistogram.aggregator.update(14);
(mockHistogram.aggregator as HistogramAggregator).reset();

// ValueRecorder
mockValueRecorder.aggregator.update(5);
});

afterEach(() => {
mockCounter.aggregator.update(-1); // Reset counter
(mockHistogram.aggregator as HistogramAggregator).reset();
});
it('should convert metric', () => {
ensureCounterIsCorrect(
Expand Down
6 changes: 5 additions & 1 deletion packages/opentelemetry-exporter-collector/test/helper.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ if (typeof Buffer === 'undefined') {
};
}

type Mutable<T> = {
-readonly [P in keyof T]: T[P];
};

const traceIdArr = [
31,
16,
Expand Down Expand Up @@ -113,7 +117,7 @@ export const mockValueRecorder: MetricRecord = {
instrumentationLibrary: { name: 'default', version: '0.0.1' },
};

export const mockHistogram: MetricRecord = {
export const mockHistogram: Mutable<MetricRecord> = {
descriptor: {
name: 'test-hist',
description: 'sample observer description',
Expand Down
6 changes: 3 additions & 3 deletions packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporter.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
Expand Down
6 changes: 3 additions & 3 deletions packages/opentelemetry-exporter-collector/test/node/CollectorMetricExporterWithJson.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
Expand Down
28 changes: 10 additions & 18 deletions ...trics/src/export/aggregators/histogram.ts → ...trics/src/export/aggregators/Histogram.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[]) {
Expand All @@ -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,
};
}

Expand Down
2 changes: 1 addition & 1 deletion packages/opentelemetry-metrics/src/export/aggregators/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,6 @@
* limitations under the License.
*/

export * from './histogram';
export * from './Histogram';
export * from './MinMaxLastSumCount';
export * from './Sum';
15 changes: 0 additions & 15 deletions ...test/export/aggregators/histogram.test.ts → ...test/export/aggregators/Histogram.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,9 @@ describe('HistogramAggregator', () => {
});

describe('.update()', () => {
it('should not update checkpoint', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this not needed ?

Copy link
Member Author

@legendecas legendecas Jul 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aggregators are expected to update the current checkpoint on each aggregator.update call. This case asserts the original HistogramAggregator update the checkpoint on histogramAggregator.reset -- this is not compliant with the current spec draft and not aligned with other existing aggregators.

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);
aggregator.reset();
const point = aggregator.toPoint().value as Histogram;
assert.equal(point.count, 1);
assert.equal(point.sum, 150);
Expand All @@ -63,7 +54,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);
Expand All @@ -75,7 +65,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);
Expand All @@ -91,7 +80,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);
Expand All @@ -104,7 +92,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);
});
Expand All @@ -126,7 +113,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);
Expand All @@ -147,7 +133,6 @@ describe('HistogramAggregator', () => {
it('should return last checkpoint if updated', () => {
const aggregator = new HistogramAggregator([100]);
aggregator.update(100);
aggregator.reset();
assert(
aggregator
.toPoint()
Expand Down