Refactor remote/dynamic config to lock during remote/manual config updates

Author: andrewlock

tracer/src/Datadog.Trace/ClrProfiler/AutoInstrumentation/ManualInstrumentation/Tracer/ConfigureIntegration.cs

@@ -73,12 +73,7 @@ internal static void ConfigureSettingsWithManualOverrides(Dictionary<string, obj
                 ? new ManualInstrumentationLegacyConfigurationSource(values, isFromDefaults)
                 : new ManualInstrumentationConfigurationSource(values, isFromDefaults);
 
-        // We need to save this immediately, even if there's no manifest changes in the final settings
-        // so that it can be picked up by other configuration updaters, e.g. remote config
-        GlobalConfigurationSource.UpdateManualConfigurationSource(manualConfig);
-        var dynamicConfig = GlobalConfigurationSource.DynamicConfigurationSource;
-
-        var wasUpdated = Datadog.Trace.Tracer.Instance.Settings.Manager.UpdateSettings(dynamicConfig, manualConfig, TelemetryFactory.Config);
+        var wasUpdated = Datadog.Trace.Tracer.Instance.Settings.Manager.UpdateManualConfigurationSettings(manualConfig, TelemetryFactory.Config);
         if (wasUpdated)
         {
             Log.Information("Setting updates made via configuration in code were applied");

tracer/src/Datadog.Trace/Configuration/ConfigurationSources/GlobalConfigurationSource.cs

@@ -22,20 +22,13 @@ namespace Datadog.Trace.Configuration;
 /// </summary>
 internal class GlobalConfigurationSource
 {
-    private static IConfigurationSource _dynamicConfigConfigurationSource = NullConfigurationSource.Instance;
-    private static ManualInstrumentationConfigurationSourceBase _manualConfigurationSource = new ManualInstrumentationConfigurationSource(new Dictionary<string, object?>(), useDefaultSources: true);
-
     /// <summary>
     /// Gets the configuration source instance.
     /// </summary>
     internal static IConfigurationSource Instance => CreationResult.ConfigurationSource;
 
     internal static GlobalConfigurationSourceResult CreationResult { get; private set; } = CreateDefaultConfigurationSource();
 
-    internal static IConfigurationSource DynamicConfigurationSource => _dynamicConfigConfigurationSource;
-
-    internal static ManualInstrumentationConfigurationSourceBase ManualConfigurationSource => _manualConfigurationSource;
-
     /// <summary>
     /// Creates a <see cref="IConfigurationSource"/> by combining environment variables,
     /// Precedence is as follows:
@@ -142,14 +135,4 @@ private static string GetCurrentDirectory()
     {
         return AppDomain.CurrentDomain.BaseDirectory ?? Directory.GetCurrentDirectory();
     }
-
-    public static void UpdateDynamicConfigConfigurationSource(IConfigurationSource dynamic)
-    {
-        Interlocked.Exchange(ref _dynamicConfigConfigurationSource, dynamic);
-    }
-
-    public static void UpdateManualConfigurationSource(ManualInstrumentationConfigurationSourceBase manual)
-    {
-        Interlocked.Exchange(ref _manualConfigurationSource, manual);
-    }
 }

tracer/src/Datadog.Trace/Configuration/DynamicConfigurationManager.cs

@@ -73,13 +73,7 @@ internal static void OnlyForTests_ApplyConfiguration(IConfigurationSource dynami
 
         private static void OnConfigurationChanged(IConfigurationSource dynamicConfig, TracerSettings tracerSettings)
         {
-            var manualConfig = GlobalConfigurationSource.ManualConfigurationSource;
-
-            // We save this immediately, even if there's no manifest changes in the final settings
-            // so that it can be picked up by other configuration updaters, e.g. config in code
-            GlobalConfigurationSource.UpdateDynamicConfigConfigurationSource(dynamicConfig);
-
-            var wasUpdated = tracerSettings.Manager.UpdateSettings(dynamicConfig, manualConfig, TelemetryFactory.Config);
+            var wasUpdated = tracerSettings.Manager.UpdateDynamicConfigurationSettings(dynamicConfig, TelemetryFactory.Config);
             if (wasUpdated)
             {
                 Log.Information("Setting updates made via dynamic configuration were applied");

tracer/src/Datadog.Trace/Configuration/SettingsManager.cs

@@ -24,6 +24,11 @@ internal class SettingsManager(
     {
         private readonly TracerSettings _tracerSettings = tracerSettings;
         private readonly List<SettingChangeSubscription> _subscribers = [];
+
+        private IConfigurationSource _dynamicConfigurationSource = NullConfigurationSource.Instance;
+        private ManualInstrumentationConfigurationSourceBase _manualConfigurationSource =
+            new ManualInstrumentationConfigurationSource(new Dictionary<string, object?>(), useDefaultSources: true);
+
         private SettingChanges? _latest;
 
         /// <summary>
@@ -71,15 +76,44 @@ public IDisposable SubscribeToChanges(Action<SettingChanges> callback)
             return subscription;
         }
 
+        /// <summary>
+        /// Regenerate the application's new <see cref="MutableSettings"/> and <see cref="ExporterSettings"/>
+        /// based on runtime configuration sources.
+        /// </summary>
+        /// <param name="manualSource">An <see cref="IConfigurationSource"/> containing the new settings created by manual configuration (in code)</param>
+        /// <param name="centralTelemetry">The central <see cref="IConfigurationTelemetry"/> to report config telemetry updates to</param>
+        /// <returns>True if changes were detected and consumers were updated, false otherwise</returns>
+        public bool UpdateManualConfigurationSettings(
+            ManualInstrumentationConfigurationSourceBase manualSource,
+            IConfigurationTelemetry centralTelemetry)
+        {
+            // we lock this whole method so that we can't conflict with UpdateDynamicConfigurationSettings calls too
+            lock (_subscribers)
+            {
+                _manualConfigurationSource = manualSource;
+                return UpdateSettings(_dynamicConfigurationSource, manualSource, centralTelemetry);
+            }
+        }
+
         /// <summary>
         /// Regenerate the application's new <see cref="MutableSettings"/> and <see cref="ExporterSettings"/>
         /// based on runtime configuration sources.
         /// </summary>
         /// <param name="dynamicConfigSource">An <see cref="IConfigurationSource"/> for dynamic config via remote config</param>
-        /// <param name="manualSource">An <see cref="IConfigurationSource"/> for manual configuration (in code)</param>
         /// <param name="centralTelemetry">The central <see cref="IConfigurationTelemetry"/> to report config telemetry updates to</param>
         /// <returns>True if changes were detected and consumers were updated, false otherwise</returns>
-        public bool UpdateSettings(
+        public bool UpdateDynamicConfigurationSettings(
+            IConfigurationSource dynamicConfigSource,
+            IConfigurationTelemetry centralTelemetry)
+        {
+            lock (_subscribers)
+            {
+                _dynamicConfigurationSource = dynamicConfigSource;
+                return UpdateSettings(dynamicConfigSource, _manualConfigurationSource, centralTelemetry);
+            }
+        }
+
+        private bool UpdateSettings(
             IConfigurationSource dynamicConfigSource,
             ManualInstrumentationConfigurationSourceBase manualSource,
             IConfigurationTelemetry centralTelemetry)
@@ -153,25 +187,17 @@ public bool UpdateSettings(
 
         private void NotifySubscribers(SettingChanges settings)
         {
-            // Strictly, for safety, we only need to lock in the subscribers list access. However,
-            // there's nothing to prevent NotifySubscribers being called concurrently,
-            // which could result in weird out-of-order notifications for customers. So for simplicity
-            // we just lock the whole method to ensure serialized updates.
+            _latest = settings;
 
-            lock (_subscribers)
+            foreach (var subscriber in _subscribers)
             {
-                Volatile.Write(ref _latest, settings);
-
-                foreach (var subscriber in _subscribers)
+                try
                 {
-                    try
-                    {
-                        subscriber.Notify(settings);
-                    }
-                    catch (Exception ex)
-                    {
-                        Log.Error(ex, "Error notifying subscriber of MutableSettings change");
-                    }
+                    subscriber.Notify(settings);
+                }
+                catch (Exception ex)
+                {
+                    Log.Error(ex, "Error notifying subscriber of MutableSettings change");
                 }
             }
         }

tracer/test/Datadog.Trace.Tests/Configuration/TracerSettingsSettingManagerTests.cs

@@ -3,9 +3,14 @@
 // This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
 // </copyright>
 
+using System.Collections.Generic;
+using System.Linq;
 using Datadog.Trace.Configuration;
+using Datadog.Trace.Configuration.ConfigurationSources;
+using Datadog.Trace.Configuration.Telemetry;
 using FluentAssertions;
 using Xunit;
+using SettingChanges = Datadog.Trace.Configuration.TracerSettings.SettingsManager.SettingChanges;
 
 namespace Datadog.Trace.Tests.Configuration;