Skip to content

Commit

Permalink
Code review.
Browse files Browse the repository at this point in the history
  • Loading branch information
CodeBlanch committed Apr 11, 2024
1 parent 2d99b73 commit 686339e
Show file tree
Hide file tree
Showing 5 changed files with 25 additions and 102 deletions.
1 change: 0 additions & 1 deletion OpenTelemetry.sln
Original file line number Diff line number Diff line change
Expand Up @@ -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}"
Expand Down
11 changes: 6 additions & 5 deletions src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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<OpenTelemetryLoggerOptions>();
// 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<OpenTelemetryLoggerOptions>();

/* Note: This ensures IConfiguration is available when using
* IServiceCollections NOT attached to a host. For example when
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,16 +66,19 @@ public static IServiceCollection RegisterOptionsFactory<T>(
}

#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<T>(
public static IServiceCollection DisableOptionsReloading<T>(
#endif
this IServiceCollection services)
where T : class
{
Debug.Assert(services != null, "services was null");

services!.TryAddSingleton<IOptionsMonitor<T>, SingletonOptionsMonitor<T>>();
services!.TryAddSingleton<IOptionsMonitor<T>>(sp
=> throw new NotSupportedException($"IOptionsMonitor is not supported with the '{typeof(T)}' options type."));
services!.TryAddSingleton<IOptionsSnapshot<T>>(sp
=> throw new NotSupportedException($"IOptionsSnapshot is not supported with the '{typeof(T)}' options type."));

return services!;
}
Expand Down
31 changes: 0 additions & 31 deletions src/Shared/Options/SingletonOptionsMonitor.cs

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -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<TestLogProcessor>();
var delegateInvocationCount = 0;

var root = new ConfigurationBuilder().Build();
Expand All @@ -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<IOptionsMonitor<OpenTelemetryLoggerOptions>>();
Assert.Throws<NotSupportedException>(
() => sp.GetRequiredService<IOptionsMonitor<OpenTelemetryLoggerOptions>>());
}

if (useOptionsSnapshot)
{
using var scope = sp.CreateScope();

// Note: Change notification is disabled for OpenTelemetryLoggerOptions.
Assert.Null(optionsMonitor.OnChange((o, n) => Assert.Fail()));
Assert.Throws<NotSupportedException>(
() => scope.ServiceProvider.GetRequiredService<IOptionsSnapshot<OpenTelemetryLoggerOptions>>());
}

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>
Expand Down

0 comments on commit 686339e

Please sign in to comment.