Skip to content

Commit 3238b3e

Browse files
andrewlockandrewlock
committed
Add fix for not re-reporting telemetry when there are "empty" dynamic config changes
1 parent 7ed9c21 commit 3238b3e

File tree

7 files changed

+230
-83
lines changed

7 files changed

+230
-83
lines changed

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1074,10 +1074,10 @@ public static MutableSettings CreateInitialMutableSettings(
10741074
/// by excluding all the default sources. Effectively gives all the settings their default
10751075
/// values. Should only be used with the manual instrumentation source
10761076
/// </summary>
1077-
public static MutableSettings CreateWithoutDefaultSources(TracerSettings tracerSettings)
1077+
public static MutableSettings CreateWithoutDefaultSources(TracerSettings tracerSettings, ConfigurationTelemetry telemetry)
10781078
=> CreateInitialMutableSettings(
10791079
NullConfigurationSource.Instance,
1080-
new ConfigurationTelemetry(),
1080+
telemetry,
10811081
new OverrideErrorLog(),
10821082
tracerSettings);
10831083

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

Lines changed: 69 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,36 +7,51 @@
77

88
using System;
99
using System.Collections.Generic;
10+
using System.Diagnostics.CodeAnalysis;
1011
using System.Threading;
1112
using Datadog.Trace.Configuration.ConfigurationSources;
1213
using Datadog.Trace.Configuration.ConfigurationSources.Telemetry;
1314
using Datadog.Trace.Configuration.Telemetry;
14-
using Datadog.Trace.Logging;
1515

1616
namespace Datadog.Trace.Configuration;
1717

1818
public partial record TracerSettings
1919
{
20-
internal class SettingsManager(
21-
TracerSettings tracerSettings,
22-
MutableSettings initialMutable,
23-
ExporterSettings initialExporter)
20+
internal class SettingsManager
2421
{
25-
private readonly TracerSettings _tracerSettings = tracerSettings;
22+
private readonly TracerSettings _tracerSettings;
23+
private readonly ConfigurationTelemetry _initialTelemetry;
2624
private readonly List<SettingChangeSubscription> _subscribers = [];
25+
// We delay creating these, as we likely won't need them
26+
private ConfigurationTelemetry? _noDefaultSettingsTelemetry;
27+
private MutableSettings? _noDefaultSourcesSettings;
28+
2729
private SettingChanges? _latest;
2830

31+
public SettingsManager(IConfigurationSource source, TracerSettings tracerSettings, IConfigurationTelemetry telemetry, OverrideErrorLog errorLog)
32+
{
33+
// We record the telemetry for the initial settings in a dedicated ConfigurationTelemetry,
34+
// because we need to be able to reapply this configuration on dynamic config updates
35+
// We don't re-record error logs, so we just use the built-in for that
36+
var initialTelemetry = new ConfigurationTelemetry();
37+
InitialMutableSettings = MutableSettings.CreateInitialMutableSettings(source, initialTelemetry, errorLog, tracerSettings);
38+
InitialExporterSettings = new ExporterSettings(source, initialTelemetry);
39+
_tracerSettings = tracerSettings;
40+
_initialTelemetry = initialTelemetry;
41+
initialTelemetry.CopyTo(telemetry);
42+
}
43+
2944
/// <summary>
3045
/// Gets the initial <see cref="MutableSettings"/>. On app startup, these will be the values read from
3146
/// static sources. To subscribe to updates to these settings, from code or remote config, call <see cref="SubscribeToChanges"/>.
3247
/// </summary>
33-
public MutableSettings InitialMutableSettings { get; } = initialMutable;
48+
public MutableSettings InitialMutableSettings { get; }
3449

3550
/// <summary>
3651
/// Gets the initial <see cref="ExporterSettings"/>. On app startup, these will be the values read from
3752
/// static sources. To subscribe to updates to these settings, from code or remote config, call <see cref="SubscribeToChanges"/>.
3853
/// </summary>
39-
public ExporterSettings InitialExporterSettings { get; } = initialExporter;
54+
public ExporterSettings InitialExporterSettings { get; }
4055

4156
/// <summary>
4257
/// Subscribe to changes in <see cref="MutableSettings"/> and/or <see cref="ExporterSettings"/>.
@@ -99,9 +114,27 @@ public bool UpdateSettings(
99114
ManualInstrumentationConfigurationSourceBase manualSource,
100115
IConfigurationTelemetry telemetry)
101116
{
102-
var initialSettings = manualSource.UseDefaultSources
103-
? InitialMutableSettings
104-
: MutableSettings.CreateWithoutDefaultSources(_tracerSettings);
117+
// we create a temporary ConfigurationTelemetry object to hold the changes to settings
118+
// if nothing is actually written, and nothing changes compared to the default, then we
119+
// don't need to report it to the provided telemetry
120+
var tempTelemetry = new ConfigurationTelemetry();
121+
ConfigurationTelemetry defaultTelemetry;
122+
MutableSettings initialSettings;
123+
if (manualSource.UseDefaultSources)
124+
{
125+
defaultTelemetry = _initialTelemetry;
126+
initialSettings = InitialMutableSettings;
127+
}
128+
else
129+
{
130+
if (_noDefaultSourcesSettings is null || _noDefaultSettingsTelemetry is null)
131+
{
132+
InitialiseNoDefaultSourceSettings();
133+
}
134+
135+
defaultTelemetry = _noDefaultSettingsTelemetry;
136+
initialSettings = _noDefaultSourcesSettings;
137+
}
105138

106139
var current = Volatile.Read(ref _latest);
107140
var currentMutable = current?.UpdatedMutable ?? current?.PreviousMutable ?? InitialMutableSettings;
@@ -113,7 +146,7 @@ public bool UpdateSettings(
113146
manualSource,
114147
initialSettings,
115148
_tracerSettings,
116-
telemetry,
149+
tempTelemetry,
117150
overrideErrorLog); // TODO: We'll later report these
118151

119152
// The only exporter setting we currently _allow_ to change is the AgentUri, but if that does change,
@@ -125,7 +158,7 @@ public bool UpdateSettings(
125158
var newRawExporterSettings = ExporterSettings.Raw.CreateUpdatedFromManualConfig(
126159
currentExporter.RawSettings,
127160
manualSource,
128-
telemetry,
161+
tempTelemetry,
129162
manualSource.UseDefaultSources);
130163

131164
var isSameMutableSettings = currentMutable.Equals(newMutableSettings);
@@ -137,13 +170,36 @@ public bool UpdateSettings(
137170
return null;
138171
}
139172

173+
// we have changes, so we need to report them
174+
// First record the "default"/fallback values, then record the "new" values
175+
defaultTelemetry.CopyTo(telemetry);
176+
tempTelemetry.CopyTo(telemetry);
177+
140178
Log.Information("Notifying consumers of new settings");
141179
var updatedMutableSettings = isSameMutableSettings ? null : newMutableSettings;
142180
var updatedExporterSettings = isSameExporterSettings ? null : new ExporterSettings(newRawExporterSettings, telemetry);
143181

144182
return new SettingChanges(updatedMutableSettings, updatedExporterSettings, currentMutable, currentExporter);
145183
}
146184

185+
[MemberNotNull(nameof(_noDefaultSettingsTelemetry))]
186+
[MemberNotNull(nameof(_noDefaultSourcesSettings))]
187+
private void InitialiseNoDefaultSourceSettings()
188+
{
189+
lock (_tracerSettings)
190+
{
191+
if (_noDefaultSourcesSettings is not null
192+
&& _noDefaultSettingsTelemetry is not null)
193+
{
194+
return;
195+
}
196+
197+
var telemetry = new ConfigurationTelemetry();
198+
_noDefaultSettingsTelemetry = telemetry;
199+
_noDefaultSourcesSettings = MutableSettings.CreateWithoutDefaultSources(_tracerSettings, telemetry);
200+
}
201+
}
202+
147203
private void NotifySubscribers(SettingChanges settings)
148204
{
149205
// Strictly, for safety, we only need to lock in the subscribers list access. However,

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

Lines changed: 66 additions & 65 deletions
Original file line numberDiff line numberDiff line change
@@ -140,8 +140,6 @@ internal TracerSettings(IConfigurationSource? source, IConfigurationTelemetry te
140140
.AsBoolResult()
141141
.OverrideWith(in otelActivityListenerEnabled, ErrorLog, defaultValue: false);
142142

143-
var exporter = new ExporterSettings(source, _telemetry);
144-
145143
PeerServiceTagsEnabled = config
146144
.WithKeys(ConfigurationKeys.PeerServiceDefaultsEnabled)
147145
.AsBool(defaultValue: false);
@@ -350,66 +348,6 @@ not null when string.Equals(value, "otlp", StringComparison.OrdinalIgnoreCase) =
350348

351349
OpenTelemetryLogsEnabled = OpenTelemetryLogsEnabled && OtelLogsExporterEnabled;
352350

353-
DataPipelineEnabled = config
354-
.WithKeys(ConfigurationKeys.TraceDataPipelineEnabled)
355-
.AsBool(defaultValue: EnvironmentHelpers.IsUsingAzureAppServicesSiteExtension() && !EnvironmentHelpers.IsAzureFunctions());
356-
357-
if (DataPipelineEnabled)
358-
{
359-
// Due to missing quantization and obfuscation in native side, we can't enable the native trace exporter
360-
// as it may lead to different stats results than the managed one.
361-
if (StatsComputationEnabled)
362-
{
363-
DataPipelineEnabled = false;
364-
Log.Warning(
365-
$"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but {ConfigurationKeys.StatsComputationEnabled} is enabled. Disabling data pipeline.");
366-
_telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated);
367-
}
368-
369-
// Windows supports UnixDomainSocket https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/
370-
// but tokio hasn't added support for it yet https://github.com/tokio-rs/tokio/issues/2201
371-
// There's an issue here, in that technically a user can initially be configured to send over TCP/named pipes,
372-
// and so we allow and enable the datapipeline. Later, they could configure the app in code to send over UDS.
373-
// This is a problem, as we currently don't support toggling the data pipeline at runtime, so we explicitly block
374-
// this scenario in the public API.
375-
if (exporter.TracesTransport == TracesTransportType.UnixDomainSocket && FrameworkDescription.Instance.IsWindows())
376-
{
377-
DataPipelineEnabled = false;
378-
Log.Warning(
379-
$"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but TracesTransport is set to UnixDomainSocket which is not supported on Windows. Disabling data pipeline.");
380-
_telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated);
381-
}
382-
383-
if (!isLibDatadogAvailable.IsAvailable)
384-
{
385-
DataPipelineEnabled = false;
386-
if (isLibDatadogAvailable.Exception is not null)
387-
{
388-
Log.Warning(
389-
isLibDatadogAvailable.Exception,
390-
$"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but libdatadog is not available. Disabling data pipeline.");
391-
}
392-
else
393-
{
394-
Log.Warning(
395-
$"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but libdatadog is not available. Disabling data pipeline.");
396-
}
397-
398-
_telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated);
399-
}
400-
401-
// SSI already utilizes libdatadog. To prevent unexpected behavior,
402-
// we proactively disable the data pipeline when SSI is enabled. Theoretically, this should not cause any issues,
403-
// but as a precaution, we are taking a conservative approach during the initial rollout phase.
404-
if (!string.IsNullOrEmpty(EnvironmentHelpers.GetEnvironmentVariable("DD_INJECTION_ENABLED")))
405-
{
406-
DataPipelineEnabled = false;
407-
Log.Warning(
408-
$"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but SSI is enabled. Disabling data pipeline.");
409-
_telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated);
410-
}
411-
}
412-
413351
// We should also be writing telemetry for OTEL_LOGS_EXPORTER similar to OTEL_METRICS_EXPORTER, but we don't have a corresponding Datadog config
414352
// When we do, we can insert that here
415353
CustomSamplingRulesFormat = config.WithKeys(ConfigurationKeys.CustomSamplingRulesFormat)
@@ -742,9 +680,72 @@ not null when string.Equals(value, "otlp", StringComparison.OrdinalIgnoreCase) =
742680
// We create a lazy here because this is kind of expensive, and we want to avoid calling it if we can
743681
_fallbackApplicationName = new(() => ApplicationNameHelpers.GetFallbackApplicationName(this));
744682

745-
// Move the creation of these settings inside SettingsManager?
746-
var initialMutableSettings = MutableSettings.CreateInitialMutableSettings(source, telemetry, errorLog, this);
747-
Manager = new(this, initialMutableSettings, exporter);
683+
// There's a circular dependency here because DataPipeline depends on ExporterSettings,
684+
// but the settings manager depends on TracerSettings. Basically this is all fine as long
685+
// as nothing in the MutableSettings or ExporterSettings depends on the value of DataPipelineEnabled!
686+
Manager = new(source, this, telemetry, errorLog);
687+
688+
var exporter = new ExporterSettings(source, _telemetry);
689+
690+
DataPipelineEnabled = config
691+
.WithKeys(ConfigurationKeys.TraceDataPipelineEnabled)
692+
.AsBool(defaultValue: EnvironmentHelpers.IsUsingAzureAppServicesSiteExtension() && !EnvironmentHelpers.IsAzureFunctions());
693+
694+
if (DataPipelineEnabled)
695+
{
696+
// Due to missing quantization and obfuscation in native side, we can't enable the native trace exporter
697+
// as it may lead to different stats results than the managed one.
698+
if (StatsComputationEnabled)
699+
{
700+
DataPipelineEnabled = false;
701+
Log.Warning(
702+
$"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but {ConfigurationKeys.StatsComputationEnabled} is enabled. Disabling data pipeline.");
703+
_telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated);
704+
}
705+
706+
// Windows supports UnixDomainSocket https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/
707+
// but tokio hasn't added support for it yet https://github.com/tokio-rs/tokio/issues/2201
708+
// There's an issue here, in that technically a user can initially be configured to send over TCP/named pipes,
709+
// and so we allow and enable the datapipeline. Later, they could configure the app in code to send over UDS.
710+
// This is a problem, as we currently don't support toggling the data pipeline at runtime, so we explicitly block
711+
// this scenario in the public API.
712+
if (exporter.TracesTransport == TracesTransportType.UnixDomainSocket && FrameworkDescription.Instance.IsWindows())
713+
{
714+
DataPipelineEnabled = false;
715+
Log.Warning(
716+
$"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but TracesTransport is set to UnixDomainSocket which is not supported on Windows. Disabling data pipeline.");
717+
_telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated);
718+
}
719+
720+
if (!isLibDatadogAvailable.IsAvailable)
721+
{
722+
DataPipelineEnabled = false;
723+
if (isLibDatadogAvailable.Exception is not null)
724+
{
725+
Log.Warning(
726+
isLibDatadogAvailable.Exception,
727+
$"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but libdatadog is not available. Disabling data pipeline.");
728+
}
729+
else
730+
{
731+
Log.Warning(
732+
$"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but libdatadog is not available. Disabling data pipeline.");
733+
}
734+
735+
_telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated);
736+
}
737+
738+
// SSI already utilizes libdatadog. To prevent unexpected behavior,
739+
// we proactively disable the data pipeline when SSI is enabled. Theoretically, this should not cause any issues,
740+
// but as a precaution, we are taking a conservative approach during the initial rollout phase.
741+
if (!string.IsNullOrEmpty(EnvironmentHelpers.GetEnvironmentVariable("DD_INJECTION_ENABLED")))
742+
{
743+
DataPipelineEnabled = false;
744+
Log.Warning(
745+
$"{ConfigurationKeys.TraceDataPipelineEnabled} is enabled, but SSI is enabled. Disabling data pipeline.");
746+
_telemetry.Record(ConfigurationKeys.TraceDataPipelineEnabled, false, ConfigurationOrigins.Calculated);
747+
}
748+
}
748749
}
749750

