From 3c7f6746ec828c3cd941078b545f7d04c86bb55a Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Thu, 4 Apr 2024 14:04:46 -0700 Subject: [PATCH 01/15] Fixes for log options reload. --- OpenTelemetry.sln | 1 + .../ILogger/OpenTelemetryLoggerOptions.cs | 42 ++++++++++- .../ILogger/OpenTelemetryLoggingExtensions.cs | 4 ++ .../Options/DelegatingOptionsFactory.cs | 14 ++-- ...tionsFactoryServiceCollectionExtensions.cs | 24 +++++++ .../SingletonDelegatingOptionsFactory.cs | 58 +++++++++++++++ .../OpenTelemetryLoggingExtensionsTests.cs | 71 ++++++++++++++++++- 7 files changed, 205 insertions(+), 9 deletions(-) create mode 100644 src/Shared/Options/SingletonDelegatingOptionsFactory.cs diff --git a/OpenTelemetry.sln b/OpenTelemetry.sln index c9a9a4c3f6f..49e01dbea6f 100644 --- a/OpenTelemetry.sln +++ b/OpenTelemetry.sln @@ -299,6 +299,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\SingletonDelegatingOptionsFactory.cs = src\Shared\Options\SingletonDelegatingOptionsFactory.cs EndProjectSection EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Shims", "Shims", "{A0CB9A10-F22D-4E66-A449-74B3D0361A9C}" diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs index 15575f1c71b..6685849d43d 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs @@ -14,6 +14,7 @@ public class OpenTelemetryLoggerOptions { internal readonly List>> ProcessorFactories = new(); internal ResourceBuilder? ResourceBuilder; + private bool hasReloaded; /// /// Gets or sets a value indicating whether or not formatted log message @@ -86,7 +87,19 @@ public OpenTelemetryLoggerOptions AddProcessor(BaseProcessor processo { Guard.ThrowIfNull(processor); - this.ProcessorFactories.Add(_ => processor); + if (this.hasReloaded) + { + // Note: Processors may only be added during startup. If we reach + // here it means options were reloaded and a processor is being + // added via a configuration delegate. We dispose the processor + // immediately to prevent leaks of resources (for example the + // background thread created by batch processor). + processor.Dispose(); + } + else + { + this.ProcessorFactories.Add(_ => processor); + } return this; } @@ -101,7 +114,10 @@ public OpenTelemetryLoggerOptions AddProcessor( { Guard.ThrowIfNull(implementationFactory); - this.ProcessorFactories.Add(implementationFactory); + if (!this.hasReloaded) + { + this.ProcessorFactories.Add(implementationFactory); + } return this; } @@ -116,10 +132,30 @@ public OpenTelemetryLoggerOptions SetResourceBuilder(ResourceBuilder resourceBui { Guard.ThrowIfNull(resourceBuilder); - this.ResourceBuilder = resourceBuilder; + if (!this.hasReloaded) + { + this.ResourceBuilder = resourceBuilder; + } + return this; } + internal void ResetAfterConfigurationReload() + { + // Note: This is called when a configuration reload is fired. The goal + // here is to put options back into the default state so that when + // configurations are re-run everything appears as it did during + // startup and the behavior is deterministic. + + this.IncludeFormattedMessage = false; + this.IncludeScopes = false; + this.ParseStateValues = false; + this.IncludeAttributes = true; + this.IncludeTraceState = false; + + this.hasReloaded = true; + } + internal OpenTelemetryLoggerOptions Copy() { return new() diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs index 6d7afc1ee68..0a9530fd7aa 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs @@ -173,6 +173,10 @@ private static ILoggingBuilder AddOpenTelemetryInternal( // Note: This will bind logger options element (e.g., "Logging:OpenTelemetry") to OpenTelemetryLoggerOptions RegisterLoggerProviderOptions(services); + services.RegisterSingletonOptionsFactory( + c => new OpenTelemetryLoggerOptions(), + o => o.ResetAfterConfigurationReload()); + /* Note: This ensures IConfiguration is available when using * IServiceCollections NOT attached to a host. For example when * performing: diff --git a/src/Shared/Options/DelegatingOptionsFactory.cs b/src/Shared/Options/DelegatingOptionsFactory.cs index 315b38caab2..65988fcdf06 100644 --- a/src/Shared/Options/DelegatingOptionsFactory.cs +++ b/src/Shared/Options/DelegatingOptionsFactory.cs @@ -24,7 +24,7 @@ namespace Microsoft.Extensions.Options; /// Implementation of . /// /// The type of options being requested. -internal sealed class DelegatingOptionsFactory : +internal class DelegatingOptionsFactory : IOptionsFactory where TOptions : class { @@ -71,9 +71,17 @@ public DelegatingOptionsFactory( /// The created instance with the given . /// One or more return failed when validating the instance been created. /// The does not have a public parameterless constructor or is . - public TOptions Create(string name) + public virtual TOptions Create(string name) { TOptions options = this.optionsFactoryFunc(this.configuration, name); + + RunConfigurationsAndValidations(name, options); + + return options; + } + + protected void RunConfigurationsAndValidations(string name, TOptions options) + { foreach (IConfigureOptions setup in _setups) { if (setup is IConfigureNamedOptions namedSetup) @@ -106,7 +114,5 @@ public TOptions Create(string name) throw new OptionsValidationException(name, typeof(TOptions), failures); } } - - return options; } } diff --git a/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs b/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs index 1140478f740..d608e512074 100644 --- a/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs +++ b/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs @@ -53,4 +53,28 @@ public static IServiceCollection RegisterOptionsFactory( return services!; } + + public static IServiceCollection RegisterSingletonOptionsFactory( + this IServiceCollection services, + Func optionsFactoryFunc, + Action optionsResetAction) + where T : class + { + Debug.Assert(services != null, "services was null"); + Debug.Assert(optionsFactoryFunc != null, "optionsFactoryFunc was null"); + Debug.Assert(optionsResetAction != null, "optionsResetAction was null"); + + services!.TryAddSingleton>(sp => + { + return new SingletonDelegatingOptionsFactory( + (c, n) => optionsFactoryFunc!(c), + (n, o) => optionsResetAction!(o), + sp.GetRequiredService(), + sp.GetServices>(), + sp.GetServices>(), + sp.GetServices>()); + }); + + return services!; + } } diff --git a/src/Shared/Options/SingletonDelegatingOptionsFactory.cs b/src/Shared/Options/SingletonDelegatingOptionsFactory.cs new file mode 100644 index 00000000000..8d46c4d0da6 --- /dev/null +++ b/src/Shared/Options/SingletonDelegatingOptionsFactory.cs @@ -0,0 +1,58 @@ +// Copyright The OpenTelemetry Authors +// SPDX-License-Identifier: Apache-2.0 + +#nullable enable + +using System.Diagnostics; +using Microsoft.Extensions.Configuration; + +namespace Microsoft.Extensions.Options; + +internal sealed class SingletonDelegatingOptionsFactory : DelegatingOptionsFactory + where TOptions : class +{ + private readonly Action optionsResetAction; + private readonly Dictionary instances = new(); + + /// + /// Initializes a new instance of the class. + /// + /// Factory delegate used to create instances. + /// Delegate called to reset instances. + /// . + /// The configuration actions to run. + /// The initialization actions to run. + /// The validations to run. + public SingletonDelegatingOptionsFactory( + Func optionsFactoryFunc, + Action optionsResetAction, + IConfiguration configuration, + IEnumerable> setups, + IEnumerable> postConfigures, + IEnumerable> validations) + : base(optionsFactoryFunc, configuration, setups, postConfigures, validations) + { + Debug.Assert(optionsResetAction != null, "optionsResetAction was null"); + + this.optionsResetAction = optionsResetAction!; + } + + public override TOptions Create(string name) + { + lock (this.instances) + { + if (!this.instances.TryGetValue(name, out var instance)) + { + instance = base.Create(name); + this.instances.Add(name, instance); + return instance; + } + + this.optionsResetAction(name, instance); + + this.RunConfigurationsAndValidations(name, instance); + + return instance; + } + } +} diff --git a/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs b/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs index cdebc61f176..103d9a7b52c 100644 --- a/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs +++ b/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs @@ -297,11 +297,78 @@ public void CircularReferenceTest(bool requestLoggerProviderDirectly) Assert.True(loggerProvider.Processor is TestLogProcessorWithILoggerFactoryDependency); } - private class TestLogProcessor : BaseProcessor + [Fact] + public void OptionReloadingTest() { + var defaultInstance = new OpenTelemetryLoggerOptions(); + + OpenTelemetryLoggerOptions? lastOptions = null; + var processors = new List(); + 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 => + { + Assert.Equal(defaultInstance.IncludeFormattedMessage, options.IncludeFormattedMessage); + Assert.Equal(defaultInstance.IncludeScopes, options.IncludeScopes); + Assert.Equal(defaultInstance.ParseStateValues, options.ParseStateValues); + Assert.Equal(defaultInstance.IncludeAttributes, options.IncludeAttributes); + Assert.Equal(defaultInstance.IncludeTraceState, options.IncludeTraceState); + + if (lastOptions != null) + { + Assert.True(ReferenceEquals(options, lastOptions)); + } + + lastOptions = options; + + delegateInvocationCount++; + var processor = new TestLogProcessor(); + processors.Add(processor); + options.AddProcessor(processor); + + options.IncludeFormattedMessage = !defaultInstance.IncludeFormattedMessage; + options.IncludeScopes = !defaultInstance.IncludeScopes; + options.ParseStateValues = !defaultInstance.ParseStateValues; + options.IncludeAttributes = !defaultInstance.IncludeAttributes; + options.IncludeTraceState = !defaultInstance.IncludeTraceState; + })); + + using var sp = services.BuildServiceProvider(); + + var loggerFactory = sp.GetRequiredService(); + + Assert.Equal(1, delegateInvocationCount); + Assert.Single(processors); + Assert.DoesNotContain(processors, p => p.Disposed); + + root.Reload(); + + Assert.Equal(3, delegateInvocationCount); + Assert.Equal(3, processors.Count); + Assert.Equal(2, processors.Count(p => p.Disposed)); + } + + 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; From 38ddd364d51976e0861a143da0060d484113ccd2 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 5 Apr 2024 14:08:37 -0700 Subject: [PATCH 02/15] Add code comments. --- .../Logs/ILogger/OpenTelemetryLoggingExtensions.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs index 0a9530fd7aa..db23154b846 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs @@ -173,6 +173,9 @@ private static ILoggingBuilder AddOpenTelemetryInternal( // Note: This will bind logger options element (e.g., "Logging:OpenTelemetry") to OpenTelemetryLoggerOptions RegisterLoggerProviderOptions(services); + // Note: We force OpenTelemetryLoggerOptions to be a singleton and + // handle reloads explicitly. This is done to prevent leaks of batch + // processors added by configuration delegates during reload. services.RegisterSingletonOptionsFactory( c => new OpenTelemetryLoggerOptions(), o => o.ResetAfterConfigurationReload()); From 081a06f603e415dfc9b01b3ed63d269919f5f880 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Fri, 5 Apr 2024 15:13:38 -0700 Subject: [PATCH 03/15] CHANGELOG patch. --- src/OpenTelemetry/CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index f372be6be71..104a4563933 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -2,6 +2,12 @@ ## Unreleased +* `LogRecord` processors added to `OpenTelemetryLoggerOptions` during reload of + configuration will now be immediately disposed. This was done to prevent the + accumulation of extra background threads created by additional + `BatchLogRecordExportProcessor`s which are never used. + ([#5514](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5514)) + ## 1.8.0 Released 2024-Apr-02 From 57ee19990db2b38234caaf47d36f3efaec194ab2 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 9 Apr 2024 12:04:01 -0700 Subject: [PATCH 04/15] Add trim annotations. --- .../DelegatingOptionsFactoryServiceCollectionExtensions.cs | 4 ++++ src/Shared/Options/SingletonDelegatingOptionsFactory.cs | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs b/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs index f29c36441e1..6969bd0db55 100644 --- a/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs +++ b/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs @@ -65,7 +65,11 @@ public static IServiceCollection RegisterOptionsFactory( return services!; } +#if NET6_0_OR_GREATER + public static IServiceCollection RegisterSingletonOptionsFactory<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>( +#else public static IServiceCollection RegisterSingletonOptionsFactory( +#endif this IServiceCollection services, Func optionsFactoryFunc, Action optionsResetAction) diff --git a/src/Shared/Options/SingletonDelegatingOptionsFactory.cs b/src/Shared/Options/SingletonDelegatingOptionsFactory.cs index 8d46c4d0da6..712e1dd5a24 100644 --- a/src/Shared/Options/SingletonDelegatingOptionsFactory.cs +++ b/src/Shared/Options/SingletonDelegatingOptionsFactory.cs @@ -4,11 +4,18 @@ #nullable enable using System.Diagnostics; +#if NET6_0_OR_GREATER +using System.Diagnostics.CodeAnalysis; +#endif using Microsoft.Extensions.Configuration; namespace Microsoft.Extensions.Options; +#if NET6_0_OR_GREATER +internal sealed class SingletonDelegatingOptionsFactory<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] TOptions> : DelegatingOptionsFactory +#else internal sealed class SingletonDelegatingOptionsFactory : DelegatingOptionsFactory +#endif where TOptions : class { private readonly Action optionsResetAction; From 51ae4e995e9b20d02ed42e1a2fbbcdd64a7e5d98 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 9 Apr 2024 12:57:37 -0700 Subject: [PATCH 05/15] Tweaks. --- OpenTelemetry.sln | 2 +- .../ILogger/OpenTelemetryLoggerOptions.cs | 41 +----------- .../ILogger/OpenTelemetryLoggingExtensions.cs | 14 ++-- ...tionsFactoryServiceCollectionExtensions.cs | 21 ++---- .../SingletonDelegatingOptionsFactory.cs | 65 ------------------- src/Shared/Options/SingletonOptionsMonitor.cs | 31 +++++++++ .../OpenTelemetryLoggingExtensionsTests.cs | 47 ++++++++++++-- 7 files changed, 87 insertions(+), 134 deletions(-) delete mode 100644 src/Shared/Options/SingletonDelegatingOptionsFactory.cs create mode 100644 src/Shared/Options/SingletonOptionsMonitor.cs diff --git a/OpenTelemetry.sln b/OpenTelemetry.sln index e896735cd1c..9a352d5828d 100644 --- a/OpenTelemetry.sln +++ b/OpenTelemetry.sln @@ -300,7 +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\SingletonDelegatingOptionsFactory.cs = src\Shared\Options\SingletonDelegatingOptionsFactory.cs + src\Shared\Options\SingletonOptionsMonitor.cs = src\Shared\Options\SingletonOptionsMonitor.cs EndProjectSection EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Shims", "Shims", "{A0CB9A10-F22D-4E66-A449-74B3D0361A9C}" diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs index 6685849d43d..06b3478e548 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs @@ -14,7 +14,6 @@ public class OpenTelemetryLoggerOptions { internal readonly List>> ProcessorFactories = new(); internal ResourceBuilder? ResourceBuilder; - private bool hasReloaded; /// /// Gets or sets a value indicating whether or not formatted log message @@ -87,19 +86,7 @@ public OpenTelemetryLoggerOptions AddProcessor(BaseProcessor processo { Guard.ThrowIfNull(processor); - if (this.hasReloaded) - { - // Note: Processors may only be added during startup. If we reach - // here it means options were reloaded and a processor is being - // added via a configuration delegate. We dispose the processor - // immediately to prevent leaks of resources (for example the - // background thread created by batch processor). - processor.Dispose(); - } - else - { - this.ProcessorFactories.Add(_ => processor); - } + this.ProcessorFactories.Add(_ => processor); return this; } @@ -114,10 +101,7 @@ public OpenTelemetryLoggerOptions AddProcessor( { Guard.ThrowIfNull(implementationFactory); - if (!this.hasReloaded) - { - this.ProcessorFactories.Add(implementationFactory); - } + this.ProcessorFactories.Add(implementationFactory); return this; } @@ -132,30 +116,11 @@ public OpenTelemetryLoggerOptions SetResourceBuilder(ResourceBuilder resourceBui { Guard.ThrowIfNull(resourceBuilder); - if (!this.hasReloaded) - { - this.ResourceBuilder = resourceBuilder; - } + this.ResourceBuilder = resourceBuilder; return this; } - internal void ResetAfterConfigurationReload() - { - // Note: This is called when a configuration reload is fired. The goal - // here is to put options back into the default state so that when - // configurations are re-run everything appears as it did during - // startup and the behavior is deterministic. - - this.IncludeFormattedMessage = false; - this.IncludeScopes = false; - this.ParseStateValues = false; - this.IncludeAttributes = true; - this.IncludeTraceState = false; - - this.hasReloaded = true; - } - internal OpenTelemetryLoggerOptions Copy() { return new() diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs index db23154b846..e85b2d6a2e5 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs @@ -173,12 +173,10 @@ private static ILoggingBuilder AddOpenTelemetryInternal( // Note: This will bind logger options element (e.g., "Logging:OpenTelemetry") to OpenTelemetryLoggerOptions RegisterLoggerProviderOptions(services); - // Note: We force OpenTelemetryLoggerOptions to be a singleton and - // handle reloads explicitly. This is done to prevent leaks of batch - // processors added by configuration delegates during reload. - services.RegisterSingletonOptionsFactory( - c => new OpenTelemetryLoggerOptions(), - o => o.ResetAfterConfigurationReload()); + // Note: We disable built-in IOptionsMonitor features for + // OpenTelemetryLoggerOptions to prevent leaks of batch processors added + // by configuration delegates during reload of IConfiguration. + services.DisableOptionsMonitor(); /* Note: This ensures IConfiguration is available when using * IServiceCollections NOT attached to a host. For example when @@ -199,7 +197,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) { @@ -256,7 +254,7 @@ private static ILoggingBuilder AddOpenTelemetryInternal( return new OpenTelemetryLoggerProvider( provider, - sp.GetRequiredService>().CurrentValue, + sp.GetRequiredService>().Value, disposeProvider: false); })); diff --git a/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs b/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs index 6969bd0db55..6a5ef0dbdd5 100644 --- a/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs +++ b/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs @@ -66,29 +66,16 @@ public static IServiceCollection RegisterOptionsFactory( } #if NET6_0_OR_GREATER - public static IServiceCollection RegisterSingletonOptionsFactory<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>( + public static IServiceCollection DisableOptionsMonitor<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>( #else - public static IServiceCollection RegisterSingletonOptionsFactory( + public static IServiceCollection DisableOptionsMonitor( #endif - this IServiceCollection services, - Func optionsFactoryFunc, - Action optionsResetAction) + this IServiceCollection services) where T : class { Debug.Assert(services != null, "services was null"); - Debug.Assert(optionsFactoryFunc != null, "optionsFactoryFunc was null"); - Debug.Assert(optionsResetAction != null, "optionsResetAction was null"); - services!.TryAddSingleton>(sp => - { - return new SingletonDelegatingOptionsFactory( - (c, n) => optionsFactoryFunc!(c), - (n, o) => optionsResetAction!(o), - sp.GetRequiredService(), - sp.GetServices>(), - sp.GetServices>(), - sp.GetServices>()); - }); + services!.TryAddSingleton, SingletonOptionsMonitor>(); return services!; } diff --git a/src/Shared/Options/SingletonDelegatingOptionsFactory.cs b/src/Shared/Options/SingletonDelegatingOptionsFactory.cs deleted file mode 100644 index 712e1dd5a24..00000000000 --- a/src/Shared/Options/SingletonDelegatingOptionsFactory.cs +++ /dev/null @@ -1,65 +0,0 @@ -// Copyright The OpenTelemetry Authors -// SPDX-License-Identifier: Apache-2.0 - -#nullable enable - -using System.Diagnostics; -#if NET6_0_OR_GREATER -using System.Diagnostics.CodeAnalysis; -#endif -using Microsoft.Extensions.Configuration; - -namespace Microsoft.Extensions.Options; - -#if NET6_0_OR_GREATER -internal sealed class SingletonDelegatingOptionsFactory<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] TOptions> : DelegatingOptionsFactory -#else -internal sealed class SingletonDelegatingOptionsFactory : DelegatingOptionsFactory -#endif - where TOptions : class -{ - private readonly Action optionsResetAction; - private readonly Dictionary instances = new(); - - /// - /// Initializes a new instance of the class. - /// - /// Factory delegate used to create instances. - /// Delegate called to reset instances. - /// . - /// The configuration actions to run. - /// The initialization actions to run. - /// The validations to run. - public SingletonDelegatingOptionsFactory( - Func optionsFactoryFunc, - Action optionsResetAction, - IConfiguration configuration, - IEnumerable> setups, - IEnumerable> postConfigures, - IEnumerable> validations) - : base(optionsFactoryFunc, configuration, setups, postConfigures, validations) - { - Debug.Assert(optionsResetAction != null, "optionsResetAction was null"); - - this.optionsResetAction = optionsResetAction!; - } - - public override TOptions Create(string name) - { - lock (this.instances) - { - if (!this.instances.TryGetValue(name, out var instance)) - { - instance = base.Create(name); - this.instances.Add(name, instance); - return instance; - } - - this.optionsResetAction(name, instance); - - this.RunConfigurationsAndValidations(name, instance); - - return instance; - } - } -} diff --git a/src/Shared/Options/SingletonOptionsMonitor.cs b/src/Shared/Options/SingletonOptionsMonitor.cs new file mode 100644 index 00000000000..783fea06de1 --- /dev/null +++ b/src/Shared/Options/SingletonOptionsMonitor.cs @@ -0,0 +1,31 @@ +// 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 SingletonOptionsMonitor<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] TOptions> : IOptionsMonitor +#else +internal sealed class SingletonOptionsMonitor : IOptionsMonitor +#endif + where TOptions : class +{ + private readonly TOptions instance; + + public SingletonOptionsMonitor(IOptions options) + { + this.instance = options.Value; + } + + public TOptions CurrentValue => this.instance; + + public TOptions Get(string? name) => this.instance; + + public IDisposable? OnChange(Action listener) => null; +} diff --git a/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs b/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs index 103d9a7b52c..3d0fb8c950e 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,8 +298,10 @@ public void CircularReferenceTest(bool requestLoggerProviderDirectly) Assert.True(loggerProvider.Processor is TestLogProcessorWithILoggerFactoryDependency); } - [Fact] - public void OptionReloadingTest() + [Theory] + [InlineData(true)] + [InlineData(false)] + public void OptionReloadingTest(bool useOptionsMonitor) { var defaultInstance = new OpenTelemetryLoggerOptions(); @@ -343,6 +346,14 @@ public void OptionReloadingTest() using var sp = services.BuildServiceProvider(); + if (useOptionsMonitor) + { + var optionsMonitor = sp.GetRequiredService>(); + + // Note: Change notification is disabled for OpenTelemetryLoggerOptions. + Assert.Null(optionsMonitor.OnChange((o, n) => Assert.Fail())); + } + var loggerFactory = sp.GetRequiredService(); Assert.Equal(1, delegateInvocationCount); @@ -351,9 +362,35 @@ public void OptionReloadingTest() root.Reload(); - Assert.Equal(3, delegateInvocationCount); - Assert.Equal(3, processors.Count); - Assert.Equal(2, processors.Count(p => p.Disposed)); + Assert.Equal(1, delegateInvocationCount); + Assert.Single(processors); + Assert.DoesNotContain(processors, p => p.Disposed); + } + + [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)); } private sealed class TestLogProcessor : BaseProcessor From 63194a29c71de655801dd45994071f899835bd76 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 9 Apr 2024 13:05:44 -0700 Subject: [PATCH 06/15] Adjust CHANGELOG. --- src/OpenTelemetry/CHANGELOG.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 104a4563933..4ecce325771 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -2,10 +2,10 @@ ## Unreleased -* `LogRecord` processors added to `OpenTelemetryLoggerOptions` during reload of - configuration will now be immediately disposed. This was done to prevent the - accumulation of extra background threads created by additional - `BatchLogRecordExportProcessor`s which are never used. +* New instances of `OpenTelemetryLoggerOptions` will no longer be created during + configuration reload(s). This was done to prevent the accumulation of extra + background threads created by additional `BatchLogRecordExportProcessor`s + added during reload which are never used. ([#5514](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5514)) ## 1.8.0 From 5c95b817a522567bff478885c0f486faa9d4ca90 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 9 Apr 2024 13:06:53 -0700 Subject: [PATCH 07/15] Revert changes to DelegatingOptionsFactory. --- src/Shared/Options/DelegatingOptionsFactory.cs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/Shared/Options/DelegatingOptionsFactory.cs b/src/Shared/Options/DelegatingOptionsFactory.cs index 95271be264b..1b8fe621887 100644 --- a/src/Shared/Options/DelegatingOptionsFactory.cs +++ b/src/Shared/Options/DelegatingOptionsFactory.cs @@ -28,9 +28,9 @@ namespace Microsoft.Extensions.Options; /// /// The type of options being requested. #if NET6_0_OR_GREATER -internal class DelegatingOptionsFactory<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] TOptions> : +internal sealed class DelegatingOptionsFactory<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] TOptions> : #else -internal class DelegatingOptionsFactory : +internal sealed class DelegatingOptionsFactory : #endif IOptionsFactory where TOptions : class @@ -78,17 +78,10 @@ public DelegatingOptionsFactory( /// The created instance with the given . /// One or more return failed when validating the instance been created. /// The does not have a public parameterless constructor or is . - public virtual TOptions Create(string name) + public TOptions Create(string name) { TOptions options = this.optionsFactoryFunc(this.configuration, name); - RunConfigurationsAndValidations(name, options); - - return options; - } - - protected void RunConfigurationsAndValidations(string name, TOptions options) - { foreach (IConfigureOptions setup in _setups) { if (setup is IConfigureNamedOptions namedSetup) @@ -121,5 +114,7 @@ protected void RunConfigurationsAndValidations(string name, TOptions options) throw new OptionsValidationException(name, typeof(TOptions), failures); } } + + return options; } } From e467ecca93ebcba3228b4e48117404e991debd8f Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 9 Apr 2024 13:17:59 -0700 Subject: [PATCH 08/15] CHANGELOG tweaks. --- src/OpenTelemetry/CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 4ecce325771..a5cb995bba5 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -3,9 +3,9 @@ ## Unreleased * New instances of `OpenTelemetryLoggerOptions` will no longer be created during - configuration reload(s). This was done to prevent the accumulation of extra - background threads created by additional `BatchLogRecordExportProcessor`s - added during reload which are never used. + configuration reload(s). This was done to prevent the creation of unwanted + objects (processors, exporters, etc.) inside configuration delegates + automatically executed by the Options API on reload. ([#5514](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5514)) ## 1.8.0 From 702274e14b9dcbfd70c10d242d5d0aa31149cc2c Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Tue, 9 Apr 2024 14:58:56 -0700 Subject: [PATCH 09/15] CHANGELOG tweaks. --- src/OpenTelemetry/CHANGELOG.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index a5cb995bba5..4995ad0271e 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -2,10 +2,9 @@ ## Unreleased -* New instances of `OpenTelemetryLoggerOptions` will no longer be created during - configuration reload(s). This was done to prevent the creation of unwanted - objects (processors, exporters, etc.) inside configuration delegates - automatically executed by the Options API on reload. +* 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 From 2d99b7333dcb967d842f49d4527baa25f49fcfe2 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 10 Apr 2024 09:13:41 -0700 Subject: [PATCH 10/15] Update src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs Co-authored-by: Reiley Yang --- .../Logs/ILogger/OpenTelemetryLoggingExtensions.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs index e85b2d6a2e5..367c2195ce8 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs @@ -174,8 +174,9 @@ private static ILoggingBuilder AddOpenTelemetryInternal( RegisterLoggerProviderOptions(services); // Note: We disable built-in IOptionsMonitor features for - // OpenTelemetryLoggerOptions to prevent leaks of batch processors added - // by configuration delegates during reload of IConfiguration. + // OpenTelemetryLoggerOptions as a workaround to prevent unwanted + // objects (processors, exporters, etc.) being created by + // configuration delegates during reload of IConfiguration. services.DisableOptionsMonitor(); /* Note: This ensures IConfiguration is available when using From 686339e63b0727598445216f81f37e42bf66ee8a Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Thu, 11 Apr 2024 14:39:40 -0700 Subject: [PATCH 11/15] Code review. --- OpenTelemetry.sln | 1 - .../ILogger/OpenTelemetryLoggingExtensions.cs | 11 +-- ...tionsFactoryServiceCollectionExtensions.cs | 9 ++- src/Shared/Options/SingletonOptionsMonitor.cs | 31 -------- .../OpenTelemetryLoggingExtensionsTests.cs | 75 ++++--------------- 5 files changed, 25 insertions(+), 102 deletions(-) delete mode 100644 src/Shared/Options/SingletonOptionsMonitor.cs diff --git a/OpenTelemetry.sln b/OpenTelemetry.sln index 384cc0ea8fe..9f8349ef6c5 100644 --- a/OpenTelemetry.sln +++ b/OpenTelemetry.sln @@ -300,7 +300,6 @@ 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\SingletonOptionsMonitor.cs = src\Shared\Options\SingletonOptionsMonitor.cs EndProjectSection EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Shims", "Shims", "{A0CB9A10-F22D-4E66-A449-74B3D0361A9C}" diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs index 367c2195ce8..6fdf9244170 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs @@ -173,11 +173,12 @@ 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 features for - // OpenTelemetryLoggerOptions as a workaround to prevent unwanted - // objects (processors, exporters, etc.) being created by - // configuration delegates during reload of IConfiguration. - services.DisableOptionsMonitor(); + // 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 diff --git a/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs b/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs index 6a5ef0dbdd5..49ad9bc5d51 100644 --- a/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs +++ b/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs @@ -66,16 +66,19 @@ public static IServiceCollection RegisterOptionsFactory( } #if NET6_0_OR_GREATER - public static IServiceCollection DisableOptionsMonitor<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>( + public static IServiceCollection DisableOptionsReloading<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>( #else - public static IServiceCollection DisableOptionsMonitor( + public static IServiceCollection DisableOptionsReloading( #endif this IServiceCollection services) where T : class { Debug.Assert(services != null, "services was null"); - services!.TryAddSingleton, SingletonOptionsMonitor>(); + services!.TryAddSingleton>(sp + => throw new NotSupportedException($"IOptionsMonitor is not supported with the '{typeof(T)}' options type.")); + services!.TryAddSingleton>(sp + => throw new NotSupportedException($"IOptionsSnapshot is not supported with the '{typeof(T)}' options type.")); return services!; } diff --git a/src/Shared/Options/SingletonOptionsMonitor.cs b/src/Shared/Options/SingletonOptionsMonitor.cs deleted file mode 100644 index 783fea06de1..00000000000 --- a/src/Shared/Options/SingletonOptionsMonitor.cs +++ /dev/null @@ -1,31 +0,0 @@ -// 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 SingletonOptionsMonitor<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] TOptions> : IOptionsMonitor -#else -internal sealed class SingletonOptionsMonitor : IOptionsMonitor -#endif - where TOptions : class -{ - private readonly TOptions instance; - - public SingletonOptionsMonitor(IOptions options) - { - this.instance = options.Value; - } - - public TOptions CurrentValue => this.instance; - - public TOptions Get(string? name) => this.instance; - - public IDisposable? OnChange(Action listener) => null; -} diff --git a/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs b/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs index 3d0fb8c950e..762b67d68b4 100644 --- a/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs +++ b/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs @@ -299,14 +299,11 @@ public void CircularReferenceTest(bool requestLoggerProviderDirectly) } [Theory] - [InlineData(true)] - [InlineData(false)] - public void OptionReloadingTest(bool useOptionsMonitor) + [InlineData(true, false)] + [InlineData(false, true)] + [InlineData(false, false)] + public void OptionReloadingTest(bool useOptionsMonitor, bool useOptionsSnapshot) { - var defaultInstance = new OpenTelemetryLoggerOptions(); - - OpenTelemetryLoggerOptions? lastOptions = null; - var processors = new List(); var delegateInvocationCount = 0; var root = new ConfigurationBuilder().Build(); @@ -319,78 +316,32 @@ public void OptionReloadingTest(bool useOptionsMonitor) .AddConfiguration(root.GetSection("logging")) .AddOpenTelemetry(options => { - Assert.Equal(defaultInstance.IncludeFormattedMessage, options.IncludeFormattedMessage); - Assert.Equal(defaultInstance.IncludeScopes, options.IncludeScopes); - Assert.Equal(defaultInstance.ParseStateValues, options.ParseStateValues); - Assert.Equal(defaultInstance.IncludeAttributes, options.IncludeAttributes); - Assert.Equal(defaultInstance.IncludeTraceState, options.IncludeTraceState); - - if (lastOptions != null) - { - Assert.True(ReferenceEquals(options, lastOptions)); - } - - lastOptions = options; - delegateInvocationCount++; - var processor = new TestLogProcessor(); - processors.Add(processor); - options.AddProcessor(processor); - - options.IncludeFormattedMessage = !defaultInstance.IncludeFormattedMessage; - options.IncludeScopes = !defaultInstance.IncludeScopes; - options.ParseStateValues = !defaultInstance.ParseStateValues; - options.IncludeAttributes = !defaultInstance.IncludeAttributes; - options.IncludeTraceState = !defaultInstance.IncludeTraceState; })); using var sp = services.BuildServiceProvider(); if (useOptionsMonitor) { - var optionsMonitor = sp.GetRequiredService>(); + Assert.Throws( + () => sp.GetRequiredService>()); + } + + if (useOptionsSnapshot) + { + using var scope = sp.CreateScope(); - // Note: Change notification is disabled for OpenTelemetryLoggerOptions. - Assert.Null(optionsMonitor.OnChange((o, n) => Assert.Fail())); + Assert.Throws( + () => scope.ServiceProvider.GetRequiredService>()); } var loggerFactory = sp.GetRequiredService(); Assert.Equal(1, delegateInvocationCount); - Assert.Single(processors); - Assert.DoesNotContain(processors, p => p.Disposed); root.Reload(); Assert.Equal(1, delegateInvocationCount); - Assert.Single(processors); - Assert.DoesNotContain(processors, p => p.Disposed); - } - - [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)); } private sealed class TestLogProcessor : BaseProcessor From a048557ff3a2cc73dbb4e2bac43dbd34653e17d4 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Thu, 11 Apr 2024 14:43:19 -0700 Subject: [PATCH 12/15] Tweak CHANGELOG. --- src/OpenTelemetry/CHANGELOG.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 4995ad0271e..4d7d3019c87 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -2,9 +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. +* **Breaking Change** 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. Accessing + `OpenTelemetryLoggerOptions` using `IOptionsMonitor` or `IOptionsSnapshot` + will now result in a `NotSupportedException` being thrown. ([#5514](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5514)) ## 1.8.0 From a1c1acd019fa8ae1bc5be5ef7e2ab27326aa73e0 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Thu, 11 Apr 2024 15:01:43 -0700 Subject: [PATCH 13/15] Revert "Tweak CHANGELOG." This reverts commit a048557ff3a2cc73dbb4e2bac43dbd34653e17d4. --- src/OpenTelemetry/CHANGELOG.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 4d7d3019c87..4995ad0271e 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -2,11 +2,9 @@ ## Unreleased -* **Breaking Change** 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. Accessing - `OpenTelemetryLoggerOptions` using `IOptionsMonitor` or `IOptionsSnapshot` - will now result in a `NotSupportedException` being thrown. +* 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 From a1d746148ef78fd93921eb9a98be88bdbb79a0a5 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Thu, 11 Apr 2024 15:01:51 -0700 Subject: [PATCH 14/15] Revert "Code review." This reverts commit 686339e63b0727598445216f81f37e42bf66ee8a. --- OpenTelemetry.sln | 1 + .../ILogger/OpenTelemetryLoggingExtensions.cs | 11 ++- ...tionsFactoryServiceCollectionExtensions.cs | 9 +-- src/Shared/Options/SingletonOptionsMonitor.cs | 31 ++++++++ .../OpenTelemetryLoggingExtensionsTests.cs | 75 +++++++++++++++---- 5 files changed, 102 insertions(+), 25 deletions(-) create mode 100644 src/Shared/Options/SingletonOptionsMonitor.cs diff --git a/OpenTelemetry.sln b/OpenTelemetry.sln index 9f8349ef6c5..384cc0ea8fe 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\SingletonOptionsMonitor.cs = src\Shared\Options\SingletonOptionsMonitor.cs EndProjectSection EndProject Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "Shims", "Shims", "{A0CB9A10-F22D-4E66-A449-74B3D0361A9C}" diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs index 6fdf9244170..367c2195ce8 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs @@ -173,12 +173,11 @@ 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: We disable built-in IOptionsMonitor features for + // OpenTelemetryLoggerOptions as a workaround to prevent unwanted + // objects (processors, exporters, etc.) being created by + // configuration delegates during reload of IConfiguration. + services.DisableOptionsMonitor(); /* Note: This ensures IConfiguration is available when using * IServiceCollections NOT attached to a host. For example when diff --git a/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs b/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs index 49ad9bc5d51..6a5ef0dbdd5 100644 --- a/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs +++ b/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs @@ -66,19 +66,16 @@ public static IServiceCollection RegisterOptionsFactory( } #if NET6_0_OR_GREATER - public static IServiceCollection DisableOptionsReloading<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>( + public static IServiceCollection DisableOptionsMonitor<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>( #else - public static IServiceCollection DisableOptionsReloading( + public static IServiceCollection DisableOptionsMonitor( #endif this IServiceCollection services) where T : class { Debug.Assert(services != null, "services was null"); - services!.TryAddSingleton>(sp - => throw new NotSupportedException($"IOptionsMonitor is not supported with the '{typeof(T)}' options type.")); - services!.TryAddSingleton>(sp - => throw new NotSupportedException($"IOptionsSnapshot is not supported with the '{typeof(T)}' options type.")); + services!.TryAddSingleton, SingletonOptionsMonitor>(); return services!; } diff --git a/src/Shared/Options/SingletonOptionsMonitor.cs b/src/Shared/Options/SingletonOptionsMonitor.cs new file mode 100644 index 00000000000..783fea06de1 --- /dev/null +++ b/src/Shared/Options/SingletonOptionsMonitor.cs @@ -0,0 +1,31 @@ +// 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 SingletonOptionsMonitor<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] TOptions> : IOptionsMonitor +#else +internal sealed class SingletonOptionsMonitor : IOptionsMonitor +#endif + where TOptions : class +{ + private readonly TOptions instance; + + public SingletonOptionsMonitor(IOptions options) + { + this.instance = options.Value; + } + + public TOptions CurrentValue => this.instance; + + public TOptions Get(string? name) => this.instance; + + public IDisposable? OnChange(Action listener) => null; +} diff --git a/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs b/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs index 762b67d68b4..3d0fb8c950e 100644 --- a/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs +++ b/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs @@ -299,11 +299,14 @@ public void CircularReferenceTest(bool requestLoggerProviderDirectly) } [Theory] - [InlineData(true, false)] - [InlineData(false, true)] - [InlineData(false, false)] - public void OptionReloadingTest(bool useOptionsMonitor, bool useOptionsSnapshot) + [InlineData(true)] + [InlineData(false)] + public void OptionReloadingTest(bool useOptionsMonitor) { + var defaultInstance = new OpenTelemetryLoggerOptions(); + + OpenTelemetryLoggerOptions? lastOptions = null; + var processors = new List(); var delegateInvocationCount = 0; var root = new ConfigurationBuilder().Build(); @@ -316,32 +319,78 @@ public void OptionReloadingTest(bool useOptionsMonitor, bool useOptionsSnapshot) .AddConfiguration(root.GetSection("logging")) .AddOpenTelemetry(options => { + Assert.Equal(defaultInstance.IncludeFormattedMessage, options.IncludeFormattedMessage); + Assert.Equal(defaultInstance.IncludeScopes, options.IncludeScopes); + Assert.Equal(defaultInstance.ParseStateValues, options.ParseStateValues); + Assert.Equal(defaultInstance.IncludeAttributes, options.IncludeAttributes); + Assert.Equal(defaultInstance.IncludeTraceState, options.IncludeTraceState); + + if (lastOptions != null) + { + Assert.True(ReferenceEquals(options, lastOptions)); + } + + lastOptions = options; + delegateInvocationCount++; + var processor = new TestLogProcessor(); + processors.Add(processor); + options.AddProcessor(processor); + + options.IncludeFormattedMessage = !defaultInstance.IncludeFormattedMessage; + options.IncludeScopes = !defaultInstance.IncludeScopes; + options.ParseStateValues = !defaultInstance.ParseStateValues; + options.IncludeAttributes = !defaultInstance.IncludeAttributes; + options.IncludeTraceState = !defaultInstance.IncludeTraceState; })); using var sp = services.BuildServiceProvider(); if (useOptionsMonitor) { - Assert.Throws( - () => sp.GetRequiredService>()); - } - - if (useOptionsSnapshot) - { - using var scope = sp.CreateScope(); + var optionsMonitor = sp.GetRequiredService>(); - Assert.Throws( - () => scope.ServiceProvider.GetRequiredService>()); + // Note: Change notification is disabled for OpenTelemetryLoggerOptions. + Assert.Null(optionsMonitor.OnChange((o, n) => Assert.Fail())); } var loggerFactory = sp.GetRequiredService(); Assert.Equal(1, delegateInvocationCount); + Assert.Single(processors); + Assert.DoesNotContain(processors, p => p.Disposed); root.Reload(); Assert.Equal(1, delegateInvocationCount); + Assert.Single(processors); + Assert.DoesNotContain(processors, p => p.Disposed); + } + + [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)); } private sealed class TestLogProcessor : BaseProcessor From 57acafdabe59c234cd5b99c345f045def72b1757 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Thu, 11 Apr 2024 15:19:15 -0700 Subject: [PATCH 15/15] Code review. --- OpenTelemetry.sln | 2 +- .../ILogger/OpenTelemetryLoggingExtensions.cs | 11 ++-- ...tionsFactoryServiceCollectionExtensions.cs | 7 ++- src/Shared/Options/SingletonOptionsManager.cs | 47 ++++++++++++++++ src/Shared/Options/SingletonOptionsMonitor.cs | 31 ---------- .../OpenTelemetryLoggingExtensionsTests.cs | 56 +++++++------------ 6 files changed, 79 insertions(+), 75 deletions(-) create mode 100644 src/Shared/Options/SingletonOptionsManager.cs delete mode 100644 src/Shared/Options/SingletonOptionsMonitor.cs diff --git a/OpenTelemetry.sln b/OpenTelemetry.sln index 384cc0ea8fe..7a98293e6db 100644 --- a/OpenTelemetry.sln +++ b/OpenTelemetry.sln @@ -300,7 +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\SingletonOptionsMonitor.cs = src\Shared\Options\SingletonOptionsMonitor.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/Logs/ILogger/OpenTelemetryLoggingExtensions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs index 367c2195ce8..6fdf9244170 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs @@ -173,11 +173,12 @@ 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 features for - // OpenTelemetryLoggerOptions as a workaround to prevent unwanted - // objects (processors, exporters, etc.) being created by - // configuration delegates during reload of IConfiguration. - services.DisableOptionsMonitor(); + // 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 diff --git a/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs b/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs index 6a5ef0dbdd5..69c7b6c3b62 100644 --- a/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs +++ b/src/Shared/Options/DelegatingOptionsFactoryServiceCollectionExtensions.cs @@ -66,16 +66,17 @@ public static IServiceCollection RegisterOptionsFactory( } #if NET6_0_OR_GREATER - public static IServiceCollection DisableOptionsMonitor<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>( + public static IServiceCollection DisableOptionsReloading<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>( #else - public static IServiceCollection DisableOptionsMonitor( + public static IServiceCollection DisableOptionsReloading( #endif this IServiceCollection services) where T : class { Debug.Assert(services != null, "services was null"); - services!.TryAddSingleton, SingletonOptionsMonitor>(); + 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/src/Shared/Options/SingletonOptionsMonitor.cs b/src/Shared/Options/SingletonOptionsMonitor.cs deleted file mode 100644 index 783fea06de1..00000000000 --- a/src/Shared/Options/SingletonOptionsMonitor.cs +++ /dev/null @@ -1,31 +0,0 @@ -// 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 SingletonOptionsMonitor<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] TOptions> : IOptionsMonitor -#else -internal sealed class SingletonOptionsMonitor : IOptionsMonitor -#endif - where TOptions : class -{ - private readonly TOptions instance; - - public SingletonOptionsMonitor(IOptions options) - { - this.instance = options.Value; - } - - public TOptions CurrentValue => this.instance; - - public TOptions Get(string? name) => this.instance; - - public IDisposable? OnChange(Action listener) => null; -} diff --git a/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs b/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs index 3d0fb8c950e..e0c59ca7bf7 100644 --- a/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs +++ b/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs @@ -299,14 +299,11 @@ public void CircularReferenceTest(bool requestLoggerProviderDirectly) } [Theory] - [InlineData(true)] - [InlineData(false)] - public void OptionReloadingTest(bool useOptionsMonitor) + [InlineData(true, false)] + [InlineData(false, true)] + [InlineData(false, false)] + public void OptionReloadingTest(bool useOptionsMonitor, bool useOptionsSnapshot) { - var defaultInstance = new OpenTelemetryLoggerOptions(); - - OpenTelemetryLoggerOptions? lastOptions = null; - var processors = new List(); var delegateInvocationCount = 0; var root = new ConfigurationBuilder().Build(); @@ -319,29 +316,9 @@ public void OptionReloadingTest(bool useOptionsMonitor) .AddConfiguration(root.GetSection("logging")) .AddOpenTelemetry(options => { - Assert.Equal(defaultInstance.IncludeFormattedMessage, options.IncludeFormattedMessage); - Assert.Equal(defaultInstance.IncludeScopes, options.IncludeScopes); - Assert.Equal(defaultInstance.ParseStateValues, options.ParseStateValues); - Assert.Equal(defaultInstance.IncludeAttributes, options.IncludeAttributes); - Assert.Equal(defaultInstance.IncludeTraceState, options.IncludeTraceState); - - if (lastOptions != null) - { - Assert.True(ReferenceEquals(options, lastOptions)); - } - - lastOptions = options; - delegateInvocationCount++; - var processor = new TestLogProcessor(); - processors.Add(processor); - options.AddProcessor(processor); - - options.IncludeFormattedMessage = !defaultInstance.IncludeFormattedMessage; - options.IncludeScopes = !defaultInstance.IncludeScopes; - options.ParseStateValues = !defaultInstance.ParseStateValues; - options.IncludeAttributes = !defaultInstance.IncludeAttributes; - options.IncludeTraceState = !defaultInstance.IncludeTraceState; + + options.AddProcessor(new TestLogProcessor()); })); using var sp = services.BuildServiceProvider(); @@ -350,21 +327,25 @@ public void OptionReloadingTest(bool useOptionsMonitor) { var optionsMonitor = sp.GetRequiredService>(); - // Note: Change notification is disabled for OpenTelemetryLoggerOptions. - Assert.Null(optionsMonitor.OnChange((o, n) => Assert.Fail())); + 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); - Assert.Single(processors); - Assert.DoesNotContain(processors, p => p.Disposed); root.Reload(); Assert.Equal(1, delegateInvocationCount); - Assert.Single(processors); - Assert.DoesNotContain(processors, p => p.Disposed); } [Fact] @@ -391,6 +372,11 @@ public void MixedOptionsUsageTest() 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