From f64ef47b155a26e1963e7dd277babc793d92db90 Mon Sep 17 00:00:00 2001 From: legendecas Date: Fri, 17 Dec 2021 17:02:14 +0800 Subject: [PATCH] fixup! --- .../src/MeterProvider.ts | 23 ++++++++----------- .../src/state/MetricCollector.ts | 18 +++++++++++++-- .../src/state/TemporalMetricProcessor.ts | 4 ++-- .../test/state/MetricCollector.test.ts | 16 ++++++------- .../test/state/SyncMetricStorage.test.ts | 2 +- 5 files changed, 36 insertions(+), 27 deletions(-) diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts index e62d34ac1e..62c8892cbe 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/MeterProvider.ts @@ -45,18 +45,12 @@ export class MeterProvider { api.diag.warn('A shutdown MeterProvider cannot provide a Meter') return metrics.NOOP_METER; } - // TODO: add schemaUrl to id? - const id = `${name}#${version}`; - if (this._sharedState.meters.has(id)) { - return this._sharedState.meters.get(id)!; - } // Spec leaves it unspecified if creating a meter with duplicate // name/version returns the same meter. We create a new one here // for simplicity. This may change in the future. - const meter = new Meter(this._sharedState, { name, version, schemaUrl: options.schemaUrl }); - this._sharedState.meters.set(id, meter); - return meter; + // TODO: consider returning the same meter if the same name/version is used + return new Meter(this._sharedState, { name, version, schemaUrl: options.schemaUrl }); } addMetricReader(metricReader: MetricReader) { @@ -71,7 +65,8 @@ export class MeterProvider { } /** - * Flush all buffered data and shut down the MeterProvider and all metric readers. + * Flush all buffered data and shut down the MeterProvider and all registered + * MetricReaders. * Returns a promise which is resolved when all flushes are complete. * * TODO: return errors to caller somehow? @@ -87,12 +82,11 @@ export class MeterProvider { // TODO add a timeout - spec leaves it up the the SDK if this is configurable this._shutdown = true; - // Shut down all readers. - // Log all Errors. for (const collector of this._sharedState.metricCollectors) { try { - await collector.metricReader.shutdown(); + await collector.shutdown(); } catch (e) { + // Log all Errors. if (e instanceof Error) { api.diag.error(`Error shutting down: ${e.message}`) } @@ -101,7 +95,7 @@ export class MeterProvider { } /** - * Notifies all exporters and metric readers to flush any buffered data. + * Notifies all registered MetricReaders to flush any buffered data. * Returns a promise which is resolved when all flushes are complete. * * TODO: return errors to caller somehow? @@ -119,8 +113,9 @@ export class MeterProvider { for (const collector of this._sharedState.metricCollectors) { try { - await collector.metricReader.forceFlush(); + await collector.forceFlush(); } catch (e) { + // Log all Errors. if (e instanceof Error) { api.diag.error(`Error flushing: ${e.message}`) } diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricCollector.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricCollector.ts index e89ef4a121..c1bea5e369 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricCollector.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/MetricCollector.ts @@ -28,8 +28,8 @@ import { MeterProviderSharedState } from './MeterProviderSharedState'; */ export class MetricCollector implements MetricProducer { public readonly aggregatorTemporality: AggregationTemporality; - constructor(private _sharedState: MeterProviderSharedState, public metricReader: MetricReader) { - this.aggregatorTemporality = this.metricReader.getPreferredAggregationTemporality(); + constructor(private _sharedState: MeterProviderSharedState, private _metricReader: MetricReader) { + this.aggregatorTemporality = this._metricReader.getPreferredAggregationTemporality(); } async collect(): Promise { @@ -39,6 +39,20 @@ export class MetricCollector implements MetricProducer { return results.reduce((cumulation, current) => cumulation.concat(current), []); } + + /** + * Delegates for MetricReader.forceFlush. + */ + async forceFlush(): Promise { + return this._metricReader.forceFlush(); + } + + /** + * Delegates for MetricReader.shutdown. + */ + async shutdown(): Promise { + return this._metricReader.shutdown(); + } } /** diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/TemporalMetricProcessor.ts b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/TemporalMetricProcessor.ts index c419ef6bec..f7154469dd 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/src/state/TemporalMetricProcessor.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/src/state/TemporalMetricProcessor.ts @@ -52,8 +52,8 @@ export class TemporalMetricProcessor { constructor(private _aggregator: Aggregator) {} /** - * Builds the {@link MetricData} streams to report against a specific metric reader. - * @param collector The information of the metric reader. + * Builds the {@link MetricData} streams to report against a specific MetricCollector. + * @param collector The information of the MetricCollector. * @param collectors The registered collectors. * @param resource The resource to attach these metrics against. * @param instrumentationLibrary The instrumentation library that generated these metrics. diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricCollector.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricCollector.test.ts index 6c92eed51d..900225cdf3 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricCollector.test.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/state/MetricCollector.test.ts @@ -16,11 +16,11 @@ import * as assert from 'assert'; import * as sinon from 'sinon'; -import { MeterProvider } from '../../src'; import { AggregationTemporality } from '../../src/export/AggregationTemporality'; import { MetricData, PointDataType } from '../../src/export/MetricData'; import { MetricExporter } from '../../src/export/MetricExporter'; import { MetricReader } from '../../src/export/MetricReader'; +import { Meter } from '../../src/Meter'; import { MeterProviderSharedState } from '../../src/state/MeterProviderSharedState'; import { MetricCollector } from '../../src/state/MetricCollector'; import { defaultInstrumentationLibrary, defaultResource, assertMetricData, assertPointData } from '../util'; @@ -64,21 +64,21 @@ describe('MetricCollector', () => { const metricCollector = new MetricCollector(meterProviderSharedState, reader); assert.strictEqual(metricCollector.aggregatorTemporality, exporter.getPreferredAggregationTemporality()); - assert.strictEqual(metricCollector.metricReader, reader); } }); }); describe('collect', () => { function setupInstruments(exporter: MetricExporter) { - const meterProvider = new MeterProvider({ resource: defaultResource }); + // TODO(legendecas): setup with MeterProvider when meter identity was settled. + const meterProviderSharedState = new MeterProviderSharedState(defaultResource); + const reader = new TestMetricReader(exporter); - meterProvider.addMetricReader(reader); - const metricCollector = reader.getMetricCollector(); + const metricCollector = new MetricCollector(meterProviderSharedState, reader); + meterProviderSharedState.metricCollectors.push(metricCollector); - const meter = meterProvider.getMeter(defaultInstrumentationLibrary.name, defaultInstrumentationLibrary.version, { - schemaUrl: defaultInstrumentationLibrary.schemaUrl, - }); + const meter = new Meter(meterProviderSharedState, defaultInstrumentationLibrary); + meterProviderSharedState.meters.set('test-meter', meter); return { metricCollector, meter }; } diff --git a/experimental/packages/opentelemetry-sdk-metrics-base/test/state/SyncMetricStorage.test.ts b/experimental/packages/opentelemetry-sdk-metrics-base/test/state/SyncMetricStorage.test.ts index ac2f73ea2e..e782c14876 100644 --- a/experimental/packages/opentelemetry-sdk-metrics-base/test/state/SyncMetricStorage.test.ts +++ b/experimental/packages/opentelemetry-sdk-metrics-base/test/state/SyncMetricStorage.test.ts @@ -49,7 +49,7 @@ describe('SyncMetricStorage', () => { }); }); - describe('collectAndReset', () => { + describe('collect', () => { describe('Delta Collector', () => { const collectors = [deltaCollector]; it('should collect and reset memos', async () => {