From 53dc806e2142b6022bc551840459955254006fe3 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 21 Oct 2022 13:55:16 -0700 Subject: [PATCH] Fix circular reference issue building up meter provider. --- ...viderBuilderServiceCollectionExtensions.cs | 3 +- .../Builder/MeterProviderBuilderBase.cs | 8 +-- .../Builder/MeterProviderBuilderState.cs | 11 ++++ src/OpenTelemetry/Metrics/MeterProviderSdk.cs | 6 ++- .../MeterProviderBuilderExtensionsTests.cs | 50 +++++++++++++++++++ 5 files changed, 72 insertions(+), 6 deletions(-) diff --git a/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs b/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs index 7fd082d9614..8ae5bd26e48 100644 --- a/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs +++ b/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs @@ -31,6 +31,7 @@ public static IServiceCollection AddOpenTelemetryMeterProviderBuilderServices(th { services.AddOpenTelemetryProviderBuilderServices(); + services.TryAddSingleton(); services.RegisterOptionsFactory(configuration => new MetricReaderOptions(configuration)); return services; @@ -40,8 +41,8 @@ public static IServiceCollection AddOpenTelemetryTracerProviderBuilderServices(t { services.AddOpenTelemetryProviderBuilderServices(); - services.RegisterOptionsFactory(configuration => new ExportActivityProcessorOptions(configuration)); services.TryAddSingleton(); + services.RegisterOptionsFactory(configuration => new ExportActivityProcessorOptions(configuration)); return services; } diff --git a/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderBase.cs b/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderBase.cs index 9d98c308566..14cdd0d408d 100644 --- a/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderBase.cs +++ b/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderBase.cs @@ -46,14 +46,14 @@ internal MeterProviderBuilderBase(MeterProviderBuilderState state) this.State = state; } - // This ctor is for AddOpenTelemetryTracing scenario where the builder - // is bound to an external service collection. + // This ctor is for ConfigureOpenTelemetryMetrics + + // AddOpenTelemetryMetrics scenarios where the builder is bound to an + // external service collection. internal MeterProviderBuilderBase(IServiceCollection services) { Guard.ThrowIfNull(services); services.AddOpenTelemetryMeterProviderBuilderServices(); - services.TryAddSingleton(sp => new MeterProviderSdk(sp, ownsServiceProvider: false)); this.services = services; @@ -67,6 +67,8 @@ protected MeterProviderBuilderBase() var services = new ServiceCollection(); services.AddOpenTelemetryMeterProviderBuilderServices(); + services.AddSingleton( + sp => throw new NotSupportedException("External MeterProvider created through Sdk.CreateMeterProviderBuilder cannot be accessed using service provider.")); this.services = services; this.ownsServices = true; diff --git a/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderState.cs b/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderState.cs index 5518ae1e9e3..0a312eb35fb 100644 --- a/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderState.cs +++ b/src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderState.cs @@ -41,6 +41,7 @@ internal sealed class MeterProviderBuilderState internal int MaxMetricStreams = MaxMetricsDefault; internal int MaxMetricPointsPerMetricStream = MaxMetricPointsPerMetricDefault; + private bool hasEnteredBuildPhase; private MeterProviderBuilderSdk? builder; public MeterProviderBuilderState(IServiceProvider serviceProvider) @@ -52,6 +53,16 @@ public MeterProviderBuilderState(IServiceProvider serviceProvider) public MeterProviderBuilderSdk Builder => this.builder ??= new MeterProviderBuilderSdk(this); + public void CheckForCircularBuild() + { + if (this.hasEnteredBuildPhase) + { + throw new NotSupportedException("MeterProvider cannot be accessed while build is executing."); + } + + this.hasEnteredBuildPhase = true; + } + public void AddInstrumentation( string instrumentationName, string instrumentationVersion, diff --git a/src/OpenTelemetry/Metrics/MeterProviderSdk.cs b/src/OpenTelemetry/Metrics/MeterProviderSdk.cs index 949cc25cb80..3af2055349d 100644 --- a/src/OpenTelemetry/Metrics/MeterProviderSdk.cs +++ b/src/OpenTelemetry/Metrics/MeterProviderSdk.cs @@ -22,6 +22,7 @@ using System.Diagnostics.Metrics; using System.Linq; using System.Text; +using Microsoft.Extensions.DependencyInjection; using OpenTelemetry.Internal; using OpenTelemetry.Resources; @@ -47,6 +48,9 @@ internal MeterProviderSdk( { Debug.Assert(serviceProvider != null, "serviceProvider was null"); + var state = serviceProvider!.GetRequiredService(); + state.CheckForCircularBuild(); + this.ServiceProvider = serviceProvider!; if (ownsServiceProvider) @@ -57,8 +61,6 @@ internal MeterProviderSdk( OpenTelemetrySdkEventSource.Log.MeterProviderSdkEvent("Building MeterProvider."); - var state = new MeterProviderBuilderState(serviceProvider!); - MeterProviderBuilderServiceCollectionHelper.InvokeRegisteredConfigureStateCallbacks( serviceProvider!, state); diff --git a/test/OpenTelemetry.Tests/Metrics/MeterProviderBuilderExtensionsTests.cs b/test/OpenTelemetry.Tests/Metrics/MeterProviderBuilderExtensionsTests.cs index 1bcd70a9263..5fac30cdd53 100644 --- a/test/OpenTelemetry.Tests/Metrics/MeterProviderBuilderExtensionsTests.cs +++ b/test/OpenTelemetry.Tests/Metrics/MeterProviderBuilderExtensionsTests.cs @@ -261,6 +261,56 @@ public void ConfigureBuilderIConfigurationModifiableTest() Assert.True(configureBuilderCalled); } + [Theory] + [InlineData(true)] + [InlineData(false)] + public void MeterProviderNestedResolutionUsingBuilderTest(bool callNestedConfigure) + { + bool innerTestExecuted = false; + + using var provider = Sdk.CreateMeterProviderBuilder() + .ConfigureServices(services => + { + if (callNestedConfigure) + { + services.ConfigureOpenTelemetryMetrics(); + } + }) + .ConfigureBuilder((sp, builder) => + { + innerTestExecuted = true; + Assert.Throws(() => sp.GetService()); + }) + .Build(); + + Assert.True(innerTestExecuted); + + Assert.Throws(() => provider.GetServiceProvider()?.GetService()); + } + + [Fact] + public void MeterProviderNestedResolutionUsingConfigureTest() + { + bool innerTestExecuted = false; + + var serviceCollection = new ServiceCollection(); + + serviceCollection.ConfigureOpenTelemetryMetrics(builder => + { + builder.ConfigureBuilder((sp, builder) => + { + innerTestExecuted = true; + Assert.Throws(() => sp.GetService()); + }); + }); + + using var serviceProvider = serviceCollection.BuildServiceProvider(); + + var resolvedProvider = serviceProvider.GetRequiredService(); + + Assert.True(innerTestExecuted); + } + private static void RunBuilderServiceLifecycleTest( MeterProviderBuilder builder, Func buildFunc,