diff --git a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs b/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs index 1629655b022..55eb8f23764 100644 --- a/src/OpenTelemetry.Exporter.Jaeger/JaegerExporterHelperExtensions.cs +++ b/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>().Get(name))); }); diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol.Logs/OtlpLogExporterHelperExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol.Logs/OtlpLogExporterHelperExtensions.cs index 59861d3bd34..464b8abe201 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol.Logs/OtlpLogExporterHelperExtensions.cs +++ b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol.Logs/OtlpLogExporterHelperExtensions.cs @@ -14,6 +14,7 @@ // limitations under the License. // +using System.Diagnostics; using OpenTelemetry.Exporter; namespace OpenTelemetry.Logs @@ -30,7 +31,7 @@ public static class OtlpLogExporterHelperExtensions /// options to use. /// The instance of to chain the calls. public static OpenTelemetryLoggerOptions AddOtlpExporter(this OpenTelemetryLoggerOptions loggerOptions) - => AddOtlpExporter(loggerOptions, configure: null); + => AddOtlpExporterInternal(loggerOptions, configure: null); /// /// 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 configure) - => AddOtlpExporter(loggerOptions, new(), configure); + => AddOtlpExporterInternal(loggerOptions, configure); - private static OpenTelemetryLoggerOptions AddOtlpExporter( + private static OpenTelemetryLoggerOptions AddOtlpExporterInternal( OpenTelemetryLoggerOptions loggerOptions, - OtlpExporterOptions exporterOptions, Action 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; diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs index 3c9169c2b59..35abee387ba 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpExporterOptions.cs +++ b/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 was modified via its setter. /// internal bool ProgrammaticallyModifiedEndpoint { get; private set; } + + internal static void RegisterOtlpExporterOptionsFactory(IServiceCollection services) + { + services.RegisterOptionsFactory( + (sp, configuration, name) => new OtlpExporterOptions( + configuration, + sp.GetRequiredService>().Get(name))); + } } } diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterExtensions.cs index 82e991f15c7..b965fff68c0 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpMetricExporterExtensions.cs +++ b/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>().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>().Get(name), - sp.GetRequiredService>().Get(name), + exporterOptions, + sp.GetRequiredService>().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) => diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpTraceExporterHelperExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpTraceExporterHelperExtensions.cs index 5fee6a6c577..a57b5a8be27 100644 --- a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpTraceExporterHelperExtensions.cs +++ b/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>().Get(name))); }); return builder.ConfigureBuilder((sp, builder) => { - var exporterOptions = sp.GetRequiredService>().Get(name); + var exporterOptions = sp.GetRequiredService>().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>().CurrentValue; AddOtlpExporter(builder, exporterOptions, sdkOptionsManager, sp); diff --git a/src/OpenTelemetry.Exporter.Zipkin/ZipkinExporterHelperExtensions.cs b/src/OpenTelemetry.Exporter.Zipkin/ZipkinExporterHelperExtensions.cs index a616fb01385..5c0f2d268cc 100644 --- a/src/OpenTelemetry.Exporter.Zipkin/ZipkinExporterHelperExtensions.cs +++ b/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>().Get(name))); }); diff --git a/src/OpenTelemetry/Internal/ConfigurationExtensions.cs b/src/OpenTelemetry/Internal/ConfigurationExtensions.cs index 6259d571f3a..5a07e2cb2e7 100644 --- a/src/OpenTelemetry/Internal/ConfigurationExtensions.cs +++ b/src/OpenTelemetry/Internal/ConfigurationExtensions.cs @@ -125,7 +125,7 @@ public static IServiceCollection RegisterOptionsFactory( services!.TryAddSingleton>(sp => { return new DelegatingOptionsFactory( - optionsFactoryFunc!, + (c, n) => optionsFactoryFunc!(c), sp.GetRequiredService(), sp.GetServices>(), sp.GetServices>(), @@ -137,7 +137,7 @@ public static IServiceCollection RegisterOptionsFactory( public static IServiceCollection RegisterOptionsFactory( this IServiceCollection services, - Func optionsFactoryFunc) + Func optionsFactoryFunc) where T : class { Debug.Assert(services != null, "services was null"); @@ -146,7 +146,7 @@ public static IServiceCollection RegisterOptionsFactory( services!.TryAddSingleton>(sp => { return new DelegatingOptionsFactory( - c => optionsFactoryFunc!(sp, c), + (c, n) => optionsFactoryFunc!(sp, c, n), sp.GetRequiredService(), sp.GetServices>(), sp.GetServices>(), @@ -159,11 +159,11 @@ public static IServiceCollection RegisterOptionsFactory( private sealed class DelegatingOptionsFactory : OptionsFactory where T : class { - private readonly Func optionsFactoryFunc; + private readonly Func optionsFactoryFunc; private readonly IConfiguration configuration; public DelegatingOptionsFactory( - Func optionsFactoryFunc, + Func optionsFactoryFunc, IConfiguration configuration, IEnumerable> setups, IEnumerable> postConfigures, @@ -178,6 +178,6 @@ public DelegatingOptionsFactory( } protected override T CreateInstance(string name) - => this.optionsFactoryFunc(this.configuration); + => this.optionsFactoryFunc(this.configuration, name); } } diff --git a/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpExporterOptionsTests.cs b/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpExporterOptionsTests.cs index c60ac1498a2..83bcc70fa33 100644 --- a/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpExporterOptionsTests.cs +++ b/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); diff --git a/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpTraceExporterTests.cs b/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpTraceExporterTests.cs index 37ae08abb7c..dd94e5dc95e 100644 --- a/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpTraceExporterTests.cs +++ b/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(); + + // 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(); + + // 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(); + + // 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(); + + // 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); + } } }