Skip to content

Commit 9aa97ec

Browse files
andrewlockandrewlock
committed
Refactor remote/dynamic config to lock during remote/manual config updates
1 parent 30f0481 commit 9aa97ec

File tree

5 files changed

+51
-44
lines changed

5 files changed

+51
-44
lines changed

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

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -73,12 +73,7 @@ internal static void ConfigureSettingsWithManualOverrides(Dictionary<string, obj
7373
? new ManualInstrumentationLegacyConfigurationSource(values, isFromDefaults)
7474
: new ManualInstrumentationConfigurationSource(values, isFromDefaults);
7575

76-
// We need to save this immediately, even if there's no manifest changes in the final settings
77-
// so that it can be picked up by other configuration updaters, e.g. remote config
78-
GlobalConfigurationSource.UpdateManualConfigurationSource(manualConfig);
79-
var dynamicConfig = GlobalConfigurationSource.DynamicConfigurationSource;
80-
81-
var wasUpdated = Datadog.Trace.Tracer.Instance.Settings.Manager.UpdateSettings(dynamicConfig, manualConfig, TelemetryFactory.Config);
76+
var wasUpdated = Datadog.Trace.Tracer.Instance.Settings.Manager.UpdateManualConfigurationSettings(manualConfig, TelemetryFactory.Config);
8277
if (wasUpdated)
8378
{
8479
Log.Information("Setting updates made via configuration in code were applied");

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

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -22,20 +22,13 @@ namespace Datadog.Trace.Configuration;
2222
/// </summary>
2323
internal class GlobalConfigurationSource
2424
{
25-
private static IConfigurationSource _dynamicConfigConfigurationSource = NullConfigurationSource.Instance;
26-
private static ManualInstrumentationConfigurationSourceBase _manualConfigurationSource = new ManualInstrumentationConfigurationSource(new Dictionary<string, object?>(), useDefaultSources: true);
27-
2825
/// <summary>
2926
/// Gets the configuration source instance.
3027
/// </summary>
3128
internal static IConfigurationSource Instance => CreationResult.ConfigurationSource;
3229

3330
internal static GlobalConfigurationSourceResult CreationResult { get; private set; } = CreateDefaultConfigurationSource();
3431

35-
internal static IConfigurationSource DynamicConfigurationSource => _dynamicConfigConfigurationSource;
36-
37-
internal static ManualInstrumentationConfigurationSourceBase ManualConfigurationSource => _manualConfigurationSource;
38-
3932
/// <summary>
4033
/// Creates a <see cref="IConfigurationSource"/> by combining environment variables,
4134
/// Precedence is as follows:
@@ -142,14 +135,4 @@ private static string GetCurrentDirectory()
142135
{
143136
return AppDomain.CurrentDomain.BaseDirectory ?? Directory.GetCurrentDirectory();
144137
}
145-
146-
public static void UpdateDynamicConfigConfigurationSource(IConfigurationSource dynamic)
147-
{
148-
Interlocked.Exchange(ref _dynamicConfigConfigurationSource, dynamic);
149-
}
150-
151-
public static void UpdateManualConfigurationSource(ManualInstrumentationConfigurationSourceBase manual)
152-
{
153-
Interlocked.Exchange(ref _manualConfigurationSource, manual);
154-
}
155138
}

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

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -73,13 +73,7 @@ internal static void OnlyForTests_ApplyConfiguration(IConfigurationSource dynami
7373

7474
private static void OnConfigurationChanged(IConfigurationSource dynamicConfig, TracerSettings tracerSettings)
7575
{
76-
var manualConfig = GlobalConfigurationSource.ManualConfigurationSource;
77-
78-
// We save this immediately, even if there's no manifest changes in the final settings
79-
// so that it can be picked up by other configuration updaters, e.g. config in code
80-
GlobalConfigurationSource.UpdateDynamicConfigConfigurationSource(dynamicConfig);
81-
82-
var wasUpdated = tracerSettings.Manager.UpdateSettings(dynamicConfig, manualConfig, TelemetryFactory.Config);
76+
var wasUpdated = tracerSettings.Manager.UpdateDynamicConfigurationSettings(dynamicConfig, TelemetryFactory.Config);
8377
if (wasUpdated)
8478
{
8579
Log.Information("Setting updates made via dynamic configuration were applied");

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

Lines changed: 44 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,11 @@ internal class SettingsManager(
2424
{
2525
private readonly TracerSettings _tracerSettings = tracerSettings;
2626
private readonly List<SettingChangeSubscription> _subscribers = [];
27+
28+
private IConfigurationSource _dynamicConfigurationSource = NullConfigurationSource.Instance;
29+
private ManualInstrumentationConfigurationSourceBase _manualConfigurationSource =
30+
new ManualInstrumentationConfigurationSource(new Dictionary<string, object?>(), useDefaultSources: true);
31+
2732
private SettingChanges? _latest;
2833

2934
/// <summary>
@@ -71,15 +76,44 @@ public IDisposable SubscribeToChanges(Action<SettingChanges> callback)
7176
return subscription;
7277
}
7378

79+
/// <summary>
80+
/// Regenerate the application's new <see cref="MutableSettings"/> and <see cref="ExporterSettings"/>
81+
/// based on runtime configuration sources.
82+
/// </summary>
83+
/// <param name="manualSource">An <see cref="IConfigurationSource"/> containing the new settings created by manual configuration (in code)</param>
84+
/// <param name="centralTelemetry">The central <see cref="IConfigurationTelemetry"/> to report config telemetry updates to</param>
85+
/// <returns>True if changes were detected and consumers were updated, false otherwise</returns>
86+
public bool UpdateManualConfigurationSettings(
87+
ManualInstrumentationConfigurationSourceBase manualSource,
88+
IConfigurationTelemetry centralTelemetry)
89+
{
90+
// we lock this whole method so that we can't conflict with UpdateDynamicConfigurationSettings calls too
91+
lock (_subscribers)
92+
{
93+
_manualConfigurationSource = manualSource;
94+
return UpdateSettings(_dynamicConfigurationSource, manualSource, centralTelemetry);
95+
}
96+
}
97+
7498
/// <summary>
7599
/// Regenerate the application's new <see cref="MutableSettings"/> and <see cref="ExporterSettings"/>
76100
/// based on runtime configuration sources.
77101
/// </summary>
78102
/// <param name="dynamicConfigSource">An <see cref="IConfigurationSource"/> for dynamic config via remote config</param>
79-
/// <param name="manualSource">An <see cref="IConfigurationSource"/> for manual configuration (in code)</param>
80103
/// <param name="centralTelemetry">The central <see cref="IConfigurationTelemetry"/> to report config telemetry updates to</param>
81104
/// <returns>True if changes were detected and consumers were updated, false otherwise</returns>
82-
public bool UpdateSettings(
105+
public bool UpdateDynamicConfigurationSettings(
106+
IConfigurationSource dynamicConfigSource,
107+
IConfigurationTelemetry centralTelemetry)
108+
{
109+
lock (_subscribers)
110+
{
111+
_dynamicConfigurationSource = dynamicConfigSource;
112+
return UpdateSettings(dynamicConfigSource, _manualConfigurationSource, centralTelemetry);
113+
}
114+
}
115+
116+
private bool UpdateSettings(
83117
IConfigurationSource dynamicConfigSource,
84118
ManualInstrumentationConfigurationSourceBase manualSource,
85119
IConfigurationTelemetry centralTelemetry)
@@ -157,21 +191,17 @@ private void NotifySubscribers(SettingChanges settings)
157191
// there's nothing to prevent NotifySubscribers being called concurrently,
158192
// which could result in weird out-of-order notifications for customers. So for simplicity
159193
// we just lock the whole method to ensure serialized updates.
194+
Volatile.Write(ref _latest, settings);
160195

161-
lock (_subscribers)
196+
foreach (var subscriber in _subscribers)
162197
{
163-
Volatile.Write(ref _latest, settings);
164-
165-
foreach (var subscriber in _subscribers)
198+
try
166199
{
167-
try
168-
{
169-
subscriber.Notify(settings);
170-
}
171-
catch (Exception ex)
172-
{
173-
Log.Error(ex, "Error notifying subscriber of MutableSettings change");
174-
}
200+
subscriber.Notify(settings);
201+
}
202+
catch (Exception ex)
203+
{
204+
Log.Error(ex, "Error notifying subscriber of MutableSettings change");
175205
}
176206
}
177207
}

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,14 @@
33
// This product includes software developed at Datadog (https://www.datadoghq.com/). Copyright 2017 Datadog, Inc.
44
// </copyright>
55

6+
using System.Collections.Generic;
7+
using System.Linq;
68
using Datadog.Trace.Configuration;
9+
using Datadog.Trace.Configuration.ConfigurationSources;
10+
using Datadog.Trace.Configuration.Telemetry;
711
using FluentAssertions;
812
using Xunit;
13+
using SettingChanges = Datadog.Trace.Configuration.TracerSettings.SettingsManager.SettingChanges;
914

1015
namespace Datadog.Trace.Tests.Configuration;
1116

0 commit comments

Comments
 (0)