Fix DatadogLoggingScope sometimes injecting incorrect values for ILogger logs injection

tracer/missing-nullability-files.csv

@@ -611,7 +611,6 @@ src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Logging/ILogger/DirectSubmissi
 src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Logging/ILogger/DirectSubmission/ILoggerFactory.cs
 src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Logging/ILogger/LogsInjection/LoggerExternalScopeProviderForEachScopeIntegration.cs
 src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Logging/ILogger/LogsInjection/LoggerFactoryScopeProviderForEachScopeIntegration.cs
-src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Logging/ILogger/LogsInjection/LoggerIntegrationCommon.cs
 src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Logging/Log4Net/DirectSubmission/ILoggingEventDuck.cs
 src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Logging/Log4Net/DirectSubmission/ILoggingEventDuckBase.cs
 src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Logging/Log4Net/DirectSubmission/ILoggingEventLegacyDuck.cs

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Logging/ILogger/LogsInjection/DatadogLoggingScope.cs

@@ -9,6 +9,7 @@
 using System.Collections;
 using System.Collections.Generic;
 using System.Globalization;
+using Datadog.Trace.Configuration;
 
 namespace Datadog.Trace.ClrProfiler.AutoInstrumentation.Logging.ILogger
 {
@@ -21,19 +22,13 @@ internal class DatadogLoggingScope : IReadOnlyList<KeyValuePair<string, object>>
         private readonly bool _use128Bits;
         private readonly string _cachedFormat;
 
-        public DatadogLoggingScope()
-            : this(Tracer.Instance)
-        {
-        }
-
-        internal DatadogLoggingScope(Tracer tracer)
+        public DatadogLoggingScope(Tracer tracer, MutableSettings settings)
         {
+            Settings = settings;
             _tracer = tracer;
-            // TODO: Subscribe to changes in settings
-            var mutableSettings = tracer.CurrentTraceSettings.Settings;
-            _service = mutableSettings.DefaultServiceName;
-            _env = mutableSettings.Environment ?? string.Empty;
-            _version = mutableSettings.ServiceVersion ?? string.Empty;
+            _service = settings.DefaultServiceName;
+            _env = settings.Environment ?? string.Empty;
+            _version = settings.ServiceVersion ?? string.Empty;
             _use128Bits = _tracer.Settings.TraceId128BitLoggingEnabled;
 
             _cachedFormat = string.Format(
@@ -44,6 +39,8 @@ internal DatadogLoggingScope(Tracer tracer)
                 _version);
         }
 
+        public MutableSettings Settings { get; }
+
         public int Count => 5;
 
         public KeyValuePair<string, object> this[int index]

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/Logging/ILogger/LogsInjection/LoggerIntegrationCommon.cs

@@ -3,7 +3,10 @@
 // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
 // </copyright>
 
+#nullable enable
+
 using System;
+using System.Threading;
 using Datadog.Trace.Configuration;
 
 namespace Datadog.Trace.ClrProfiler.AutoInstrumentation.Logging.ILogger
@@ -13,17 +16,26 @@ internal static class LoggerIntegrationCommon
         public const string IntegrationName = nameof(Configuration.IntegrationId.ILogger);
         private const IntegrationId IntegrationId = Configuration.IntegrationId.ILogger;
 
-        // TODO: Subscribe to changes in settings
-        private static readonly DatadogLoggingScope DatadogScope = new();
+        private static DatadogLoggingScope? _datadogScope;
 
-        public static void AddScope<TAction, TState>(Tracer tracer, TAction callback, TState state)
+        public static void AddScope<TAction, TState>(Tracer tracer, TAction callback, TState? state)
         {
             var settings = tracer.CurrentTraceSettings.Settings;
             if (settings.LogsInjectionEnabled
              && settings.IsIntegrationEnabled(IntegrationId)
-             && callback is Action<object, TState> foreachCallback)
+             && callback is Action<object, TState?> foreachCallback)
             {
-                foreachCallback.Invoke(DatadogScope, state);
+                var scope = Volatile.Read(ref _datadogScope);
+                if (!ReferenceEquals(scope?.Settings, settings))
+                {
+                    // mutable settings have changed, create a new scope and update the cached value
+                    // This is just best-effort for updating the scope. There's a small risk of
+                    // ping-pong if there's a long-lived trace for example, but it's a slim chance
+                    scope = new DatadogLoggingScope(tracer, settings);
+                    Volatile.Write(ref _datadogScope, scope);
+                }
+
+                foreachCallback.Invoke(scope, state);
             }
         }
     }

tracer/test/Datadog.Trace.ClrProfiler.Managed.Tests/AutoInstrumentation/Logging/ILogger/DatadogLoggingScopeTests.cs

@@ -29,7 +29,7 @@ public async Task OutputsJsonFormattedStringWhenNoActiveTrace()
 
             await using var tracer = TracerHelper.Create(settings,  new Mock<IAgentWriter>().Object);
 
-            var scope = new DatadogLoggingScope(tracer);
+            var scope = new DatadogLoggingScope(tracer, tracer.CurrentTraceSettings.Settings);
 
             var actual = scope.ToString();
 
