Code review.

open-telemetry · Dec 19, 2023 · e208fbc · e208fbc
1 parent c19868d
commit e208fbc
Show file tree

Hide file tree

Showing 3 changed files with 52 additions and 28 deletions.
diff --git a/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs b/src/OpenTelemetry/Logs/ILogger/OpenTelemetryLoggingExtensions.cs
@@ -174,11 +174,11 @@ private static ILoggingBuilder AddOpenTelemetryInternal(
         RegisterLoggerProviderOptions(services);
 
         /* Note: This ensures IConfiguration is available when using
-       * IServiceCollections NOT attached to a host. For example when
-       * performing:
-       *
-       * new ServiceCollection().AddLogging(b => b.AddOpenTelemetry())
-       */
+         * IServiceCollections NOT attached to a host. For example when
+         * performing:
+         *
+         * new ServiceCollection().AddLogging(b => b.AddOpenTelemetry())
+         */
         services.AddOpenTelemetrySharedProviderBuilderServices();
 
         if (configureOptions != null)

diff --git a/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs b/test/OpenTelemetry.Exporter.OpenTelemetryProtocol.Tests/OtlpLogExporterTests.cs
@@ -27,29 +27,6 @@ public class OtlpLogExporterTests : Http2UnencryptedSupportTests
 {
     private static readonly SdkLimitOptions DefaultSdkLimitOptions = new();
 
-    [Theory]
-    [InlineData(true)]
-    [InlineData(false)]
-    public void ResolutionOrderTest(bool requestLoggerProviderDirectly)
-    {
-        IServiceCollection services = new ServiceCollection();
-
-        services.AddLogging(builder => builder.AddOpenTelemetry());
-
-        using var serviceProvider = services.BuildServiceProvider();
-
-        if (requestLoggerProviderDirectly)
-        {
-            var provider = serviceProvider.GetRequiredService<LoggerProvider>();
-            Assert.NotNull(provider);
-        }
-        else
-        {
-            var factory = serviceProvider.GetRequiredService<ILoggerFactory>();
-            Assert.NotNull(factory);
-        }
-    }
-
     [Fact]
     public void AddOtlpExporterWithNamedOptions()
     {

diff --git a/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs b/test/OpenTelemetry.Tests/Logs/OpenTelemetryLoggingExtensionsTests.cs
@@ -266,7 +266,54 @@ public void VerifyExceptionIsThrownWhenImplementationFactoryIsNull()
         Assert.Throws<ArgumentNullException>(() => sp.GetRequiredService<LoggerProvider>() as LoggerProviderSdk);
     }
 
+    [Theory]
+    [InlineData(true)]
+    [InlineData(false)]
+    public void CircularReferenceTest(bool requestLoggerProviderDirectly)
+    {
+        var services = new ServiceCollection();
+
+        services.AddLogging(logging => logging.AddOpenTelemetry());
+
+        services.ConfigureOpenTelemetryLoggerProvider(builder => builder.AddProcessor<TestLogProcessorWithILoggerFactoryDependency>());
+
+        using var sp = services.BuildServiceProvider();
+
+        if (requestLoggerProviderDirectly)
+        {
+            var provider = sp.GetRequiredService<LoggerProvider>();
+            Assert.NotNull(provider);
+        }
+        else
+        {
+            var factory = sp.GetRequiredService<ILoggerFactory>();
+            Assert.NotNull(factory);
+        }
+    }
+
     private class TestLogProcessor : BaseProcessor<LogRecord>
     {
     }
+
+    private class TestLogProcessorWithILoggerFactoryDependency : BaseProcessor<LogRecord>
+    {
+        private readonly ILogger logger;
+
+        public TestLogProcessorWithILoggerFactoryDependency(ILoggerFactory loggerFactory)
+        {
+            // Note: It is NOT recommended to log from inside a processor. This
+            // test is meant to mirror someone injecting IHttpClientFactory
+            // (which itself uses ILoggerFactory) as part of an exporter. That
+            // is a more realistic scenario but needs a dependency to do that so
+            // here we approximate the graph.
+            this.logger = loggerFactory.CreateLogger("MyLogger");
+        }
+
+        protected override void Dispose(bool disposing)
+        {
+            this.logger.LogInformation("Dispose called");
+
+            base.Dispose(disposing);
+        }
+    }
 }