Skip to content

Minor refactor and added tests for DynamicConfigurationManager #7652

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
andrewlock merged 5 commits into from
Oct 15, 2025
Merged

Minor refactor and added tests for DynamicConfigurationManager #7652

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c5f0695
Refactor DynamicConfigurationManager to make it more testable
andrewlock Oct 14, 2025
cf0902f
Add unit tests for CombineApmTracingConfiguration to document expecta…
andrewlock Oct 14, 2025
d4d72c3
Reduce duration of lock
andrewlock Oct 14, 2025
a73cff2
Minor refactor to remove allocation
andrewlock Oct 14, 2025
ebdd094
nit: remove lock as we're not really threadsafe anyway and it ads a v…
andrewlock Oct 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 49 additions & 39 deletions tracer/src/Datadog.Trace/Configuration/DynamicConfigurationManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,56 +158,66 @@ private static void OnConfigurationChanged(ConfigurationBuilder settings)
.ContinueWith(t => Log.Error(t?.Exception, "Error updating dynamic configuration for debugger"), TaskContinuationOptions.OnlyOnFaulted);
}

// Internal for testing
internal static List<RemoteConfiguration> CombineApmTracingConfiguration(
Dictionary<string, RemoteConfiguration> activeConfigurations,
Dictionary<string, List<RemoteConfiguration>> configByProduct,
Dictionary<string, List<RemoteConfigurationPath>>? removedConfigByProduct,
List<ApplyDetails> applyDetailsResult)
{
// Phase 1: Handle explicit removals from removedConfigByProduct
if (removedConfigByProduct?.TryGetValue(ProductName, out var removedConfigs) == true)
{
foreach (var removedConfig in removedConfigs)
{
if (activeConfigurations.Remove(removedConfig.Id))
{
Log.Debug("Explicitly removed APM_TRACING configuration {ConfigId}", removedConfig.Id);
applyDetailsResult.Add(ApplyDetails.FromOk(removedConfig.Path));
}
}
}

// Phase 2: Handle new/updated configurations and implicit removals
if (configByProduct.TryGetValue(ProductName, out var apmLibrary))
{
// if we have some config, then we will "overwrite" everything that's currently active
if (Log.IsEnabled(LogEventLevel.Debug) && activeConfigurations.Count > 0)
{
Log.Debug<int, int>("Implicitly removing {RemovedCount} APM_TRACING configurations and replacing with {AddedCount}", activeConfigurations.Count, apmLibrary.Count);
}

activeConfigurations.Clear();

// Add/update configurations
foreach (var config in apmLibrary)
{
activeConfigurations[config.Path.Id] = config;
applyDetailsResult.Add(ApplyDetails.FromOk(config.Path.Path));
}
}

return [..activeConfigurations.Values];
}

