[OtlpExporter] Registration extension configuration delegate fix (#4058)

* Fix tracing and metric otlp exporter extensions mutating the same options instance when named options are not used.

* Log tweaks.

* Clean up.

* Issue links.

* Tweaks.

* Code review.

src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs

@@ -71,7 +71,7 @@ public static TracerProviderBuilder AddJaegerExporter(
                 }
 
                 services.RegisterOptionsFactory(
-                    (sp, configuration) => new JaegerExporterOptions(
+                    (sp, configuration, name) => new JaegerExporterOptions(
                         configuration,
                         sp.GetRequiredService<IOptionsMonitor<BatchExportActivityProcessorOptions>>().Get(name)));
             });

src/OpenTelemetry.Exporter.OpenTelemetryProtocol.Logs/OtlpLogExporterHelperExtensions.cs

@@ -14,6 +14,7 @@
 // limitations under the License.
 // </copyright>
 
+using System.Diagnostics;
 using OpenTelemetry.Exporter;
 
 namespace OpenTelemetry.Logs
@@ -30,7 +31,7 @@ public static class OtlpLogExporterHelperExtensions
         /// <param name="loggerOptions"><see cref="OpenTelemetryLoggerOptions"/> options to use.</param>
         /// <returns>The instance of <see cref="OpenTelemetryLoggerOptions"/> to chain the calls.</returns>
         public static OpenTelemetryLoggerOptions AddOtlpExporter(this OpenTelemetryLoggerOptions loggerOptions)
-            => AddOtlpExporter(loggerOptions, configure: null);
+            => AddOtlpExporterInternal(loggerOptions, configure: null);
 
         /// <summary>
         /// Adds OTLP Exporter as a configuration to the OpenTelemetry ILoggingBuilder.
@@ -46,29 +47,40 @@ public static OpenTelemetryLoggerOptions AddOtlpExporter(this OpenTelemetryLogge
         public static OpenTelemetryLoggerOptions AddOtlpExporter(
             this OpenTelemetryLoggerOptions loggerOptions,
             Action<OtlpExporterOptions> configure)
-            => AddOtlpExporter(loggerOptions, new(), configure);
+            => AddOtlpExporterInternal(loggerOptions, configure);
 
-        private static OpenTelemetryLoggerOptions AddOtlpExporter(
+        private static OpenTelemetryLoggerOptions AddOtlpExporterInternal(
             OpenTelemetryLoggerOptions loggerOptions,
-            OtlpExporterOptions exporterOptions,
             Action<OtlpExporterOptions> configure)
         {
+            loggerOptions.ParseStateValues = true;
+
+            var exporterOptions = new OtlpExporterOptions();
+
+            // TODO: We are using span/activity batch environment variable keys
+            // here when we should be using the ones for logs.
+            var defaultBatchOptions = exporterOptions.BatchExportProcessorOptions;
+
+            Debug.Assert(defaultBatchOptions != null, "defaultBatchOptions was null");
+
             configure?.Invoke(exporterOptions);
 
             var otlpExporter = new OtlpLogExporter(exporterOptions);
-            loggerOptions.ParseStateValues = true;
+
             if (exporterOptions.ExportProcessorType == ExportProcessorType.Simple)
             {
                 loggerOptions.AddProcessor(new SimpleLogRecordExportProcessor(otlpExporter));
             }
             else
             {
+                var batchOptions = exporterOptions.BatchExportProcessorOptions ?? defaultBatchOptions;
+
                 loggerOptions.AddProcessor(new BatchLogRecordExportProcessor(
                     otlpExporter,
-                    exporterOptions.BatchExportProcessorOptions.MaxQueueSize,
-                    exporterOptions.BatchExportProcessorOptions.ScheduledDelayMilliseconds,
-                    exporterOptions.BatchExportProcessorOptions.ExporterTimeoutMilliseconds,
-                    exporterOptions.BatchExportProcessorOptions.MaxExportBatchSize));
+                    batchOptions.MaxQueueSize,
+                    batchOptions.ScheduledDelayMilliseconds,
+                    batchOptions.ExporterTimeoutMilliseconds,
+                    batchOptions.MaxExportBatchSize));
             }
 
             return loggerOptions;

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs

@@ -19,6 +19,8 @@
 using System.Net.Http;
 #endif
 using Microsoft.Extensions.Configuration;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Options;
 using OpenTelemetry.Internal;
 using OpenTelemetry.Metrics;
 using OpenTelemetry.Trace;
@@ -59,9 +61,10 @@ public OtlpExporterOptions()
 
         internal OtlpExporterOptions(
             IConfiguration configuration,
-            BatchExportActivityProcessorOptions defaultBatchOptions = null)
+            BatchExportActivityProcessorOptions defaultBatchOptions)
         {
             Debug.Assert(configuration != null, "configuration was null");
+            Debug.Assert(defaultBatchOptions != null, "defaultBatchOptions was null");
 
             if (configuration.TryGetUriValue(EndpointEnvVarName, out var endpoint))
             {
@@ -188,5 +191,13 @@ public Uri Endpoint
         /// Gets a value indicating whether <see cref="Endpoint" /> was modified via its setter.
         /// </summary>
         internal bool ProgrammaticallyModifiedEndpoint { get; private set; }
+
+        internal static void RegisterOtlpExporterOptionsFactory(IServiceCollection services)
+        {
+            services.RegisterOptionsFactory(
+                (sp, configuration, name) => new OtlpExporterOptions(
+                    configuration,
+                    sp.GetRequiredService<IOptionsMonitor<BatchExportActivityProcessorOptions>>().Get(name)));
+        }
     }
 }

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterExtensions.cs

