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.

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

fix: boundaries option propagation in ValueRecorder Metric #1628

Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,9 @@ const testCollectorMetricExporter = (params: TestParams) =>
);
ensureExportedValueRecorderIsCorrect(
recorder,
recorder.intHistogram?.dataPoints[0].timeUnixNano
recorder.intHistogram?.dataPoints[0].timeUnixNano,
[0, 100],
['0', '2', '0']
);
assert.ok(
typeof resource !== 'undefined',
Expand Down
8 changes: 5 additions & 3 deletions packages/opentelemetry-exporter-collector-grpc/test/helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,9 @@ export function ensureExportedObserverIsCorrect(

export function ensureExportedValueRecorderIsCorrect(
metric: collectorTypes.opentelemetryProto.metrics.v1.Metric,
time?: number
time?: number,
explicitBounds: number[] = [Infinity],
bucketCounts: string[] = ['2', '0']
) {
assert.deepStrictEqual(metric, {
name: 'int-recorder',
Expand All @@ -388,8 +390,8 @@ export function ensureExportedValueRecorderIsCorrect(
count: '2',
startTimeUnixNano: '1592602232694000128',
timeUnixNano: String(time),
bucketCounts: ['2', '0'],
explicitBounds: [Infinity],
bucketCounts,
explicitBounds,
},
],
aggregationTemporality: 'AGGREGATION_TEMPORALITY_CUMULATIVE',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,9 @@ describe('CollectorMetricExporter - node with proto over http', () => {
);
ensureExportedValueRecorderIsCorrect(
metric3,
metric3.intHistogram?.dataPoints[0].timeUnixNano
metric3.intHistogram?.dataPoints[0].timeUnixNano,
[0, 100],
['0', '2', '0']
);

ensureExportMetricsServiceRequestIsSet(json);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,9 @@ export function ensureExportedObserverIsCorrect(

export function ensureExportedValueRecorderIsCorrect(
metric: collectorTypes.opentelemetryProto.metrics.v1.Metric,
time?: number
time?: number,
explicitBounds: number[] = [Infinity],
bucketCounts: string[] = ['2', '0']
) {
assert.deepStrictEqual(metric, {
name: 'int-recorder',
Expand All @@ -350,8 +352,8 @@ export function ensureExportedValueRecorderIsCorrect(
count: '2',
startTimeUnixNano: '1592602232694000128',
timeUnixNano: time,
bucketCounts: ['2', '0'],
explicitBounds: ['Infinity'],
bucketCounts,
explicitBounds,
},
],
aggregationTemporality: 'AGGREGATION_TEMPORALITY_CUMULATIVE',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,8 @@ describe('CollectorMetricExporter - web', () => {
ensureValueRecorderIsCorrect(
metric3,
hrTimeToNanoseconds(metrics[2].aggregator.toPoint().timestamp),
true
[0, 100],
[0, 2, 0]
);
}

Expand Down Expand Up @@ -253,7 +254,8 @@ describe('CollectorMetricExporter - web', () => {
ensureValueRecorderIsCorrect(
metric3,
hrTimeToNanoseconds(metrics[2].aggregator.toPoint().timestamp),
true
[0, 100],
[0, 2, 0]
);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,9 @@ describe('transformMetrics', () => {

ensureValueRecorderIsCorrect(
transform.toCollectorMetric(recorder, 1592602232694000000),
hrTimeToNanoseconds(recorder.aggregator.toPoint().timestamp)
hrTimeToNanoseconds(recorder.aggregator.toPoint().timestamp),
[0, 100],
[0, 2, 0]
);

ensureDoubleCounterIsCorrect(
Expand Down
7 changes: 4 additions & 3 deletions packages/opentelemetry-exporter-collector/test/helper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -573,7 +573,8 @@ export function ensureObserverIsCorrect(
export function ensureValueRecorderIsCorrect(
metric: collectorTypes.opentelemetryProto.metrics.v1.Metric,
time: number,
infinityIsNull?: boolean
explicitBounds: (number | null)[] = [Infinity],
bucketCounts: number[] = [2, 0]
) {
assert.deepStrictEqual(metric, {
name: 'int-recorder',
Expand All @@ -587,8 +588,8 @@ export function ensureValueRecorderIsCorrect(
count: 2,
startTimeUnixNano: 1592602232694000000,
timeUnixNano: time,
bucketCounts: [2, 0],
explicitBounds: [infinityIsNull ? null : Infinity],
bucketCounts,
explicitBounds,
},
],
aggregationTemporality:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,8 @@ describe('CollectorMetricExporter - node with json over http', () => {
ensureValueRecorderIsCorrect(
metric3,
core.hrTimeToNanoseconds(metrics[2].aggregator.toPoint().timestamp),
true
[0, 100],
[0, 2, 0]
);

ensureExportMetricsServiceRequestIsSet(json);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,16 +206,33 @@ export class PrometheusSerializer {
}

let cumulativeSum = 0;
for (const [idx, val] of value.buckets.counts.entries()) {
const countEntries = value.buckets.counts.entries();
let infiniteBoundaryDefined = false;
for (const [idx, val] of countEntries) {
cumulativeSum += val;
const upperBound = value.buckets.boundaries[idx];
/** HistogramAggregator is producing different boundary output -
* in one case not including inifinity values, in other -
* full, e.g. [0, 100] and [0, 100, Infinity]
* we should consider that in export, if Infinity is defined, use it
* as boundary
*/
if (upperBound === undefined && infiniteBoundaryDefined) {
break;
}
if (upperBound === Infinity) {
infiniteBoundaryDefined = true;
}
results += stringify(
name + '_bucket',
record.labels,
cumulativeSum,
this._appendTimestamp ? timestamp : undefined,
{
le: upperBound === undefined ? '+Inf' : String(upperBound),
le:
upperBound === undefined || upperBound === Infinity
? '+Inf'
: String(upperBound),
}
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -538,7 +538,6 @@ describe('PrometheusExporter', () => {
'# TYPE value_recorder histogram',
`value_recorder_count{key1="labelValue1"} 1 ${mockedHrTimeMs}`,
`value_recorder_sum{key1="labelValue1"} 20 ${mockedHrTimeMs}`,
`value_recorder_bucket{key1="labelValue1",le="Infinity"} 1 ${mockedHrTimeMs}`,
`value_recorder_bucket{key1="labelValue1",le="+Inf"} 1 ${mockedHrTimeMs}`,
'',
]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,34 @@ describe('PrometheusSerializer', () => {
);
});

it('should serialize metric record with sum aggregator, boundaries defined in constructor', async () => {
const serializer = new PrometheusSerializer();

const meter = new MeterProvider().getMeter('test');
const recorder = meter.createValueRecorder('test', {
description: 'foobar',
boundaries: [1, 10, 100],
}) as ValueRecorderMetric;
recorder.bind(labels).record(5);

const records = await recorder.getMetricRecord();
const record = records[0];

const result = serializer.serializeRecord(
record.descriptor.name,
record
);
assert.strictEqual(
result,
`test_count{foo1="bar1",foo2="bar2"} 1 ${mockedHrTimeMs}\n` +
`test_sum{foo1="bar1",foo2="bar2"} 5 ${mockedHrTimeMs}\n` +
`test_bucket{foo1="bar1",foo2="bar2",le="1"} 0 ${mockedHrTimeMs}\n` +
`test_bucket{foo1="bar1",foo2="bar2",le="10"} 1 ${mockedHrTimeMs}\n` +
`test_bucket{foo1="bar1",foo2="bar2",le="100"} 1 ${mockedHrTimeMs}\n` +
`test_bucket{foo1="bar1",foo2="bar2",le="+Inf"} 1 ${mockedHrTimeMs}\n`
);
});

it('serialize metric record with sum aggregator without timestamp', async () => {
const serializer = new PrometheusSerializer(undefined, false);

Expand Down
3 changes: 3 additions & 0 deletions packages/opentelemetry-metrics/src/Metric.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ export abstract class Metric<T extends BaseBoundInstrument>
protected readonly _valueType: api.ValueType;
protected readonly _logger: api.Logger;
protected readonly _descriptor: MetricDescriptor;
protected readonly _boundaries: number[] | undefined;
private readonly _instruments: Map<string, T> = new Map();

constructor(
Expand All @@ -43,6 +44,7 @@ export abstract class Metric<T extends BaseBoundInstrument>
? _options.valueType
: api.ValueType.DOUBLE;
this._logger = _options.logger ?? new NoopLogger();
this._boundaries = _options.boundaries;
this._descriptor = this._getMetricDescriptor();
}

Expand Down Expand Up @@ -105,6 +107,7 @@ export abstract class Metric<T extends BaseBoundInstrument>
unit: this._options.unit || '1',
metricKind: this._kind,
valueType: this._valueType,
...(this._boundaries && { boundaries: this._boundaries }),
};
}

Expand Down
27 changes: 27 additions & 0 deletions packages/opentelemetry-metrics/test/Meter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -556,6 +556,33 @@ describe('Meter', () => {
assert.ok(valueRecorder instanceof Metric);
});

it('should set histogram boundaries for value recorder', async () => {
const valueRecorder = meter.createValueRecorder('name', {
description: 'desc',
unit: '1',
disabled: false,
boundaries: [10, 20, 30, 100],
}) as ValueRecorderMetric;

valueRecorder.record(10);
valueRecorder.record(30);
valueRecorder.record(50);
valueRecorder.record(200);

await meter.collect();
const [record] = meter.getBatcher().checkPointSet();
assert.deepStrictEqual(record.aggregator.toPoint().value as Histogram, {
buckets: {
boundaries: [10, 20, 30, 100],
counts: [0, 1, 0, 2, 1],
},
count: 4,
sum: 290,
});

assert.ok(valueRecorder instanceof Metric);
});

it('should pipe through resource', async () => {
const valueRecorder = meter.createValueRecorder(
'name'
Expand Down