From a47b2221015240694050e5fb3fcf12b4b43a0630 Mon Sep 17 00:00:00 2001 From: Mikel Blanchard Date: Wed, 29 Nov 2023 15:15:46 -0800 Subject: [PATCH] [logging] UseOpenTelemetry extension & WithLogging default behavior (#5072) --- .../Experimental/PublicAPI.Unshipped.txt | 1 + .../CHANGELOG.md | 5 + .../OpenTelemetryBuilder.cs | 61 +++++++- .../Experimental/PublicAPI.Unshipped.txt | 3 + src/OpenTelemetry/CHANGELOG.md | 12 ++ .../ILogger/OpenTelemetryLoggingExtensions.cs | 114 ++++++++++++++- .../OpenTelemetryServicesExtensionsTests.cs | 8 +- .../OpenTelemetryLoggingExtensionsTests.cs | 137 ++++++++++++++++-- 8 files changed, 304 insertions(+), 37 deletions(-) diff --git a/src/OpenTelemetry.Extensions.Hosting/.publicApi/Experimental/PublicAPI.Unshipped.txt b/src/OpenTelemetry.Extensions.Hosting/.publicApi/Experimental/PublicAPI.Unshipped.txt index 8617a4e70a1..f83d7ca4a0f 100644 --- a/src/OpenTelemetry.Extensions.Hosting/.publicApi/Experimental/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry.Extensions.Hosting/.publicApi/Experimental/PublicAPI.Unshipped.txt @@ -1,2 +1,3 @@ OpenTelemetry.OpenTelemetryBuilder.WithLogging() -> OpenTelemetry.OpenTelemetryBuilder! OpenTelemetry.OpenTelemetryBuilder.WithLogging(System.Action! configure) -> OpenTelemetry.OpenTelemetryBuilder! +OpenTelemetry.OpenTelemetryBuilder.WithLogging(System.Action? configureBuilder, System.Action? configureOptions) -> OpenTelemetry.OpenTelemetryBuilder! diff --git a/src/OpenTelemetry.Extensions.Hosting/CHANGELOG.md b/src/OpenTelemetry.Extensions.Hosting/CHANGELOG.md index 6b801fc615c..01bff688c3b 100644 --- a/src/OpenTelemetry.Extensions.Hosting/CHANGELOG.md +++ b/src/OpenTelemetry.Extensions.Hosting/CHANGELOG.md @@ -12,6 +12,11 @@ APIs. ([#4958](https://github.com/open-telemetry/opentelemetry-dotnet/pull/4958)) +* The `OpenTelemetryBuilder.WithLogging` experimental API method will now + register an `ILoggerProvider` named 'OpenTelemetry' into the + `IServiceCollection` to enable `ILoggerFactory` integration. + ([#5072](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5072)) + ## 1.7.0-alpha.1 Released 2023-Oct-16 diff --git a/src/OpenTelemetry.Extensions.Hosting/OpenTelemetryBuilder.cs b/src/OpenTelemetry.Extensions.Hosting/OpenTelemetryBuilder.cs index 00af83db167..ee120839dc8 100644 --- a/src/OpenTelemetry.Extensions.Hosting/OpenTelemetryBuilder.cs +++ b/src/OpenTelemetry.Extensions.Hosting/OpenTelemetryBuilder.cs @@ -16,6 +16,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Diagnostics.Metrics; +using Microsoft.Extensions.Logging; using OpenTelemetry.Internal; using OpenTelemetry.Logs; using OpenTelemetry.Metrics; @@ -146,10 +147,17 @@ public OpenTelemetryBuilder WithTracing(Action configure) /// Adds logging services into the builder. /// /// - /// WARNING: This is an experimental API which might change or be removed in the future. Use at your own risk. - /// Note: This is safe to be called multiple times and by library authors. + /// WARNING: This is an experimental API which might change or + /// be removed in the future. Use at your own risk. + /// Notes: + /// + /// This is safe to be called multiple times and by library authors. /// Only a single will be created for a given - /// . + /// . + /// This method automatically registers an named 'OpenTelemetry' into the . + /// /// /// The supplied for chaining /// calls. @@ -159,16 +167,22 @@ public OpenTelemetryBuilder WithTracing(Action configure) /// Adds logging services into the builder. /// /// - /// Note: This is safe to be called multiple times and by library authors. + /// Notes: + /// + /// This is safe to be called multiple times and by library authors. /// Only a single will be created for a given - /// . + /// . + /// This method automatically registers an named 'OpenTelemetry' into the . + /// /// /// The supplied for chaining /// calls. internal #endif OpenTelemetryBuilder WithLogging() - => this.WithLogging(b => { }); + => this.WithLogging(configureBuilder: null, configureOptions: null); #if EXPOSE_EXPERIMENTAL_FEATURES /// @@ -195,9 +209,40 @@ OpenTelemetryBuilder WithLogging(Action configure) { Guard.ThrowIfNull(configure); - var builder = new LoggerProviderBuilderBase(this.Services); + return this.WithLogging(configureBuilder: configure, configureOptions: null); + } - configure(builder); +#if EXPOSE_EXPERIMENTAL_FEATURES + /// + /// Adds logging services into the builder. + /// + /// + /// Optional configuration callback. + /// Optional configuration callback. + /// The supplied for chaining + /// calls. + public +#else + /// + /// Adds logging services into the builder. + /// + /// + /// Optional configuration callback. + /// Optional configuration callback. + /// The supplied for chaining + /// calls. + internal +#endif + OpenTelemetryBuilder WithLogging( + Action? configureBuilder, + Action? configureOptions) + { + this.Services.AddLogging( + logging => logging.UseOpenTelemetry(configureBuilder, configureOptions)); return this; } diff --git a/src/OpenTelemetry/.publicApi/Experimental/PublicAPI.Unshipped.txt b/src/OpenTelemetry/.publicApi/Experimental/PublicAPI.Unshipped.txt index 862c997fe22..bb5bd1824f6 100644 --- a/src/OpenTelemetry/.publicApi/Experimental/PublicAPI.Unshipped.txt +++ b/src/OpenTelemetry/.publicApi/Experimental/PublicAPI.Unshipped.txt @@ -40,3 +40,6 @@ override OpenTelemetry.Metrics.AlwaysOnExemplarFilter.ShouldSample(double value, override OpenTelemetry.Metrics.AlwaysOnExemplarFilter.ShouldSample(long value, System.ReadOnlySpan> tags) -> bool override OpenTelemetry.Metrics.TraceBasedExemplarFilter.ShouldSample(double value, System.ReadOnlySpan> tags) -> bool override OpenTelemetry.Metrics.TraceBasedExemplarFilter.ShouldSample(long value, System.ReadOnlySpan> tags) -> bool +static Microsoft.Extensions.Logging.OpenTelemetryLoggingExtensions.UseOpenTelemetry(this Microsoft.Extensions.Logging.ILoggingBuilder! builder) -> Microsoft.Extensions.Logging.ILoggingBuilder! +static Microsoft.Extensions.Logging.OpenTelemetryLoggingExtensions.UseOpenTelemetry(this Microsoft.Extensions.Logging.ILoggingBuilder! builder, System.Action! configure) -> Microsoft.Extensions.Logging.ILoggingBuilder! +static Microsoft.Extensions.Logging.OpenTelemetryLoggingExtensions.UseOpenTelemetry(this Microsoft.Extensions.Logging.ILoggingBuilder! builder, System.Action? configureBuilder, System.Action? configureOptions) -> Microsoft.Extensions.Logging.ILoggingBuilder! diff --git a/src/OpenTelemetry/CHANGELOG.md b/src/OpenTelemetry/CHANGELOG.md index 79926bb48d2..d4c1f180197 100644 --- a/src/OpenTelemetry/CHANGELOG.md +++ b/src/OpenTelemetry/CHANGELOG.md @@ -64,6 +64,18 @@ [#4563](https://github.com/open-telemetry/opentelemetry-dotnet/issues/4563). ([#5089](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5089)) +* Added the `ILoggingBuilder.UseOpenTelemetry` experimental API extension for + registering OpenTelemetry `ILogger` integration using `LoggerProviderBuilder` + which supports the full DI (`IServiceCollection` \ `IServiceProvider`) API + surface (mirrors tracing & metrics). + ([#5072](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5072)) + +* Changed the `ILoggingBuilder` registration extensions (`AddOpenTelemetry` & + `UseOpenTelemetry`) to fire the optional `OpenTelemetryLoggerOptions` + configuration delegate AFTER the "Logging:OpenTelemetry" `IConfiguration` + section has been applied. + ([#5072](https://github.com/open-telemetry/opentelemetry-dotnet/pull/5072)) + ## 1.7.0-alpha.1 Released 2023-Oct-16 diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs index 1dfbd6fe408..6a262a7b3b0 100644 --- a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs +++ b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs @@ -17,6 +17,9 @@ #if NET6_0_OR_GREATER using System.Diagnostics.CodeAnalysis; #endif +#if EXPOSE_EXPERIMENTAL_FEATURES +using System.ComponentModel; +#endif using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.Logging; @@ -48,6 +51,11 @@ public static class OpenTelemetryLoggingExtensions /// /// The to use. /// The supplied for call chaining. +#if EXPOSE_EXPERIMENTAL_FEATURES + // todo: [Obsolete("Call UseOpenTelemetry instead this method will be removed in a future version.")] + // Note: We hide AddOpenTelemetry from IDEs using EditorBrowsable when UseOpenTelemetry is present to reduce confusion. + [EditorBrowsable(EditorBrowsableState.Never)] +#endif public static ILoggingBuilder AddOpenTelemetry( this ILoggingBuilder builder) => AddOpenTelemetryInternal(builder, configureBuilder: null, configureOptions: null); @@ -59,11 +67,101 @@ public static ILoggingBuilder AddOpenTelemetry( /// The to use. /// Optional configuration action. /// The supplied for call chaining. +#if EXPOSE_EXPERIMENTAL_FEATURES + // todo: [Obsolete("Call UseOpenTelemetry instead this method will be removed in a future version.")] + // Note: We hide AddOpenTelemetry from IDEs using EditorBrowsable when UseOpenTelemetry is present to reduce confusion. + [EditorBrowsable(EditorBrowsableState.Never)] +#endif public static ILoggingBuilder AddOpenTelemetry( this ILoggingBuilder builder, Action? configure) => AddOpenTelemetryInternal(builder, configureBuilder: null, configureOptions: configure); +#if EXPOSE_EXPERIMENTAL_FEATURES + /// + /// Adds an OpenTelemetry logger named 'OpenTelemetry' to the . + /// + /// + /// WARNING: This is an experimental API which might change or be removed in the future. Use at your own risk. + /// Note: This is safe to be called multiple times and by library authors. + /// Only a single will be created + /// for a given . + /// + /// The to use. + /// The supplied for call chaining. + public +#else + /// + /// Adds an OpenTelemetry logger named 'OpenTelemetry' to the . + /// + /// + /// Note: This is safe to be called multiple times and by library authors. + /// Only a single will be created + /// for a given . + /// + /// The to use. + /// The supplied for call chaining. + internal +#endif + static ILoggingBuilder UseOpenTelemetry( + this ILoggingBuilder builder) + => AddOpenTelemetryInternal(builder, configureBuilder: null, configureOptions: null); + +#if EXPOSE_EXPERIMENTAL_FEATURES + /// + /// Adds an OpenTelemetry logger named 'OpenTelemetry' to the . + /// + /// + /// The to use. + /// Optional configuration action. + /// The supplied for call chaining. + public +#else + /// + /// Adds an OpenTelemetry logger named 'OpenTelemetry' to the . + /// + /// + /// The to use. + /// configuration action. + /// The supplied for call chaining. + internal +#endif + static ILoggingBuilder UseOpenTelemetry( + this ILoggingBuilder builder, + Action configure) + { + Guard.ThrowIfNull(configure); + + return AddOpenTelemetryInternal(builder, configureBuilder: configure, configureOptions: null); + } + +#if EXPOSE_EXPERIMENTAL_FEATURES + /// + /// Adds an OpenTelemetry logger named 'OpenTelemetry' to the . + /// + /// + /// The to use. + /// Optional configuration action. + /// Optional configuration action. + /// The supplied for call chaining. + public +#else + /// + /// Adds an OpenTelemetry logger named 'OpenTelemetry' to the . + /// + /// + /// The to use. + /// Optional configuration action. + /// Optional configuration action. + /// The supplied for call chaining. + internal +#endif + static ILoggingBuilder UseOpenTelemetry( + this ILoggingBuilder builder, + Action? configureBuilder, + Action? configureOptions) + => AddOpenTelemetryInternal(builder, configureBuilder, configureOptions); + private static ILoggingBuilder AddOpenTelemetryInternal( ILoggingBuilder builder, Action? configureBuilder, @@ -75,19 +173,19 @@ private static ILoggingBuilder AddOpenTelemetryInternal( var services = builder.Services; - if (configureOptions != null) - { - // TODO: Move this below the RegisterLoggerProviderOptions call so - // that user-supplied delegate fires AFTER the options are bound to - // Logging:OpenTelemetry configuration. - services.Configure(configureOptions); - } - // Note: This will bind logger options element (eg "Logging:OpenTelemetry") to OpenTelemetryLoggerOptions RegisterLoggerProviderOptions(services); services.AddOpenTelemetrySharedProviderBuilderServices(); + if (configureOptions != null) + { + // Note: Order is important here so that user-supplied delegate + // fires AFTER the options are bound to Logging:OpenTelemetry + // configuration. + services.Configure(configureOptions); + } + var loggingBuilder = new LoggerProviderBuilderBase(services).ConfigureBuilder( (sp, logging) => { diff --git a/test/OpenTelemetry.Extensions.Hosting.Tests/OpenTelemetryServicesExtensionsTests.cs b/test/OpenTelemetry.Extensions.Hosting.Tests/OpenTelemetryServicesExtensionsTests.cs index 2b447a816d8..069c6c446bc 100644 --- a/test/OpenTelemetry.Extensions.Hosting.Tests/OpenTelemetryServicesExtensionsTests.cs +++ b/test/OpenTelemetry.Extensions.Hosting.Tests/OpenTelemetryServicesExtensionsTests.cs @@ -381,7 +381,7 @@ public void AddOpenTelemetry_WithLogging_DisposalTest() } [Fact] - public async Task AddOpenTelemetry_WithLogging_HostConfigurationHonoredTest() + public void AddOpenTelemetry_WithLogging_HostConfigurationHonoredTest() { bool configureBuilderCalled = false; @@ -416,14 +416,8 @@ public async Task AddOpenTelemetry_WithLogging_HostConfigurationHonoredTest() var host = builder.Build(); - Assert.False(configureBuilderCalled); - - await host.StartAsync(); - Assert.True(configureBuilderCalled); - await host.StopAsync(); - host.Dispose(); } diff --git a/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs b/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs index 308062be65a..896ceae0776 100644 --- a/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs +++ b/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs @@ -17,6 +17,7 @@ #nullable enable using System.Reflection; +using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Xunit; @@ -25,16 +26,25 @@ namespace OpenTelemetry.Logs.Tests; public sealed class OpenTelemetryLoggingExtensionsTests { - [Fact] - public void ServiceCollectionAddOpenTelemetryNoParametersTest() + [Theory] + [InlineData(false)] + [InlineData(true)] + public void ServiceCollectionAddOpenTelemetryNoParametersTest(bool callUseExtension) { bool optionsCallbackInvoked = false; var serviceCollection = new ServiceCollection(); - serviceCollection.AddLogging(configure => + serviceCollection.AddLogging(logging => { - configure.AddOpenTelemetry(); + if (callUseExtension) + { + logging.UseOpenTelemetry(); + } + else + { + logging.AddOpenTelemetry(); + } }); serviceCollection.Configure(options => @@ -52,10 +62,16 @@ public void ServiceCollectionAddOpenTelemetryNoParametersTest() } [Theory] - [InlineData(1, 0)] - [InlineData(1, 1)] - [InlineData(5, 5)] - public void ServiceCollectionAddOpenTelemetryConfigureActionTests(int numberOfBuilderRegistrations, int numberOfOptionsRegistrations) + [InlineData(false, 1, 0)] + [InlineData(false, 1, 1)] + [InlineData(false, 5, 5)] + [InlineData(true, 1, 0)] + [InlineData(true, 1, 1)] + [InlineData(true, 5, 5)] + public void ServiceCollectionAddOpenTelemetryConfigureActionTests( + bool callUseExtension, + int numberOfBuilderRegistrations, + int numberOfOptionsRegistrations) { int configureCallbackInvocations = 0; int optionsCallbackInvocations = 0; @@ -63,11 +79,18 @@ public void ServiceCollectionAddOpenTelemetryConfigureActionTests(int numberOfBu var serviceCollection = new ServiceCollection(); - serviceCollection.AddLogging(configure => + serviceCollection.AddLogging(logging => { for (int i = 0; i < numberOfBuilderRegistrations; i++) { - configure.AddOpenTelemetry(ConfigureCallback); + if (callUseExtension) + { + logging.UseOpenTelemetry(configureBuilder: null, configureOptions: ConfigureCallback); + } + else + { + logging.AddOpenTelemetry(ConfigureCallback); + } } }); @@ -116,6 +139,92 @@ void OptionsCallback(OpenTelemetryLoggerOptions options) } } + [Fact] + public void UseOpenTelemetryDependencyInjectionTest() + { + var serviceCollection = new ServiceCollection(); + + serviceCollection.AddLogging(logging => + { + logging.UseOpenTelemetry(builder => + { + builder.ConfigureServices(services => + { + services.AddSingleton(); + }); + + builder.ConfigureBuilder((sp, builder) => + { + builder.AddProcessor( + sp.GetRequiredService()); + }); + }); + }); + + using var sp = serviceCollection.BuildServiceProvider(); + + var loggerProvider = sp.GetRequiredService() as LoggerProviderSdk; + + Assert.NotNull(loggerProvider); + + Assert.NotNull(loggerProvider.Processor); + + Assert.True(loggerProvider.Processor is TestLogProcessor); + } + + [Fact] + public void UseOpenTelemetryOptionsOrderingTest() + { + int currentIndex = -1; + int beforeDelegateIndex = -1; + int extensionDelegateIndex = -1; + int afterDelegateIndex = -1; + + var serviceCollection = new ServiceCollection(); + + var config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary { ["Logging:OpenTelemetry:IncludeFormattedMessage"] = "true" }) + .Build(); + + serviceCollection.Configure(o => + { + // Verify this fires BEFORE options are bound + Assert.False(o.IncludeFormattedMessage); + + beforeDelegateIndex = ++currentIndex; + }); + + serviceCollection.AddLogging(logging => + { + // Note: Typically the host binds logging configuration to the + // "Logging" section but since we aren't using a host we do this + // manually. + logging.AddConfiguration(config.GetSection("Logging")); + + logging.UseOpenTelemetry( + configureBuilder: null, + configureOptions: o => + { + // Verify this fires AFTER options are bound + Assert.True(o.IncludeFormattedMessage); + + extensionDelegateIndex = ++currentIndex; + }); + }); + + serviceCollection.Configure(o => afterDelegateIndex = ++currentIndex); + + using var sp = serviceCollection.BuildServiceProvider(); + + var loggerProvider = sp.GetRequiredService() as LoggerProviderSdk; + + Assert.NotNull(loggerProvider); + + Assert.Equal(0, beforeDelegateIndex); + Assert.Equal(1, extensionDelegateIndex); + Assert.Equal(2, afterDelegateIndex); + } + // This test validates that the OpenTelemetryLoggerOptions contains only primitive type properties. // This is necessary to ensure trim correctness since that class is effectively deserialized from // configuration. The top level properties are ensured via annotation on the RegisterProviderOptions API @@ -136,11 +245,11 @@ public void VerifyAddProcessorOverloadWithImplementationFactory() // arrange var services = new ServiceCollection(); - services.AddSingleton(); + services.AddSingleton(); services.AddLogging(logging => logging.AddOpenTelemetry( - o => o.AddProcessor(sp => sp.GetRequiredService()))); + o => o.AddProcessor(sp => sp.GetRequiredService()))); // act using var sp = services.BuildServiceProvider(); @@ -150,7 +259,7 @@ public void VerifyAddProcessorOverloadWithImplementationFactory() // assert Assert.NotNull(loggerProvider); Assert.NotNull(loggerProvider.Processor); - Assert.True(loggerProvider.Processor is MyProcessor); + Assert.True(loggerProvider.Processor is TestLogProcessor); } [Fact] @@ -170,7 +279,7 @@ public void VerifyExceptionIsThrownWhenImplementationFactoryIsNull() Assert.Throws(() => sp.GetRequiredService() as LoggerProviderSdk); } - private class MyProcessor : BaseProcessor + private class TestLogProcessor : BaseProcessor { } }