From 4ccaed572ef51645c57816ee1dbca1160bad8772 Mon Sep 17 00:00:00 2001 From: legendecas Date: Wed, 2 Dec 2020 15:34:21 +0800 Subject: [PATCH 1/3] refactor: batch observer to be independent from metric types - BatchObservers do not have to be created with names - BatchObservers do not have instruments for their own --- examples/metrics/metrics/observer.js | 2 +- .../opentelemetry-api/src/metrics/Meter.ts | 7 +- .../opentelemetry-api/src/metrics/Metric.ts | 3 - .../src/metrics/NoopMeter.ts | 12 +- packages/opentelemetry-metrics/README.md | 2 +- ...atchObserverMetric.ts => BatchObserver.ts} | 43 ++----- packages/opentelemetry-metrics/src/Meter.ts | 53 +++------ .../opentelemetry-metrics/test/Meter.test.ts | 105 +++++++++--------- 8 files changed, 80 insertions(+), 147 deletions(-) rename packages/opentelemetry-metrics/src/{BatchObserverMetric.ts => BatchObserver.ts} (65%) diff --git a/examples/metrics/metrics/observer.js b/examples/metrics/metrics/observer.js index f8393122c18..aaece51489e 100644 --- a/examples/metrics/metrics/observer.js +++ b/examples/metrics/metrics/observer.js @@ -40,7 +40,7 @@ const cpuUsageMetric = meter.createValueObserver('cpu_usage_per_app', { description: 'Example of sync value observer used with async batch observer', }); -meter.createBatchObserver('metric_batch_observer', (observerBatchResult) => { +meter.createBatchObserver((observerBatchResult) => { Promise.all([ someAsyncMetrics(), // simulate waiting diff --git a/packages/opentelemetry-api/src/metrics/Meter.ts b/packages/opentelemetry-api/src/metrics/Meter.ts index eb570a48027..22fe74834b1 100644 --- a/packages/opentelemetry-api/src/metrics/Meter.ts +++ b/packages/opentelemetry-api/src/metrics/Meter.ts @@ -20,7 +20,6 @@ import { Counter, ValueRecorder, ValueObserver, - BatchObserver, BatchMetricOptions, UpDownCounter, } from './Metric'; @@ -82,15 +81,13 @@ export interface Meter { ): ValueObserver; /** - * Creates a new `BatchObserver` metric, can be used to update many metrics + * Creates a new `BatchObserver`, can be used to update many observer metrics * at the same time and when operations needs to be async - * @param name the name of the metric. * @param callback the batch observer callback * @param [options] the metric batch options. */ createBatchObserver( - name: string, callback: (batchObserverResult: BatchObserverResult) => void, options?: BatchMetricOptions - ): BatchObserver; + ): void; } diff --git a/packages/opentelemetry-api/src/metrics/Metric.ts b/packages/opentelemetry-api/src/metrics/Metric.ts index 083055fbed2..b0e2e1c9c31 100644 --- a/packages/opentelemetry-api/src/metrics/Metric.ts +++ b/packages/opentelemetry-api/src/metrics/Metric.ts @@ -166,9 +166,6 @@ export type UpDownSumObserver = BaseObserver; /** Base interface for the SumObserver metrics. */ export type SumObserver = BaseObserver; -/** Base interface for the Batch Observer metrics. */ -export type BatchObserver = Metric; - /** * key-value pairs passed by the user. */ diff --git a/packages/opentelemetry-api/src/metrics/NoopMeter.ts b/packages/opentelemetry-api/src/metrics/NoopMeter.ts index d59d658b1e3..1d86354cd76 100644 --- a/packages/opentelemetry-api/src/metrics/NoopMeter.ts +++ b/packages/opentelemetry-api/src/metrics/NoopMeter.ts @@ -23,7 +23,6 @@ import { Counter, ValueRecorder, ValueObserver, - BatchObserver, UpDownCounter, BaseObserver, } from './Metric'; @@ -90,10 +89,9 @@ export class NoopMeter implements Meter { * @param callback the batch observer callback */ createBatchObserver( - _name: string, _callback: (batchObserverResult: BatchObserverResult) => void - ): BatchObserver { - return NOOP_BATCH_OBSERVER_METRIC; + ): void { + return; } } @@ -158,10 +156,6 @@ export class NoopBaseObserverMetric } } -export class NoopBatchObserverMetric - extends NoopMetric - implements BatchObserver {} - export class NoopBoundCounter implements BoundCounter { add(_value: number): void { return; @@ -203,5 +197,3 @@ export const NOOP_UP_DOWN_SUM_OBSERVER_METRIC = new NoopBaseObserverMetric( export const NOOP_SUM_OBSERVER_METRIC = new NoopBaseObserverMetric( NOOP_BOUND_BASE_OBSERVER ); - -export const NOOP_BATCH_OBSERVER_METRIC = new NoopBatchObserverMetric(); diff --git a/packages/opentelemetry-metrics/README.md b/packages/opentelemetry-metrics/README.md index 7f200f9418e..1216aca0c52 100644 --- a/packages/opentelemetry-metrics/README.md +++ b/packages/opentelemetry-metrics/README.md @@ -222,7 +222,7 @@ const MemUsageMetric = meter.createValueObserver('mem_usage_per_app', { description: 'Memory', }); -meter.createBatchObserver('metric_batch_observer', (observerBatchResult) => { +meter.createBatchObserver((observerBatchResult) => { getSomeAsyncMetrics().then(metrics => { observerBatchResult.observe({ app: 'myApp' }, [ cpuUsageMetric.observation(metrics.value1), diff --git a/packages/opentelemetry-metrics/src/BatchObserverMetric.ts b/packages/opentelemetry-metrics/src/BatchObserver.ts similarity index 65% rename from packages/opentelemetry-metrics/src/BatchObserverMetric.ts rename to packages/opentelemetry-metrics/src/BatchObserver.ts index 241ff0813a0..81c68cfeba1 100644 --- a/packages/opentelemetry-metrics/src/BatchObserverMetric.ts +++ b/packages/opentelemetry-metrics/src/BatchObserver.ts @@ -15,57 +15,32 @@ */ import * as api from '@opentelemetry/api'; -import { InstrumentationLibrary } from '@opentelemetry/core'; -import { Resource } from '@opentelemetry/resources'; +import { Logger, NoopLogger } from '@opentelemetry/api'; import { BatchObserverResult } from './BatchObserverResult'; -import { BoundObserver } from './BoundInstrument'; -import { Batcher } from './export/Batcher'; -import { MetricKind, MetricRecord } from './export/types'; -import { Metric } from './Metric'; +import { MetricRecord } from './export/types'; const NOOP_CALLBACK = () => {}; const MAX_TIMEOUT_UPDATE_MS = 500; /** This is a SDK implementation of Batch Observer Metric. */ -export class BatchObserverMetric - extends Metric - implements api.BatchObserver { +export class BatchObserver { private _callback: (observerResult: api.BatchObserverResult) => void; private _maxTimeoutUpdateMS: number; + private _logger: Logger; constructor( - name: string, options: api.BatchMetricOptions, - private readonly _batcher: Batcher, - resource: Resource, - instrumentationLibrary: InstrumentationLibrary, callback?: (observerResult: api.BatchObserverResult) => void ) { - super( - name, - options, - MetricKind.BATCH_OBSERVER, - resource, - instrumentationLibrary - ); + this._logger = options.logger ?? new NoopLogger(); this._maxTimeoutUpdateMS = options.maxTimeoutUpdateMS ?? MAX_TIMEOUT_UPDATE_MS; this._callback = callback || NOOP_CALLBACK; } - protected _makeInstrument(labels: api.Labels): BoundObserver { - return new BoundObserver( - labels, - this._disabled, - this._valueType, - this._logger, - this._batcher.aggregatorFor(this._descriptor) - ); - } - - getMetricRecord(): Promise { + collect(): Promise { this._logger.debug('getMetricRecord - start'); - return new Promise((resolve, reject) => { + return new Promise(resolve => { const observerResult = new BatchObserverResult(); // cancels after MAX_TIMEOUT_MS - no more waiting for results @@ -74,14 +49,14 @@ export class BatchObserverMetric // remove callback to prevent user from updating the values later if // for any reason the observerBatchResult will be referenced observerResult.onObserveCalled(); - super.getMetricRecord().then(resolve, reject); + resolve(); this._logger.debug('getMetricRecord - timeout'); }, this._maxTimeoutUpdateMS); // sets callback for each "observe" method observerResult.onObserveCalled(() => { clearTimeout(timer); - super.getMetricRecord().then(resolve, reject); + resolve(); this._logger.debug('getMetricRecord - end'); }); diff --git a/packages/opentelemetry-metrics/src/Meter.ts b/packages/opentelemetry-metrics/src/Meter.ts index d0a54049b3e..2f8bd6b756a 100644 --- a/packages/opentelemetry-metrics/src/Meter.ts +++ b/packages/opentelemetry-metrics/src/Meter.ts @@ -17,9 +17,8 @@ import * as api from '@opentelemetry/api'; import { ConsoleLogger, InstrumentationLibrary } from '@opentelemetry/core'; import { Resource } from '@opentelemetry/resources'; -import { BatchObserverMetric } from './BatchObserverMetric'; +import { BatchObserver } from './BatchObserver'; import { BaseBoundInstrument } from './BoundInstrument'; -import { MetricKind } from './export/types'; import { UpDownCounterMetric } from './UpDownCounterMetric'; import { CounterMetric } from './CounterMetric'; import { UpDownSumObserverMetric } from './UpDownSumObserverMetric'; @@ -37,6 +36,7 @@ import { NoopExporter } from './export/NoopExporter'; */ export class Meter implements api.Meter { private readonly _logger: api.Logger; + private readonly _batchObservers: BatchObserver[] = []; private readonly _metrics = new Map>(); private readonly _batcher: Batcher; private readonly _resource: Resource; @@ -258,35 +258,20 @@ export class Meter implements api.Meter { /** * Creates a new batch observer metric. - * @param name the name of the metric. * @param callback the batch observer callback * @param [options] the metric batch options. */ createBatchObserver( - name: string, callback: (observerResult: api.BatchObserverResult) => void, options: api.BatchMetricOptions = {} - ): api.BatchObserver { - if (!this._isValidName(name)) { - this._logger.warn( - `Invalid metric name ${name}. Defaulting to noop metric implementation.` - ); - return api.NOOP_BATCH_OBSERVER_METRIC; - } + ) { const opt: api.BatchMetricOptions = { logger: this._logger, ...DEFAULT_METRIC_OPTIONS, ...options, }; - const batchObserver = new BatchObserverMetric( - name, - opt, - this._batcher, - this._resource, - this._instrumentationLibrary, - callback - ); - this._registerMetric(name, batchObserver); + const batchObserver = new BatchObserver(opt, callback); + this._batchObservers.push(batchObserver); return batchObserver; } @@ -299,27 +284,17 @@ export class Meter implements api.Meter { */ async collect(): Promise { // call batch observers first - const batchObservers = Array.from(this._metrics.values()) - .filter(metric => { - return metric.getKind() === MetricKind.BATCH_OBSERVER; - }) - .map(metric => { - return metric.getMetricRecord(); - }); - await Promise.all(batchObservers).then(records => { - records.forEach(metrics => { - metrics.forEach(metric => this._batcher.process(metric)); - }); - }); + const observations = Array.from(this._batchObservers.values()).map( + observer => { + return observer.collect(); + } + ); + await Promise.all(observations); // after this all remaining metrics can be run - const metrics = Array.from(this._metrics.values()) - .filter(metric => { - return metric.getKind() !== MetricKind.BATCH_OBSERVER; - }) - .map(metric => { - return metric.getMetricRecord(); - }); + const metrics = Array.from(this._metrics.values()).map(metric => { + return metric.getMetricRecord(); + }); await Promise.all(metrics).then(records => { records.forEach(metrics => { diff --git a/packages/opentelemetry-metrics/test/Meter.test.ts b/packages/opentelemetry-metrics/test/Meter.test.ts index 6c6a995796f..07f8de437a8 100644 --- a/packages/opentelemetry-metrics/test/Meter.test.ts +++ b/packages/opentelemetry-metrics/test/Meter.test.ts @@ -43,6 +43,7 @@ import { UpDownSumObserverMetric } from '../src/UpDownSumObserverMetric'; import { hashLabels } from '../src/Utils'; import { Batcher } from '../src/export/Batcher'; import { ValueType } from '@opentelemetry/api'; +import { BatchObserver } from '../src/BatchObserver'; const nonNumberValues = [ // type undefined @@ -1137,18 +1138,18 @@ describe('Meter', () => { describe('#batchObserver', () => { it('should create a batch observer', () => { - const measure = meter.createBatchObserver('name', () => {}); - assert.ok(measure instanceof Metric); + const measure = meter.createBatchObserver(() => {}); + assert.ok(measure instanceof BatchObserver); }); it('should create batch observer with options', () => { - const measure = meter.createBatchObserver('name', () => {}, { + const measure = meter.createBatchObserver(() => {}, { description: 'desc', unit: '1', disabled: false, maxTimeoutUpdateMS: 100, }); - assert.ok(measure instanceof Metric); + assert.ok(measure instanceof BatchObserver); }); it('should use callback to observe values ', async () => { @@ -1160,59 +1161,56 @@ describe('Meter', () => { description: 'desc', }) as ValueObserverMetric; - meter.createBatchObserver( - 'metric_batch_observer', - observerBatchResult => { - interface StatItem { - usage: number; - temp: number; - } - - interface Stat { - name: string; - core1: StatItem; - core2: StatItem; - } + meter.createBatchObserver(observerBatchResult => { + interface StatItem { + usage: number; + temp: number; + } - function someAsyncMetrics() { - return new Promise(resolve => { - const stats: Stat[] = [ - { - name: 'app1', - core1: { usage: 2.1, temp: 67 }, - core2: { usage: 3.1, temp: 69 }, - }, - { - name: 'app2', - core1: { usage: 1.2, temp: 67 }, - core2: { usage: 4.5, temp: 69 }, - }, - ]; - resolve(stats); - }); - } + interface Stat { + name: string; + core1: StatItem; + core2: StatItem; + } - Promise.all([ - someAsyncMetrics(), - // simulate waiting - new Promise((resolve, reject) => { - setTimeout(resolve, 1); - }), - ]).then((stats: unknown[]) => { - const apps = (stats[0] as unknown) as Stat[]; - apps.forEach(app => { - observerBatchResult.observe({ app: app.name, core: '1' }, [ - tempMetric.observation(app.core1.temp), - cpuUsageMetric.observation(app.core1.usage), - ]); - observerBatchResult.observe({ app: app.name, core: '2' }, [ - tempMetric.observation(app.core2.temp), - cpuUsageMetric.observation(app.core2.usage), - ]); - }); + function someAsyncMetrics() { + return new Promise(resolve => { + const stats: Stat[] = [ + { + name: 'app1', + core1: { usage: 2.1, temp: 67 }, + core2: { usage: 3.1, temp: 69 }, + }, + { + name: 'app2', + core1: { usage: 1.2, temp: 67 }, + core2: { usage: 4.5, temp: 69 }, + }, + ]; + resolve(stats); }); } - ); + + Promise.all([ + someAsyncMetrics(), + // simulate waiting + new Promise((resolve, reject) => { + setTimeout(resolve, 1); + }), + ]).then((stats: unknown[]) => { + const apps = (stats[0] as unknown) as Stat[]; + apps.forEach(app => { + observerBatchResult.observe({ app: app.name, core: '1' }, [ + tempMetric.observation(app.core1.temp), + cpuUsageMetric.observation(app.core1.usage), + ]); + observerBatchResult.observe({ app: app.name, core: '2' }, [ + tempMetric.observation(app.core2.temp), + cpuUsageMetric.observation(app.core2.usage), + ]); + }); + }); + }); await meter.collect(); const records = meter.getBatcher().checkPointSet(); @@ -1253,7 +1251,6 @@ describe('Meter', () => { }) as ValueObserverMetric; meter.createBatchObserver( - 'metric_batch_observer', observerBatchResult => { Promise.all([ // simulate waiting 11ms From a2acc7c41ed2732e14e3725dcf30440411ab5a3b Mon Sep 17 00:00:00 2001 From: legendecas Date: Wed, 2 Dec 2020 17:20:45 +0800 Subject: [PATCH 2/3] fixup: nodejs v8 compatibilities --- packages/opentelemetry-metrics/src/Meter.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/packages/opentelemetry-metrics/src/Meter.ts b/packages/opentelemetry-metrics/src/Meter.ts index 2f8bd6b756a..566c290966c 100644 --- a/packages/opentelemetry-metrics/src/Meter.ts +++ b/packages/opentelemetry-metrics/src/Meter.ts @@ -284,11 +284,9 @@ export class Meter implements api.Meter { */ async collect(): Promise { // call batch observers first - const observations = Array.from(this._batchObservers.values()).map( - observer => { - return observer.collect(); - } - ); + const observations = this._batchObservers.map(observer => { + return observer.collect(); + }); await Promise.all(observations); // after this all remaining metrics can be run From 6a1e493b947d8638249d128134675d169288d889 Mon Sep 17 00:00:00 2001 From: legendecas Date: Thu, 3 Dec 2020 14:02:07 +0800 Subject: [PATCH 3/3] fixup: replace all batch observer metric occurrences with batch observer --- packages/opentelemetry-api/src/metrics/Meter.ts | 4 ++-- packages/opentelemetry-api/src/metrics/Metric.ts | 7 ++++++- packages/opentelemetry-api/src/metrics/NoopMeter.ts | 8 ++++++-- packages/opentelemetry-metrics/src/BatchObserver.ts | 4 ++-- .../opentelemetry-metrics/src/BatchObserverResult.ts | 2 +- packages/opentelemetry-metrics/src/Meter.ts | 11 +++++------ packages/opentelemetry-metrics/test/Meter.test.ts | 11 ++++------- 7 files changed, 26 insertions(+), 21 deletions(-) diff --git a/packages/opentelemetry-api/src/metrics/Meter.ts b/packages/opentelemetry-api/src/metrics/Meter.ts index 22fe74834b1..ef6406351b1 100644 --- a/packages/opentelemetry-api/src/metrics/Meter.ts +++ b/packages/opentelemetry-api/src/metrics/Meter.ts @@ -20,7 +20,7 @@ import { Counter, ValueRecorder, ValueObserver, - BatchMetricOptions, + BatchObserverOptions, UpDownCounter, } from './Metric'; import { ObserverResult } from './ObserverResult'; @@ -88,6 +88,6 @@ export interface Meter { */ createBatchObserver( callback: (batchObserverResult: BatchObserverResult) => void, - options?: BatchMetricOptions + options?: BatchObserverOptions ): void; } diff --git a/packages/opentelemetry-api/src/metrics/Metric.ts b/packages/opentelemetry-api/src/metrics/Metric.ts index b0e2e1c9c31..75e7a78d397 100644 --- a/packages/opentelemetry-api/src/metrics/Metric.ts +++ b/packages/opentelemetry-api/src/metrics/Metric.ts @@ -66,11 +66,16 @@ export interface MetricOptions { boundaries?: number[]; } -export interface BatchMetricOptions extends MetricOptions { +export interface BatchObserverOptions { /** * Indicates how long the batch metric should wait to update before cancel */ maxTimeoutUpdateMS?: number; + + /** + * User provided logger. + */ + logger?: Logger; } /** The Type of value. It describes how the data is reported. */ diff --git a/packages/opentelemetry-api/src/metrics/NoopMeter.ts b/packages/opentelemetry-api/src/metrics/NoopMeter.ts index 1d86354cd76..17bce62a986 100644 --- a/packages/opentelemetry-api/src/metrics/NoopMeter.ts +++ b/packages/opentelemetry-api/src/metrics/NoopMeter.ts @@ -90,8 +90,8 @@ export class NoopMeter implements Meter { */ createBatchObserver( _callback: (batchObserverResult: BatchObserverResult) => void - ): void { - return; + ): NoopBatchObserver { + return NOOP_BATCH_OBSERVER; } } @@ -156,6 +156,8 @@ export class NoopBaseObserverMetric } } +export class NoopBatchObserver {} + export class NoopBoundCounter implements BoundCounter { add(_value: number): void { return; @@ -197,3 +199,5 @@ export const NOOP_UP_DOWN_SUM_OBSERVER_METRIC = new NoopBaseObserverMetric( export const NOOP_SUM_OBSERVER_METRIC = new NoopBaseObserverMetric( NOOP_BOUND_BASE_OBSERVER ); + +export const NOOP_BATCH_OBSERVER = new NoopBatchObserver(); diff --git a/packages/opentelemetry-metrics/src/BatchObserver.ts b/packages/opentelemetry-metrics/src/BatchObserver.ts index 81c68cfeba1..5663c8d08f9 100644 --- a/packages/opentelemetry-metrics/src/BatchObserver.ts +++ b/packages/opentelemetry-metrics/src/BatchObserver.ts @@ -22,14 +22,14 @@ import { MetricRecord } from './export/types'; const NOOP_CALLBACK = () => {}; const MAX_TIMEOUT_UPDATE_MS = 500; -/** This is a SDK implementation of Batch Observer Metric. */ +/** This is a SDK implementation of Batch Observer. */ export class BatchObserver { private _callback: (observerResult: api.BatchObserverResult) => void; private _maxTimeoutUpdateMS: number; private _logger: Logger; constructor( - options: api.BatchMetricOptions, + options: api.BatchObserverOptions, callback?: (observerResult: api.BatchObserverResult) => void ) { this._logger = options.logger ?? new NoopLogger(); diff --git a/packages/opentelemetry-metrics/src/BatchObserverResult.ts b/packages/opentelemetry-metrics/src/BatchObserverResult.ts index 17f382360a8..2d0e7be4d53 100644 --- a/packages/opentelemetry-metrics/src/BatchObserverResult.ts +++ b/packages/opentelemetry-metrics/src/BatchObserverResult.ts @@ -26,7 +26,7 @@ export class BatchObserverResult implements api.BatchObserverResult { * Cancels the further updates. * This is used to prevent updating the value of result that took too * long to update. For example to avoid update after timeout. - * See {@link BatchObserverMetric.getMetricRecord} + * See {@link BatchObserver.collect} */ cancelled = false; diff --git a/packages/opentelemetry-metrics/src/Meter.ts b/packages/opentelemetry-metrics/src/Meter.ts index 566c290966c..9bf50ececc5 100644 --- a/packages/opentelemetry-metrics/src/Meter.ts +++ b/packages/opentelemetry-metrics/src/Meter.ts @@ -257,17 +257,16 @@ export class Meter implements api.Meter { } /** - * Creates a new batch observer metric. + * Creates a new batch observer. * @param callback the batch observer callback - * @param [options] the metric batch options. + * @param [options] the batch options. */ createBatchObserver( callback: (observerResult: api.BatchObserverResult) => void, - options: api.BatchMetricOptions = {} - ) { - const opt: api.BatchMetricOptions = { + options: api.BatchObserverOptions = {} + ): BatchObserver { + const opt: api.BatchObserverOptions = { logger: this._logger, - ...DEFAULT_METRIC_OPTIONS, ...options, }; const batchObserver = new BatchObserver(opt, callback); diff --git a/packages/opentelemetry-metrics/test/Meter.test.ts b/packages/opentelemetry-metrics/test/Meter.test.ts index 07f8de437a8..0a2d0bd1ee0 100644 --- a/packages/opentelemetry-metrics/test/Meter.test.ts +++ b/packages/opentelemetry-metrics/test/Meter.test.ts @@ -1138,18 +1138,15 @@ describe('Meter', () => { describe('#batchObserver', () => { it('should create a batch observer', () => { - const measure = meter.createBatchObserver(() => {}); - assert.ok(measure instanceof BatchObserver); + const observer = meter.createBatchObserver(() => {}); + assert.ok(observer instanceof BatchObserver); }); it('should create batch observer with options', () => { - const measure = meter.createBatchObserver(() => {}, { - description: 'desc', - unit: '1', - disabled: false, + const observer = meter.createBatchObserver(() => {}, { maxTimeoutUpdateMS: 100, }); - assert.ok(measure instanceof BatchObserver); + assert.ok(observer instanceof BatchObserver); }); it('should use callback to observe values ', async () => {