750751
internal bool IsRunningInCiVisibility { get; }

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
#nullable enable
77

88
using System;
9+
using System.IO;
910
using System.Text;
1011
using System.Threading;
1112
using System.Threading.Tasks;
@@ -15,6 +16,7 @@
1516
using Datadog.Trace.Logging;
1617
using Datadog.Trace.PlatformHelpers;
1718
using Datadog.Trace.RemoteConfigurationManagement.Protocol;
19+
using Datadog.Trace.Util.Streams;
1820
using Datadog.Trace.Vendors.Newtonsoft.Json;
1921

2022
namespace Datadog.Trace.RemoteConfigurationManagement.Transport
@@ -91,7 +93,9 @@ public static RemoteConfigurationApi Create(IApiRequestFactory apiRequestFactory
9193

9294
try
9395
{
94-
return await apiResponse.ReadAsTypeAsync<GetRcmResponse>().ConfigureAwait(false);
96+
var result = await apiResponse.ReadAsTypeAsync<GetRcmResponse>().ConfigureAwait(false);
97+
Log.Debug("Received RCM response: {Response}", JsonConvert.SerializeObject(result));
98+
return result;
9599
}
96100
catch (Exception ex)
97101
{

‎tracer/test/Datadog.Trace.Tests/Agent/AgentWriterTests.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
using Datadog.Trace.Agent;
1212
using Datadog.Trace.Agent.MessagePack;
1313
using Datadog.Trace.Configuration;
14+
using Datadog.Trace.Configuration.Telemetry;
1415
using Datadog.Trace.DogStatsd;
1516
using Datadog.Trace.Sampling;
1617
using Datadog.Trace.TestHelpers;
@@ -427,7 +428,7 @@ public async Task AddsTraceKeepRateMetricToRootSpan()
427428

428429
var tracer = new Mock<IDatadogTracer>();
429430
tracer.Setup(x => x.DefaultServiceName).Returns("Default");
430-
tracer.Setup(x => x.PerTraceSettings).Returns(new PerTraceSettings(null, null, null!, MutableSettings.CreateWithoutDefaultSources(new(NullConfigurationSource.Instance))));
431+
tracer.Setup(x => x.PerTraceSettings).Returns(new PerTraceSettings(null, null, null!, MutableSettings.CreateWithoutDefaultSources(new(NullConfigurationSource.Instance), new ConfigurationTelemetry())));
431432
var traceContext = new TraceContext(tracer.Object);
432433
var rootSpanContext = new SpanContext(null, traceContext, null);
433434
var rootSpan = new Span(rootSpanContext, DateTimeOffset.UtcNow);

0 commit comments

Comments
 (0)