From 0836414ac8cd728f9f67fcdea85b5b491c82b150 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Tue, 7 Apr 2020 14:49:36 -0400 Subject: [PATCH 1/5] fix: do not clear other labelsets when updating metrics --- .../src/prometheus.ts | 25 ++++++++++--------- .../src/export/Batcher.ts | 8 +++--- 2 files changed, 17 insertions(+), 16 deletions(-) diff --git a/packages/opentelemetry-exporter-prometheus/src/prometheus.ts b/packages/opentelemetry-exporter-prometheus/src/prometheus.ts index d8d9ec29e09..2623138e1f6 100644 --- a/packages/opentelemetry-exporter-prometheus/src/prometheus.ts +++ b/packages/opentelemetry-exporter-prometheus/src/prometheus.ts @@ -123,7 +123,10 @@ export class PrometheusExporter implements MetricExporter { const metric = this._registerMetric(record); if (!metric) return; - const labelKeys = record.descriptor.labelKeys; + const labelValues = this._getLabelValues( + record.descriptor.labelKeys, + record.labels + ); const point = record.aggregator.toPoint(); if (metric instanceof Counter) { @@ -131,21 +134,15 @@ export class PrometheusExporter implements MetricExporter { // MetricRecord value is the current state, not the delta to be incremented by. // Currently, _registerMetric creates a new counter every time the value changes, // so the increment here behaves as a set value (increment from 0) - metric.inc( - this._getLabelValues(labelKeys, record.labels), - point.value as Sum - ); + metric.inc(labelValues, point.value as Sum); } if (metric instanceof Gauge) { if (record.aggregator instanceof CounterSumAggregator) { - metric.set( - this._getLabelValues(labelKeys, record.labels), - point.value as Sum - ); + metric.set(labelValues, point.value as Sum); } else if (record.aggregator instanceof ObserverAggregator) { metric.set( - this._getLabelValues(labelKeys, record.labels), + labelValues, point.value as LastValue, hrTimeToMilliseconds(point.timestamp) ); @@ -179,8 +176,12 @@ export class PrometheusExporter implements MetricExporter { * https://prometheus.io/docs/instrumenting/exposition_formats/ */ if (metric instanceof Counter) { - this._registry.removeSingleMetric(metricName); - } else if (metric) return metric; + metric.remove( + ...record.descriptor.labelKeys.map(k => record.labels[k].toString()) + ); + } + + if (metric) return metric; return this._newMetric(record, metricName); } diff --git a/packages/opentelemetry-metrics/src/export/Batcher.ts b/packages/opentelemetry-metrics/src/export/Batcher.ts index 85ecbfb7fe3..7caec738381 100644 --- a/packages/opentelemetry-metrics/src/export/Batcher.ts +++ b/packages/opentelemetry-metrics/src/export/Batcher.ts @@ -59,9 +59,9 @@ export class UngroupedBatcher extends Batcher { } process(record: MetricRecord): void { - this._batchMap.set( - record.descriptor.name + record.labels.identifier, - record - ); + const labels = record.descriptor.labelKeys + .map(k => `${k}=${record.labels[k]}`) + .join(','); + this._batchMap.set(record.descriptor.name + labels, record); } } From f644f4a3f71038d6e78c568e6c00b8d710b30c50 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Tue, 7 Apr 2020 15:19:35 -0400 Subject: [PATCH 2/5] chore: add test for batcher --- .../test/Batcher.test.ts | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 packages/opentelemetry-metrics/test/Batcher.test.ts diff --git a/packages/opentelemetry-metrics/test/Batcher.test.ts b/packages/opentelemetry-metrics/test/Batcher.test.ts new file mode 100644 index 00000000000..edb37d260bb --- /dev/null +++ b/packages/opentelemetry-metrics/test/Batcher.test.ts @@ -0,0 +1,60 @@ +/*! + * Copyright 2019, OpenTelemetry Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import * as assert from 'assert'; +import * as types from '@opentelemetry/api'; +import { NoopLogger } from '@opentelemetry/core'; +import { Meter, MeterProvider } from '../src'; + + +describe('Batcher', () => { + describe('Ungrouped', () => { + let meter: Meter; + let fooCounter: types.BoundCounter + let barCounter: types.BoundCounter + let counter: types.Metric + beforeEach(() => { + meter = new MeterProvider({ + logger: new NoopLogger(), + interval: 10000, + }).getMeter('test-meter'); + counter = meter.createCounter("ungrouped-batcher-test", { labelKeys: ['key'] }); + fooCounter = counter.bind({ key: 'foo' }); + barCounter = counter.bind({ key: 'bar' }); + }); + + it('should process a batch', () => { + fooCounter.add(1); + barCounter.add(1); + barCounter.add(2); + meter.collect(); + const checkPointSet = meter.getBatcher().checkPointSet(); + assert.strictEqual(checkPointSet.length, 2); + for (const record of checkPointSet) { + switch(record.labels.key) { + case 'foo': + assert.strictEqual(record.aggregator.toPoint().value, 1); + break; + case 'bar': + assert.strictEqual(record.aggregator.toPoint().value, 3); + break; + default: + throw new Error('Unknown labelset') + } + } + }) + }); +}); From 067d068b90cfbcf625d2aa51c6bd995e9e202ee4 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Tue, 7 Apr 2020 15:26:56 -0400 Subject: [PATCH 3/5] chore: test for one counter with multiple labels --- .../opentelemetry-exporter-prometheus/test/prometheus.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts b/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts index e4dcb005789..1dba630675f 100644 --- a/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts +++ b/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts @@ -266,13 +266,14 @@ describe('PrometheusExporter', () => { }); }); - it('should export multiple aggregations', done => { + it('should export multiple labelsets', done => { const counter = meter.createCounter('counter', { description: 'a test description', labelKeys: ['counterKey1'], }) as CounterMetric; counter.bind({ counterKey1: 'labelValue1' }).add(10); + counter.bind({ counterKey1: 'labelValue2' }).add(20); meter.collect(); exporter.export(meter.getBatcher().checkPointSet(), () => { http @@ -285,6 +286,7 @@ describe('PrometheusExporter', () => { '# HELP counter a test description', '# TYPE counter counter', 'counter{counterKey1="labelValue1"} 10', + 'counter{counterKey1="labelValue2"} 20', '', ]); From af8a6a57e254a6e542d0cee01d76e1ceae098782 Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Tue, 7 Apr 2020 16:02:45 -0400 Subject: [PATCH 4/5] chore: lint --- .../test/Batcher.test.ts | 71 ++++++++++--------- 1 file changed, 36 insertions(+), 35 deletions(-) diff --git a/packages/opentelemetry-metrics/test/Batcher.test.ts b/packages/opentelemetry-metrics/test/Batcher.test.ts index edb37d260bb..30928f8b5cc 100644 --- a/packages/opentelemetry-metrics/test/Batcher.test.ts +++ b/packages/opentelemetry-metrics/test/Batcher.test.ts @@ -19,42 +19,43 @@ import * as types from '@opentelemetry/api'; import { NoopLogger } from '@opentelemetry/core'; import { Meter, MeterProvider } from '../src'; - describe('Batcher', () => { - describe('Ungrouped', () => { - let meter: Meter; - let fooCounter: types.BoundCounter - let barCounter: types.BoundCounter - let counter: types.Metric - beforeEach(() => { - meter = new MeterProvider({ - logger: new NoopLogger(), - interval: 10000, - }).getMeter('test-meter'); - counter = meter.createCounter("ungrouped-batcher-test", { labelKeys: ['key'] }); - fooCounter = counter.bind({ key: 'foo' }); - barCounter = counter.bind({ key: 'bar' }); - }); + describe('Ungrouped', () => { + let meter: Meter; + let fooCounter: types.BoundCounter; + let barCounter: types.BoundCounter; + let counter: types.Metric; + beforeEach(() => { + meter = new MeterProvider({ + logger: new NoopLogger(), + interval: 10000, + }).getMeter('test-meter'); + counter = meter.createCounter('ungrouped-batcher-test', { + labelKeys: ['key'], + }); + fooCounter = counter.bind({ key: 'foo' }); + barCounter = counter.bind({ key: 'bar' }); + }); - it('should process a batch', () => { - fooCounter.add(1); - barCounter.add(1); - barCounter.add(2); - meter.collect(); - const checkPointSet = meter.getBatcher().checkPointSet(); - assert.strictEqual(checkPointSet.length, 2); - for (const record of checkPointSet) { - switch(record.labels.key) { - case 'foo': - assert.strictEqual(record.aggregator.toPoint().value, 1); - break; - case 'bar': - assert.strictEqual(record.aggregator.toPoint().value, 3); - break; - default: - throw new Error('Unknown labelset') - } - } - }) + it('should process a batch', () => { + fooCounter.add(1); + barCounter.add(1); + barCounter.add(2); + meter.collect(); + const checkPointSet = meter.getBatcher().checkPointSet(); + assert.strictEqual(checkPointSet.length, 2); + for (const record of checkPointSet) { + switch (record.labels.key) { + case 'foo': + assert.strictEqual(record.aggregator.toPoint().value, 1); + break; + case 'bar': + assert.strictEqual(record.aggregator.toPoint().value, 3); + break; + default: + throw new Error('Unknown labelset'); + } + } }); + }); }); From dcc1600a1d88f0bb8379efea35f6a0a351eb148d Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Tue, 7 Apr 2020 17:05:03 -0400 Subject: [PATCH 5/5] chore: fix year in copyright --- packages/opentelemetry-metrics/test/Batcher.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/opentelemetry-metrics/test/Batcher.test.ts b/packages/opentelemetry-metrics/test/Batcher.test.ts index 30928f8b5cc..c09998bd32d 100644 --- a/packages/opentelemetry-metrics/test/Batcher.test.ts +++ b/packages/opentelemetry-metrics/test/Batcher.test.ts @@ -1,5 +1,5 @@ /*! - * Copyright 2019, OpenTelemetry Authors + * Copyright 2020, OpenTelemetry Authors * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License.