Merge branch 'main' into jacksonweber/sampler-without-exporter

open-telemetry · Jan 24, 2024 · e0bd102 · e0bd102
2 parents 1d01797 + 0635ab1
commit e0bd102
Show file tree

Hide file tree

Showing 20 changed files with 194 additions and 107 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -11,9 +11,13 @@ For experimental package changes, see the [experimental CHANGELOG](experimental/
 
 ### :rocket: (Enhancement)
 
+* feat(sdk-metrics): add constructor option to add metric readers [#4427](https://github.com/open-telemetry/opentelemetry-js/pull/4427) @pichlermarc
+  * deprecates `MeterProvider.addMetricReader()` please use the constructor option `readers` instead.
+
 ### :bug: (Bug Fix)
 
 * fix(sdk-trace-base): ensure attribute value length limit is enforced on span creation [#4417](https://github.com/open-telemetry/opentelemetry-js/pull/4417) @pichlermarc
+* fix(sdk-trace-base): Export processed spans while exporter failed [#4287](https://github.com/open-telemetry/opentelemetry-js/pull/4287) @Zirak
 
 ### :books: (Refine Doc)
 

diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/test/metricsHelper.ts b/experimental/packages/opentelemetry-exporter-metrics-otlp-grpc/test/metricsHelper.ts
@@ -55,10 +55,11 @@ const testResource = new Resource({
   cost: 112.12,
 });
 
-let meterProvider = new MeterProvider({ resource: testResource });
-
 let reader = new TestMetricReader();
-meterProvider.addMetricReader(reader);
+let meterProvider = new MeterProvider({
+  resource: testResource,
+  readers: [reader],
+});
 
 let meter = meterProvider.getMeter('default', '0.0.1');
 
@@ -67,6 +68,7 @@ export async function collect() {
 }
 
 export function setUp() {
+  reader = new TestMetricReader();
   meterProvider = new MeterProvider({
     resource: testResource,
     views: [
@@ -75,9 +77,8 @@ export function setUp() {
         instrumentName: 'int-histogram',
       }),
     ],
+    readers: [reader],
   });
-  reader = new TestMetricReader();
-  meterProvider.addMetricReader(reader);
   meter = meterProvider.getMeter('default', '0.0.1');
 }
 

diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/metricsHelper.ts b/experimental/packages/opentelemetry-exporter-metrics-otlp-http/test/metricsHelper.ts
@@ -72,19 +72,24 @@ const defaultResource = Resource.default().merge(
   })
 );
 
-let meterProvider = new MeterProvider({ resource: defaultResource });
 let reader = new TestMetricReader();
-meterProvider.addMetricReader(reader);
+let meterProvider = new MeterProvider({
+  resource: defaultResource,
+  readers: [reader],
+});
 let meter = meterProvider.getMeter('default', '0.0.1');
 
 export async function collect() {
   return (await reader.collect())!;
 }
 
 export function setUp(views?: View[]) {
-  meterProvider = new MeterProvider({ resource: defaultResource, views });
   reader = new TestMetricReader();
-  meterProvider.addMetricReader(reader);
+  meterProvider = new MeterProvider({
+    resource: defaultResource,
+    views,
+    readers: [reader],
+  });
   meter = meterProvider.getMeter('default', '0.0.1');
 }
 

diff --git a/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/test/metricsHelper.ts b/experimental/packages/opentelemetry-exporter-metrics-otlp-proto/test/metricsHelper.ts
@@ -54,10 +54,11 @@ const testResource = new Resource({
   cost: 112.12,
 });
 
-let meterProvider = new MeterProvider({ resource: testResource });
-
 let reader = new TestMetricReader();
-meterProvider.addMetricReader(reader);
+let meterProvider = new MeterProvider({
+  resource: testResource,
+  readers: [reader],
+});
 
 let meter = meterProvider.getMeter('default', '0.0.1');
 
@@ -66,6 +67,7 @@ export async function collect() {
 }
 
 export function setUp() {
+  reader = new TestMetricReader();
   meterProvider = new MeterProvider({
     resource: testResource,
     views: [
@@ -74,9 +76,8 @@ export function setUp() {
         instrumentName: 'int-histogram',
       }),
     ],
+    readers: [reader],
   });
-  reader = new TestMetricReader();
-  meterProvider.addMetricReader(reader);
   meter = meterProvider.getMeter('default', '0.0.1');
 }
 