private ApplyDetails[] ConfigurationUpdated(
Dictionary<string, List<RemoteConfiguration>> configByProduct,
Dictionary<string, List<RemoteConfigurationPath>>? removedConfigByProduct)
{
lock (_configLock)
{
var applyDetailsResult = new List<ApplyDetails>();

try
{
// Phase 1: Handle explicit removals from removedConfigByProduct
if (removedConfigByProduct?.TryGetValue(ProductName, out var removedConfigs) == true)
{
foreach (var removedConfig in removedConfigs)
{
if (_activeConfigurations.Remove(removedConfig.Id))
{
Log.Debug("Explicitly removed APM_TRACING configuration {ConfigId}", removedConfig.Id);
applyDetailsResult.Add(ApplyDetails.FromOk(removedConfig.Path));
}
}
}

// Phase 2: Handle new/updated configurations and implicit removals
if (configByProduct.TryGetValue(ProductName, out var apmLibrary))
// Technically we're single threaded here so locking should not be necessary, but playing it safe
List<RemoteConfiguration> valuesToApply;
lock (_configLock)
{
var receivedConfigIds = new HashSet<string>();

// Add/update configurations
foreach (var config in apmLibrary)
{
receivedConfigIds.Add(config.Path.Id);
_activeConfigurations[config.Path.Id] = config;
applyDetailsResult.Add(ApplyDetails.FromOk(config.Path.Path));
}

// Remove configurations not in this update
var configsToRemove = _activeConfigurations.Keys
.Where(configId => !receivedConfigIds.Contains(configId))
.ToList();

foreach (var configId in configsToRemove)
{
_activeConfigurations.Remove(configId);
Log.Debug("Implicitly removed APM_TRACING configuration {ConfigId} (not in update)", configId);
}
valuesToApply = CombineApmTracingConfiguration(_activeConfigurations, configByProduct, removedConfigByProduct, applyDetailsResult);
}

// Phase 3: Apply merged configuration
ApplyMergedConfiguration();
ApplyMergedConfiguration(valuesToApply);

Comment on lines 202 to 217
Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge ApplyMergedConfiguration must run under _configLock

The previous implementation held _configLock across both the mutation of _activeConfigurations and the call to ApplyMergedConfiguration. After the refactor the lock now wraps only CombineApmTracingConfiguration, while ApplyMergedConfiguration(valuesToApply) runs without synchronization. If two configuration updates arrive at about the same time, one thread can capture valuesToApply for an older configuration, release the lock, another thread updates the dictionary and applies the new configuration, and then the first thread applies the stale snapshot. The tracer ends up running with an out-of-date configuration while _activeConfigurations contains the newer state, and _configurationTelemetry is mutated concurrently as well. Please keep the ApplyMergedConfiguration call inside the lock (or provide equivalent ordering guarantees) so updates remain serialized.

Useful? React with 👍 / 👎.

Copy link
Member Author

@andrewlock andrewlock Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's fair, it suggests to me that we should just drop the lock entirely 🤔 This is called in a single-threaded manner by the Remote config manager anyway 🤔

Copy link
Collaborator

@bouwkast bouwkast Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks reasonable to me

Fine if we do, fine if we don't though

Copy link
Member Author

@andrewlock andrewlock Oct 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remove the lock entirely as it seemed more likely to add a veneer of thread-safety without technically being safe

return applyDetailsResult.ToArray();
}
Expand All @@ -219,15 +229,15 @@ private ApplyDetails[] ConfigurationUpdated(
}
}

private void ApplyMergedConfiguration()
private void ApplyMergedConfiguration(List<RemoteConfiguration> remoteConfigurations)
{
// Get current service/environment for filtering
var currentSettings = Tracer.Instance.Settings;
var serviceName = currentSettings.ServiceName;
var environment = currentSettings.Environment ?? Tracer.Instance.DefaultServiceName;

var mergedConfigJToken = ApmTracingConfigMerger.MergeConfigurations(
_activeConfigurations.Values.ToList(),
remoteConfigurations,
serviceName,
environment);

Expand Down
244 changes: 244 additions & 0 deletions tracer/test/Datadog.Trace.Tests/Configuration/DynamicConfigurationManagerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,244 @@
// <copyright file="DynamicConfigurationManagerTests.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 Datadog.Trace.Configuration;
using Datadog.Trace.RemoteConfigurationManagement;
using FluentAssertions;
using Xunit;

namespace Datadog.Trace.Tests.Configuration;

public class DynamicConfigurationManagerTests
{
private const string ProductName = DynamicConfigurationManager.ProductName;
private static int _version;

[Fact]
public void CombineApmTracingConfiguration_WhenNoConfiguration_ReturnsEmptyCollection()
{
Dictionary<string, RemoteConfiguration> activeConfigs = new();
Dictionary<string, List<RemoteConfiguration>> configByProduct = new();
Dictionary<string, List<RemoteConfigurationPath>> removedConfigByProduct = new();
List<ApplyDetails> applyDetails = [];
var results = DynamicConfigurationManager.CombineApmTracingConfiguration(
new(activeConfigs), // create a copy to avoid mutating the original
configByProduct,
removedConfigByProduct,
applyDetails);

results.Should().BeEmpty();
applyDetails.Should().BeEmpty();
}

[Fact]
public void CombineApmTracingConfiguration_WhenUnknownProducts_ReturnsEmptyCollection()
{
Dictionary<string, RemoteConfiguration> activeConfigs = new();
Dictionary<string, List<RemoteConfiguration>> configByProduct = new()
{
{ "Something else", [CreateConfig()] },
{ "Blah", [CreateConfig()] },
};
Dictionary<string, List<RemoteConfigurationPath>> removedConfigByProduct = new();
List<ApplyDetails> applyDetails = [];
var results = DynamicConfigurationManager.CombineApmTracingConfiguration(
new(activeConfigs), // create a copy to avoid mutating the original
configByProduct,
removedConfigByProduct,
applyDetails);

results.Should().BeEmpty();
AssertApplyDetails(applyDetails, activeConfigs, configByProduct, removedConfigByProduct);
}

[Fact]
public void CombineApmTracingConfiguration_WhenNoConfigsByProduct_ReturnsEmptyCollection()
{
Dictionary<string, RemoteConfiguration> activeConfigs = new();
Dictionary<string, List<RemoteConfiguration>> configByProduct = new();
Dictionary<string, List<RemoteConfigurationPath>> removedConfigByProduct = new()
{
{ ProductName, [RemoteConfigurationPath.FromPath("datadog/1/a/b/c"), RemoteConfigurationPath.FromPath("employee/1/2/3")] },
{ "Blep", [RemoteConfigurationPath.FromPath("datadog/1/d/b/c"), RemoteConfigurationPath.FromPath("employee/4/3/2")] },
};

List<ApplyDetails> applyDetails = [];
var results = DynamicConfigurationManager.CombineApmTracingConfiguration(
new(activeConfigs), // create a copy to avoid mutating the original
configByProduct,
removedConfigByProduct,
applyDetails);

results.Should().BeEmpty();
AssertApplyDetails(applyDetails, activeConfigs, configByProduct, removedConfigByProduct);
}

[Fact]
public void CombineApmTracingConfiguration_WhenExistingConfigIsRemoved_RemovesConfigAndLeavesRemaining()
{
const string pathToRemove = "datadog/1/a/some-random-id/c";
const string expectedPath = "employee/1/other-ID/3";
var configToRemove = CreateConfig(pathToRemove);
var expected = CreateConfig(expectedPath);
Dictionary<string, RemoteConfiguration> activeConfigs = new()
{
{ configToRemove.Path.Id, CreateConfig(pathToRemove) },
{ expected.Path.Id, expected },
};
Dictionary<string, List<RemoteConfiguration>> configByProduct = new();
Dictionary<string, List<RemoteConfigurationPath>> removedConfigByProduct = new()
{
{ ProductName, [RemoteConfigurationPath.FromPath(pathToRemove), RemoteConfigurationPath.FromPath("datadog/2/dont/remove/me")] },
};

List<ApplyDetails> applyDetails = [];
var results = DynamicConfigurationManager.CombineApmTracingConfiguration(
new(activeConfigs), // create a copy to avoid mutating the original
configByProduct,
removedConfigByProduct,
applyDetails);

results.Should().BeEquivalentTo([expected]);
AssertApplyDetails(applyDetails, activeConfigs, configByProduct, removedConfigByProduct);
}

[Fact]
public void CombineApmTracingConfiguration_WhenNewConfigIsProvided_ReplacesExistingConfig()
{
const string pathToRemove = "datadog/1/a/some-random-id/c";
const string expectedPath = "employee/1/other-ID/3";
var configToRemove = CreateConfig(pathToRemove);
var previous = CreateConfig(expectedPath);
var updated = CreateConfig(expectedPath); // different config for same id
Dictionary<string, RemoteConfiguration> activeConfigs = new()
{
{ configToRemove.Path.Id, CreateConfig(pathToRemove) },
{ previous.Path.Id, previous },
};
Dictionary<string, List<RemoteConfiguration>> configByProduct = new()
{
{ ProductName, [updated] },
};
Dictionary<string, List<RemoteConfigurationPath>> removedConfigByProduct = new()
{
{ ProductName, [RemoteConfigurationPath.FromPath(pathToRemove), RemoteConfigurationPath.FromPath("datadog/2/dont/remove/me")] },
};

List<ApplyDetails> applyDetails = [];
var results = DynamicConfigurationManager.CombineApmTracingConfiguration(
new(activeConfigs), // create a copy to avoid mutating the original
configByProduct,
removedConfigByProduct,
applyDetails);

results.Should().BeEquivalentTo([updated]);
AssertApplyDetails(applyDetails, activeConfigs, configByProduct, removedConfigByProduct);
}

[Fact]
public void CombineApmTracingConfiguration_WhenNewConfigIsProvided_ReplacesExistingConfigRegardlessOfRemovedConfig()
{
const string pathToRemove = "datadog/1/a/some-random-id/c";
const string expectedPath = "employee/1/other-ID/3";
var configToRemove = CreateConfig(pathToRemove);
var previous = CreateConfig(expectedPath);
var updated = CreateConfig(expectedPath); // different config for same id
Dictionary<string, RemoteConfiguration> activeConfigs = new()
{
{ configToRemove.Path.Id, CreateConfig(pathToRemove) },
{ previous.Path.Id, previous },
};
Dictionary<string, List<RemoteConfiguration>> configByProduct = new()
{
{ ProductName, [updated] },
};
Dictionary<string, List<RemoteConfigurationPath>> removedConfigByProduct = new()
{
// Note that we're _not_ explicitly removing the pathToRemove config, but as it's not in the "config by product", it gets removed
{ ProductName, [RemoteConfigurationPath.FromPath("datadog/2/dont/remove/me")] },
};

List<ApplyDetails> applyDetails = [];
var results = DynamicConfigurationManager.CombineApmTracingConfiguration(
new(activeConfigs), // create a copy to avoid mutating the original
configByProduct,
removedConfigByProduct,
applyDetails);

results.Should().BeEquivalentTo([updated]);
AssertApplyDetails(applyDetails, activeConfigs, configByProduct, removedConfigByProduct);
}

[Fact]
public void CombineApmTracingConfiguration_WhenNewConfigIsProvided_OverwritesNewAndExistingConfig()
{
const string pathToRemove = "datadog/1/a/some-random-id/c";
const string expectedPath = "employee/1/other-ID/3";
var configToRemove = CreateConfig(pathToRemove);
var previous = CreateConfig(expectedPath);
var updated1 = CreateConfig(expectedPath); // different configs for same id
var updated2 = CreateConfig(expectedPath); // different configs for same id
Dictionary<string, RemoteConfiguration> activeConfigs = new()
{
{ configToRemove.Path.Id, CreateConfig(pathToRemove) },
{ previous.Path.Id, previous },
};
Dictionary<string, List<RemoteConfiguration>> configByProduct = new()
{
{ ProductName, [updated1, updated2] },
};
Dictionary<string, List<RemoteConfigurationPath>> removedConfigByProduct = new()
{
// Note that we're _not_ explicitly removing the pathToRemove config, but as it's not in the "config by product", it gets removed
{ ProductName, [RemoteConfigurationPath.FromPath("datadog/2/dont/remove/me")] },
};

List<ApplyDetails> applyDetails = [];
var results = DynamicConfigurationManager.CombineApmTracingConfiguration(
new(activeConfigs), // create a copy to avoid mutating the original
configByProduct,
removedConfigByProduct,
applyDetails);

results.Should().BeEquivalentTo([updated2]); // updated1 is replaced
AssertApplyDetails(applyDetails, activeConfigs, configByProduct, removedConfigByProduct);
}

private static void AssertApplyDetails(
List<ApplyDetails> applyDetails,
Dictionary<string, RemoteConfiguration> originalConfig,
Dictionary<string, List<RemoteConfiguration>> configByProduct,
Dictionary<string, List<RemoteConfigurationPath>> removedConfigByProduct)
{
var removed = removedConfigByProduct
.Where(x => x.Key == ProductName)
.SelectMany(x => x.Value)
.Where(x => originalConfig.ContainsKey(x.Id)) // Only acknowledge the configs that we actually have
.Select(x => ApplyDetails.FromOk(x.Path));
var added = configByProduct
.Where(x => x.Key == ProductName)
.SelectMany(x => x.Value)
.Select(x => ApplyDetails.FromOk(x.Path.Path));
List<ApplyDetails> expected = [..removed, ..added];

applyDetails.Should().BeEquivalentTo(expected);
}

private static RemoteConfiguration CreateConfig(string path = null)
{
var version = Interlocked.Increment(ref _version);
path ??= $"datadog/{version}/product-{version}/id-{version}/path";
return new RemoteConfiguration(
RemoteConfigurationPath.FromPath(path),
contents: [],
length: 0,
hashes: new(),
version: Interlocked.Increment(ref _version)); // use version to create unique config values
}
}
Loading