Add fix for not re-reporting telemetry when there are "empty" dynamic config changes (#7796)

## Summary of changes

A fix for #7724 to handle telemetry reporting in dynamic config "reset"
scenarios

## Reason for change

The system tests for #7724 were failing in some dynamic configuration
scenarios. Specifically, the tests were sending remote config _without_
any configuration values "i.e. 'reset to use defaults'" and were waiting
a telemetry update. However, we never sent it, because there was "no
telemetry to record".

Note that we _did_ correctly apply the new configuration, we just didn't
report the telemetry correctly, primarily due to limitations in the
telemetry protocol. This PR adds a fix for that, and will be merged into
#7724.

## Implementation details

The solution is to "remember" the telemetry from the default mutable
configuration values, _without_ any dynamic sources, and "replay" this
telemetry when we update telemetry. This feels kind of hacky, but it's
something I suspected we might need to do, and had been avoiding up to
this point because we do a "full reconfigure" anyway.

## Test coverage

Added a specific unit test that mimics the behaviour of the system-test
(i.e. an "empty" dynamic config response) and confirms the telemetry is
recorded as expected

## Other details

https://datadoghq.atlassian.net/browse/LANGPLAT-819

Part of a config stack

- #7522
- #7525
- #7530
- #7532
- #7543
- #7544
- #7721
- #7722
- #7695
- #7723
- #7724
- #7796 👈

Unlike other PRs in the stack, I'll merge this directly into #7724 to
fix the tests there, just thought I'd keep this separate for easier
reviewing

Author: andrewlock

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

@@ -1074,10 +1074,10 @@ public static MutableSettings CreateInitialMutableSettings(
     /// by excluding all the default sources. Effectively gives all the settings their default
     /// values. Should only be used with the manual instrumentation source
     /// </summary>
-    public static MutableSettings CreateWithoutDefaultSources(TracerSettings tracerSettings)
+    public static MutableSettings CreateWithoutDefaultSources(TracerSettings tracerSettings, ConfigurationTelemetry telemetry)
         => CreateInitialMutableSettings(
             NullConfigurationSource.Instance,
-            new ConfigurationTelemetry(),
+            telemetry,
             new OverrideErrorLog(),
             tracerSettings);
 

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

@@ -7,41 +7,56 @@
 
 using System;
 using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
 using System.Threading;
 using Datadog.Trace.Configuration.ConfigurationSources;
 using Datadog.Trace.Configuration.ConfigurationSources.Telemetry;
 using Datadog.Trace.Configuration.Telemetry;
-using Datadog.Trace.Logging;
 
 namespace Datadog.Trace.Configuration;
 
 public partial record TracerSettings
 {
-    internal class SettingsManager(
-        TracerSettings tracerSettings,
-        MutableSettings initialMutable,
-        ExporterSettings initialExporter)
+    internal class SettingsManager
     {
-        private readonly TracerSettings _tracerSettings = tracerSettings;
+        private readonly TracerSettings _tracerSettings;
+        private readonly ConfigurationTelemetry _initialTelemetry;
         private readonly List<SettingChangeSubscription> _subscribers = [];
 
         private IConfigurationSource _dynamicConfigurationSource = NullConfigurationSource.Instance;
         private ManualInstrumentationConfigurationSourceBase _manualConfigurationSource =
             new ManualInstrumentationConfigurationSource(new Dictionary<string, object?>(), useDefaultSources: true);
 
+        // We delay creating these, as we likely won't need them
+        private ConfigurationTelemetry? _noDefaultSettingsTelemetry;
+        private MutableSettings? _noDefaultSourcesSettings;
+
         private SettingChanges? _latest;
 
+        public SettingsManager(IConfigurationSource source, TracerSettings tracerSettings, IConfigurationTelemetry telemetry, OverrideErrorLog errorLog)
+        {
+            // We record the telemetry for the initial settings in a dedicated ConfigurationTelemetry,
+            // because we need to be able to reapply this configuration on dynamic config updates
+            // We don't re-record error logs, so we just use the built-in for that
+            var initialTelemetry = new ConfigurationTelemetry();
+            InitialMutableSettings = MutableSettings.CreateInitialMutableSettings(source, initialTelemetry, errorLog, tracerSettings);
+            InitialExporterSettings = new ExporterSettings(source, initialTelemetry);
+            _tracerSettings = tracerSettings;
+            _initialTelemetry = initialTelemetry;
+            initialTelemetry.CopyTo(telemetry);
+        }
+
         /// <summary>
         /// Gets the initial <see cref="MutableSettings"/>. On app startup, these will be the values read from
         /// static sources. To subscribe to updates to these settings, from code or remote config, call <see cref="SubscribeToChanges"/>.
         /// </summary>
-        public MutableSettings InitialMutableSettings { get; } = initialMutable;
+        public MutableSettings InitialMutableSettings { get; }
 
         /// <summary>
         /// Gets the initial <see cref="ExporterSettings"/>. On app startup, these will be the values read from
         /// static sources. To subscribe to updates to these settings, from code or remote config, call <see cref="SubscribeToChanges"/>.
         /// </summary>
-        public ExporterSettings InitialExporterSettings { get; } = initialExporter;
+        public ExporterSettings InitialExporterSettings { get; }
 
         /// <summary>
         /// Subscribe to changes in <see cref="MutableSettings"/> and/or <see cref="ExporterSettings"/>.
@@ -133,21 +148,45 @@ private bool UpdateSettings(
             ManualInstrumentationConfigurationSourceBase manualSource,
             IConfigurationTelemetry telemetry)
         {
-            var initialSettings = manualSource.UseDefaultSources
-                                      ? InitialMutableSettings
-                                      : MutableSettings.CreateWithoutDefaultSources(_tracerSettings);
+            // Set the correct default telemetry and initial settings depending
+            // on whether the manual config source explicitly disables using the default sources
+            ConfigurationTelemetry defaultTelemetry;
+            MutableSettings initialSettings;
+            if (manualSource.UseDefaultSources)
+            {
+                defaultTelemetry = _initialTelemetry;
+                initialSettings = InitialMutableSettings;
+            }
+            else
+            {
+                // We only need to initialize the "no default sources" settings once
+                // and we don't want to initialize them if we don't _need_ to
+                // so lazy-initialize here
+                if (_noDefaultSourcesSettings is null || _noDefaultSettingsTelemetry is null)
+                {
+                    InitialiseNoDefaultSourceSettings();
+                }
+
+                defaultTelemetry = _noDefaultSettingsTelemetry;
+                initialSettings = _noDefaultSourcesSettings;
+            }
 
             var current = _latest;
             var currentMutable = current?.UpdatedMutable ?? current?.PreviousMutable ?? InitialMutableSettings;
             var currentExporter = current?.UpdatedExporter ?? current?.PreviousExporter ?? InitialExporterSettings;
 
+            // we create a temporary ConfigurationTelemetry object to hold the changes to settings
+            // if nothing is actually written, and nothing changes compared to the default, then we
+            // don't need to report it to the provided telemetry
+            var tempTelemetry = new ConfigurationTelemetry();
+
             var overrideErrorLog = new OverrideErrorLog();
             var newMutableSettings = MutableSettings.CreateUpdatedMutableSettings(
                 dynamicConfigSource,
                 manualSource,
                 initialSettings,
                 _tracerSettings,
-                telemetry,
+                tempTelemetry,
                 overrideErrorLog); // TODO: We'll later report these
 
             // The only exporter setting we currently _allow_ to change is the AgentUri, but if that does change,
@@ -159,7 +198,7 @@ private bool UpdateSettings(
             var newRawExporterSettings = ExporterSettings.Raw.CreateUpdatedFromManualConfig(
                 currentExporter.RawSettings,
                 manualSource,
-                telemetry,
+                tempTelemetry,
                 manualSource.UseDefaultSources);
 
             var isSameMutableSettings = currentMutable.Equals(newMutableSettings);
@@ -171,13 +210,33 @@ private bool UpdateSettings(
                 return null;
             }
 
+            // we have changes, so we need to report them
+            // First record the "default"/fallback values, then record the "new" values
+            defaultTelemetry.CopyTo(telemetry);
+            tempTelemetry.CopyTo(telemetry);
+
             Log.Information("Notifying consumers of new settings");
             var updatedMutableSettings = isSameMutableSettings ? null : newMutableSettings;
             var updatedExporterSettings = isSameExporterSettings ? null : new ExporterSettings(newRawExporterSettings, telemetry);
 
             return new SettingChanges(updatedMutableSettings, updatedExporterSettings, currentMutable, currentExporter);
         }
 
+        [MemberNotNull(nameof(_noDefaultSettingsTelemetry))]
+        [MemberNotNull(nameof(_noDefaultSourcesSettings))]
+        private void InitialiseNoDefaultSourceSettings()
+        {
+            if (_noDefaultSourcesSettings is not null
+             && _noDefaultSettingsTelemetry is not null)
+            {
+                return;
+            }
+
+            var telemetry = new ConfigurationTelemetry();
+            _noDefaultSettingsTelemetry = telemetry;
+            _noDefaultSourcesSettings = MutableSettings.CreateWithoutDefaultSources(_tracerSettings, telemetry);
+        }
+
         private void NotifySubscribers(SettingChanges settings)
         {
             _latest = settings;

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

@@ -125,8 +125,6 @@ internal TracerSettings(IConfigurationSource? source, IConfigurationTelemetry te
                                        .AsBoolResult()
                                        .OverrideWith(in otelActivityListenerEnabled, ErrorLog, defaultValue: false);
 
-            var exporter = new ExporterSettings(source, _telemetry);
-
             PeerServiceTagsEnabled = config
                .WithKeys(ConfigurationKeys.PeerServiceDefaultsEnabled)
                .AsBool(defaultValue: false);
@@ -335,66 +333,6 @@ not null when string.Equals(value, "otlp", StringComparison.OrdinalIgnoreCase) =
 
             OpenTelemetryLogsEnabled = OpenTelemetryLogsEnabled && OtelLogsExporterEnabled;
 
-            DataPipelineEnabled = config
-                                  .WithKeys(ConfigurationKeys.TraceDataPipelineEnabled)
-                                  .AsBool(defaultValue: EnvironmentHelpers.IsUsingAzureAppServicesSiteExtension() && !EnvironmentHelpers.IsAzureFunctions());
-
-            if (DataPipelineEnabled)
-            {
-                // Due to missing quantization and obfuscation in native side, we can't enable the native trace exporter
-                // as it may lead to different stats results than the managed one.
-                if (StatsComputationEnabled)
-                {
-                    DataPipelineEnabled = false;
-                    Log.Warning(
-                        $"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but {ConfigurationKeys.StatsComputationEnabled} is enabled. Disabling data pipeline.");
-                    _telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated);
-                }
-
-                // Windows supports UnixDomainSocket https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/
-                // but tokio hasn't added support for it yet https://github.com/tokio-rs/tokio/issues/2201
-                // There's an issue here, in that technically a user can initially be configured to send over TCP/named pipes,
-                // and so we allow and enable the datapipeline. Later, they could configure the app in code to send over UDS.
-                // This is a problem, as we currently don't support toggling the data pipeline at runtime, so we explicitly block
-                // this scenario in the public API.
-                if (exporter.TracesTransport == TracesTransportType.UnixDomainSocket && FrameworkDescription.Instance.IsWindows())
-                {
-                    DataPipelineEnabled = false;
-                    Log.Warning(
-                        $"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but TracesTransport is set to UnixDomainSocket which is not supported on Windows. Disabling data pipeline.");
-                    _telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated);
-                }
-
-                if (!isLibDatadogAvailable.IsAvailable)
-                {
-                    DataPipelineEnabled = false;
-                    if (isLibDatadogAvailable.Exception is not null)
-                    {
-                        Log.Warning(
-                            isLibDatadogAvailable.Exception,
-                            $"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but libdatadog is not available. Disabling data pipeline.");
-                    }
-                    else
-                    {
-                        Log.Warning(
-                            $"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but libdatadog is not available. Disabling data pipeline.");
-                    }
-
-                    _telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated);
-                }
-
-                // SSI already utilizes libdatadog. To prevent unexpected behavior,
-                // we proactively disable the data pipeline when SSI is enabled. Theoretically, this should not cause any issues,
-                // but as a precaution, we are taking a conservative approach during the initial rollout phase.
-                if (!string.IsNullOrEmpty(EnvironmentHelpers.GetEnvironmentVariable("DD_INJECTION_ENABLED")))
-                {
-                    DataPipelineEnabled = false;
-                    Log.Warning(
-                        $"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but SSI is enabled. Disabling data pipeline.");
-                    _telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated);
-                }
-            }
-
             // We should also be writing telemetry for OTEL_LOGS_EXPORTER similar to OTEL_METRICS_EXPORTER, but we don't have a corresponding Datadog config
             // When we do, we can insert that here
             CustomSamplingRulesFormat = config.WithKeys(ConfigurationKeys.CustomSamplingRulesFormat)
@@ -731,9 +669,70 @@ not null when string.Equals(value, "otlp", StringComparison.OrdinalIgnoreCase) =
             // We create a lazy here because this is kind of expensive, and we want to avoid calling it if we can
             _fallbackApplicationName = new(() => ApplicationNameHelpers.GetFallbackApplicationName(this));
 
-            // Move the creation of these settings inside SettingsManager?
-            var initialMutableSettings = MutableSettings.CreateInitialMutableSettings(source, telemetry, errorLog, this);
-            Manager = new(this, initialMutableSettings, exporter);
+            // There's a circular dependency here because DataPipeline depends on ExporterSettings,
+            // but the settings manager depends on TracerSettings. Basically this is all fine as long
+            // as nothing in the MutableSettings or ExporterSettings depends on the value of DataPipelineEnabled!
+            Manager = new(source, this, telemetry, errorLog);
+
+            DataPipelineEnabled = config
+                                  .WithKeys(ConfigurationKeys.TraceDataPipelineEnabled)
+                                  .AsBool(defaultValue: EnvironmentHelpers.IsUsingAzureAppServicesSiteExtension() && !EnvironmentHelpers.IsAzureFunctions());
+
+            if (DataPipelineEnabled)
+            {
+                // Due to missing quantization and obfuscation in native side, we can't enable the native trace exporter
+                // as it may lead to different stats results than the managed one.
+                if (StatsComputationEnabled)
+                {
+                    DataPipelineEnabled = false;
+                    Log.Warning(
+                        $"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but {ConfigurationKeys.StatsComputationEnabled} is enabled. Disabling data pipeline.");
+                    _telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated);
+                }
+
+                // Windows supports UnixDomainSocket https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/
+                // but tokio hasn't added support for it yet https://github.com/tokio-rs/tokio/issues/2201
+                // There's an issue here, in that technically a user can initially be configured to send over TCP/named pipes,
+                // and so we allow and enable the datapipeline. Later, they could configure the app in code to send over UDS.
+                // This is a problem, as we currently don't support toggling the data pipeline at runtime, so we explicitly block
+                // this scenario in the public API.
+                if (Manager.InitialExporterSettings.TracesTransport == TracesTransportType.UnixDomainSocket && FrameworkDescription.Instance.IsWindows())
+                {
+                    DataPipelineEnabled = false;
+                    Log.Warning(
+                        $"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but TracesTransport is set to UnixDomainSocket which is not supported on Windows. Disabling data pipeline.");
+                    _telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated);
+                }
+
+                if (!isLibDatadogAvailable.IsAvailable)
+                {
+                    DataPipelineEnabled = false;
+                    if (isLibDatadogAvailable.Exception is not null)
+                    {
+                        Log.Warning(
+                            isLibDatadogAvailable.Exception,
+                            $"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but libdatadog is not available. Disabling data pipeline.");
+                    }
+                    else
+                    {
+                        Log.Warning(
+                            $"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but libdatadog is not available. Disabling data pipeline.");
+                    }
+
+                    _telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated);
+                }
+
+                // SSI already utilizes libdatadog. To prevent unexpected behavior,
+                // we proactively disable the data pipeline when SSI is enabled. Theoretically, this should not cause any issues,
+                // but as a precaution, we are taking a conservative approach during the initial rollout phase.
+                if (!string.IsNullOrEmpty(EnvironmentHelpers.GetEnvironmentVariable("DD_INJECTION_ENABLED")))
+                {
+                    DataPipelineEnabled = false;
+                    Log.Warning(
+                        $"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but SSI is enabled. Disabling data pipeline.");
+                    _telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated);
+                }
+            }
         }
 
         internal bool IsRunningInCiVisibility { get; }