diff --git a/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts b/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusExporter.test.ts
@@ -261,8 +261,9 @@ describe('PrometheusExporter', () => {
 
     beforeEach(done => {
       exporter = new PrometheusExporter({}, () => {
-        meterProvider = new MeterProvider();
-        meterProvider.addMetricReader(exporter);
+        meterProvider = new MeterProvider({
+          readers: [exporter],
+        });
         meter = meterProvider.getMeter('test-prometheus', '1');
         done();
       });
@@ -533,8 +534,9 @@ describe('PrometheusExporter', () => {
     let exporter: PrometheusExporter;
 
     function setup(reader: PrometheusExporter) {
-      meterProvider = new MeterProvider();
-      meterProvider.addMetricReader(reader);
+      meterProvider = new MeterProvider({
+        readers: [exporter],
+      });
 
       meter = meterProvider.getMeter('test-prometheus');
       counter = meter.createCounter('counter');

diff --git a/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts b/experimental/packages/opentelemetry-exporter-prometheus/test/PrometheusSerializer.test.ts
@@ -91,8 +91,8 @@ describe('PrometheusSerializer', () => {
               instrumentName: '*',
             }),
           ],
+          readers: [reader],
         });
-        meterProvider.addMetricReader(reader);
         const meter = meterProvider.getMeter('test');
 
         const counter = meter.createCounter('test_total');
@@ -141,8 +141,8 @@ describe('PrometheusSerializer', () => {
               instrumentName: '*',
             }),
           ],
+          readers: [reader],
         });
-        meterProvider.addMetricReader(reader);
         const meter = meterProvider.getMeter('test');
 
         const histogram = meter.createHistogram('test');
@@ -206,8 +206,8 @@ describe('PrometheusSerializer', () => {
               instrumentName: '*',
             }),
           ],
+          readers: [reader],
         });
-        meterProvider.addMetricReader(reader);
         const meter = meterProvider.getMeter('test');
 
         const counter = meter.createCounter('test_total', {
@@ -261,8 +261,8 @@ describe('PrometheusSerializer', () => {
               instrumentName: '*',
             }),
           ],
+          readers: [reader],
         });
-        meterProvider.addMetricReader(reader);
         const meter = meterProvider.getMeter('test');
 
         const counter = meter.createUpDownCounter('test_total', {
@@ -315,8 +315,8 @@ describe('PrometheusSerializer', () => {
               instrumentName: '*',
             }),
           ],
+          readers: [reader],
         });
-        meterProvider.addMetricReader(reader);
         const meter = meterProvider.getMeter('test');
 
         const counter = meter.createUpDownCounter('test_total', {
@@ -369,8 +369,8 @@ describe('PrometheusSerializer', () => {
               instrumentName: '*',
             }),
           ],
+          readers: [reader],
         });
-        meterProvider.addMetricReader(reader);
         const meter = meterProvider.getMeter('test');
 
         const histogram = meter.createHistogram('test', {
@@ -424,8 +424,8 @@ describe('PrometheusSerializer', () => {
               instrumentName: '*',
             }),
           ],
+          readers: [reader],
         });
