[Otlp] Make sure Otlp trace and metric exporters have dedicated optio…

…ns instances (#4200) * Make sure Otlp trace and metric exporters have dedicated options instances. * CHANGELOG patch.
open-telemetry · Feb 16, 2023 · 7139c7a · 7139c7a
1 parent d0829ff
commit 7139c7a
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 16 deletions.
diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+* `AddOtlpExporter` extension methods will now always create a new options
+  instance when named options are NOT used.
+  ([#4200](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4200))
+
 ## 1.4.0-rc.4
 
 Released 2023-Feb-10

diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterExtensions.cs
@@ -73,16 +73,25 @@ public static MeterProviderBuilder AddOtlpExporter(
 
             return builder.AddReader(sp =>
             {
-                var exporterOptions = sp.GetRequiredService<IOptionsMonitor<OtlpExporterOptions>>().Get(finalOptionsName);
+                OtlpExporterOptions exporterOptions;
 
-                if (name == null && configureExporter != null)
+                if (name == null)
                 {
-                    // If we are NOT using named options, we execute the
-                    // configuration delegate inline. The reason for this is
+                    // If we are NOT using named options we create a new
+                    // instance always. The reason for this is
                     // OtlpExporterOptions is shared by all signals. Without a
                     // name, delegates for all signals will mix together. See:
                     // https://github.com/open-telemetry/opentelemetry-dotnet/issues/4043
-                    configureExporter(exporterOptions);
+                    exporterOptions = sp.GetRequiredService<IOptionsFactory<OtlpExporterOptions>>().Create(finalOptionsName);
+
+                    // Configuration delegate is executed inline on the fresh instance.
+                    configureExporter?.Invoke(exporterOptions);
+                }
+                else
+                {
+                    // When using named options we can properly utilize Options
+                    // API to create or reuse an instance.
+                    exporterOptions = sp.GetRequiredService<IOptionsMonitor<OtlpExporterOptions>>().Get(finalOptionsName);
                 }
 
                 return BuildOtlpExporterMetricReader(

diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpTraceExporterHelperExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpTraceExporterHelperExtensions.cs
@@ -76,16 +76,25 @@ public static TracerProviderBuilder AddOtlpExporter(
 
             return builder.AddProcessor(sp =>
             {
-                var exporterOptions = sp.GetRequiredService<IOptionsMonitor<OtlpExporterOptions>>().Get(finalOptionsName);
+                OtlpExporterOptions exporterOptions;
 
-                if (name == null && configure != null)
+                if (name == null)
                 {
-                    // If we are NOT using named options, we execute the
-                    // configuration delegate inline. The reason for this is
+                    // If we are NOT using named options we create a new
+                    // instance always. The reason for this is
                     // OtlpExporterOptions is shared by all signals. Without a
                     // name, delegates for all signals will mix together. See:
                     // https://github.com/open-telemetry/opentelemetry-dotnet/issues/4043
-                    configure(exporterOptions);
+                    exporterOptions = sp.GetRequiredService<IOptionsFactory<OtlpExporterOptions>>().Create(finalOptionsName);
+
+                    // Configuration delegate is executed inline on the fresh instance.
+                    configure?.Invoke(exporterOptions);
+                }
+                else
+                {
+                    // When using named options we can properly utilize Options
+                    // API to create or reuse an instance.
+                    exporterOptions = sp.GetRequiredService<IOptionsMonitor<OtlpExporterOptions>>().Get(finalOptionsName);
                 }
 
                 // Note: Not using finalOptionsName here for SdkLimitOptions.

diff --git a/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpTraceExporterTests.cs b/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpTraceExporterTests.cs
@@ -641,6 +641,8 @@ public void Null_BatchExportProcessorOptions_SupportedTest()
         [Fact]
         public void NonnamedOptionsMutateSharedInstanceTest()
         {
+            var testOptionsInstance = new OtlpExporterOptions();
+
             OtlpExporterOptions tracerOptions = null;
             OtlpExporterOptions meterOptions = null;
 
@@ -649,11 +651,15 @@ public void NonnamedOptionsMutateSharedInstanceTest()
             services.AddOpenTelemetry()
                 .WithTracing(builder => builder.AddOtlpExporter(o =>
                 {
+                    Assert.Equal(testOptionsInstance.Endpoint, o.Endpoint);
+
                     tracerOptions = o;
                     o.Endpoint = new("http://localhost/traces");
                 }))
                 .WithMetrics(builder => builder.AddOtlpExporter(o =>
                 {
+                    Assert.Equal(testOptionsInstance.Endpoint, o.Endpoint);
+
                     meterOptions = o;
                     o.Endpoint = new("http://localhost/metrics");
                 }));
@@ -676,12 +682,7 @@ public void NonnamedOptionsMutateSharedInstanceTest()
             Assert.NotNull(meterOptions);
             Assert.Equal("http://localhost/metrics", meterOptions.Endpoint.OriginalString);
 
-            // Note: tracerOptions & meterOptions are actually the same instance
-            // in memory and that instance was actually mutated after
-            // OtlpTraceExporter was created but this is OK because it doesn't
-            // use the options after ctor.
-            Assert.True(ReferenceEquals(tracerOptions, meterOptions));
-            Assert.Equal("http://localhost/metrics", tracerOptions.Endpoint.OriginalString);
+            Assert.False(ReferenceEquals(tracerOptions, meterOptions));
         }
 
         [Fact]