From 083b2d65ba61fe9ade29cee0378f358a5662f7de Mon Sep 17 00:00:00 2001 From: Daniel Dyla Date: Tue, 7 Apr 2020 18:29:15 -0400 Subject: [PATCH] fix: do not clear other labelsets when updating metrics (#941) * fix: do not clear other labelsets when updating metrics * chore: add test for batcher * chore: test for one counter with multiple labels * chore: lint * chore: fix year in copyright --- .../src/prometheus.ts | 25 ++++---- .../test/prometheus.test.ts | 4 +- .../src/export/Batcher.ts | 8 +-- .../test/Batcher.test.ts | 61 +++++++++++++++++++ 4 files changed, 81 insertions(+), 17 deletions(-) create mode 100644 packages/opentelemetry-metrics/test/Batcher.test.ts 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-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', '', ]); 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); } } diff --git a/packages/opentelemetry-metrics/test/Batcher.test.ts b/packages/opentelemetry-metrics/test/Batcher.test.ts new file mode 100644 index 00000000000..c09998bd32d --- /dev/null +++ b/packages/opentelemetry-metrics/test/Batcher.test.ts @@ -0,0 +1,61 @@ +/*! + * 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. + * 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'); + } + } + }); + }); +});