From 6b50ba9d1a284d8efb59a4e025e0d7f3625c9665 Mon Sep 17 00:00:00 2001 From: Naseem Date: Sun, 31 May 2020 00:24:11 -0400 Subject: [PATCH 1/5] feat: remove label keys as they are no longer part of the spec Signed-off-by: Naseem --- .../opentelemetry-api/src/metrics/Metric.ts | 3 -- .../src/prometheus.ts | 30 +++++-------------- .../test/prometheus.test.ts | 21 +++++++------ packages/opentelemetry-metrics/src/Metric.ts | 1 - .../src/export/Batcher.ts | 4 +-- .../opentelemetry-metrics/src/export/types.ts | 1 - packages/opentelemetry-metrics/src/types.ts | 4 --- .../test/Batcher.test.ts | 6 ++-- .../opentelemetry-metrics/test/Meter.test.ts | 6 ---- .../test/export/ConsoleMetricExporter.test.ts | 2 -- 10 files changed, 21 insertions(+), 57 deletions(-) diff --git a/packages/opentelemetry-api/src/metrics/Metric.ts b/packages/opentelemetry-api/src/metrics/Metric.ts index 3996e5498b..c749c08217 100644 --- a/packages/opentelemetry-api/src/metrics/Metric.ts +++ b/packages/opentelemetry-api/src/metrics/Metric.ts @@ -38,9 +38,6 @@ export interface MetricOptions { */ unit?: string; - /** The list of label keys for the Metric. */ - labelKeys?: string[]; - /** The map of constant labels for the Metric. */ constantLabels?: Map; diff --git a/packages/opentelemetry-exporter-prometheus/src/prometheus.ts b/packages/opentelemetry-exporter-prometheus/src/prometheus.ts index 84c2fed821..fa1bdc3a35 100644 --- a/packages/opentelemetry-exporter-prometheus/src/prometheus.ts +++ b/packages/opentelemetry-exporter-prometheus/src/prometheus.ts @@ -31,7 +31,7 @@ import { } from '@opentelemetry/metrics'; import * as api from '@opentelemetry/api'; import { createServer, IncomingMessage, Server, ServerResponse } from 'http'; -import { Counter, Gauge, labelValues, Metric, Registry } from 'prom-client'; +import { Counter, Gauge, Metric, Registry } from 'prom-client'; import * as url from 'url'; import { ExporterConfig } from './export/types'; @@ -126,19 +126,17 @@ export class PrometheusExporter implements MetricExporter { const metric = this._registerMetric(record); if (!metric) return; - const labelValues = this._getLabelValues( - record.descriptor.labelKeys, - record.labels - ); const point = record.aggregator.toPoint(); + const labels = record.labels; + if (metric instanceof Counter) { // Prometheus counter saves internal state and increments by given value. // 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( - labelValues, + labels, point.value as Sum, hrTimeToMilliseconds(point.timestamp) ); @@ -146,10 +144,10 @@ export class PrometheusExporter implements MetricExporter { if (metric instanceof Gauge) { if (record.aggregator instanceof CounterSumAggregator) { - metric.set(labelValues, point.value as Sum); + metric.set(labels, point.value as Sum); } else if (record.aggregator instanceof ObserverAggregator) { metric.set( - labelValues, + labels, point.value as LastValue, hrTimeToMilliseconds(point.timestamp) ); @@ -159,16 +157,6 @@ export class PrometheusExporter implements MetricExporter { // TODO: only counter and gauge are implemented in metrics so far } - private _getLabelValues(keys: string[], labels: api.Labels) { - const labelValues: labelValues = {}; - for (let i = 0; i < keys.length; i++) { - if (labels[keys[i]] !== null) { - labelValues[keys[i]] = labels[keys[i]]; - } - } - return labelValues; - } - private _registerMetric(record: MetricRecord): Metric | undefined { const metricName = this._getPrometheusMetricName(record.descriptor); const metric = this._registry.getSingleMetric(metricName); @@ -183,9 +171,7 @@ export class PrometheusExporter implements MetricExporter { * https://prometheus.io/docs/instrumenting/exposition_formats/ */ if (metric instanceof Counter) { - metric.remove( - ...record.descriptor.labelKeys.map(k => record.labels[k].toString()) - ); + metric.remove(...Object.values(record.labels)); } if (metric) return metric; @@ -198,7 +184,7 @@ export class PrometheusExporter implements MetricExporter { name, // prom-client throws with empty description which is our default help: record.descriptor.description || 'description missing', - labelNames: record.descriptor.labelKeys, + labelNames: Object.keys(record.labels), // list of registries to register the newly created metric registers: [this._registry], }; diff --git a/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts b/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts index dd1294ca3a..adad7aab13 100644 --- a/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts +++ b/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts @@ -200,7 +200,6 @@ describe('PrometheusExporter', () => { it('should export a count aggregation', done => { const counter = meter.createCounter('counter', { description: 'a test description', - labelKeys: ['key1'], }); const boundCounter = counter.bind({ key1: 'labelValue1' }); @@ -245,7 +244,6 @@ describe('PrometheusExporter', () => { const observer = meter.createObserver('metric_observer', { description: 'a test description', - labelKeys: ['pid'], }) as ObserverMetric; observer.setCallback((observerResult: ObserverResult) => { @@ -269,7 +267,10 @@ describe('PrometheusExporter', () => { assert.strictEqual(lines[1], '# TYPE metric_observer gauge'); const line3 = lines[2].split(' '); - assert.strictEqual(line3[0], 'metric_observer{pid="123"}'); + assert.strictEqual( + line3[0], + 'metric_observer{pid="123",core="1"}' + ); assert.ok( parseFloat(line3[1]) >= 0 && parseFloat(line3[1]) <= 1 ); @@ -286,7 +287,6 @@ describe('PrometheusExporter', () => { it('should export multiple labels', done => { const counter = meter.createCounter('counter', { description: 'a test description', - labelKeys: ['counterKey1'], }) as CounterMetric; counter.bind({ counterKey1: 'labelValue1' }).add(10); @@ -302,7 +302,7 @@ describe('PrometheusExporter', () => { assert.deepStrictEqual(lines, [ '# HELP counter a test description', '# TYPE counter counter', - `counter{counterKey1="labelValue1"} 10 ${mockedTimeMS}`, + // `counter{counterKey1="labelValue1"} 10 ${mockedTimeMS}`, `counter{counterKey1="labelValue2"} 20 ${mockedTimeMS}`, '', ]); @@ -347,7 +347,7 @@ describe('PrometheusExporter', () => { assert.deepStrictEqual(lines, [ '# HELP counter description missing', '# TYPE counter counter', - `counter 10 ${mockedTimeMS}`, + `counter{key1="labelValue1"} 10 ${mockedTimeMS}`, '', ]); @@ -373,7 +373,7 @@ describe('PrometheusExporter', () => { assert.deepStrictEqual(lines, [ '# HELP counter_bad_name description missing', '# TYPE counter_bad_name counter', - `counter_bad_name 10 ${mockedTimeMS}`, + `counter_bad_name{key1="labelValue1"} 10 ${mockedTimeMS}`, '', ]); @@ -388,7 +388,6 @@ describe('PrometheusExporter', () => { const counter = meter.createCounter('counter', { description: 'a test description', monotonic: false, - labelKeys: ['key1'], }); counter.bind({ key1: 'labelValue1' }).add(20); @@ -449,7 +448,7 @@ describe('PrometheusExporter', () => { assert.deepStrictEqual(lines, [ '# HELP test_prefix_counter description missing', '# TYPE test_prefix_counter counter', - `test_prefix_counter 10 ${mockedTimeMS}`, + `test_prefix_counter{key1="labelValue1"} 10 ${mockedTimeMS}`, '', ]); @@ -478,7 +477,7 @@ describe('PrometheusExporter', () => { assert.deepStrictEqual(lines, [ '# HELP counter description missing', '# TYPE counter counter', - `counter 10 ${mockedTimeMS}`, + `counter{key1="labelValue1"} 10 ${mockedTimeMS}`, '', ]); @@ -507,7 +506,7 @@ describe('PrometheusExporter', () => { assert.deepStrictEqual(lines, [ '# HELP counter description missing', '# TYPE counter counter', - `counter 10 ${mockedTimeMS}`, + `counter{key1="labelValue1"} 10 ${mockedTimeMS}`, '', ]); diff --git a/packages/opentelemetry-metrics/src/Metric.ts b/packages/opentelemetry-metrics/src/Metric.ts index 9547d210e3..a3522470f6 100644 --- a/packages/opentelemetry-metrics/src/Metric.ts +++ b/packages/opentelemetry-metrics/src/Metric.ts @@ -98,7 +98,6 @@ export abstract class Metric unit: this._options.unit, metricKind: this._kind, valueType: this._valueType, - labelKeys: this._options.labelKeys, monotonic: this._monotonic, }; } diff --git a/packages/opentelemetry-metrics/src/export/Batcher.ts b/packages/opentelemetry-metrics/src/export/Batcher.ts index 7b2fbd417f..07fe09a8df 100644 --- a/packages/opentelemetry-metrics/src/export/Batcher.ts +++ b/packages/opentelemetry-metrics/src/export/Batcher.ts @@ -64,9 +64,7 @@ export class UngroupedBatcher extends Batcher { } process(record: MetricRecord): void { - const labels = record.descriptor.labelKeys - .map(k => `${k}=${record.labels[k]}`) - .join(','); + const labels = record.labels; this._batchMap.set(record.descriptor.name + labels, record); } } diff --git a/packages/opentelemetry-metrics/src/export/types.ts b/packages/opentelemetry-metrics/src/export/types.ts index 0fe741905e..82e9b88329 100644 --- a/packages/opentelemetry-metrics/src/export/types.ts +++ b/packages/opentelemetry-metrics/src/export/types.ts @@ -78,7 +78,6 @@ export interface MetricDescriptor { readonly unit: string; readonly metricKind: MetricKind; readonly valueType: ValueType; - readonly labelKeys: string[]; readonly monotonic: boolean; } diff --git a/packages/opentelemetry-metrics/src/types.ts b/packages/opentelemetry-metrics/src/types.ts index 8927a80830..4c03c8f742 100644 --- a/packages/opentelemetry-metrics/src/types.ts +++ b/packages/opentelemetry-metrics/src/types.ts @@ -31,9 +31,6 @@ export interface MetricOptions { /** The unit of the Metric values. */ unit: string; - /** The list of label keys for the Metric. */ - labelKeys: string[]; - /** The map of constant labels for the Metric. */ constantLabels?: Map; @@ -84,6 +81,5 @@ export const DEFAULT_METRIC_OPTIONS = { disabled: false, description: '', unit: '1', - labelKeys: [], valueType: ValueType.DOUBLE, }; diff --git a/packages/opentelemetry-metrics/test/Batcher.test.ts b/packages/opentelemetry-metrics/test/Batcher.test.ts index 72c68608bb..c322317ab2 100644 --- a/packages/opentelemetry-metrics/test/Batcher.test.ts +++ b/packages/opentelemetry-metrics/test/Batcher.test.ts @@ -30,9 +30,7 @@ describe('Batcher', () => { logger: new NoopLogger(), interval: 10000, }).getMeter('test-meter'); - counter = meter.createCounter('ungrouped-batcher-test', { - labelKeys: ['key'], - }); + counter = meter.createCounter('ungrouped-batcher-test', {}); fooCounter = counter.bind({ key: 'foo' }); barCounter = counter.bind({ key: 'bar' }); }); @@ -43,7 +41,7 @@ describe('Batcher', () => { barCounter.add(2); meter.collect(); const checkPointSet = meter.getBatcher().checkPointSet(); - assert.strictEqual(checkPointSet.length, 2); + // assert.strictEqual(checkPointSet.length, 2); for (const record of checkPointSet) { switch (record.labels.key) { case 'foo': diff --git a/packages/opentelemetry-metrics/test/Meter.test.ts b/packages/opentelemetry-metrics/test/Meter.test.ts index 5826f09a90..77191767e7 100644 --- a/packages/opentelemetry-metrics/test/Meter.test.ts +++ b/packages/opentelemetry-metrics/test/Meter.test.ts @@ -228,7 +228,6 @@ describe('Meter', () => { assert.strictEqual(record.length, 1); assert.deepStrictEqual(record[0].descriptor, { description: '', - labelKeys: [], metricKind: MetricKind.COUNTER, monotonic: true, name: 'name1', @@ -469,7 +468,6 @@ describe('Meter', () => { it('should set callback and observe value ', () => { const measure = meter.createObserver('name', { description: 'desc', - labelKeys: ['pid', 'core'], }) as ObserverMetric; function getCpuUsage() { @@ -528,7 +526,6 @@ describe('Meter', () => { const key = 'key'; const counter = meter.createCounter('counter', { description: 'test', - labelKeys: [key], }); const labels = { [key]: 'counter-value' }; const boundCounter = counter.bind(labels); @@ -545,7 +542,6 @@ describe('Meter', () => { monotonic: true, unit: '1', valueType: ValueType.DOUBLE, - labelKeys: ['key'], }); assert.strictEqual(record[0].labels, labels); const value = record[0].aggregator.toPoint().value as Sum; @@ -556,7 +552,6 @@ describe('Meter', () => { const key = 'key'; const counter = meter.createCounter('counter', { description: 'test', - labelKeys: [key], valueType: api.ValueType.INT, }); const labels = { [key]: 'counter-value' }; @@ -574,7 +569,6 @@ describe('Meter', () => { monotonic: true, unit: '1', valueType: ValueType.INT, - labelKeys: ['key'], }); assert.strictEqual(record[0].labels, labels); const value = record[0].aggregator.toPoint().value as Sum; diff --git a/packages/opentelemetry-metrics/test/export/ConsoleMetricExporter.test.ts b/packages/opentelemetry-metrics/test/export/ConsoleMetricExporter.test.ts index d9dc83315f..d47bc2981c 100644 --- a/packages/opentelemetry-metrics/test/export/ConsoleMetricExporter.test.ts +++ b/packages/opentelemetry-metrics/test/export/ConsoleMetricExporter.test.ts @@ -42,7 +42,6 @@ describe('ConsoleMetricExporter', () => { ); const counter = meter.createCounter('counter', { description: 'a test description', - labelKeys: ['key1', 'key2'], }); const boundCounter = counter.bind({ key1: 'labelValue1', @@ -57,7 +56,6 @@ describe('ConsoleMetricExporter', () => { assert.deepStrictEqual(descriptor, [ { description: 'a test description', - labelKeys: ['key1', 'key2'], metricKind: MetricKind.COUNTER, monotonic: true, name: 'counter', From 6d90e67bf676f8afbaae8079c7912108b96457cd Mon Sep 17 00:00:00 2001 From: Naseem Date: Sun, 31 May 2020 16:12:09 -0400 Subject: [PATCH 2/5] remove empty object Co-authored-by: Mayur Kale --- 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 c322317ab2..b6a00765dc 100644 --- a/packages/opentelemetry-metrics/test/Batcher.test.ts +++ b/packages/opentelemetry-metrics/test/Batcher.test.ts @@ -30,7 +30,7 @@ describe('Batcher', () => { logger: new NoopLogger(), interval: 10000, }).getMeter('test-meter'); - counter = meter.createCounter('ungrouped-batcher-test', {}); + counter = meter.createCounter('ungrouped-batcher-test'); fooCounter = counter.bind({ key: 'foo' }); barCounter = counter.bind({ key: 'bar' }); }); From 9087bbbc6b8bc45bcfb43dab6d71b0760a922bf2 Mon Sep 17 00:00:00 2001 From: Naseem Date: Sun, 31 May 2020 16:27:21 -0400 Subject: [PATCH 3/5] chore: remove remaining references of labelKeys Signed-off-by: Naseem --- examples/metrics/metrics/observer.js | 1 - examples/prometheus/index.js | 2 -- getting-started/README.md | 2 -- getting-started/monitored-example/monitoring.js | 1 - getting-started/ts-example/README.md | 2 -- getting-started/ts-example/monitoring.ts | 1 - packages/opentelemetry-exporter-prometheus/README.md | 1 - packages/opentelemetry-metrics/README.md | 2 -- 8 files changed, 12 deletions(-) diff --git a/examples/metrics/metrics/observer.js b/examples/metrics/metrics/observer.js index 6cced2142c..64e96013c9 100644 --- a/examples/metrics/metrics/observer.js +++ b/examples/metrics/metrics/observer.js @@ -19,7 +19,6 @@ const meter = new MeterProvider({ const otelCpuUsage = meter.createObserver('metric_observer', { monotonic: false, - labelKeys: ['pid', 'core'], description: 'Example of a observer', }); diff --git a/examples/prometheus/index.js b/examples/prometheus/index.js index 5d9b52c17a..775f8a6d8a 100644 --- a/examples/prometheus/index.js +++ b/examples/prometheus/index.js @@ -20,14 +20,12 @@ const meter = new MeterProvider({ // Monotonic counters can only be increased. const monotonicCounter = meter.createCounter('monotonic_counter', { monotonic: true, - labelKeys: ['pid'], description: 'Example of a monotonic counter', }); // Non-monotonic counters can be increased or decreased. const nonMonotonicCounter = meter.createCounter('non_monotonic_counter', { monotonic: false, - labelKeys: ['pid'], description: 'Example of a non-monotonic counter', }); diff --git a/getting-started/README.md b/getting-started/README.md index ce7b341dc5..1565398dbc 100644 --- a/getting-started/README.md +++ b/getting-started/README.md @@ -253,7 +253,6 @@ const meter = new MeterProvider().getMeter('your-meter-name'); const requestCount = meter.createCounter("requests", { monotonic: true, - labelKeys: ["route"], description: "Count all incoming requests" }); @@ -319,7 +318,6 @@ const meter = new MeterProvider({ const requestCount = meter.createCounter("requests", { monotonic: true, - labelKeys: ["route"], description: "Count all incoming requests" }); diff --git a/getting-started/monitored-example/monitoring.js b/getting-started/monitored-example/monitoring.js index 3f34387711..19d9e802aa 100644 --- a/getting-started/monitored-example/monitoring.js +++ b/getting-started/monitored-example/monitoring.js @@ -19,7 +19,6 @@ const meter = new MeterProvider({ const requestCount = meter.createCounter("requests", { monotonic: true, - labelKeys: ["route"], description: "Count all incoming requests" }); diff --git a/getting-started/ts-example/README.md b/getting-started/ts-example/README.md index 4414049bf7..c1d81dccde 100644 --- a/getting-started/ts-example/README.md +++ b/getting-started/ts-example/README.md @@ -252,7 +252,6 @@ const meter = new MeterProvider().getMeter('your-meter-name'); const requestCount: Metric = meter.createCounter("requests", { monotonic: true, - labelKeys: ["route"], description: "Count all incoming requests" }); @@ -317,7 +316,6 @@ const meter = new MeterProvider({ const requestCount: Metric = meter.createCounter("requests", { monotonic: true, - labelKeys: ["route"], description: "Count all incoming requests" }); diff --git a/getting-started/ts-example/monitoring.ts b/getting-started/ts-example/monitoring.ts index a7e544d762..42612f3cf6 100644 --- a/getting-started/ts-example/monitoring.ts +++ b/getting-started/ts-example/monitoring.ts @@ -18,7 +18,6 @@ const meter = new MeterProvider({ const requestCount: Metric = meter.createCounter("requests", { monotonic: true, - labelKeys: ["route"], description: "Count all incoming requests" }); diff --git a/packages/opentelemetry-exporter-prometheus/README.md b/packages/opentelemetry-exporter-prometheus/README.md index dee3d8e227..3511d63d85 100644 --- a/packages/opentelemetry-exporter-prometheus/README.md +++ b/packages/opentelemetry-exporter-prometheus/README.md @@ -36,7 +36,6 @@ const meter = new MeterProvider({ // Now, start recording data const counter = meter.createCounter('metric_name', { - labelKeys: ['pid'], description: 'Example of a counter' }); counter.add(10, { pid: process.pid }); diff --git a/packages/opentelemetry-metrics/README.md b/packages/opentelemetry-metrics/README.md index 1de26eab56..dba4e57a78 100644 --- a/packages/opentelemetry-metrics/README.md +++ b/packages/opentelemetry-metrics/README.md @@ -25,7 +25,6 @@ const { MeterProvider } = require('@opentelemetry/metrics'); const meter = new MeterProvider().getMeter('your-meter-name'); const counter = meter.createCounter('metric_name', { - labelKeys: ['pid'], description: 'Example of a counter' }); @@ -47,7 +46,6 @@ const { MeterProvider, MetricObservable } = require('@opentelemetry/metrics'); const meter = new MeterProvider().getMeter('your-meter-name'); const observer = meter.createObserver('metric_name', { - labelKeys: ['pid', 'core'], description: 'Example of a observer' }); From a64d472553c6bc09679e46ba1096a73f5ef4d1dd Mon Sep 17 00:00:00 2001 From: Naseem Date: Sun, 31 May 2020 21:43:50 -0400 Subject: [PATCH 4/5] fix: serialize labels object into string before adding it to the map (_batchMap) Signed-off-by: Naseem --- packages/opentelemetry-metrics/src/export/Batcher.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/opentelemetry-metrics/src/export/Batcher.ts b/packages/opentelemetry-metrics/src/export/Batcher.ts index 07fe09a8df..f85c7e11c8 100644 --- a/packages/opentelemetry-metrics/src/export/Batcher.ts +++ b/packages/opentelemetry-metrics/src/export/Batcher.ts @@ -64,7 +64,9 @@ export class UngroupedBatcher extends Batcher { } process(record: MetricRecord): void { - const labels = record.labels; + const labels = Object.keys(record.labels) + .map(k => `${k}=${record.labels[k]}`) + .join(','); this._batchMap.set(record.descriptor.name + labels, record); } } From 9a3b37d45afe77fb050f3880a6026805a6707e3b Mon Sep 17 00:00:00 2001 From: Naseem Date: Sun, 31 May 2020 21:50:25 -0400 Subject: [PATCH 5/5] test: uncomment expected result Signed-off-by: Naseem --- .../opentelemetry-exporter-prometheus/test/prometheus.test.ts | 2 +- packages/opentelemetry-metrics/test/Batcher.test.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts b/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts index adad7aab13..5e1e426862 100644 --- a/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts +++ b/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts @@ -302,7 +302,7 @@ describe('PrometheusExporter', () => { assert.deepStrictEqual(lines, [ '# HELP counter a test description', '# TYPE counter counter', - // `counter{counterKey1="labelValue1"} 10 ${mockedTimeMS}`, + `counter{counterKey1="labelValue1"} 10 ${mockedTimeMS}`, `counter{counterKey1="labelValue2"} 20 ${mockedTimeMS}`, '', ]); diff --git a/packages/opentelemetry-metrics/test/Batcher.test.ts b/packages/opentelemetry-metrics/test/Batcher.test.ts index b6a00765dc..a295883cc6 100644 --- a/packages/opentelemetry-metrics/test/Batcher.test.ts +++ b/packages/opentelemetry-metrics/test/Batcher.test.ts @@ -41,7 +41,7 @@ describe('Batcher', () => { barCounter.add(2); meter.collect(); const checkPointSet = meter.getBatcher().checkPointSet(); - // assert.strictEqual(checkPointSet.length, 2); + assert.strictEqual(checkPointSet.length, 2); for (const record of checkPointSet) { switch (record.labels.key) { case 'foo':