diff --git a/OpenTelemetry.sln b/OpenTelemetry.sln index 9f8349ef6c5..7a98293e6db 100644 --- a/OpenTelemetry.sln +++ b/OpenTelemetry.sln @@ -300,6 +300,7 @@ Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Options", "Options", "{4949 ProjectSection(SolutionItems) = preProject src\Shared\Options\DelegatingOptionsFactory.cs = src\Shared\Options\DelegatingOptionsFactory.cs src\Shared\Options\DelegatingOptionsFactoryServiceCollectionExtensions.cs = src\Shared\Options\DelegatingOptionsFactoryServiceCollectionExtensions.cs + src\Shared\Options\SingletonOptionsManager.cs = src\Shared\Options\SingletonOptionsManager.cs EndProjectSection EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Shims", "Shims", "{A0CB9A10-F22D-4E66-A449-74B3D0361A9C}" diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index f372be6be71..4995ad0271e 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -2,6 +2,11 @@ ## Unreleased +* Fixed an issue in Logging where unwanted objects (processors, exporters, etc.) + could be created inside delegates automatically executed by the Options API + during configuration reload. + ([#5514](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5514)) + ## 1.8.0 Released 2024-Apr-02 diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs index 15575f1c71b..06b3478e548 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs @@ -117,6 +117,7 @@ public OpenTelemetryLoggerOptions SetResourceBuilder(ResourceBuilder resourceBui Guard.ThrowIfNull(resourceBuilder); this.ResourceBuilder = resourceBuilder; + return this; } diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs index 6d7afc1ee68..6fdf9244170 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs @@ -173,6 +173,13 @@ private static ILoggingBuilder AddOpenTelemetryInternal( // Note: This will bind logger options element (e.g., "Logging:OpenTelemetry") to OpenTelemetryLoggerOptions RegisterLoggerProviderOptions(services); + // Note: We disable built-in IOptionsMonitor and IOptionsSnapshot + // features for OpenTelemetryLoggerOptions as a workaround to prevent + // unwanted objects (processors, exporters, etc.) being created by + // configuration delegates being re-run during reload of IConfiguration + // or from options created while under scopes. + services.DisableOptionsReloading(); + /* Note: This ensures IConfiguration is available when using * IServiceCollections NOT attached to a host. For example when * performing: @@ -192,7 +199,7 @@ private static ILoggingBuilder AddOpenTelemetryInternal( var loggingBuilder = new LoggerProviderBuilderBase(services).ConfigureBuilder( (sp, logging) => { - var options = sp.GetRequiredService>().CurrentValue; + var options = sp.GetRequiredService>().Value; if (options.ResourceBuilder != null) { @@ -249,7 +256,7 @@ private static ILoggingBuilder AddOpenTelemetryInternal( return new OpenTelemetryLoggerProvider( provider, - sp.GetRequiredService>().CurrentValue, + sp.GetRequiredService>().Value, disposeProvider: false); })); diff --git a/src/Shared/Options/DelegatingOptionsFactory.cs b/src/Shared/Options/DelegatingOptionsFactory.cs index b76fdc2babc..1b8fe621887 100644 --- a/src/Shared/Options/DelegatingOptionsFactory.cs +++ b/src/Shared/Options/DelegatingOptionsFactory.cs @@ -81,6 +81,7 @@ public DelegatingOptionsFactory( public TOptions Create(string name) { TOptions options = this.optionsFactoryFunc(this.configuration, name); + foreach (IConfigureOptions setup in _setups) { if (setup is IConfigureNamedOptions namedSetup) diff --git a/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs b/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs index 200e3b8c03f..69c7b6c3b62 100644 --- a/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs +++ b/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs @@ -62,6 +62,22 @@ public static IServiceCollection RegisterOptionsFactory( sp.GetServices>()); }); + return services!; + } + +#if NET6_0_OR_GREATER + public static IServiceCollection DisableOptionsReloading<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>( +#else + public static IServiceCollection DisableOptionsReloading( +#endif + this IServiceCollection services) + where T : class + { + Debug.Assert(services != null, "services was null"); + + services!.TryAddSingleton, SingletonOptionsManager>(); + services!.TryAddScoped, SingletonOptionsManager>(); + return services!; } } diff --git a/src/Shared/Options/SingletonOptionsManager.cs b/src/Shared/Options/SingletonOptionsManager.cs new file mode 100644 index 00000000000..c1807183e3f --- /dev/null +++ b/src/Shared/Options/SingletonOptionsManager.cs @@ -0,0 +1,47 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#nullable enable + +#if NET6_0_OR_GREATER +using System.Diagnostics.CodeAnalysis; +#endif + +namespace Microsoft.Extensions.Options; + +#if NET6_0_OR_GREATER +internal sealed class SingletonOptionsManager<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] TOptions> : IOptionsMonitor, IOptionsSnapshot +#else +internal sealed class SingletonOptionsManager : IOptionsMonitor, IOptionsSnapshot +#endif + where TOptions : class +{ + private readonly TOptions instance; + + public SingletonOptionsManager(IOptions options) + { + this.instance = options.Value; + } + + public TOptions CurrentValue => this.instance; + + public TOptions Value => this.instance; + + public TOptions Get(string? name) => this.instance; + + public IDisposable? OnChange(Action listener) + => NoopChangeNotification.Instance; + + private sealed class NoopChangeNotification : IDisposable + { + private NoopChangeNotification() + { + } + + public static NoopChangeNotification Instance { get; } = new(); + + public void Dispose() + { + } + } +} diff --git a/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs b/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs index cdebc61f176..e0c59ca7bf7 100644 --- a/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs +++ b/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs @@ -7,6 +7,7 @@ using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; using Xunit; namespace OpenTelemetry.Logs.Tests; @@ -297,11 +298,100 @@ public void CircularReferenceTest(bool requestLoggerProviderDirectly) Assert.True(loggerProvider.Processor is TestLogProcessorWithILoggerFactoryDependency); } - private class TestLogProcessor : BaseProcessor + [Theory] + [InlineData(true, false)] + [InlineData(false, true)] + [InlineData(false, false)] + public void OptionReloadingTest(bool useOptionsMonitor, bool useOptionsSnapshot) + { + var delegateInvocationCount = 0; + + var root = new ConfigurationBuilder().Build(); + + var services = new ServiceCollection(); + + services.AddSingleton(root); + + services.AddLogging(logging => logging + .AddConfiguration(root.GetSection("logging")) + .AddOpenTelemetry(options => + { + delegateInvocationCount++; + + options.AddProcessor(new TestLogProcessor()); + })); + + using var sp = services.BuildServiceProvider(); + + if (useOptionsMonitor) + { + var optionsMonitor = sp.GetRequiredService>(); + + Assert.NotNull(optionsMonitor.CurrentValue); + } + + if (useOptionsSnapshot) + { + using var scope = sp.CreateScope(); + + var optionsSnapshot = scope.ServiceProvider.GetRequiredService>(); + + Assert.NotNull(optionsSnapshot.Value); + } + + var loggerFactory = sp.GetRequiredService(); + + Assert.Equal(1, delegateInvocationCount); + + root.Reload(); + + Assert.Equal(1, delegateInvocationCount); + } + + [Fact] + public void MixedOptionsUsageTest() + { + var root = new ConfigurationBuilder().Build(); + + var services = new ServiceCollection(); + + services.AddSingleton(root); + + services.AddLogging(logging => logging + .AddConfiguration(root.GetSection("logging")) + .AddOpenTelemetry(options => + { + options.AddProcessor(new TestLogProcessor()); + })); + + using var sp = services.BuildServiceProvider(); + + var loggerFactory = sp.GetRequiredService(); + + var optionsMonitor = sp.GetRequiredService>().CurrentValue; + var options = sp.GetRequiredService>().Value; + + Assert.True(ReferenceEquals(options, optionsMonitor)); + + using var scope = sp.CreateScope(); + + var optionsSnapshot = scope.ServiceProvider.GetRequiredService>().Value; + Assert.True(ReferenceEquals(options, optionsSnapshot)); + } + + private sealed class TestLogProcessor : BaseProcessor { + public bool Disposed; + + protected override void Dispose(bool disposing) + { + this.Disposed = true; + + base.Dispose(disposing); + } } - private class TestLogProcessorWithILoggerFactoryDependency : BaseProcessor + private sealed class TestLogProcessorWithILoggerFactoryDependency : BaseProcessor { private readonly ILogger logger;