From 7139c7a656ad43baff471ef0641c7dc828902c48 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Thu, 16 Feb 2023 14:04:45 -0800 Subject: [PATCH] [Otlp] Make sure Otlp trace and metric exporters have dedicated options instances (#4200) * Make sure Otlp trace and metric exporters have dedicated options instances. * CHANGELOG patch. --- .../CHANGELOG.md | 4 ++++ .../OtlpMetricExporterExtensions.cs | 19 ++++++++++++++----- .../OtlpTraceExporterHelperExtensions.cs | 19 ++++++++++++++----- .../OtlpTraceExporterTests.cs | 13 +++++++------ 4 files changed, 39 insertions(+), 16 deletions(-) diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/CHANGELOG.md index f6822468af9..f4b74cb539f 100644 --- 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 index 80979dd38f9..df4f9e5be80 100644 --- 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>().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>().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>().Get(finalOptionsName); } return BuildOtlpExporterMetricReader( diff --git a/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpTraceExporterHelperExtensions.cs b/src/OpenTelemetry.Exporter.OpenTelemetryProtocol/OtlpTraceExporterHelperExtensions.cs index 728ce1f503a..31e8cbbfe2f 100644 --- 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>().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>().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>().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 index dd94e5dc95e..a3003baf8a7 100644 --- 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]