@@ -48,7 +48,7 @@ public async Task OutputsJsonFormattedStringWhenActiveTrace()
 
             await using var tracer = TracerHelper.Create(settings,  new Mock<IAgentWriter>().Object);
             using var spanScope = tracer.StartActive("test");
-            var scope = new DatadogLoggingScope(tracer);
+            var scope = new DatadogLoggingScope(tracer, tracer.CurrentTraceSettings.Settings);
 
             var actual = scope.ToString();
 

tracer/test/Datadog.Trace.ClrProfiler.Managed.Tests/AutoInstrumentation/Logging/ILogger/LoggerIntegrationCommonTests.cs

@@ -0,0 +1,134 @@
+// <copyright file="LoggerIntegrationCommonTests.cs" company="Datadog">
+// Unless explicitly stated otherwise all files in this repository are licensed under the Apache 2 License.
+// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
+// </copyright>
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Threading;
+using System.Threading.Tasks;
+using Datadog.Trace.ClrProfiler.AutoInstrumentation.Logging.ILogger;
+using Datadog.Trace.ClrProfiler.AutoInstrumentation.ManualInstrumentation;
+using Datadog.Trace.Configuration;
+using Datadog.Trace.Configuration.ConfigurationSources;
+using Datadog.Trace.Configuration.Telemetry;
+using Datadog.Trace.TestHelpers.TestTracer;
+using FluentAssertions;
+using Xunit;
+
+namespace Datadog.Trace.Tests.ClrProfiler.AutoInstrumentation.Logging.ILogger;
+
+public class LoggerIntegrationCommonTests
+{
+    [Fact]
+    public async Task AddScope_LogInjectionDisabled()
+    {
+        var settings = TracerSettings.Create(
+            new()
+            {
+                { ConfigurationKeys.LogsInjectionEnabled, false },
+                { ConfigurationKeys.Environment, "env" },
+                { ConfigurationKeys.ServiceName, "serviceName" },
+            });
+
+        await using var tracer = TracerHelper.Create(settings);
+
+        int callCount = 0;
+        Action<object, State> callback = (_, _) =>
+        {
+            Interlocked.Increment(ref callCount);
+        };
+        LoggerIntegrationCommon.AddScope(tracer, callback, new State());
+
+        callCount.Should().Be(0);
+    }
+
+    [Fact]
+    public async Task AddScope_LogInjectionEnabled()
+    {
+        var settings = TracerSettings.Create(
+            new()
+            {
+                { ConfigurationKeys.LogsInjectionEnabled, true },
+                { ConfigurationKeys.Environment, "env" },
+                { ConfigurationKeys.ServiceName, "serviceName" },
+            });
+
+        await using var tracer = TracerHelper.Create(settings);
+
+        Dictionary<string, object> values = null;
+        Action<object, State> callback = (target, _) =>
+        {
+            var scope = target.Should().BeOfType<DatadogLoggingScope>().Subject;
+            values = scope.ToDictionary(x => x.Key, x => x.Value);
+        };
+
+        LoggerIntegrationCommon.AddScope(tracer, callback, new State());
+
+        values.Should()
+              .NotBeNull()
+              .And.ContainKey("dd_service")
+              .And.ContainKey("dd_env");
+
+        values["dd_service"].Should().Be("serviceName");
+        values["dd_env"].Should().Be("env");
+    }
+
+    [Fact]
+    public async Task AddScope_UpdatedSettings_LogInjectionEnabled()
+    {
+        var settings = TracerSettings.Create(
+            new()
+            {
+                { ConfigurationKeys.LogsInjectionEnabled, true },
+                { ConfigurationKeys.Environment, "original_env" },
+                { ConfigurationKeys.ServiceName, "original_serviceName" },
+            });
+
+        await using var tracer = TracerHelper.Create(settings);
+
+        Dictionary<string, object> values = null;
+        Action<object, State> callback = (target, _) =>
+        {
+            var scope = target.Should().BeOfType<DatadogLoggingScope>().Subject;
+            values = scope.ToDictionary(x => x.Key, x => x.Value);
+        };
+
+        LoggerIntegrationCommon.AddScope(tracer, callback, new State());
+
+        values.Should()
+              .NotBeNull()
+              .And.ContainKey("dd_service")
+              .And.ContainKey("dd_env");
+
+        values["dd_service"].Should().Be("original_serviceName");
+        values["dd_env"].Should().Be("original_env");
+
+        // update the settings
+        tracer.Settings.Manager.UpdateManualConfigurationSettings(
+            new ManualInstrumentationConfigurationSource(
+                new Dictionary<string, object>
+                {
+                    { TracerSettingKeyConstants.ServiceNameKey, "updated_service" },
+                    { TracerSettingKeyConstants.EnvironmentKey, "updated_env" },
+                },
+                useDefaultSources: true),
+            NullConfigurationTelemetry.Instance);
+
+        // Should have new values
+        LoggerIntegrationCommon.AddScope(tracer, callback, new State());
+
+        values.Should()
+              .NotBeNull()
+              .And.ContainKey("dd_service")
+              .And.ContainKey("dd_env");
+
+        values["dd_service"].Should().Be("updated_service");
+        values["dd_env"].Should().Be("updated_env");
+    }
+
+    private class State
+    {
+    }
+}