Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[logs] Mitigate unwanted object creation during configuration reload #5514

Merged
merged 19 commits into from
Apr 12, 2024
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions OpenTelemetry.sln
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Expand Down
5 changes: 5 additions & 0 deletions src/OpenTelemetry/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ public OpenTelemetryLoggerOptions SetResourceBuilder(ResourceBuilder resourceBui
Guard.ThrowIfNull(resourceBuilder);

this.ResourceBuilder = resourceBuilder;

return this;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +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 features for
// OpenTelemetryLoggerOptions to prevent leaks of batch processors added
// by configuration delegates during reload of IConfiguration.
CodeBlanch marked this conversation as resolved.
Show resolved Hide resolved
services.DisableOptionsMonitor<OpenTelemetryLoggerOptions>();

/* Note: This ensures IConfiguration is available when using
* IServiceCollections NOT attached to a host. For example when
* performing:
Expand All @@ -192,7 +197,7 @@ private static ILoggingBuilder AddOpenTelemetryInternal(
var loggingBuilder = new LoggerProviderBuilderBase(services).ConfigureBuilder(
(sp, logging) =>
{
var options = sp.GetRequiredService<IOptionsMonitor<OpenTelemetryLoggerOptions>>().CurrentValue;
var options = sp.GetRequiredService<IOptions<OpenTelemetryLoggerOptions>>().Value;

if (options.ResourceBuilder != null)
{
Expand Down Expand Up @@ -249,7 +254,7 @@ private static ILoggingBuilder AddOpenTelemetryInternal(

return new OpenTelemetryLoggerProvider(
provider,
sp.GetRequiredService<IOptionsMonitor<OpenTelemetryLoggerOptions>>().CurrentValue,
sp.GetRequiredService<IOptions<OpenTelemetryLoggerOptions>>().Value,
disposeProvider: false);
}));

Expand Down
1 change: 1 addition & 0 deletions src/Shared/Options/DelegatingOptionsFactory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ public DelegatingOptionsFactory(
public TOptions Create(string name)
{
TOptions options = this.optionsFactoryFunc(this.configuration, name);

foreach (IConfigureOptions<TOptions> setup in _setups)
{
if (setup is IConfigureNamedOptions<TOptions> namedSetup)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,21 @@ public static IServiceCollection RegisterOptionsFactory<T>(
sp.GetServices<IValidateOptions<T>>());
});

return services!;
}

#if NET6_0_OR_GREATER
public static IServiceCollection DisableOptionsMonitor<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] T>(
#else
public static IServiceCollection DisableOptionsMonitor<T>(
#endif
this IServiceCollection services)
where T : class
{
Debug.Assert(services != null, "services was null");

services!.TryAddSingleton<IOptionsMonitor<T>, SingletonOptionsMonitor<T>>();

return services!;
}
}
31 changes: 31 additions & 0 deletions src/Shared/Options/SingletonOptionsMonitor.cs
Original file line number Diff line number Diff line change
@@ -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<TOptions>
#else
internal sealed class SingletonOptionsMonitor<TOptions> : IOptionsMonitor<TOptions>
#endif
where TOptions : class
{
private readonly TOptions instance;

public SingletonOptionsMonitor(IOptions<TOptions> options)
{
this.instance = options.Value;
}

public TOptions CurrentValue => this.instance;

public TOptions Get(string? name) => this.instance;

public IDisposable? OnChange(Action<TOptions, string?> listener) => null;
}
108 changes: 106 additions & 2 deletions test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -297,11 +298,114 @@ public void CircularReferenceTest(bool requestLoggerProviderDirectly)
Assert.True(loggerProvider.Processor is TestLogProcessorWithILoggerFactoryDependency);
}

private class TestLogProcessor : BaseProcessor<LogRecord>
[Theory]
[InlineData(true)]
[InlineData(false)]
public void OptionReloadingTest(bool useOptionsMonitor)
{
var defaultInstance = new OpenTelemetryLoggerOptions();

OpenTelemetryLoggerOptions? lastOptions = null;
var processors = new List<TestLogProcessor>();
var delegateInvocationCount = 0;

var root = new ConfigurationBuilder().Build();

var services = new ServiceCollection();

services.AddSingleton<IConfiguration>(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();

if (useOptionsMonitor)
{
var optionsMonitor = sp.GetRequiredService<IOptionsMonitor<OpenTelemetryLoggerOptions>>();

// Note: Change notification is disabled for OpenTelemetryLoggerOptions.
Assert.Null(optionsMonitor.OnChange((o, n) => Assert.Fail()));
}

var loggerFactory = sp.GetRequiredService<ILoggerFactory>();

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<IConfiguration>(root);

services.AddLogging(logging => logging
.AddConfiguration(root.GetSection("logging"))
.AddOpenTelemetry(options =>
{
options.AddProcessor(new TestLogProcessor());
}));

using var sp = services.BuildServiceProvider();

var loggerFactory = sp.GetRequiredService<ILoggerFactory>();

var optionsMonitor = sp.GetRequiredService<IOptionsMonitor<OpenTelemetryLoggerOptions>>().CurrentValue;
var options = sp.GetRequiredService<IOptions<OpenTelemetryLoggerOptions>>().Value;

Assert.True(ReferenceEquals(options, optionsMonitor));
}

private sealed class TestLogProcessor : BaseProcessor<LogRecord>
{
public bool Disposed;

protected override void Dispose(bool disposing)
{
this.Disposed = true;

base.Dispose(disposing);
}
}

private class TestLogProcessorWithILoggerFactoryDependency : BaseProcessor<LogRecord>
private sealed class TestLogProcessorWithILoggerFactoryDependency : BaseProcessor<LogRecord>
{
private readonly ILogger logger;

Expand Down