From 70c724f607b8dbafaaf3e8160ae15322ac55dca0 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 17 May 2023 15:46:27 -0700 Subject: [PATCH] Support builder behavior on OpenTelemetryLoggerOptions. --- ...viderBuilderServiceCollectionExtensions.cs | 1 + .../LoggerProviderBuilderExtensions.cs | 19 +++ .../ILogger/OpenTelemetryLoggerOptions.cs | 122 +++++++++++++++-- .../ILogger/OpenTelemetryLoggerProvider.cs | 17 +-- .../ILogger/OpenTelemetryLoggingExtensions.cs | 56 +++----- src/OpenTelemetry/Logs/LoggerProviderSdk.cs | 6 + .../OpenTelemetryLoggingExtensionsTests.cs | 127 ++++++++++++++++-- 7 files changed, 277 insertions(+), 71 deletions(-) diff --git a/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs b/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs index 2325bf3bec4..be35a6dc042 100644 --- a/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs +++ b/src/OpenTelemetry/Internal/Builder/ProviderBuilderServiceCollectionExtensions.cs @@ -35,6 +35,7 @@ public static IServiceCollection AddOpenTelemetryLoggerProviderBuilderServices(t services!.TryAddSingleton(); services!.RegisterOptionsFactory(configuration => new BatchExportLogRecordProcessorOptions(configuration)); + services!.RegisterOptionsFactory((sp, configuration, name) => new OpenTelemetryLoggerOptions(sp.GetRequiredService())); return services!; } diff --git a/src/OpenTelemetry/Logs/Builder/LoggerProviderBuilderExtensions.cs b/src/OpenTelemetry/Logs/Builder/LoggerProviderBuilderExtensions.cs index 78364924e03..a07436014fa 100644 --- a/src/OpenTelemetry/Logs/Builder/LoggerProviderBuilderExtensions.cs +++ b/src/OpenTelemetry/Logs/Builder/LoggerProviderBuilderExtensions.cs @@ -18,6 +18,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; +using Microsoft.Extensions.Logging; using OpenTelemetry.Internal; using OpenTelemetry.Resources; @@ -28,6 +29,24 @@ namespace OpenTelemetry.Logs; /// internal static class LoggerProviderBuilderExtensions { + /// + /// Registers a configuration action for the used by + /// integration (). + /// + /// . + /// Configuration action. + /// Returns for chaining. + public static LoggerProviderBuilder ConfigureLoggerOptions( + this LoggerProviderBuilder loggerProviderBuilder, + Action configure) + { + Guard.ThrowIfNull(configure); + + return loggerProviderBuilder.ConfigureServices( + services => services.Configure(configure)); + } + /// /// Sets the from which the Resource associated with /// this provider is built from. Overwrites currently set ResourceBuilder. diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs index f2a392e52e7..7dc54d9e175 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerOptions.cs @@ -17,6 +17,7 @@ #nullable enable using System.Diagnostics; +using Microsoft.Extensions.DependencyInjection; using OpenTelemetry.Internal; using OpenTelemetry.Resources; @@ -27,8 +28,30 @@ namespace OpenTelemetry.Logs; /// public class OpenTelemetryLoggerOptions { - internal readonly List> Processors = new(); - internal ResourceBuilder? ResourceBuilder; + private readonly LoggerProviderBuilder? loggerProviderBuilder; + private readonly LoggerProviderServiceCollectionBuilder? serviceCollectionBuilder; + private readonly LoggerProviderBuilderSdk? loggerProviderBuilderSdk; + + private bool includeFormattedMessage; + private bool includeScopes; + private bool parseStateValues; + private bool includeAttributes = true; + private bool includeTraceState; + + public OpenTelemetryLoggerOptions() + { + this.loggerProviderBuilder = Sdk.CreateLoggerProviderBuilder(); + } + + internal OpenTelemetryLoggerOptions(LoggerProviderServiceCollectionBuilder loggerProviderServiceCollection) + { + this.serviceCollectionBuilder = loggerProviderServiceCollection; + } + + internal OpenTelemetryLoggerOptions(LoggerProviderBuilderSdk loggerProviderBuilderSdk) + { + this.loggerProviderBuilderSdk = loggerProviderBuilderSdk; + } /// /// Gets or sets a value indicating whether or not formatted log message @@ -41,14 +64,22 @@ public class OpenTelemetryLoggerOptions /// message template is not found, a formatted log message is always /// included. /// - public bool IncludeFormattedMessage { get; set; } + public bool IncludeFormattedMessage + { + get => this.GetFeature(o => o.includeFormattedMessage); + set => this.SetFeature(o => o.includeFormattedMessage = value); + } /// /// Gets or sets a value indicating whether or not log scopes should be /// included on generated s. Default value: /// . /// - public bool IncludeScopes { get; set; } + public bool IncludeScopes + { + get => this.GetFeature(o => o.includeScopes); + set => this.SetFeature(o => o.includeScopes = value); + } /// /// Gets or sets a value indicating whether or not log state should be @@ -67,14 +98,22 @@ public class OpenTelemetryLoggerOptions /// langword="null"/>. /// /// - public bool ParseStateValues { get; set; } + public bool ParseStateValues + { + get => this.GetFeature(o => o.parseStateValues); + set => this.SetFeature(o => o.parseStateValues = value); + } /// /// Gets or sets a value indicating whether or not attributes specified /// via log state should be included on generated s. Default value: . /// - internal bool IncludeAttributes { get; set; } = true; + internal bool IncludeAttributes + { + get => this.GetFeature(o => o.includeAttributes); + set => this.SetFeature(o => o.includeAttributes = value); + } /// /// Gets or sets a value indicating whether or not the should be included on generated s. Default value: . /// - internal bool IncludeTraceState { get; set; } + internal bool IncludeTraceState + { + get => this.GetFeature(o => o.includeTraceState); + set => this.SetFeature(o => o.includeTraceState = value); + } /// /// Adds processor to the options. /// /// Log processor to add. /// Returns for chaining. + // todo: [Obsolete("Call ConfigureOpenTelemetry instead AddProcessor will be removed in a future version.")] public OpenTelemetryLoggerOptions AddProcessor(BaseProcessor processor) { Guard.ThrowIfNull(processor); - this.Processors.Add(processor); + this.ConfigureOpenTelemetry(builder => builder.AddProcessor(processor)); return this; } @@ -104,14 +148,51 @@ public OpenTelemetryLoggerOptions AddProcessor(BaseProcessor processo /// /// from which Resource will be built. /// Returns for chaining. + // todo: [Obsolete("Call ConfigureOpenTelemetry instead SetResourceBuilder will be removed in a future version.")] public OpenTelemetryLoggerOptions SetResourceBuilder(ResourceBuilder resourceBuilder) { Guard.ThrowIfNull(resourceBuilder); - this.ResourceBuilder = resourceBuilder; + this.ConfigureOpenTelemetry(builder => builder.SetResourceBuilder(resourceBuilder)); + return this; } + internal void ConfigureOpenTelemetry(Action configure) + { + Guard.ThrowIfNull(configure); + + if (this.serviceCollectionBuilder != null) + { + // Used during AddOpenTelemetry lifetime. IServiceCollection is open, IServiceProvider is unavailable. + configure(this.serviceCollectionBuilder); + } + else if (this.loggerProviderBuilderSdk != null) + { + // Used during OpenTelemetryLoggerProvider ctor lifetime. IServiceCollection is closed, IServiceProvider is available. + configure(this.loggerProviderBuilderSdk); + } + else if (this.loggerProviderBuilder != null) + { + // Only used for new OpenTelemetryLoggerOptions(). Shouldn't really be done but it is possible in the shipped APIs. + configure(this.loggerProviderBuilder); + } + else + { + throw new NotSupportedException("ConfigureOpenTelemetry is not supported on manually created OpenTelemetryLoggerOptions instances."); + } + } + + internal LoggerProvider Build() + { + if (this.loggerProviderBuilder != null) + { + return this.loggerProviderBuilder.Build(); + } + + throw new NotSupportedException("Build is only supported on manually created OpenTelemetryLoggerOptions instances."); + } + internal OpenTelemetryLoggerOptions Copy() { return new() @@ -123,4 +204,27 @@ internal OpenTelemetryLoggerOptions Copy() IncludeTraceState = this.IncludeTraceState, }; } + + private void SetFeature(Action action) + { + if (this.serviceCollectionBuilder != null) + { + this.serviceCollectionBuilder.ConfigureServices(services => + services.Configure(o => action(o))); + } + else + { + action(this); + } + } + + private bool GetFeature(Func func) + { + if (this.serviceCollectionBuilder != null) + { + throw new NotSupportedException("OpenTelemetryLoggerOptions cannot be read during AddOpenTelemetry invocation."); + } + + return func(this); + } } diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerProvider.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerProvider.cs index 2744b617d24..50cc23c4986 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerProvider.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggerProvider.cs @@ -54,22 +54,7 @@ public OpenTelemetryLoggerProvider(IOptionsMonitor o var optionsInstance = options.CurrentValue; - this.Provider = Sdk - .CreateLoggerProviderBuilder() - .ConfigureBuilder((sp, builder) => - { - if (optionsInstance.ResourceBuilder != null) - { - builder.SetResourceBuilder(optionsInstance.ResourceBuilder); - } - - foreach (var processor in optionsInstance.Processors) - { - builder.AddProcessor(processor); - } - }) - .Build(); - + this.Provider = optionsInstance.Build(); this.Options = optionsInstance.Copy(); this.ownsProvider = true; } diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs index 6f63fce672a..0e04f9fec65 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs @@ -43,6 +43,18 @@ public static class OpenTelemetryLoggingExtensions /// The supplied for call chaining. public static ILoggingBuilder AddOpenTelemetry( this ILoggingBuilder builder) + => AddOpenTelemetry(builder, configure: null); + + /// + /// Adds an OpenTelemetry logger named 'OpenTelemetry' to the . + /// + /// + /// The to use. + /// Optional configuration action. + /// The supplied for call chaining. + public static ILoggingBuilder AddOpenTelemetry( + this ILoggingBuilder builder, + Action? configure) { Guard.ThrowIfNull(builder); @@ -51,26 +63,6 @@ public static ILoggingBuilder AddOpenTelemetry( // Note: This will bind logger options element (eg "Logging:OpenTelemetry") to OpenTelemetryLoggerOptions LoggerProviderOptions.RegisterProviderOptions(builder.Services); - new LoggerProviderServiceCollectionBuilder(builder.Services).ConfigureBuilder( - (sp, logging) => - { - var options = sp.GetRequiredService>().CurrentValue; - - if (options.ResourceBuilder != null) - { - logging.SetResourceBuilder(options.ResourceBuilder); - - options.ResourceBuilder = null; - } - - foreach (var processor in options.Processors) - { - logging.AddProcessor(processor); - } - - options.Processors.Clear(); - }); - builder.Services.TryAddEnumerable( ServiceDescriptor.Singleton( sp => new OpenTelemetryLoggerProvider( @@ -78,25 +70,13 @@ public static ILoggingBuilder AddOpenTelemetry( sp.GetRequiredService>().CurrentValue, disposeProvider: false))); - return builder; - } + builder.Services.AddOpenTelemetrySharedProviderBuilderServices(); - /// - /// Adds an OpenTelemetry logger named 'OpenTelemetry' to the . - /// - /// - /// The to use. - /// Optional configuration action. - /// The supplied for call chaining. - public static ILoggingBuilder AddOpenTelemetry( - this ILoggingBuilder builder, - Action? configure) - { - if (configure != null) - { - builder.Services.Configure(configure); - } + var options = new OpenTelemetryLoggerOptions( + new LoggerProviderServiceCollectionBuilder(builder.Services)); + + configure?.Invoke(options); - return AddOpenTelemetry(builder); + return builder; } } diff --git a/src/OpenTelemetry/Logs/LoggerProviderSdk.cs b/src/OpenTelemetry/Logs/LoggerProviderSdk.cs index c860d525a9b..97872741b59 100644 --- a/src/OpenTelemetry/Logs/LoggerProviderSdk.cs +++ b/src/OpenTelemetry/Logs/LoggerProviderSdk.cs @@ -19,6 +19,7 @@ using System.Diagnostics; using System.Text; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; using OpenTelemetry.Internal; using OpenTelemetry.Resources; @@ -62,6 +63,11 @@ public LoggerProviderSdk( configureProviderBuilder.ConfigureBuilder(serviceProvider!, state); } + // Note: Accessing options here allows the final instance to apply + // ConfigureOpenTelemetry actions against the LoggerProviderBuilderSdk + // instance + _ = serviceProvider!.GetRequiredService>().CurrentValue; + var resourceBuilder = state.ResourceBuilder ?? ResourceBuilder.CreateDefault(); resourceBuilder.ServiceProvider = serviceProvider; this.Resource = resourceBuilder.Build(); diff --git a/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs b/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs index c6a6aa5a893..631daf46f0f 100644 --- a/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs +++ b/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs @@ -18,6 +18,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; +using OpenTelemetry.Resources; using Xunit; namespace OpenTelemetry.Logs.Tests; @@ -58,7 +59,8 @@ public void ServiceCollectionAddOpenTelemetryConfigureActionTests(int numberOfBu { int configureCallbackInvocations = 0; int optionsCallbackInvocations = 0; - OpenTelemetryLoggerOptions? optionsInstance = null; + OpenTelemetryLoggerOptions? addOpenTelemetryOptionsInstance = null; + OpenTelemetryLoggerOptions? configureOptionsInstance = null; var serviceCollection = new ServiceCollection(); @@ -81,20 +83,41 @@ public void ServiceCollectionAddOpenTelemetryConfigureActionTests(int numberOfBu Assert.NotNull(loggerFactory); - Assert.NotNull(optionsInstance); + if (numberOfBuilderRegistrations > 0) + { + Assert.NotNull(addOpenTelemetryOptionsInstance); + } + else + { + Assert.Null(addOpenTelemetryOptionsInstance); + } + + if (numberOfOptionsRegistrations > 0) + { + Assert.NotNull(configureOptionsInstance); + } + else + { + Assert.Null(configureOptionsInstance); + } + + if (numberOfBuilderRegistrations > 0 || numberOfOptionsRegistrations > 0) + { + Assert.False(ReferenceEquals(addOpenTelemetryOptionsInstance, configureOptionsInstance)); + } Assert.Equal(numberOfBuilderRegistrations, configureCallbackInvocations); Assert.Equal(numberOfOptionsRegistrations, optionsCallbackInvocations); void ConfigureCallback(OpenTelemetryLoggerOptions options) { - if (optionsInstance == null) + if (addOpenTelemetryOptionsInstance == null) { - optionsInstance = options; + addOpenTelemetryOptionsInstance = options; } else { - Assert.Equal(optionsInstance, options); + Assert.False(ReferenceEquals(configureOptionsInstance, options)); } configureCallbackInvocations++; @@ -102,16 +125,104 @@ void ConfigureCallback(OpenTelemetryLoggerOptions options) void OptionsCallback(OpenTelemetryLoggerOptions options) { - if (optionsInstance == null) + if (configureOptionsInstance == null) { - optionsInstance = options; + configureOptionsInstance = options; } else { - Assert.Equal(optionsInstance, options); + Assert.True(ReferenceEquals(configureOptionsInstance, options)); + Assert.Equal(configureOptionsInstance, options); } optionsCallbackInvocations++; } } + + [Fact] + public void ServiceCollectionAddOpenTelemetryConfigureOpenTelemetryTest() + { + var serviceCollection = new ServiceCollection(); + + serviceCollection.AddLogging(logging => logging + .AddOpenTelemetry(options => + { + options.IncludeFormattedMessage = true; + + options.AddProcessor(new CustomProcessor()); + options.SetResourceBuilder( + ResourceBuilder.CreateEmpty().AddAttributes( + new Dictionary { ["key1"] = "value1" })); + + options.ConfigureOpenTelemetry(builder => builder + .ConfigureServices(services => services.AddSingleton()) + .ConfigureResource(r => r.AddAttributes(new Dictionary { ["key2"] = "value2" })) + .AddProcessor(new CustomProcessor()) + .AddProcessor()); + })); + + serviceCollection.Configure(options => + { + options.ConfigureOpenTelemetry(builder => + { + Assert.Throws(() => builder.AddProcessor()); + builder.AddProcessor(new CustomProcessor()); + + var deferredBuilder = builder as IDeferredLoggerProviderBuilder; + Assert.NotNull(deferredBuilder); + + deferredBuilder.Configure((sp, builder) => + builder.ConfigureResource(r => r.AddAttributes(new Dictionary { ["key3"] = "value3" }))); + }); + }); + + using var sp = serviceCollection.BuildServiceProvider(); + + var customType = sp.GetService(); + + Assert.NotNull(customType); + + var loggerFactory = sp.GetService(); + + Assert.NotNull(loggerFactory); + + var loggerProvider = sp.GetRequiredService() as LoggerProviderSdk; + + Assert.NotNull(loggerProvider); + + var compositeProcessor = loggerProvider.Processor as CompositeProcessor; + + Assert.NotNull(compositeProcessor); + + var processorCount = 0; + var current = compositeProcessor.Head; + while (current != null) + { + processorCount++; + current = current.Next; + } + + Assert.Equal(4, processorCount); + + Assert.Contains(loggerProvider.Resource.Attributes, kvp => kvp.Key == "key1" && (string)kvp.Value == "value1"); + Assert.Contains(loggerProvider.Resource.Attributes, kvp => kvp.Key == "key2" && (string)kvp.Value == "value2"); + Assert.Contains(loggerProvider.Resource.Attributes, kvp => kvp.Key == "key3" && (string)kvp.Value == "value3"); + + var iloggerProviders = sp.GetServices(); + + Assert.NotNull(iloggerProviders); + Assert.Single(iloggerProviders); + + var iloggerProvider = iloggerProviders.First() as OpenTelemetryLoggerProvider; + + Assert.NotNull(iloggerProvider); + } + + private sealed class CustomType + { + } + + private sealed class CustomProcessor : BaseProcessor + { + } }