From 155650a7d84703ef6df39329d57914f363844483 Mon Sep 17 00:00:00 2001 From: Mayur Kale Date: Mon, 30 Mar 2020 15:09:17 -0700 Subject: [PATCH 1/3] metrics: remove LabeSet API --- examples/metrics/metrics/observer.js | 8 +- examples/prometheus/index.js | 4 +- getting-started/README.md | 8 +- .../monitored-example/monitoring.js | 4 +- getting-started/ts-example/README.md | 8 +- getting-started/ts-example/monitoring.ts | 4 +- .../opentelemetry-api/src/metrics/Meter.ts | 10 +- .../opentelemetry-api/src/metrics/Metric.ts | 31 ++--- .../src/metrics/NoopMeter.ts | 33 +++--- .../src/metrics/ObserverResult.ts | 6 +- .../noop-implementations/noop-meter.test.ts | 11 +- .../README.md | 4 +- .../src/prometheus.ts | 3 +- .../test/prometheus.test.ts | 17 ++- packages/opentelemetry-metrics/README.md | 2 +- .../src/BoundInstrument.ts | 32 +++--- .../opentelemetry-metrics/src/LabelSet.ts | 30 ----- packages/opentelemetry-metrics/src/Meter.ts | 23 ---- packages/opentelemetry-metrics/src/Metric.ts | 52 ++++----- .../src/ObserverResult.ts | 12 +- .../opentelemetry-metrics/src/export/types.ts | 5 +- packages/opentelemetry-metrics/src/index.ts | 1 - .../opentelemetry-metrics/test/Meter.test.ts | 106 +++++++----------- .../test/export/ConsoleMetricExporter.test.ts | 7 +- 24 files changed, 157 insertions(+), 264 deletions(-) delete mode 100644 packages/opentelemetry-metrics/src/LabelSet.ts diff --git a/examples/metrics/metrics/observer.js b/examples/metrics/metrics/observer.js index bfc28707a7..bb398bb95a 100644 --- a/examples/metrics/metrics/observer.js +++ b/examples/metrics/metrics/observer.js @@ -28,8 +28,8 @@ function getCpuUsage() { } otelCpuUsage.setCallback((observerResult) => { - observerResult.observe(getCpuUsage, meter.labels({ pid: process.pid, core: '1' })); - observerResult.observe(getCpuUsage, meter.labels({ pid: process.pid, core: '2' })); - observerResult.observe(getCpuUsage, meter.labels({ pid: process.pid, core: '3' })); - observerResult.observe(getCpuUsage, meter.labels({ pid: process.pid, core: '4' })); + observerResult.observe(getCpuUsage, { pid: process.pid, core: '1' }); + observerResult.observe(getCpuUsage, { pid: process.pid, core: '2' }); + observerResult.observe(getCpuUsage, { pid: process.pid, core: '3' }); + observerResult.observe(getCpuUsage, { pid: process.pid, core: '4' }); }); diff --git a/examples/prometheus/index.js b/examples/prometheus/index.js index a2d328383f..5d9b52c17a 100644 --- a/examples/prometheus/index.js +++ b/examples/prometheus/index.js @@ -31,9 +31,9 @@ const nonMonotonicCounter = meter.createCounter('non_monotonic_counter', { description: 'Example of a non-monotonic counter', }); -setInterval(() => { - const labels = meter.labels({ pid: process.pid }); +const labels = { pid: process.pid }; +setInterval(() => { monotonicCounter.bind(labels).add(1); nonMonotonicCounter.bind(labels).add(Math.random() > 0.5 ? 1 : -1); }, 1000); diff --git a/getting-started/README.md b/getting-started/README.md index 2e38ce9c0a..ce7b341dc5 100644 --- a/getting-started/README.md +++ b/getting-started/README.md @@ -262,8 +262,8 @@ const boundInstruments = new Map(); module.exports.countAllRequests = () => { return (req, res, next) => { if (!boundInstruments.has(req.path)) { - const labelSet = meter.labels({ route: req.path }); - const boundCounter = requestCount.bind(labelSet); + const labels = { route: req.path }; + const boundCounter = requestCount.bind(labels); boundInstruments.set(req.path, boundCounter); } @@ -328,8 +328,8 @@ const boundInstruments = new Map(); module.exports.countAllRequests = () => { return (req, res, next) => { if (!boundInstruments.has(req.path)) { - const labelSet = meter.labels({ route: req.path }); - const boundCounter = requestCount.bind(labelSet); + const labels = { route: req.path }; + const boundCounter = requestCount.bind(labels); boundInstruments.set(req.path, boundCounter); } diff --git a/getting-started/monitored-example/monitoring.js b/getting-started/monitored-example/monitoring.js index cedc2f13e5..3f34387711 100644 --- a/getting-started/monitored-example/monitoring.js +++ b/getting-started/monitored-example/monitoring.js @@ -28,8 +28,8 @@ const boundInstruments = new Map(); module.exports.countAllRequests = () => { return (req, res, next) => { if (!boundInstruments.has(req.path)) { - const labelSet = meter.labels({ route: req.path }); - const boundCounter = requestCount.bind(labelSet); + const labels = { route: req.path }; + const boundCounter = requestCount.bind(labels); boundInstruments.set(req.path, boundCounter); } diff --git a/getting-started/ts-example/README.md b/getting-started/ts-example/README.md index baefb9f26e..4414049bf7 100644 --- a/getting-started/ts-example/README.md +++ b/getting-started/ts-example/README.md @@ -261,8 +261,8 @@ const handles = new Map(); export const countAllRequests = () => { return (req, res, next) => { if (!handles.has(req.path)) { - const labelSet = meter.labels({ route: req.path }); - const handle = requestCount.bind(labelSet); + const labels = { route: req.path }; + const handle = requestCount.bind(labels); handles.set(req.path, handle); } @@ -326,8 +326,8 @@ const handles = new Map(); export const countAllRequests = () => { return (req, res, next) => { if (!handles.has(req.path)) { - const labelSet = meter.labels({ route: req.path }); - const handle = requestCount.bind(labelSet); + const labels = { route: req.path }; + const handle = requestCount.bind(labels); handles.set(req.path, handle); } diff --git a/getting-started/ts-example/monitoring.ts b/getting-started/ts-example/monitoring.ts index 0499685a29..a7e544d762 100644 --- a/getting-started/ts-example/monitoring.ts +++ b/getting-started/ts-example/monitoring.ts @@ -27,8 +27,8 @@ const handles = new Map(); export const countAllRequests = () => { return (req, res, next) => { if (!handles.has(req.path)) { - const labelSet = meter.labels({ route: req.path }); - const handle = requestCount.bind(labelSet); + const labels = { route: req.path }; + const handle = requestCount.bind(labels); handles.set(req.path, handle); } diff --git a/packages/opentelemetry-api/src/metrics/Meter.ts b/packages/opentelemetry-api/src/metrics/Meter.ts index 4348370fd0..5231629c91 100644 --- a/packages/opentelemetry-api/src/metrics/Meter.ts +++ b/packages/opentelemetry-api/src/metrics/Meter.ts @@ -14,7 +14,7 @@ * limitations under the License. */ -import { Metric, MetricOptions, Labels, LabelSet } from './Metric'; +import { Metric, MetricOptions } from './Metric'; import { BoundCounter, BoundMeasure, BoundObserver } from './BoundInstrument'; /** @@ -47,12 +47,4 @@ export interface Meter { * @param [options] the metric options. */ createObserver(name: string, options?: MetricOptions): Metric; - - /** - * Provide a pre-computed re-useable LabelSet by - * converting the unordered labels into a canonicalized - * set of labels with an unique identifier, useful for pre-aggregation. - * @param labels user provided unordered Labels. - */ - labels(labels: Labels): LabelSet; } diff --git a/packages/opentelemetry-api/src/metrics/Metric.ts b/packages/opentelemetry-api/src/metrics/Metric.ts index 23f19f6af2..c8ce15c2c4 100644 --- a/packages/opentelemetry-api/src/metrics/Metric.ts +++ b/packages/opentelemetry-api/src/metrics/Metric.ts @@ -79,20 +79,19 @@ export enum ValueType { */ export interface Metric { /** - * Returns a Instrument associated with specified LabelSet. + * Returns a Instrument associated with specified Labels. * It is recommended to keep a reference to the Instrument instead of always * calling this method for every operations. - * @param labels the canonicalized LabelSet used to associate with this - * metric instrument. + * @param labels key-values pairs that are associated with a specific metric + * that you want to record. */ - bind(labels: LabelSet): T; + bind(labels: Labels): T; /** * Removes the Instrument from the metric, if it is present. - * @param labels the canonicalized LabelSet used to associate with this - * metric instrument. + * @param labels key-values pairs that are associated with a specific metric. */ - unbind(labels: LabelSet): void; + unbind(labels: Labels): void; /** * Clears all timeseries from the Metric. @@ -104,7 +103,7 @@ export interface MetricUtils { /** * Adds the given value to the current value. Values cannot be negative. */ - add(value: number, labelSet: LabelSet): void; + add(value: number, labels: Labels): void; /** * Sets a callback where user can observe value for certain labels @@ -116,22 +115,22 @@ export interface MetricUtils { /** * Sets the given value. Values can be negative. */ - set(value: number, labelSet: LabelSet): void; + set(value: number, labels: Labels): void; /** * Records the given value to this measure. */ - record(value: number, labelSet: LabelSet): void; + record(value: number, labels: Labels): void; record( value: number, - labelSet: LabelSet, + labels: Labels, correlationContext: CorrelationContext ): void; record( value: number, - labelSet: LabelSet, + labels: Labels, correlationContext: CorrelationContext, spanContext: SpanContext ): void; @@ -141,11 +140,3 @@ export interface MetricUtils { * key-value pairs passed by the user. */ export type Labels = Record; - -/** - * Canonicalized labels with an unique string identifier. - */ -export interface LabelSet { - identifier: string; - labels: Labels; -} diff --git a/packages/opentelemetry-api/src/metrics/NoopMeter.ts b/packages/opentelemetry-api/src/metrics/NoopMeter.ts index 02d4cb2bdc..919f84aa79 100644 --- a/packages/opentelemetry-api/src/metrics/NoopMeter.ts +++ b/packages/opentelemetry-api/src/metrics/NoopMeter.ts @@ -15,7 +15,7 @@ */ import { Meter } from './Meter'; -import { MetricOptions, Metric, Labels, LabelSet, MetricUtils } from './Metric'; +import { MetricOptions, Metric, Labels, MetricUtils } from './Metric'; import { BoundMeasure, BoundCounter, BoundObserver } from './BoundInstrument'; import { CorrelationContext } from '../correlation_context/CorrelationContext'; import { SpanContext } from '../trace/span_context'; @@ -54,10 +54,6 @@ export class NoopMeter implements Meter { createObserver(name: string, options?: MetricOptions): Metric { return NOOP_OBSERVER_METRIC; } - - labels(labels: Labels): LabelSet { - return NOOP_LABEL_SET; - } } export class NoopMetric implements Metric { @@ -67,22 +63,21 @@ export class NoopMetric implements Metric { this._instrument = instrument; } /** - * Returns a Bound Instrument associated with specified LabelSet. + * Returns a Bound Instrument associated with specified Labels. * It is recommended to keep a reference to the Bound Instrument instead of * always calling this method for every operations. - * @param labels the canonicalized LabelSet used to associate with this - * metric instrument. + * @param labels key-values pairs that are associated with a specific metric + * that you want to record. */ - bind(labels: LabelSet): T { + bind(labels: Labels): T { return this._instrument; } /** * Removes the Binding from the metric, if it is present. - * @param labels the canonicalized LabelSet used to associate with this - * metric instrument. + * @param labels key-values pairs that are associated with a specific metric. */ - unbind(labels: LabelSet): void { + unbind(labels: Labels): void { return; } @@ -96,8 +91,8 @@ export class NoopMetric implements Metric { export class NoopCounterMetric extends NoopMetric implements Pick { - add(value: number, labelSet: LabelSet) { - this.bind(labelSet).add(value); + add(value: number, labels: Labels) { + this.bind(labels).add(value); } } @@ -105,16 +100,16 @@ export class NoopMeasureMetric extends NoopMetric implements Pick { record( value: number, - labelSet: LabelSet, + labels: Labels, correlationContext?: CorrelationContext, spanContext?: SpanContext ) { if (typeof correlationContext === 'undefined') { - this.bind(labelSet).record(value); + this.bind(labels).record(value); } else if (typeof spanContext === 'undefined') { - this.bind(labelSet).record(value, correlationContext); + this.bind(labels).record(value, correlationContext); } else { - this.bind(labelSet).record(value, correlationContext, spanContext); + this.bind(labels).record(value, correlationContext, spanContext); } } } @@ -154,4 +149,4 @@ export const NOOP_MEASURE_METRIC = new NoopMeasureMetric(NOOP_BOUND_MEASURE); export const NOOP_BOUND_OBSERVER = new NoopBoundObserver(); export const NOOP_OBSERVER_METRIC = new NoopObserverMetric(NOOP_BOUND_OBSERVER); -export const NOOP_LABEL_SET = {} as LabelSet; +export const NOOP_LABELS = {} as Labels; diff --git a/packages/opentelemetry-api/src/metrics/ObserverResult.ts b/packages/opentelemetry-api/src/metrics/ObserverResult.ts index f3eb082fd4..6fb4edba70 100644 --- a/packages/opentelemetry-api/src/metrics/ObserverResult.ts +++ b/packages/opentelemetry-api/src/metrics/ObserverResult.ts @@ -14,12 +14,12 @@ * limitations under the License. */ -import { LabelSet } from './Metric'; +import { Labels } from './Metric'; /** * Interface that is being used in function setCallback for Observer Metric */ export interface ObserverResult { - observers: Map; - observe(callback: Function, labelSet: LabelSet): void; + observers: Map; + observe(callback: Function, labels: Labels): void; } diff --git a/packages/opentelemetry-api/test/noop-implementations/noop-meter.test.ts b/packages/opentelemetry-api/test/noop-implementations/noop-meter.test.ts index c3d0388436..a4b4e6c444 100644 --- a/packages/opentelemetry-api/test/noop-implementations/noop-meter.test.ts +++ b/packages/opentelemetry-api/test/noop-implementations/noop-meter.test.ts @@ -29,23 +29,22 @@ describe('NoopMeter', () => { const meter = new NoopMeterProvider().getMeter('test-noop'); const counter = meter.createCounter('some-name'); const labels = {} as Labels; - const labelSet = meter.labels(labels); // ensure NoopMetric does not crash. - counter.bind(labelSet).add(1); - counter.unbind(labelSet); + counter.bind(labels).add(1); + counter.unbind(labels); // ensure the correct noop const is returned assert.strictEqual(counter, NOOP_COUNTER_METRIC); - assert.strictEqual(counter.bind(labelSet), NOOP_BOUND_COUNTER); + assert.strictEqual(counter.bind(labels), NOOP_BOUND_COUNTER); counter.clear(); const measure = meter.createMeasure('some-name'); - measure.bind(labelSet).record(1); + measure.bind(labels).record(1); // ensure the correct noop const is returned assert.strictEqual(measure, NOOP_MEASURE_METRIC); - assert.strictEqual(measure.bind(labelSet), NOOP_BOUND_MEASURE); + assert.strictEqual(measure.bind(labels), NOOP_BOUND_MEASURE); const options = { component: 'tests', diff --git a/packages/opentelemetry-exporter-prometheus/README.md b/packages/opentelemetry-exporter-prometheus/README.md index b576d842e7..69ab91e5ff 100644 --- a/packages/opentelemetry-exporter-prometheus/README.md +++ b/packages/opentelemetry-exporter-prometheus/README.md @@ -36,11 +36,11 @@ const meter = new MeterProvider({ // Now, start recording data const counter = meter.createCounter('metric_name'); -counter.add(10, meter.labels({ [key]: 'value' })); +counter.add(10, { [key]: 'value' }); // Record data using Instrument: It is recommended to keep a reference to the Bound Instrument instead of // always calling `bind()` method for every operations. -const boundCounter = counter.bind(meter.labels({ [key]: 'value' })); +const boundCounter = counter.bind({ [key]: 'value' }); boundCounter.add(10); // .. some other work diff --git a/packages/opentelemetry-exporter-prometheus/src/prometheus.ts b/packages/opentelemetry-exporter-prometheus/src/prometheus.ts index b3a8aac621..d8d9ec29e0 100644 --- a/packages/opentelemetry-exporter-prometheus/src/prometheus.ts +++ b/packages/opentelemetry-exporter-prometheus/src/prometheus.ts @@ -155,9 +155,8 @@ export class PrometheusExporter implements MetricExporter { // TODO: only counter and gauge are implemented in metrics so far } - private _getLabelValues(keys: string[], values: types.LabelSet) { + private _getLabelValues(keys: string[], labels: types.Labels) { const labelValues: labelValues = {}; - const labels = values.labels; for (let i = 0; i < keys.length; i++) { if (labels[keys[i]] !== null) { labelValues[keys[i]] = labels[keys[i]]; diff --git a/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts b/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts index 263e624ade..e4dcb00578 100644 --- a/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts +++ b/packages/opentelemetry-exporter-prometheus/test/prometheus.test.ts @@ -186,7 +186,7 @@ describe('PrometheusExporter', () => { labelKeys: ['key1'], }); - const boundCounter = counter.bind(meter.labels({ key1: 'labelValue1' })); + const boundCounter = counter.bind({ key1: 'labelValue1' }); boundCounter.add(10); meter.collect(); exporter.export(meter.getBatcher().checkPointSet(), () => { @@ -232,10 +232,7 @@ describe('PrometheusExporter', () => { }) as ObserverMetric; observer.setCallback((observerResult: ObserverResult) => { - observerResult.observe( - getCpuUsage, - meter.labels({ pid: String(123), core: '1' }) - ); + observerResult.observe(getCpuUsage, { pid: String(123), core: '1' }); }); meter.collect(); @@ -275,7 +272,7 @@ describe('PrometheusExporter', () => { labelKeys: ['counterKey1'], }) as CounterMetric; - counter.bind(meter.labels({ counterKey1: 'labelValue1' })).add(10); + counter.bind({ counterKey1: 'labelValue1' }).add(10); meter.collect(); exporter.export(meter.getBatcher().checkPointSet(), () => { http @@ -318,7 +315,7 @@ describe('PrometheusExporter', () => { it('should add a description if missing', done => { const counter = meter.createCounter('counter'); - const boundCounter = counter.bind(meter.labels({ key1: 'labelValue1' })); + const boundCounter = counter.bind({ key1: 'labelValue1' }); boundCounter.add(10); meter.collect(); exporter.export(meter.getBatcher().checkPointSet(), () => { @@ -344,7 +341,7 @@ describe('PrometheusExporter', () => { it('should sanitize names', done => { const counter = meter.createCounter('counter.bad-name'); - const boundCounter = counter.bind(meter.labels({ key1: 'labelValue1' })); + const boundCounter = counter.bind({ key1: 'labelValue1' }); boundCounter.add(10); meter.collect(); exporter.export(meter.getBatcher().checkPointSet(), () => { @@ -375,7 +372,7 @@ describe('PrometheusExporter', () => { labelKeys: ['key1'], }); - counter.bind(meter.labels({ key1: 'labelValue1' })).add(20); + counter.bind({ key1: 'labelValue1' }).add(20); meter.collect(); exporter.export(meter.getBatcher().checkPointSet(), () => { http @@ -404,7 +401,7 @@ describe('PrometheusExporter', () => { beforeEach(() => { meter = new MeterProvider().getMeter('test-prometheus'); counter = meter.createCounter('counter') as CounterMetric; - counter.bind(meter.labels({ key1: 'labelValue1' })).add(10); + counter.bind({ key1: 'labelValue1' }).add(10); }); afterEach(done => { diff --git a/packages/opentelemetry-metrics/README.md b/packages/opentelemetry-metrics/README.md index abf4052e49..eb516f6948 100644 --- a/packages/opentelemetry-metrics/README.md +++ b/packages/opentelemetry-metrics/README.md @@ -29,7 +29,7 @@ const counter = meter.createCounter('metric_name', { description: "Example of a counter" }); -const labels = meter.labels({ pid: process.pid }); +const labels = { pid: process.pid }; // Create a BoundInstrument associated with specified label values. const boundCounter = counter.bind(labels); diff --git a/packages/opentelemetry-metrics/src/BoundInstrument.ts b/packages/opentelemetry-metrics/src/BoundInstrument.ts index 7a98698dba..297934cb85 100644 --- a/packages/opentelemetry-metrics/src/BoundInstrument.ts +++ b/packages/opentelemetry-metrics/src/BoundInstrument.ts @@ -23,19 +23,19 @@ import { ObserverResult } from './ObserverResult'; * the TimeSeries. */ export class BaseBoundInstrument { - protected _labelSet: types.LabelSet; + protected _labels: types.Labels; protected _logger: types.Logger; protected _monotonic: boolean; constructor( - labelSet: types.LabelSet, + labels: types.Labels, logger: types.Logger, monotonic: boolean, private readonly _disabled: boolean, private readonly _valueType: types.ValueType, private readonly _aggregator: Aggregator ) { - this._labelSet = labelSet; + this._labels = labels; this._logger = logger; this._monotonic = monotonic; } @@ -46,7 +46,7 @@ export class BaseBoundInstrument { if (this._valueType === types.ValueType.INT && !Number.isInteger(value)) { this._logger.warn( `INT value type cannot accept a floating-point value for ${Object.values( - this._labelSet.labels + this._labels )}, ignoring the fractional digits.` ); value = Math.trunc(value); @@ -55,8 +55,8 @@ export class BaseBoundInstrument { this._aggregator.update(value); } - getLabelSet(): types.LabelSet { - return this._labelSet; + getLabels(): types.Labels { + return this._labels; } getAggregator(): Aggregator { @@ -66,27 +66,25 @@ export class BaseBoundInstrument { /** * BoundCounter allows the SDK to observe/record a single metric event. The - * value of single instrument in the `Counter` associated with specified LabelSet. + * value of single instrument in the `Counter` associated with specified Labels. */ export class BoundCounter extends BaseBoundInstrument implements types.BoundCounter { constructor( - labelSet: types.LabelSet, + labels: types.Labels, disabled: boolean, monotonic: boolean, valueType: types.ValueType, logger: types.Logger, aggregator: Aggregator ) { - super(labelSet, logger, monotonic, disabled, valueType, aggregator); + super(labels, logger, monotonic, disabled, valueType, aggregator); } add(value: number): void { if (this._monotonic && value < 0) { this._logger.error( - `Monotonic counter cannot descend for ${Object.values( - this._labelSet.labels - )}` + `Monotonic counter cannot descend for ${Object.values(this._labels)}` ); return; } @@ -103,7 +101,7 @@ export class BoundMeasure extends BaseBoundInstrument private readonly _absolute: boolean; constructor( - labelSet: types.LabelSet, + labels: types.Labels, disabled: boolean, monotonic: boolean, absolute: boolean, @@ -111,7 +109,7 @@ export class BoundMeasure extends BaseBoundInstrument logger: types.Logger, aggregator: Aggregator ) { - super(labelSet, logger, monotonic, disabled, valueType, aggregator); + super(labels, logger, monotonic, disabled, valueType, aggregator); this._absolute = absolute; } @@ -123,7 +121,7 @@ export class BoundMeasure extends BaseBoundInstrument if (this._absolute && value < 0) { this._logger.error( `Absolute measure cannot contain negative values for ${Object.values( - this._labelSet.labels + this._labels )}}` ); return; @@ -139,14 +137,14 @@ export class BoundMeasure extends BaseBoundInstrument export class BoundObserver extends BaseBoundInstrument implements types.BoundObserver { constructor( - labelSet: types.LabelSet, + labels: types.Labels, disabled: boolean, monotonic: boolean, valueType: types.ValueType, logger: types.Logger, aggregator: Aggregator ) { - super(labelSet, logger, monotonic, disabled, valueType, aggregator); + super(labels, logger, monotonic, disabled, valueType, aggregator); } setCallback(callback: (observerResult: types.ObserverResult) => {}): void { diff --git a/packages/opentelemetry-metrics/src/LabelSet.ts b/packages/opentelemetry-metrics/src/LabelSet.ts deleted file mode 100644 index 633f113ac5..0000000000 --- a/packages/opentelemetry-metrics/src/LabelSet.ts +++ /dev/null @@ -1,30 +0,0 @@ -/*! - * 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 types from '@opentelemetry/api'; - -/** - * Canonicalized labels with an unique string identifier. - */ -export class LabelSet implements types.LabelSet { - identifier: string; - labels: types.Labels; - - constructor(identifier: string, labels: types.Labels) { - this.identifier = identifier; - this.labels = labels; - } -} diff --git a/packages/opentelemetry-metrics/src/Meter.ts b/packages/opentelemetry-metrics/src/Meter.ts index 483e553722..524b2858cc 100644 --- a/packages/opentelemetry-metrics/src/Meter.ts +++ b/packages/opentelemetry-metrics/src/Meter.ts @@ -25,7 +25,6 @@ import { DEFAULT_CONFIG, MeterConfig, } from './types'; -import { LabelSet } from './LabelSet'; import { Batcher, UngroupedBatcher } from './export/Batcher'; import { PushController } from './export/Controller'; import { NoopExporter } from './export/NoopExporter'; @@ -38,7 +37,6 @@ export class Meter implements types.Meter { private readonly _metrics = new Map>(); private readonly _batcher: Batcher; private readonly _resource: Resource; - readonly labels = Meter.labels; /** * Constructs a new Meter instance. @@ -161,27 +159,6 @@ export class Meter implements types.Meter { return this._batcher; } - /** - * Provide a pre-computed re-useable LabelSet by - * converting the unordered labels into a canonicalized - * set of labels with an unique identifier, useful for pre-aggregation. - * @param labels user provided unordered Labels. - */ - static labels(labels: types.Labels): types.LabelSet { - const keys = Object.keys(labels).sort(); - const identifier = keys.reduce((result, key) => { - if (result.length > 2) { - result += ','; - } - return (result += key + ':' + labels[key]); - }, '|#'); - const sortedLabels: types.Labels = {}; - keys.forEach(key => { - sortedLabels[key] = labels[key]; - }); - return new LabelSet(identifier, sortedLabels); - } - /** * Registers metric to register. * @param name The name of the metric. diff --git a/packages/opentelemetry-metrics/src/Metric.ts b/packages/opentelemetry-metrics/src/Metric.ts index f98a5a256e..12c4a2c259 100644 --- a/packages/opentelemetry-metrics/src/Metric.ts +++ b/packages/opentelemetry-metrics/src/Metric.ts @@ -51,27 +51,26 @@ export abstract class Metric } /** - * Returns an Instrument associated with specified LabelSet. + * Returns an Instrument associated with specified Labels. * It is recommended to keep a reference to the Instrument instead of always * calling this method for each operation. - * @param labelSet the canonicalized LabelSet used to associate with this metric instrument. + * @param labels key-values pairs that are associated with a specific metric + * that you want to record. */ - bind(labelSet: types.LabelSet): T { - if (this._instruments.has(labelSet.identifier)) - return this._instruments.get(labelSet.identifier)!; + bind(labels: types.Labels): T { + if (this._instruments.has(labels)) return this._instruments.get(labels)!; - const instrument = this._makeInstrument(labelSet); - this._instruments.set(labelSet.identifier, instrument); + const instrument = this._makeInstrument(labels); + this._instruments.set(labels, instrument); return instrument; } /** * Removes the Instrument from the metric, if it is present. - * @param labelSet the canonicalized LabelSet used to associate with this - * metric instrument. + * @param labels key-values pairs that are associated with a specific metric. */ - unbind(labelSet: types.LabelSet): void { - this._instruments.delete(labelSet.identifier); + unbind(labels: types.Labels): void { + this._instruments.delete(labels); } /** @@ -84,7 +83,7 @@ export abstract class Metric getMetricRecord(): MetricRecord[] { return Array.from(this._instruments.values()).map(instrument => ({ descriptor: this._descriptor, - labels: instrument.getLabelSet(), + labels: instrument.getLabels(), aggregator: instrument.getAggregator(), })); } @@ -101,7 +100,7 @@ export abstract class Metric }; } - protected abstract _makeInstrument(labelSet: types.LabelSet): T; + protected abstract _makeInstrument(labels: types.Labels): T; } /** This is a SDK implementation of Counter Metric. */ @@ -115,9 +114,9 @@ export class CounterMetric extends Metric ) { super(name, options, MetricKind.COUNTER, resource); } - protected _makeInstrument(labelSet: types.LabelSet): BoundCounter { + protected _makeInstrument(labels: types.Labels): BoundCounter { return new BoundCounter( - labelSet, + labels, this._disabled, this._monotonic, this._valueType, @@ -130,10 +129,11 @@ export class CounterMetric extends Metric /** * Adds the given value to the current value. Values cannot be negative. * @param value the value to add. - * @param labelSet the canonicalized LabelSet used to associate with this metric's instrument. + * @param labels key-values pairs that are associated with a specific metric + * that you want to record. */ - add(value: number, labelSet: types.LabelSet) { - this.bind(labelSet).add(value); + add(value: number, labels: types.Labels) { + this.bind(labels).add(value); } } @@ -151,9 +151,9 @@ export class MeasureMetric extends Metric this._absolute = options.absolute !== undefined ? options.absolute : true; // Absolute default is true } - protected _makeInstrument(labelSet: types.LabelSet): BoundMeasure { + protected _makeInstrument(labels: types.Labels): BoundMeasure { return new BoundMeasure( - labelSet, + labels, this._disabled, this._monotonic, this._absolute, @@ -163,8 +163,8 @@ export class MeasureMetric extends Metric ); } - record(value: number, labelSet: types.LabelSet) { - this.bind(labelSet).record(value); + record(value: number, labels: types.Labels) { + this.bind(labels).record(value); } } @@ -182,9 +182,9 @@ export class ObserverMetric extends Metric super(name, options, MetricKind.OBSERVER, resource); } - protected _makeInstrument(labelSet: types.LabelSet): BoundObserver { + protected _makeInstrument(labels: types.Labels): BoundObserver { return new BoundObserver( - labelSet, + labels, this._disabled, this._monotonic, this._valueType, @@ -194,8 +194,8 @@ export class ObserverMetric extends Metric } getMetricRecord(): MetricRecord[] { - this._observerResult.observers.forEach((callback, labelSet) => { - const instrument = this.bind(labelSet); + this._observerResult.observers.forEach((callback, labels) => { + const instrument = this.bind(labels); instrument.update(callback()); }); return super.getMetricRecord(); diff --git a/packages/opentelemetry-metrics/src/ObserverResult.ts b/packages/opentelemetry-metrics/src/ObserverResult.ts index 6f7e629269..1cb9ecebd4 100644 --- a/packages/opentelemetry-metrics/src/ObserverResult.ts +++ b/packages/opentelemetry-metrics/src/ObserverResult.ts @@ -14,15 +14,17 @@ * limitations under the License. */ -import { ObserverResult as TypeObserverResult } from '@opentelemetry/api'; -import { LabelSet } from './LabelSet'; +import { + ObserverResult as TypeObserverResult, + Labels, +} from '@opentelemetry/api'; /** * Implementation of {@link TypeObserverResult} */ export class ObserverResult implements TypeObserverResult { - observers = new Map(); - observe(callback: any, labelSet: LabelSet): void { - this.observers.set(labelSet, callback); + observers = new Map(); + observe(callback: any, labels: Labels): void { + this.observers.set(labels, callback); } } diff --git a/packages/opentelemetry-metrics/src/export/types.ts b/packages/opentelemetry-metrics/src/export/types.ts index 32e0c077a5..ff4f1173eb 100644 --- a/packages/opentelemetry-metrics/src/export/types.ts +++ b/packages/opentelemetry-metrics/src/export/types.ts @@ -14,9 +14,8 @@ * limitations under the License. */ -import { ValueType, HrTime } from '@opentelemetry/api'; +import { ValueType, HrTime, Labels } from '@opentelemetry/api'; import { ExportResult } from '@opentelemetry/base'; -import { LabelSet } from '../LabelSet'; /** The kind of metric. */ export enum MetricKind { @@ -40,7 +39,7 @@ export interface Distribution { export interface MetricRecord { readonly descriptor: MetricDescriptor; - readonly labels: LabelSet; + readonly labels: Labels; readonly aggregator: Aggregator; } diff --git a/packages/opentelemetry-metrics/src/index.ts b/packages/opentelemetry-metrics/src/index.ts index 2ecbb7c751..c9f158528f 100644 --- a/packages/opentelemetry-metrics/src/index.ts +++ b/packages/opentelemetry-metrics/src/index.ts @@ -14,7 +14,6 @@ * limitations under the License. */ -export * from './LabelSet'; export * from './BoundInstrument'; export * from './Meter'; export * from './Metric'; diff --git a/packages/opentelemetry-metrics/test/Meter.test.ts b/packages/opentelemetry-metrics/test/Meter.test.ts index 2a91e2bd82..487a58e6b5 100644 --- a/packages/opentelemetry-metrics/test/Meter.test.ts +++ b/packages/opentelemetry-metrics/test/Meter.test.ts @@ -28,38 +28,24 @@ import { MetricRecord, } from '../src'; import * as types from '@opentelemetry/api'; -import { LabelSet } from '../src/LabelSet'; import { NoopLogger, hrTime, hrTimeToNanoseconds } from '@opentelemetry/core'; import { CounterSumAggregator, ObserverAggregator, } from '../src/export/Aggregator'; -import { ValueType } from '@opentelemetry/api'; +import { ValueType, Labels } from '@opentelemetry/api'; import { Resource } from '@opentelemetry/resources'; describe('Meter', () => { let meter: Meter; const keya = 'keya'; const keyb = 'keyb'; - let labels: types.Labels = { [keyb]: 'value2', [keya]: 'value1' }; - let labelSet: types.LabelSet; + const labels: types.Labels = { [keyb]: 'value2', [keya]: 'value1' }; beforeEach(() => { meter = new MeterProvider({ logger: new NoopLogger(), }).getMeter('test-meter'); - labelSet = meter.labels(labels); - }); - - describe('#meter', () => { - it('should re-order labels to a canonicalized set', () => { - const orderedLabels: types.Labels = { - [keya]: 'value1', - [keyb]: 'value2', - }; - const identifier = '|#keya:value1,keyb:value2'; - assert.deepEqual(labelSet, new LabelSet(identifier, orderedLabels)); - }); }); describe('#counter', () => { @@ -82,7 +68,7 @@ describe('Meter', () => { it('should be able to call add() directly on counter', () => { const counter = meter.createCounter('name') as CounterMetric; - counter.add(10, labelSet); + counter.add(10, labels); meter.collect(); const [record1] = meter.getBatcher().checkPointSet(); @@ -92,7 +78,7 @@ describe('Meter', () => { hrTimeToNanoseconds(lastTimestamp) > hrTimeToNanoseconds(performanceTimeOrigin) ); - counter.add(10, labelSet); + counter.add(10, labels); assert.strictEqual(record1.aggregator.toPoint().value, 20); assert.ok( @@ -109,7 +95,7 @@ describe('Meter', () => { describe('.bind()', () => { it('should create a counter instrument', () => { const counter = meter.createCounter('name') as CounterMetric; - const boundCounter = counter.bind(labelSet); + const boundCounter = counter.bind(labels); boundCounter.add(10); meter.collect(); const [record1] = meter.getBatcher().checkPointSet(); @@ -121,15 +107,15 @@ describe('Meter', () => { it('should return the aggregator', () => { const counter = meter.createCounter('name') as CounterMetric; - const boundCounter = counter.bind(labelSet); + const boundCounter = counter.bind(labels); boundCounter.add(20); assert.ok(boundCounter.getAggregator() instanceof CounterSumAggregator); - assert.strictEqual(boundCounter.getLabelSet(), labelSet); + assert.strictEqual(boundCounter.getLabels(), labels); }); it('should add positive values by default', () => { const counter = meter.createCounter('name') as CounterMetric; - const boundCounter = counter.bind(labelSet); + const boundCounter = counter.bind(labels); boundCounter.add(10); assert.strictEqual(meter.getBatcher().checkPointSet().length, 0); meter.collect(); @@ -144,7 +130,7 @@ describe('Meter', () => { const counter = meter.createCounter('name', { disabled: true, }) as CounterMetric; - const boundCounter = counter.bind(labelSet); + const boundCounter = counter.bind(labels); boundCounter.add(10); meter.collect(); const [record1] = meter.getBatcher().checkPointSet(); @@ -155,7 +141,7 @@ describe('Meter', () => { const counter = meter.createCounter('name', { monotonic: false, }) as CounterMetric; - const boundCounter = counter.bind(labelSet); + const boundCounter = counter.bind(labels); boundCounter.add(-10); meter.collect(); const [record1] = meter.getBatcher().checkPointSet(); @@ -164,9 +150,9 @@ describe('Meter', () => { it('should return same instrument on same label values', () => { const counter = meter.createCounter('name') as CounterMetric; - const boundCounter = counter.bind(labelSet); + const boundCounter = counter.bind(labels); boundCounter.add(10); - const boundCounter1 = counter.bind(labelSet); + const boundCounter1 = counter.bind(labels); boundCounter1.add(10); meter.collect(); const [record1] = meter.getBatcher().checkPointSet(); @@ -179,23 +165,23 @@ describe('Meter', () => { describe('.unbind()', () => { it('should remove a counter instrument', () => { const counter = meter.createCounter('name') as CounterMetric; - const boundCounter = counter.bind(labelSet); + const boundCounter = counter.bind(labels); assert.strictEqual(counter['_instruments'].size, 1); - counter.unbind(labelSet); + counter.unbind(labels); assert.strictEqual(counter['_instruments'].size, 0); - const boundCounter1 = counter.bind(labelSet); + const boundCounter1 = counter.bind(labels); assert.strictEqual(counter['_instruments'].size, 1); assert.notStrictEqual(boundCounter, boundCounter1); }); it('should not fail when removing non existing instrument', () => { const counter = meter.createCounter('name'); - counter.unbind(new LabelSet('', {})); + counter.unbind({} as Labels); }); it('should clear all instruments', () => { const counter = meter.createCounter('name') as CounterMetric; - counter.bind(labelSet); + counter.bind(labels); assert.strictEqual(counter['_instruments'].size, 1); counter.clear(); assert.strictEqual(counter['_instruments'].size, 0); @@ -205,13 +191,13 @@ describe('Meter', () => { describe('.registerMetric()', () => { it('skip already registered Metric', () => { const counter1 = meter.createCounter('name1') as CounterMetric; - counter1.bind(labelSet).add(10); + counter1.bind(labels).add(10); // should skip below metric const counter2 = meter.createCounter('name1', { valueType: types.ValueType.INT, }) as CounterMetric; - counter2.bind(labelSet).add(500); + counter2.bind(labels).add(500); meter.collect(); const record = meter.getBatcher().checkPointSet(); @@ -322,13 +308,13 @@ describe('Meter', () => { it('should create a measure instrument', () => { const measure = meter.createMeasure('name') as MeasureMetric; - const boundMeasure = measure.bind(labelSet); + const boundMeasure = measure.bind(labels); assert.doesNotThrow(() => boundMeasure.record(10)); }); it('should not accept negative values by default', () => { const measure = meter.createMeasure('name'); - const boundMeasure = measure.bind(labelSet); + const boundMeasure = measure.bind(labels); boundMeasure.record(-10); meter.collect(); @@ -348,7 +334,7 @@ describe('Meter', () => { const measure = meter.createMeasure('name', { disabled: true, }) as MeasureMetric; - const boundMeasure = measure.bind(labelSet); + const boundMeasure = measure.bind(labels); boundMeasure.record(10); meter.collect(); @@ -368,7 +354,7 @@ describe('Meter', () => { const measure = meter.createMeasure('name', { absolute: false, }); - const boundMeasure = measure.bind(labelSet); + const boundMeasure = measure.bind(labels); boundMeasure.record(-10); boundMeasure.record(50); @@ -391,9 +377,9 @@ describe('Meter', () => { it('should return same instrument on same label values', () => { const measure = meter.createMeasure('name') as MeasureMetric; - const boundMeasure1 = measure.bind(labelSet); + const boundMeasure1 = measure.bind(labels); boundMeasure1.record(10); - const boundMeasure2 = measure.bind(labelSet); + const boundMeasure2 = measure.bind(labels); boundMeasure2.record(100); meter.collect(); const [record1] = meter.getBatcher().checkPointSet(); @@ -413,23 +399,23 @@ describe('Meter', () => { describe('.unbind()', () => { it('should remove the measure instrument', () => { const measure = meter.createMeasure('name') as MeasureMetric; - const boundMeasure = measure.bind(labelSet); + const boundMeasure = measure.bind(labels); assert.strictEqual(measure['_instruments'].size, 1); - measure.unbind(labelSet); + measure.unbind(labels); assert.strictEqual(measure['_instruments'].size, 0); - const boundMeasure2 = measure.bind(labelSet); + const boundMeasure2 = measure.bind(labels); assert.strictEqual(measure['_instruments'].size, 1); assert.notStrictEqual(boundMeasure, boundMeasure2); }); it('should not fail when removing non existing instrument', () => { const measure = meter.createMeasure('name'); - measure.unbind(new LabelSet('nonexistant', {})); + measure.unbind({}); }); it('should clear all instruments', () => { const measure = meter.createMeasure('name') as MeasureMetric; - measure.bind(labelSet); + measure.bind(labels); assert.strictEqual(measure['_instruments'].size, 1); measure.clear(); assert.strictEqual(measure['_instruments'].size, 0); @@ -462,22 +448,10 @@ describe('Meter', () => { } measure.setCallback((observerResult: types.ObserverResult) => { - observerResult.observe( - getCpuUsage, - meter.labels({ pid: '123', core: '1' }) - ); - observerResult.observe( - getCpuUsage, - meter.labels({ pid: '123', core: '2' }) - ); - observerResult.observe( - getCpuUsage, - meter.labels({ pid: '123', core: '3' }) - ); - observerResult.observe( - getCpuUsage, - meter.labels({ pid: '123', core: '4' }) - ); + observerResult.observe(getCpuUsage, { pid: '123', core: '1' }); + observerResult.observe(getCpuUsage, { pid: '123', core: '2' }); + observerResult.observe(getCpuUsage, { pid: '123', core: '3' }); + observerResult.observe(getCpuUsage, { pid: '123', core: '4' }); }); const metricRecords: MetricRecord[] = measure.getMetricRecord(); @@ -512,8 +486,8 @@ describe('Meter', () => { description: 'test', labelKeys: [key], }); - const labelSet = meter.labels({ [key]: 'counter-value' }); - const boundCounter = counter.bind(labelSet); + const labels = { [key]: 'counter-value' }; + const boundCounter = counter.bind(labels); boundCounter.add(10.45); meter.collect(); @@ -529,7 +503,7 @@ describe('Meter', () => { valueType: ValueType.DOUBLE, labelKeys: ['key'], }); - assert.strictEqual(record[0].labels, labelSet); + assert.strictEqual(record[0].labels, labels); const value = record[0].aggregator.toPoint().value as Sum; assert.strictEqual(value, 10.45); }); @@ -541,8 +515,8 @@ describe('Meter', () => { labelKeys: [key], valueType: types.ValueType.INT, }); - const labelSet = meter.labels({ [key]: 'counter-value' }); - const boundCounter = counter.bind(labelSet); + const labels = { [key]: 'counter-value' }; + const boundCounter = counter.bind(labels); boundCounter.add(10.45); meter.collect(); @@ -558,7 +532,7 @@ describe('Meter', () => { valueType: ValueType.INT, labelKeys: ['key'], }); - assert.strictEqual(record[0].labels, labelSet); + assert.strictEqual(record[0].labels, labels); const value = record[0].aggregator.toPoint().value as Sum; assert.strictEqual(value, 10); }); diff --git a/packages/opentelemetry-metrics/test/export/ConsoleMetricExporter.test.ts b/packages/opentelemetry-metrics/test/export/ConsoleMetricExporter.test.ts index 953d79eb99..d9dc83315f 100644 --- a/packages/opentelemetry-metrics/test/export/ConsoleMetricExporter.test.ts +++ b/packages/opentelemetry-metrics/test/export/ConsoleMetricExporter.test.ts @@ -44,9 +44,10 @@ describe('ConsoleMetricExporter', () => { description: 'a test description', labelKeys: ['key1', 'key2'], }); - const boundCounter = counter.bind( - meter.labels({ key1: 'labelValue1', key2: 'labelValue2' }) - ); + const boundCounter = counter.bind({ + key1: 'labelValue1', + key2: 'labelValue2', + }); boundCounter.add(10); meter.collect(); From e4ad8629745b48afae9a3cd92b80454f7f85c037 Mon Sep 17 00:00:00 2001 From: Mayur Kale Date: Mon, 30 Mar 2020 15:42:37 -0700 Subject: [PATCH 2/3] fix: tests --- .../opentelemetry-api/src/metrics/Metric.ts | 2 +- .../src/metrics/NoopMeter.ts | 2 -- .../noop-implementations/noop-meter.test.ts | 3 +-- .../src/BoundInstrument.ts | 10 +++------- packages/opentelemetry-metrics/src/Metric.ts | 8 +++++--- packages/opentelemetry-metrics/src/Utils.ts | 19 +++++++++++++++++++ .../src/export/ConsoleMetricExporter.ts | 2 +- .../opentelemetry-metrics/test/Meter.test.ts | 13 +++++++------ 8 files changed, 37 insertions(+), 22 deletions(-) diff --git a/packages/opentelemetry-api/src/metrics/Metric.ts b/packages/opentelemetry-api/src/metrics/Metric.ts index c8ce15c2c4..d57896a1d3 100644 --- a/packages/opentelemetry-api/src/metrics/Metric.ts +++ b/packages/opentelemetry-api/src/metrics/Metric.ts @@ -139,4 +139,4 @@ export interface MetricUtils { /** * key-value pairs passed by the user. */ -export type Labels = Record; +export type Labels = { [key: string]: string }; diff --git a/packages/opentelemetry-api/src/metrics/NoopMeter.ts b/packages/opentelemetry-api/src/metrics/NoopMeter.ts index 919f84aa79..7825863818 100644 --- a/packages/opentelemetry-api/src/metrics/NoopMeter.ts +++ b/packages/opentelemetry-api/src/metrics/NoopMeter.ts @@ -148,5 +148,3 @@ export const NOOP_MEASURE_METRIC = new NoopMeasureMetric(NOOP_BOUND_MEASURE); export const NOOP_BOUND_OBSERVER = new NoopBoundObserver(); export const NOOP_OBSERVER_METRIC = new NoopObserverMetric(NOOP_BOUND_OBSERVER); - -export const NOOP_LABELS = {} as Labels; diff --git a/packages/opentelemetry-api/test/noop-implementations/noop-meter.test.ts b/packages/opentelemetry-api/test/noop-implementations/noop-meter.test.ts index a4b4e6c444..7b86f5d59d 100644 --- a/packages/opentelemetry-api/test/noop-implementations/noop-meter.test.ts +++ b/packages/opentelemetry-api/test/noop-implementations/noop-meter.test.ts @@ -16,7 +16,6 @@ import * as assert from 'assert'; import { - Labels, NoopMeterProvider, NOOP_BOUND_COUNTER, NOOP_BOUND_MEASURE, @@ -28,7 +27,7 @@ describe('NoopMeter', () => { it('should not crash', () => { const meter = new NoopMeterProvider().getMeter('test-noop'); const counter = meter.createCounter('some-name'); - const labels = {} as Labels; + const labels = {}; // ensure NoopMetric does not crash. counter.bind(labels).add(1); diff --git a/packages/opentelemetry-metrics/src/BoundInstrument.ts b/packages/opentelemetry-metrics/src/BoundInstrument.ts index 297934cb85..e27d5658be 100644 --- a/packages/opentelemetry-metrics/src/BoundInstrument.ts +++ b/packages/opentelemetry-metrics/src/BoundInstrument.ts @@ -45,9 +45,7 @@ export class BaseBoundInstrument { if (this._valueType === types.ValueType.INT && !Number.isInteger(value)) { this._logger.warn( - `INT value type cannot accept a floating-point value for ${Object.values( - this._labels - )}, ignoring the fractional digits.` + `INT value type cannot accept a floating-point value for ${this._labels}, ignoring the fractional digits.` ); value = Math.trunc(value); } @@ -84,7 +82,7 @@ export class BoundCounter extends BaseBoundInstrument add(value: number): void { if (this._monotonic && value < 0) { this._logger.error( - `Monotonic counter cannot descend for ${Object.values(this._labels)}` + `Monotonic counter cannot descend for ${this._labels}` ); return; } @@ -120,9 +118,7 @@ export class BoundMeasure extends BaseBoundInstrument ): void { if (this._absolute && value < 0) { this._logger.error( - `Absolute measure cannot contain negative values for ${Object.values( - this._labels - )}}` + `Absolute measure cannot contain negative values for $${this._labels}` ); return; } diff --git a/packages/opentelemetry-metrics/src/Metric.ts b/packages/opentelemetry-metrics/src/Metric.ts index 12c4a2c259..6d81b4e2f2 100644 --- a/packages/opentelemetry-metrics/src/Metric.ts +++ b/packages/opentelemetry-metrics/src/Metric.ts @@ -26,6 +26,7 @@ import { ObserverResult } from './ObserverResult'; import { MetricOptions } from './types'; import { MetricKind, MetricDescriptor, MetricRecord } from './export/types'; import { Batcher } from './export/Batcher'; +import { hashLabels } from './Utils'; /** This is a SDK implementation of {@link Metric} interface. */ export abstract class Metric @@ -58,10 +59,11 @@ export abstract class Metric * that you want to record. */ bind(labels: types.Labels): T { - if (this._instruments.has(labels)) return this._instruments.get(labels)!; + const hash = hashLabels(labels); + if (this._instruments.has(hash)) return this._instruments.get(hash)!; const instrument = this._makeInstrument(labels); - this._instruments.set(labels, instrument); + this._instruments.set(hash, instrument); return instrument; } @@ -70,7 +72,7 @@ export abstract class Metric * @param labels key-values pairs that are associated with a specific metric. */ unbind(labels: types.Labels): void { - this._instruments.delete(labels); + this._instruments.delete(hashLabels(labels)); } /** diff --git a/packages/opentelemetry-metrics/src/Utils.ts b/packages/opentelemetry-metrics/src/Utils.ts index e2c8fba463..4b94defa89 100644 --- a/packages/opentelemetry-metrics/src/Utils.ts +++ b/packages/opentelemetry-metrics/src/Utils.ts @@ -14,6 +14,8 @@ * limitations under the License. */ +import { Labels } from '@opentelemetry/api'; + /** * Type guard to remove nulls from arrays * @@ -22,3 +24,20 @@ export function notNull(value: T | null): value is T { return value !== null; } + +/** + * Converting the unordered labels into unique identifier string. + * @param labels user provided unordered Labels. + */ +export function hashLabels(labels: Labels): string { + let keys = Object.keys(labels); + if (keys.length === 0) return ''; + + keys = keys.sort(); + return keys.reduce((result, key) => { + if (result.length > 2) { + result += ','; + } + return (result += key + ':' + labels[key]); + }, '|#'); +} diff --git a/packages/opentelemetry-metrics/src/export/ConsoleMetricExporter.ts b/packages/opentelemetry-metrics/src/export/ConsoleMetricExporter.ts index ba0858be1c..78e5efc069 100644 --- a/packages/opentelemetry-metrics/src/export/ConsoleMetricExporter.ts +++ b/packages/opentelemetry-metrics/src/export/ConsoleMetricExporter.ts @@ -34,7 +34,7 @@ export class ConsoleMetricExporter implements MetricExporter { ): void { for (const metric of metrics) { console.log(metric.descriptor); - console.log(metric.labels.labels); + console.log(metric.labels); switch (metric.descriptor.metricKind) { case MetricKind.COUNTER: const sum = metric.aggregator.toPoint().value as Sum; diff --git a/packages/opentelemetry-metrics/test/Meter.test.ts b/packages/opentelemetry-metrics/test/Meter.test.ts index 487a58e6b5..71cd83e9e0 100644 --- a/packages/opentelemetry-metrics/test/Meter.test.ts +++ b/packages/opentelemetry-metrics/test/Meter.test.ts @@ -33,8 +33,9 @@ import { CounterSumAggregator, ObserverAggregator, } from '../src/export/Aggregator'; -import { ValueType, Labels } from '@opentelemetry/api'; +import { ValueType } from '@opentelemetry/api'; import { Resource } from '@opentelemetry/resources'; +import { hashLabels } from '../src/Utils'; describe('Meter', () => { let meter: Meter; @@ -176,7 +177,7 @@ describe('Meter', () => { it('should not fail when removing non existing instrument', () => { const counter = meter.createCounter('name'); - counter.unbind({} as Labels); + counter.unbind({}); }); it('should clear all instruments', () => { @@ -462,10 +463,10 @@ describe('Meter', () => { const metric3 = metricRecords[2]; const metric4 = metricRecords[3]; - assert.ok(metric1.labels.identifier.indexOf('|#core:1,pid:123') === 0); - assert.ok(metric2.labels.identifier.indexOf('|#core:2,pid:123') === 0); - assert.ok(metric3.labels.identifier.indexOf('|#core:3,pid:123') === 0); - assert.ok(metric4.labels.identifier.indexOf('|#core:4,pid:123') === 0); + assert.strictEqual(hashLabels(metric1.labels), '|#core:1,pid:123'); + assert.strictEqual(hashLabels(metric2.labels), '|#core:2,pid:123'); + assert.strictEqual(hashLabels(metric3.labels), '|#core:3,pid:123'); + assert.strictEqual(hashLabels(metric4.labels), '|#core:4,pid:123'); ensureMetric(metric1); ensureMetric(metric2); From e73043639def6b66d93c7550a909bd46e662f97a Mon Sep 17 00:00:00 2001 From: Mayur Kale Date: Mon, 30 Mar 2020 15:44:56 -0700 Subject: [PATCH 3/3] fix: lint --- packages/opentelemetry-metrics/src/BoundInstrument.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/opentelemetry-metrics/src/BoundInstrument.ts b/packages/opentelemetry-metrics/src/BoundInstrument.ts index e27d5658be..69f0de62cb 100644 --- a/packages/opentelemetry-metrics/src/BoundInstrument.ts +++ b/packages/opentelemetry-metrics/src/BoundInstrument.ts @@ -45,7 +45,9 @@ export class BaseBoundInstrument { if (this._valueType === types.ValueType.INT && !Number.isInteger(value)) { this._logger.warn( - `INT value type cannot accept a floating-point value for ${this._labels}, ignoring the fractional digits.` + `INT value type cannot accept a floating-point value for ${Object.values( + this._labels + )}, ignoring the fractional digits.` ); value = Math.trunc(value); } @@ -82,7 +84,7 @@ export class BoundCounter extends BaseBoundInstrument add(value: number): void { if (this._monotonic && value < 0) { this._logger.error( - `Monotonic counter cannot descend for ${this._labels}` + `Monotonic counter cannot descend for ${Object.values(this._labels)}` ); return; } @@ -118,7 +120,9 @@ export class BoundMeasure extends BaseBoundInstrument ): void { if (this._absolute && value < 0) { this._logger.error( - `Absolute measure cannot contain negative values for $${this._labels}` + `Absolute measure cannot contain negative values for $${Object.values( + this._labels + )}` ); return; }