diff --git a/tracer/src/Datadog.Trace/Configuration/DynamicConfigurationManager.cs b/tracer/src/Datadog.Trace/Configuration/DynamicConfigurationManager.cs index 9592a4cf3a01..20d77e476909 100644 --- a/tracer/src/Datadog.Trace/Configuration/DynamicConfigurationManager.cs +++ b/tracer/src/Datadog.Trace/Configuration/DynamicConfigurationManager.cs @@ -32,7 +32,6 @@ internal class DynamicConfigurationManager : IDynamicConfigurationManager private readonly IRcmSubscriptionManager _subscriptionManager; private readonly ConfigurationTelemetry _configurationTelemetry; private readonly Dictionary _activeConfigurations = new(); - private readonly object _configLock = new(); private ISubscription? _subscription; public DynamicConfigurationManager(IRcmSubscriptionManager subscriptionManager) @@ -158,68 +157,75 @@ 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 CombineApmTracingConfiguration( + Dictionary activeConfigurations, + Dictionary> configByProduct, + Dictionary>? removedConfigByProduct, + List 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("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> configByProduct, Dictionary>? removedConfigByProduct) { - lock (_configLock) { var applyDetailsResult = new List(); 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)) - { - var receivedConfigIds = new HashSet(); - - // 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); - } - } + // This is all non-thread safe, but we're called in a single threaded way by + // the RcmSubscriptionManager so that's fine + var valuesToApply = CombineApmTracingConfiguration(_activeConfigurations, configByProduct, removedConfigByProduct, applyDetailsResult); // Phase 3: Apply merged configuration - ApplyMergedConfiguration(); + ApplyMergedConfiguration(valuesToApply); - return applyDetailsResult.ToArray(); + return [..applyDetailsResult]; } catch (Exception ex) { Log.Error(ex, "Error while applying dynamic configuration"); - return applyDetailsResult.Select(r => ApplyDetails.FromError(r.Filename, ex.ToString())).ToArray(); + return [..applyDetailsResult.Select(r => ApplyDetails.FromError(r.Filename, ex.ToString()))]; } } } - private void ApplyMergedConfiguration() + private void ApplyMergedConfiguration(List remoteConfigurations) { // Get current service/environment for filtering var currentSettings = Tracer.Instance.Settings; @@ -227,7 +233,7 @@ private void ApplyMergedConfiguration() var environment = currentSettings.Environment ?? Tracer.Instance.DefaultServiceName; var mergedConfigJToken = ApmTracingConfigMerger.MergeConfigurations( - _activeConfigurations.Values.ToList(), + remoteConfigurations, serviceName, environment); diff --git a/tracer/test/Datadog.Trace.Tests/Configuration/DynamicConfigurationManagerTests.cs b/tracer/test/Datadog.Trace.Tests/Configuration/DynamicConfigurationManagerTests.cs new file mode 100644 index 000000000000..fd8c76c7bd7b --- /dev/null +++ b/tracer/test/Datadog.Trace.Tests/Configuration/DynamicConfigurationManagerTests.cs @@ -0,0 +1,244 @@ +// +// 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. +// + +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 activeConfigs = new(); + Dictionary> configByProduct = new(); + Dictionary> removedConfigByProduct = new(); + List 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 activeConfigs = new(); + Dictionary> configByProduct = new() + { + { "Something else", [CreateConfig()] }, + { "Blah", [CreateConfig()] }, + }; + Dictionary> removedConfigByProduct = new(); + List 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 activeConfigs = new(); + Dictionary> configByProduct = new(); + Dictionary> 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 = []; + 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 activeConfigs = new() + { + { configToRemove.Path.Id, CreateConfig(pathToRemove) }, + { expected.Path.Id, expected }, + }; + Dictionary> configByProduct = new(); + Dictionary> removedConfigByProduct = new() + { + { ProductName, [RemoteConfigurationPath.FromPath(pathToRemove), RemoteConfigurationPath.FromPath("datadog/2/dont/remove/me")] }, + }; + + List 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 activeConfigs = new() + { + { configToRemove.Path.Id, CreateConfig(pathToRemove) }, + { previous.Path.Id, previous }, + }; + Dictionary> configByProduct = new() + { + { ProductName, [updated] }, + }; + Dictionary> removedConfigByProduct = new() + { + { ProductName, [RemoteConfigurationPath.FromPath(pathToRemove), RemoteConfigurationPath.FromPath("datadog/2/dont/remove/me")] }, + }; + + List 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 activeConfigs = new() + { + { configToRemove.Path.Id, CreateConfig(pathToRemove) }, + { previous.Path.Id, previous }, + }; + Dictionary> configByProduct = new() + { + { ProductName, [updated] }, + }; + Dictionary> 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 = []; + 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 activeConfigs = new() + { + { configToRemove.Path.Id, CreateConfig(pathToRemove) }, + { previous.Path.Id, previous }, + }; + Dictionary> configByProduct = new() + { + { ProductName, [updated1, updated2] }, + }; + Dictionary> 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 = []; + 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, + Dictionary originalConfig, + Dictionary> configByProduct, + Dictionary> 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 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 + } +}