-        meterProvider.addMetricReader(reader);
         const meter = meterProvider.getMeter('test');
 
         const upDownCounter = meter.createUpDownCounter('test', {
@@ -474,8 +474,8 @@ describe('PrometheusSerializer', () => {
         views: [
           new View({ aggregation: new SumAggregation(), instrumentName: '*' }),
         ],
+        readers: [reader],
       });
-      meterProvider.addMetricReader(reader);
       const meter = meterProvider.getMeter('test');
 
       const { unit, exportAll = false } = options;
@@ -563,8 +563,8 @@ describe('PrometheusSerializer', () => {
         views: [
           new View({ aggregation: new SumAggregation(), instrumentName: '*' }),
         ],
+        readers: [reader],
       });
-      meterProvider.addMetricReader(reader);
       const meter = meterProvider.getMeter('test');
 
       const counter = meter.createUpDownCounter(name);

diff --git a/...imental/packages/opentelemetry-instrumentation-http/test/functionals/http-metrics.test.ts b/...imental/packages/opentelemetry-instrumentation-http/test/functionals/http-metrics.test.ts
@@ -38,13 +38,12 @@ const protocol = 'http';
 const hostname = 'localhost';
 const pathname = '/test';
 const tracerProvider = new NodeTracerProvider();
-const meterProvider = new MeterProvider();
 const metricsMemoryExporter = new InMemoryMetricExporter(
   AggregationTemporality.DELTA
 );
 const metricReader = new TestMetricReader(metricsMemoryExporter);
+const meterProvider = new MeterProvider({ readers: [metricReader] });
 
-meterProvider.addMetricReader(metricReader);
 instrumentation.setTracerProvider(tracerProvider);
 instrumentation.setMeterProvider(meterProvider);
 

diff --git a/experimental/packages/opentelemetry-sdk-node/src/sdk.ts b/experimental/packages/opentelemetry-sdk-node/src/sdk.ts
@@ -356,15 +356,16 @@ export class NodeSDK {
     }
 
     if (this._meterProviderConfig) {
+      const readers: MetricReader[] = [];
+      if (this._meterProviderConfig.reader) {
+        readers.push(this._meterProviderConfig.reader);
+      }
       const meterProvider = new MeterProvider({
         resource: this._resource,
         views: this._meterProviderConfig?.views ?? [],
+        readers: readers,
       });
 
-      if (this._meterProviderConfig.reader) {
-        meterProvider.addMetricReader(this._meterProviderConfig.reader);
-      }
-
       this._meterProvider = meterProvider;
 
       metrics.setGlobalMeterProvider(meterProvider);

diff --git a/packages/opentelemetry-sdk-trace-base/src/export/BatchSpanProcessorBase.ts b/packages/opentelemetry-sdk-trace-base/src/export/BatchSpanProcessorBase.ts
@@ -221,7 +221,7 @@ export abstract class BatchSpanProcessorBase<T extends BufferConfig>
     const flush = () => {
       this._isExporting = true;
       this._flushOneBatch()
-        .then(() => {
+        .finally(() => {
           this._isExporting = false;
           if (this._finishedSpans.length > 0) {
             this._clearTimer();

diff --git a/packages/opentelemetry-sdk-trace-base/test/common/export/BatchSpanProcessorBase.test.ts b/packages/opentelemetry-sdk-trace-base/test/common/export/BatchSpanProcessorBase.test.ts
@@ -387,6 +387,57 @@ describe('BatchSpanProcessorBase', () => {
         });
       });
 
+      it('should still export when previously failed', async () => {
+        // The scenario is made of several parts:
+        // 1. The exporter tries to export some spans
+        // 2. While it does so, more spans are processed
+        // 3. The exporter fails
+        // 4. Spans arriving during step 2 should be exported
+
+        let firstCall = true;
+        const fillingExportStub = sinon
+          .stub(exporter, 'export')
+          .callsFake((spans, cb) => {
+            // The first time export is called, add some spans to the processor.
+            // Any other time, call through. We don't simply restore the stub
+            // so we can count the calls with `sinon.assert`
+            if (!firstCall) {
+              return fillingExportStub.wrappedMethod.call(exporter, spans, cb);
+            }
+
+            // Step 2: During export, add another span
+            firstCall = false;
+            processSpan();
+
+            return fillingExportStub.wrappedMethod.call(exporter, spans, () => {
+              // Step 3: Mock failure
+              cb({
+                code: ExportResultCode.FAILED,
+              });
+            });
+          });
+
+        const clock = sinon.useFakeTimers();
+
+        // Step 1: Export a span
+        processSpan();
+        await clock.runAllAsync();
+
+        clock.restore();
+        fillingExportStub.restore();
+
+        // Step 4: Make sure all spans were processed
+        assert.equal(exporter['_finishedSpans'].length, 2);
+        assert.equal(processor['_finishedSpans'].length, 0);
+        sinon.assert.calledTwice(fillingExportStub);
+
+        function processSpan() {
+          const span = createSampledSpan('test');
+          processor.onStart(span, ROOT_CONTEXT);
+          processor.onEnd(span);
+        }
+      });
+
       it('should wait for pending resource on flush', async () => {
         const tracer = new BasicTracerProvider({
           resource: new Resource(

diff --git a/packages/sdk-metrics/src/MeterProvider.ts b/packages/sdk-metrics/src/MeterProvider.ts
@@ -35,6 +35,7 @@ export interface MeterProviderOptions {
   /** Resource associated with metric telemetry  */
   resource?: IResource;
   views?: View[];
+  readers?: MetricReader[];
 }
 
 /**
@@ -54,6 +55,12 @@ export class MeterProvider implements IMeterProvider {
         this._sharedState.viewRegistry.addView(view);
       }
     }
+
+    if (options?.readers != null && options.readers.length > 0) {
+      for (const metricReader of options.readers) {
+        this.addMetricReader(metricReader);
+      }
+    }
   }
 
   /**
@@ -77,7 +84,13 @@ export class MeterProvider implements IMeterProvider {
    * Register a {@link MetricReader} to the meter provider. After the
    * registration, the MetricReader can start metrics collection.
    *
+   * <p> NOTE: {@link MetricReader} instances MUST be added before creating any instruments.
+   * A {@link MetricReader} instance registered later may receive no or incomplete metric data.
+   *
    * @param metricReader the metric reader to be registered.
+   *
+   * @deprecated This method will be removed in SDK 2.0. Please use
+   * {@link MeterProviderOptions.readers} via the {@link MeterProvider} constructor instead
    */
   addMetricReader(metricReader: MetricReader) {
     const collector = new MetricCollector(this._sharedState, metricReader);

diff --git a/packages/sdk-metrics/test/Instruments.test.ts b/packages/sdk-metrics/test/Instruments.test.ts
@@ -751,18 +751,19 @@ describe('Instruments', () => {
 });
 
 function setup() {
-  const meterProvider = new MeterProvider({ resource: defaultResource });
+  const deltaReader = new TestDeltaMetricReader();
+  const cumulativeReader = new TestMetricReader();
+  const meterProvider = new MeterProvider({
+    resource: defaultResource,
+    readers: [deltaReader, cumulativeReader],
+  });
   const meter = meterProvider.getMeter(
     defaultInstrumentationScope.name,
     defaultInstrumentationScope.version,
     {
       schemaUrl: defaultInstrumentationScope.schemaUrl,
     }
   );
-  const deltaReader = new TestDeltaMetricReader();
-  meterProvider.addMetricReader(deltaReader);
-  const cumulativeReader = new TestMetricReader();
-  meterProvider.addMetricReader(cumulativeReader);
 
   return {
     meterProvider,