tracer/src/Datadog.Trace/RemoteConfigurationManagement/Transport/RemoteConfigurationApi.cs

@@ -6,6 +6,7 @@
 #nullable enable
 
 using System;
+using System.IO;
 using System.Text;
 using System.Threading;
 using System.Threading.Tasks;
@@ -15,6 +16,7 @@
 using Datadog.Trace.Logging;
 using Datadog.Trace.PlatformHelpers;
 using Datadog.Trace.RemoteConfigurationManagement.Protocol;
+using Datadog.Trace.Util.Streams;
 using Datadog.Trace.Vendors.Newtonsoft.Json;
 
 namespace Datadog.Trace.RemoteConfigurationManagement.Transport

tracer/test/Datadog.Trace.Security.Unit.Tests/EmptyDatadogTracer.cs

@@ -6,6 +6,7 @@
 using System;
 using Datadog.Trace.Configuration;
 using Datadog.Trace.Configuration.Schema;
+using Datadog.Trace.Configuration.Telemetry;
 using Datadog.Trace.Sampling;
 using Moq;
 
@@ -23,7 +24,7 @@ public EmptyDatadogTracer()
             DefaultServiceName = "My Service Name";
             Settings = new TracerSettings(NullConfigurationSource.Instance);
             var namingSchema = new NamingSchema(SchemaVersion.V0, false, false, DefaultServiceName, null, null);
-            PerTraceSettings = new PerTraceSettings(null, null, namingSchema, MutableSettings.CreateWithoutDefaultSources(Settings));
+            PerTraceSettings = new PerTraceSettings(null, null, namingSchema, MutableSettings.CreateWithoutDefaultSources(Settings, new ConfigurationTelemetry()));
         }
 
         public string DefaultServiceName { get; }