Skip to content

Commit

Permalink
Fix circular reference issue building up meter provider. (#3806)
Browse files Browse the repository at this point in the history
Co-authored-by: Utkarsh Umesan Pillai <utpilla@microsoft.com>
  • Loading branch information
CodeBlanch and utpilla authored Oct 21, 2022
1 parent 3c245ef commit 0804334
Show file tree
Hide file tree
Showing 5 changed files with 72 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ public static IServiceCollection AddOpenTelemetryMeterProviderBuilderServices(th
{
services.AddOpenTelemetryProviderBuilderServices();

services.TryAddSingleton<MeterProviderBuilderState>();
services.RegisterOptionsFactory(configuration => new MetricReaderOptions(configuration));

return services;
Expand All @@ -40,8 +41,8 @@ public static IServiceCollection AddOpenTelemetryTracerProviderBuilderServices(t
{
services.AddOpenTelemetryProviderBuilderServices();

services.RegisterOptionsFactory(configuration => new ExportActivityProcessorOptions(configuration));
services.TryAddSingleton<TracerProviderBuilderState>();
services.RegisterOptionsFactory(configuration => new ExportActivityProcessorOptions(configuration));

return services;
}
Expand Down
8 changes: 5 additions & 3 deletions src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderBase.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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<MeterProvider>(sp => new MeterProviderSdk(sp, ownsServiceProvider: false));

this.services = services;
Expand All @@ -67,6 +67,8 @@ protected MeterProviderBuilderBase()
var services = new ServiceCollection();

services.AddOpenTelemetryMeterProviderBuilderServices();
services.AddSingleton<MeterProvider>(
sp => throw new NotSupportedException("External MeterProvider created through Sdk.CreateMeterProviderBuilder cannot be accessed using service provider."));

this.services = services;
this.ownsServices = true;
Expand Down
11 changes: 11 additions & 0 deletions src/OpenTelemetry/Metrics/Builder/MeterProviderBuilderState.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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,
Expand Down
6 changes: 4 additions & 2 deletions src/OpenTelemetry/Metrics/MeterProviderSdk.cs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
using System.Diagnostics.Metrics;
using System.Linq;
using System.Text;
using Microsoft.Extensions.DependencyInjection;
using OpenTelemetry.Internal;
using OpenTelemetry.Resources;

Expand All @@ -47,6 +48,9 @@ internal MeterProviderSdk(
{
Debug.Assert(serviceProvider != null, "serviceProvider was null");

var state = serviceProvider!.GetRequiredService<MeterProviderBuilderState>();
state.CheckForCircularBuild();

this.ServiceProvider = serviceProvider!;

if (ownsServiceProvider)
Expand All @@ -57,8 +61,6 @@ internal MeterProviderSdk(

OpenTelemetrySdkEventSource.Log.MeterProviderSdkEvent("Building MeterProvider.");

var state = new MeterProviderBuilderState(serviceProvider!);

MeterProviderBuilderServiceCollectionHelper.InvokeRegisteredConfigureStateCallbacks(
serviceProvider!,
state);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<NotSupportedException>(() => sp.GetService<MeterProvider>());
})
.Build();

Assert.True(innerTestExecuted);

Assert.Throws<NotSupportedException>(() => provider.GetServiceProvider()?.GetService<MeterProvider>());
}

[Fact]
public void MeterProviderNestedResolutionUsingConfigureTest()
{
bool innerTestExecuted = false;

var serviceCollection = new ServiceCollection();

serviceCollection.ConfigureOpenTelemetryMetrics(builder =>
{
builder.ConfigureBuilder((sp, builder) =>
{
innerTestExecuted = true;
Assert.Throws<NotSupportedException>(() => sp.GetService<MeterProvider>());
});
});

using var serviceProvider = serviceCollection.BuildServiceProvider();

var resolvedProvider = serviceProvider.GetRequiredService<MeterProvider>();

Assert.True(innerTestExecuted);
}

private static void RunBuilderServiceLifecycleTest(
MeterProviderBuilder builder,
Func<MeterProviderSdk> buildFunc,
Expand Down

0 comments on commit 0804334

Please sign in to comment.