@@ -57,25 +57,38 @@ public static MeterProviderBuilder AddOtlpExporter(
         {
             Guard.ThrowIfNull(builder);
 
-            name ??= Options.DefaultName;
+            var finalOptionsName = name ?? Options.DefaultName;
 
             builder.ConfigureServices(services =>
             {
-                if (configureExporter != null)
+                if (name != null && configureExporter != null)
                 {
-                    services.Configure(name, configureExporter);
+                    // If we are using named options we register the
+                    // configuration delegate into options pipeline.
+                    services.Configure(finalOptionsName, configureExporter);
                 }
 
-                services.RegisterOptionsFactory(configuration
-                    => new OtlpExporterOptions(configuration, defaultBatchOptions: null));
+                OtlpExporterOptions.RegisterOtlpExporterOptionsFactory(services);
             });
 
             return builder.ConfigureBuilder((sp, builder) =>
             {
+                var exporterOptions = sp.GetRequiredService<IOptionsMonitor<OtlpExporterOptions>>().Get(finalOptionsName);
+
+                if (name == null && configureExporter != null)
+                {
+                    // If we are NOT using named options, we execute the
+                    // configuration delegate inline. 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);
+                }
+
                 AddOtlpExporter(
                     builder,
-                    sp.GetRequiredService<IOptionsMonitor<OtlpExporterOptions>>().Get(name),
-                    sp.GetRequiredService<IOptionsMonitor<MetricReaderOptions>>().Get(name),
+                    exporterOptions,
+                    sp.GetRequiredService<IOptionsMonitor<MetricReaderOptions>>().Get(finalOptionsName),
                     sp);
             });
         }
@@ -113,8 +126,7 @@ public static MeterProviderBuilder AddOtlpExporter(
 
             builder.ConfigureServices(services =>
             {
-                services.RegisterOptionsFactory(configuration
-                    => new OtlpExporterOptions(configuration, defaultBatchOptions: null));
+                OtlpExporterOptions.RegisterOtlpExporterOptionsFactory(services);
             });
 
             return builder.ConfigureBuilder((sp, builder) =>

src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpTraceExporterHelperExtensions.cs

@@ -59,29 +59,39 @@ public static TracerProviderBuilder AddOtlpExporter(
         {
             Guard.ThrowIfNull(builder);
 
-            name ??= Options.DefaultName;
+            var finalOptionsName = name ?? Options.DefaultName;
 
             builder.ConfigureServices(services =>
             {
-                if (configure != null)
+                if (name != null && configure != null)
                 {
-                    services.Configure(name, configure);
+                    // If we are using named options we register the
+                    // configuration delegate into options pipeline.
+                    services.Configure(finalOptionsName, configure);
                 }
 
+                OtlpExporterOptions.RegisterOtlpExporterOptionsFactory(services);
                 services.RegisterOptionsFactory(configuration => new SdkLimitOptions(configuration));
-                services.RegisterOptionsFactory(
-                    (sp, configuration) => new OtlpExporterOptions(
-                        configuration,
-                        sp.GetRequiredService<IOptionsMonitor<BatchExportActivityProcessorOptions>>().Get(name)));
             });
 
             return builder.ConfigureBuilder((sp, builder) =>
             {
-                var exporterOptions = sp.GetRequiredService<IOptionsMonitor<OtlpExporterOptions>>().Get(name);
+                var exporterOptions = sp.GetRequiredService<IOptionsMonitor<OtlpExporterOptions>>().Get(finalOptionsName);
 
-                // Note: Not using name here for SdkLimitOptions. There should
-                // only be one provider for a given service collection so
-                // SdkLimitOptions is treated as a single default instance.
+                if (name == null && configure != null)
+                {
+                    // If we are NOT using named options, we execute the
+                    // configuration delegate inline. 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);
+                }
+
+                // Note: Not using finalOptionsName here for SdkLimitOptions.
+                // There should only be one provider for a given service
+                // collection so SdkLimitOptions is treated as a single default
+                // instance.
                 var sdkOptionsManager = sp.GetRequiredService<IOptionsMonitor<SdkLimitOptions>>().CurrentValue;
 
                 AddOtlpExporter(builder, exporterOptions, sdkOptionsManager, sp);

src/OpenTelemetry.Exporter.Zipkin/ZipkinExporterHelperExtensions.cs

@@ -71,7 +71,7 @@ public static TracerProviderBuilder AddZipkinExporter(
                 }
 
                 services.RegisterOptionsFactory(
-                    (sp, configuration) => new ZipkinExporterOptions(
+                    (sp, configuration, name) => new ZipkinExporterOptions(
                         configuration,
                         sp.GetRequiredService<IOptionsMonitor<BatchExportActivityProcessorOptions>>().Get(name)));
             });

src/OpenTelemetry/Internal/ConfigurationExtensions.cs

@@ -125,7 +125,7 @@ public static IServiceCollection RegisterOptionsFactory<T>(
         services!.TryAddSingleton<IOptionsFactory<T>>(sp =>
         {
             return new DelegatingOptionsFactory<T>(
-                optionsFactoryFunc!,
+                (c, n) => optionsFactoryFunc!(c),
                 sp.GetRequiredService<IConfiguration>(),
                 sp.GetServices<IConfigureOptions<T>>(),
                 sp.GetServices<IPostConfigureOptions<T>>(),
@@ -137,7 +137,7 @@ public static IServiceCollection RegisterOptionsFactory<T>(
 
     public static IServiceCollection RegisterOptionsFactory<T>(
         this IServiceCollection services,
-        Func<IServiceProvider, IConfiguration, T> optionsFactoryFunc)
+        Func<IServiceProvider, IConfiguration, string, T> optionsFactoryFunc)
         where T : class
     {
         Debug.Assert(services != null, "services was null");
@@ -146,7 +146,7 @@ public static IServiceCollection RegisterOptionsFactory<T>(
         services!.TryAddSingleton<IOptionsFactory<T>>(sp =>
         {
             return new DelegatingOptionsFactory<T>(
-                c => optionsFactoryFunc!(sp, c),
+                (c, n) => optionsFactoryFunc!(sp, c, n),
                 sp.GetRequiredService<IConfiguration>(),
                 sp.GetServices<IConfigureOptions<T>>(),
                 sp.GetServices<IPostConfigureOptions<T>>(),
@@ -159,11 +159,11 @@ public static IServiceCollection RegisterOptionsFactory<T>(
     private sealed class DelegatingOptionsFactory<T> : OptionsFactory<T>
         where T : class
     {
-        private readonly Func<IConfiguration, T> optionsFactoryFunc;
+        private readonly Func<IConfiguration, string, T> optionsFactoryFunc;
         private readonly IConfiguration configuration;
 
         public DelegatingOptionsFactory(
-            Func<IConfiguration, T> optionsFactoryFunc,
+            Func<IConfiguration, string, T> optionsFactoryFunc,
             IConfiguration configuration,
             IEnumerable<IConfigureOptions<T>> setups,
             IEnumerable<IPostConfigureOptions<T>> postConfigures,
@@ -178,6 +178,6 @@ public DelegatingOptionsFactory(
         }
 
         protected override T CreateInstance(string name)
-            => this.optionsFactoryFunc(this.configuration);
+            => this.optionsFactoryFunc(this.configuration, name);
     }
 }

test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpExporterOptionsTests.cs

@@ -87,7 +87,7 @@ public void OtlpExporterOptions_UsingIConfiguration()
                 .AddInMemoryCollection(values)
                 .Build();
 
-            var options = new OtlpExporterOptions(configuration);
+            var options = new OtlpExporterOptions(configuration, new());
 
             Assert.Equal(new Uri("http://test:8888"), options.Endpoint);
             Assert.Equal("A=2,B=3", options.Headers);

test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpTraceExporterTests.cs

@@ -20,6 +20,7 @@
 using Moq;
 using OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation;
 using OpenTelemetry.Exporter.OpenTelemetryProtocol.Implementation.ExportClient;
+using OpenTelemetry.Metrics;
 using OpenTelemetry.Resources;
 using OpenTelemetry.Tests;
 using OpenTelemetry.Trace;
@@ -636,5 +637,96 @@ public void Null_BatchExportProcessorOptions_SupportedTest()
                         o.BatchExportProcessorOptions = null;
                     });
         }
+
+        [Fact]
+        public void NonnamedOptionsMutateSharedInstanceTest()
+        {
+            OtlpExporterOptions tracerOptions = null;
+            OtlpExporterOptions meterOptions = null;
+
+            var services = new ServiceCollection();
+
+            services.AddOpenTelemetry()
+                .WithTracing(builder => builder.AddOtlpExporter(o =>
+                {
+                    tracerOptions = o;
+                    o.Endpoint = new("http://localhost/traces");
+                }))
+                .WithMetrics(builder => builder.AddOtlpExporter(o =>
+                {
+                    meterOptions = o;
+                    o.Endpoint = new("http://localhost/metrics");
+                }));
+
+            using var serviceProvider = services.BuildServiceProvider();
+
+            var tracerProvider = serviceProvider.GetRequiredService<TracerProvider>();
+
+            // Verify the OtlpTraceExporter saw the correct endpoint.
+
+            Assert.NotNull(tracerOptions);
+            Assert.Null(meterOptions);
+            Assert.Equal("http://localhost/traces", tracerOptions.Endpoint.OriginalString);
+
+            var meterProvider = serviceProvider.GetRequiredService<MeterProvider>();
+
+            // Verify the OtlpMetricExporter saw the correct endpoint.
+
+            Assert.NotNull(tracerOptions);
+            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);
+        }
+
+        [Fact]
+        public void NamedOptionsMutateSeparateInstancesTest()
+        {
+            OtlpExporterOptions tracerOptions = null;
+            OtlpExporterOptions meterOptions = null;
+
+            var services = new ServiceCollection();
+
+            services.AddOpenTelemetry()
+                .WithTracing(builder => builder.AddOtlpExporter("Trace", o =>
+                {
+                    tracerOptions = o;
+                    o.Endpoint = new("http://localhost/traces");
+                }))
+                .WithMetrics(builder => builder.AddOtlpExporter("Metrics", o =>
+                {
+                    meterOptions = o;
+                    o.Endpoint = new("http://localhost/metrics");
+                }));
+
+            using var serviceProvider = services.BuildServiceProvider();
+
+            var tracerProvider = serviceProvider.GetRequiredService<TracerProvider>();
+
+            // Verify the OtlpTraceExporter saw the correct endpoint.
+
+            Assert.NotNull(tracerOptions);
+            Assert.Null(meterOptions);
+            Assert.Equal("http://localhost/traces", tracerOptions.Endpoint.OriginalString);
+
+            var meterProvider = serviceProvider.GetRequiredService<MeterProvider>();
+
+            // Verify the OtlpMetricExporter saw the correct endpoint.
+
+            Assert.NotNull(tracerOptions);
+            Assert.NotNull(meterOptions);
+            Assert.Equal("http://localhost/metrics", meterOptions.Endpoint.OriginalString);
+
+            // Verify expected state of instances.
+
+            Assert.False(ReferenceEquals(tracerOptions, meterOptions));
+            Assert.Equal("http://localhost/traces", tracerOptions.Endpoint.OriginalString);
+            Assert.Equal("http://localhost/metrics", meterOptions.Endpoint.OriginalString);
+        }
     }
 }