From 734172a4214b08afc7421d02f42d9bc88fba1536 Mon Sep 17 00:00:00 2001 From: Martijn Laarman Date: Mon, 3 Jun 2024 17:16:30 +0200 Subject: [PATCH] Ensure we also always globally log when using .NET core IServiceCollection --- .../ApplicationBuilderExtensions.cs | 4 +-- ...etCoreLogger.cs => ApmExtensionsLogger.cs} | 6 ++-- .../CompositeLogger.cs | 35 +++++++++++++++++++ .../Config/ApmConfiguration.cs | 2 +- .../HostBuilderExtensions.cs | 9 ++++- .../ServiceCollectionExtensions.cs | 31 +++++++--------- .../Elastic.Apm.StartupHook.Loader/Loader.cs | 1 + .../ApplicationBuilderExtensionLoggingTest.cs | 6 ++-- .../AspNetCoreBasicTests.cs | 1 + .../AspNetCoreLoggerTests.cs | 6 ++-- .../TransactionIgnoreUrlsTest.cs | 1 + 11 files changed, 71 insertions(+), 31 deletions(-) rename src/integrations/Elastic.Apm.Extensions.Hosting/{NetCoreLogger.cs => ApmExtensionsLogger.cs} (84%) create mode 100644 src/integrations/Elastic.Apm.Extensions.Hosting/CompositeLogger.cs diff --git a/src/integrations/Elastic.Apm.AspNetCore/ApplicationBuilderExtensions.cs b/src/integrations/Elastic.Apm.AspNetCore/ApplicationBuilderExtensions.cs index 0a260f0d8..1efbfe841 100644 --- a/src/integrations/Elastic.Apm.AspNetCore/ApplicationBuilderExtensions.cs +++ b/src/integrations/Elastic.Apm.AspNetCore/ApplicationBuilderExtensions.cs @@ -52,7 +52,7 @@ public static IApplicationBuilder UseElasticApm( params IDiagnosticsSubscriber[] subscribers ) { - var logger = NetCoreLogger.GetApmLogger(builder.ApplicationServices); + var logger = ApmExtensionsLogger.GetApmLogger(builder.ApplicationServices); var configReader = configuration == null ? new EnvironmentConfiguration(logger) @@ -70,7 +70,7 @@ params IDiagnosticsSubscriber[] subscribers internal static IApplicationBuilder UseElasticApm( this IApplicationBuilder builder, ApmAgent agent, - IApmLogger logger, + Logging.IApmLogger logger, params IDiagnosticsSubscriber[] subscribers ) { diff --git a/src/integrations/Elastic.Apm.Extensions.Hosting/NetCoreLogger.cs b/src/integrations/Elastic.Apm.Extensions.Hosting/ApmExtensionsLogger.cs similarity index 84% rename from src/integrations/Elastic.Apm.Extensions.Hosting/NetCoreLogger.cs rename to src/integrations/Elastic.Apm.Extensions.Hosting/ApmExtensionsLogger.cs index f806f3499..48d6dd422 100644 --- a/src/integrations/Elastic.Apm.Extensions.Hosting/NetCoreLogger.cs +++ b/src/integrations/Elastic.Apm.Extensions.Hosting/ApmExtensionsLogger.cs @@ -9,11 +9,11 @@ namespace Elastic.Apm.Extensions.Hosting; -internal sealed class NetCoreLogger : IApmLogger +internal sealed class ApmExtensionsLogger : IApmLogger { private readonly ILogger _logger; - public NetCoreLogger(ILoggerFactory loggerFactory) => _logger = loggerFactory?.CreateLogger("Elastic.Apm") ?? throw new ArgumentNullException(nameof(loggerFactory)); + public ApmExtensionsLogger(ILoggerFactory loggerFactory) => _logger = loggerFactory?.CreateLogger("Elastic.Apm") ?? throw new ArgumentNullException(nameof(loggerFactory)); public bool IsEnabled(LogLevel level) => _logger.IsEnabled(Convert(level)); @@ -34,6 +34,6 @@ private static Microsoft.Extensions.Logging.LogLevel Convert(LogLevel logLevel) internal static IApmLogger GetApmLogger(IServiceProvider serviceProvider) => serviceProvider.GetService(typeof(ILoggerFactory)) is ILoggerFactory loggerFactory - ? new NetCoreLogger(loggerFactory) + ? new ApmExtensionsLogger(loggerFactory) : ConsoleLogger.Instance; } diff --git a/src/integrations/Elastic.Apm.Extensions.Hosting/CompositeLogger.cs b/src/integrations/Elastic.Apm.Extensions.Hosting/CompositeLogger.cs new file mode 100644 index 000000000..b9e52c53e --- /dev/null +++ b/src/integrations/Elastic.Apm.Extensions.Hosting/CompositeLogger.cs @@ -0,0 +1,35 @@ +// Licensed to Elasticsearch B.V under one or more agreements. +// Elasticsearch B.V licenses this file to you under the Apache 2.0 License. +// See the LICENSE file in the project root for more information + +using System; +using Elastic.Apm.Logging; +using LogLevel = Elastic.Apm.Logging.LogLevel; + +namespace Elastic.Apm.Extensions.Hosting; + +internal sealed class CompositeLogger(TraceLogger traceLogger, IApmLogger logger) : IDisposable , IApmLogger +{ + public TraceLogger TraceLogger { get; } = traceLogger; + public IApmLogger ApmLogger { get; } = logger; + + private bool _isDisposed; + + public void Dispose() => _isDisposed = true; + + public void Log(LogLevel level, TState state, Exception e, Func formatter) + { + if (_isDisposed) + return; + + if (TraceLogger.IsEnabled(level)) + TraceLogger.Log(level, state, e, formatter); + + if (ApmLogger.IsEnabled(level)) + ApmLogger.Log(level, state, e, formatter); + } + + public bool IsEnabled(LogLevel logLevel) => ApmLogger.IsEnabled(logLevel) || TraceLogger.IsEnabled(logLevel); + + +} diff --git a/src/integrations/Elastic.Apm.Extensions.Hosting/Config/ApmConfiguration.cs b/src/integrations/Elastic.Apm.Extensions.Hosting/Config/ApmConfiguration.cs index b07095571..4105fa587 100644 --- a/src/integrations/Elastic.Apm.Extensions.Hosting/Config/ApmConfiguration.cs +++ b/src/integrations/Elastic.Apm.Extensions.Hosting/Config/ApmConfiguration.cs @@ -33,7 +33,7 @@ internal class ApmConfiguration : FallbackToEnvironmentConfigurationBase { private const string ThisClassName = nameof(ApmConfiguration); - public ApmConfiguration(IConfiguration configuration, IApmLogger logger, string defaultEnvironmentName) + public ApmConfiguration(IConfiguration configuration, Apm.Logging.IApmLogger logger, string defaultEnvironmentName) : base(logger, new ConfigurationDefaults { EnvironmentName = defaultEnvironmentName, DebugName = ThisClassName }, new ConfigurationKeyValueProvider(configuration)) => diff --git a/src/integrations/Elastic.Apm.Extensions.Hosting/HostBuilderExtensions.cs b/src/integrations/Elastic.Apm.Extensions.Hosting/HostBuilderExtensions.cs index d33a62d73..91f97818b 100644 --- a/src/integrations/Elastic.Apm.Extensions.Hosting/HostBuilderExtensions.cs +++ b/src/integrations/Elastic.Apm.Extensions.Hosting/HostBuilderExtensions.cs @@ -14,6 +14,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; +using LogLevel = Elastic.Apm.Logging.LogLevel; namespace Elastic.Apm.Extensions.Hosting { @@ -54,7 +55,13 @@ public static IHostBuilder UseElasticApm(this IHostBuilder builder, params IDiag //If the static agent doesn't exist, we create one here. If there is already 1 agent created, we reuse it. if (!Agent.IsConfigured) { - services.AddSingleton(); + services.AddSingleton(sp => + { + var netCoreLogger = ApmExtensionsLogger.GetApmLogger(sp); + var globalLogger = AgentComponents.GetGlobalLogger(netCoreLogger, LogLevel.Error); + var logger = globalLogger is TraceLogger g ? new CompositeLogger(g, netCoreLogger) : netCoreLogger; + return logger; + }); services.AddSingleton(sp => new ApmConfiguration(ctx.Configuration, sp.GetService(), GetHostingEnvironmentName(ctx, sp.GetService()))); } diff --git a/src/integrations/Elastic.Apm.Extensions.Hosting/ServiceCollectionExtensions.cs b/src/integrations/Elastic.Apm.Extensions.Hosting/ServiceCollectionExtensions.cs index 1923e42e2..2931fe1a3 100644 --- a/src/integrations/Elastic.Apm.Extensions.Hosting/ServiceCollectionExtensions.cs +++ b/src/integrations/Elastic.Apm.Extensions.Hosting/ServiceCollectionExtensions.cs @@ -12,10 +12,8 @@ using Elastic.Apm.Logging; using Elastic.Apm.NetCoreAll; using Elastic.Apm.Report; -using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; -using static Microsoft.Extensions.DependencyInjection.ServiceDescriptor; namespace Microsoft.Extensions.DependencyInjection; @@ -49,28 +47,25 @@ public static IServiceCollection AddElasticApm(this IServiceCollection services, if (agentConfigured) return Agent.Instance; - var logger = NetCoreLogger.GetApmLogger(sp); - var environmentName = GetEnvironmentName(sp); - - if (environmentName is null) - { - logger?.Warning()?.Log("Failed to retrieve hosting environment name"); - environmentName = "Undetermined"; - } - + var netCoreLogger = ApmExtensionsLogger.GetApmLogger(sp); var configuration = sp.GetService(); + var environmentName = GetDefaultEnvironmentName(sp); IConfigurationReader configurationReader = configuration is null - ? new EnvironmentConfiguration(logger) - : new ApmConfiguration(configuration, logger, environmentName); + ? new EnvironmentConfiguration(netCoreLogger) + : new ApmConfiguration(configuration, netCoreLogger, environmentName ?? "Undetermined"); + + var globalLogger = AgentComponents.GetGlobalLogger(netCoreLogger, configurationReader.LogLevel); + + var logger = globalLogger is TraceLogger g ? new CompositeLogger(g, netCoreLogger) : netCoreLogger; + + if (environmentName is null) + logger?.Warning()?.Log("Failed to retrieve default hosting environment name"); // This may be null, which is fine var payloadSender = sp.GetService(); - var components = agentConfigured - ? Agent.Components - : new AgentComponents(logger, configurationReader, payloadSender); - + var components = new AgentComponents(logger, configurationReader, payloadSender); HostBuilderExtensions.UpdateServiceInformation(components.Service); Agent.Setup(components); @@ -110,7 +105,7 @@ public static IServiceCollection AddElasticApm(this IServiceCollection services, return services; } - private static string GetEnvironmentName(IServiceProvider serviceProvider) => + private static string GetDefaultEnvironmentName(IServiceProvider serviceProvider) => #if NET6_0_OR_GREATER (serviceProvider.GetService(typeof(IHostEnvironment)) as IHostEnvironment)?.EnvironmentName; // This is preferred since 3.0 #else diff --git a/src/startuphook/Elastic.Apm.StartupHook.Loader/Loader.cs b/src/startuphook/Elastic.Apm.StartupHook.Loader/Loader.cs index fc87c735f..0e3d5677d 100644 --- a/src/startuphook/Elastic.Apm.StartupHook.Loader/Loader.cs +++ b/src/startuphook/Elastic.Apm.StartupHook.Loader/Loader.cs @@ -14,6 +14,7 @@ using Elastic.Apm.GrpcClient; using Elastic.Apm.Instrumentations.SqlClient; using Elastic.Apm.Logging; +using IApmLogger = Elastic.Apm.Logging.IApmLogger; namespace Elastic.Apm.StartupHook.Loader { diff --git a/test/integrations/Elastic.Apm.AspNetCore.Tests/ApplicationBuilderExtensionLoggingTest.cs b/test/integrations/Elastic.Apm.AspNetCore.Tests/ApplicationBuilderExtensionLoggingTest.cs index 8c0480aea..1a23b5c01 100644 --- a/test/integrations/Elastic.Apm.AspNetCore.Tests/ApplicationBuilderExtensionLoggingTest.cs +++ b/test/integrations/Elastic.Apm.AspNetCore.Tests/ApplicationBuilderExtensionLoggingTest.cs @@ -20,9 +20,9 @@ public void UseElasticApmShouldUseAspNetLoggerWhenLoggingIsConfigured() var services = new ServiceCollection() .AddLogging(); - var logger = NetCoreLogger.GetApmLogger(services.BuildServiceProvider()); + var logger = ApmExtensionsLogger.GetApmLogger(services.BuildServiceProvider()); - Assert.IsType(logger); + Assert.IsType(logger); } [Fact] @@ -30,7 +30,7 @@ public void UseElasticApmShouldUseConsoleLoggerInstanceWhenLoggingIsNotConfigure { var services = new ServiceCollection(); - var logger = NetCoreLogger.GetApmLogger(services.BuildServiceProvider()); + var logger = ApmExtensionsLogger.GetApmLogger(services.BuildServiceProvider()); Assert.IsType(logger); Assert.Same(ConsoleLogger.Instance, logger); diff --git a/test/integrations/Elastic.Apm.AspNetCore.Tests/AspNetCoreBasicTests.cs b/test/integrations/Elastic.Apm.AspNetCore.Tests/AspNetCoreBasicTests.cs index 23edf20e0..10f38113d 100644 --- a/test/integrations/Elastic.Apm.AspNetCore.Tests/AspNetCoreBasicTests.cs +++ b/test/integrations/Elastic.Apm.AspNetCore.Tests/AspNetCoreBasicTests.cs @@ -24,6 +24,7 @@ using SampleAspNetCoreApp; using Xunit; using Xunit.Abstractions; +using IApmLogger = Elastic.Apm.Logging.IApmLogger; namespace Elastic.Apm.AspNetCore.Tests { diff --git a/test/integrations/Elastic.Apm.AspNetCore.Tests/AspNetCoreLoggerTests.cs b/test/integrations/Elastic.Apm.AspNetCore.Tests/AspNetCoreLoggerTests.cs index a6e4476b1..98e1844ba 100644 --- a/test/integrations/Elastic.Apm.AspNetCore.Tests/AspNetCoreLoggerTests.cs +++ b/test/integrations/Elastic.Apm.AspNetCore.Tests/AspNetCoreLoggerTests.cs @@ -11,13 +11,13 @@ namespace Elastic.Apm.AspNetCore.Tests { /// - /// Tests the type. + /// Tests the type. /// public class AspNetCoreLoggerTests { [Fact] public void AspNetCoreLoggerShouldThrowExceptionWhenLoggerFactoryIsNull() - => Assert.Throws(() => new NetCoreLogger(null)); + => Assert.Throws(() => new ApmExtensionsLogger(null)); [Fact] public void AspNetCoreLoggerShouldGetLoggerFromFactoryWithProperCategoryName() @@ -27,7 +27,7 @@ public void AspNetCoreLoggerShouldGetLoggerFromFactoryWithProperCategoryName() .Returns(() => Mock.Of()); // ReSharper disable UnusedVariable - var logger = new NetCoreLogger(loggerFactoryMock.Object); + var logger = new ApmExtensionsLogger(loggerFactoryMock.Object); // ReSharper restore UnusedVariable loggerFactoryMock.Verify(x => x.CreateLogger(It.Is(s => s.Equals("Elastic.Apm"))), Times.Once); diff --git a/test/integrations/Elastic.Apm.AspNetCore.Tests/TransactionIgnoreUrlsTest.cs b/test/integrations/Elastic.Apm.AspNetCore.Tests/TransactionIgnoreUrlsTest.cs index 369380440..7cd8c4aab 100644 --- a/test/integrations/Elastic.Apm.AspNetCore.Tests/TransactionIgnoreUrlsTest.cs +++ b/test/integrations/Elastic.Apm.AspNetCore.Tests/TransactionIgnoreUrlsTest.cs @@ -14,6 +14,7 @@ using Microsoft.AspNetCore.Mvc.Testing; using SampleAspNetCoreApp; using Xunit; +using IApmLogger = Elastic.Apm.Logging.IApmLogger; namespace Elastic.Apm.AspNetCore.Tests {