Skip to content

Commit 2fbbd7f

Browse files
andrewlockandrewlock
committed
Add fix for not re-reporting telemetry when there are "empty" dynamic config changes
1 parent 11c23d3 commit 2fbbd7f

File tree

8 files changed

+224
-83
lines changed

8 files changed

+224
-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: 72 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -7,41 +7,56 @@
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 = [];
2725

2826
private IConfigurationSource _dynamicConfigurationSource = NullConfigurationSource.Instance;
2927
private ManualInstrumentationConfigurationSourceBase _manualConfigurationSource =
3028
new ManualInstrumentationConfigurationSource(new Dictionary<string, object?>(), useDefaultSources: true);
3129

30+
// We delay creating these, as we likely won't need them
31+
private ConfigurationTelemetry? _noDefaultSettingsTelemetry;
32+
private MutableSettings? _noDefaultSourcesSettings;
33+
3234
private SettingChanges? _latest;
3335

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

4055
/// <summary>
4156
/// Gets the initial <see cref="ExporterSettings"/>. On app startup, these will be the values read from
4257
/// static sources. To subscribe to updates to these settings, from code or remote config, call <see cref="SubscribeToChanges"/>.
4358
/// </summary>
44-
public ExporterSettings InitialExporterSettings { get; } = initialExporter;
59+
public ExporterSettings InitialExporterSettings { get; }
4560

4661
/// <summary>
4762
/// Subscribe to changes in <see cref="MutableSettings"/> and/or <see cref="ExporterSettings"/>.
@@ -133,21 +148,45 @@ private bool UpdateSettings(
133148
ManualInstrumentationConfigurationSourceBase manualSource,
134149
IConfigurationTelemetry telemetry)
135150
{
136-
var initialSettings = manualSource.UseDefaultSources
137-
? InitialMutableSettings
138-
: MutableSettings.CreateWithoutDefaultSources(_tracerSettings);
151+
// Set the correct default telemetry and initial settings depending
152+
// on whether the manual config source explicitly disables using the default sources
153+
ConfigurationTelemetry defaultTelemetry;
154+
MutableSettings initialSettings;
155+
if (manualSource.UseDefaultSources)
156+
{
157+
defaultTelemetry = _initialTelemetry;
158+
initialSettings = InitialMutableSettings;
159+
}
160+
else
161+
{
162+
// We only need to initialize the "no default sources" settings once
163+
// and we don't want to initialize them if we don't _need_ to
164+
// so lazy-initialize here
165+
if (_noDefaultSourcesSettings is null || _noDefaultSettingsTelemetry is null)
166+
{
167+
InitialiseNoDefaultSourceSettings();
168+
}
169+
170+
defaultTelemetry = _noDefaultSettingsTelemetry;
171+
initialSettings = _noDefaultSourcesSettings;
172+
}
139173

140174
var current = Volatile.Read(ref _latest);
141175
var currentMutable = current?.UpdatedMutable ?? current?.PreviousMutable ?? InitialMutableSettings;
142176
var currentExporter = current?.UpdatedExporter ?? current?.PreviousExporter ?? InitialExporterSettings;
143177

178+
// we create a temporary ConfigurationTelemetry object to hold the changes to settings
179+
// if nothing is actually written, and nothing changes compared to the default, then we
180+
// don't need to report it to the provided telemetry
181+
var tempTelemetry = new ConfigurationTelemetry();
182+
144183
var overrideErrorLog = new OverrideErrorLog();
145184
var newMutableSettings = MutableSettings.CreateUpdatedMutableSettings(
146185
dynamicConfigSource,
147186
manualSource,
148187
initialSettings,
149188
_tracerSettings,
150-
telemetry,
189+
tempTelemetry,
151190
overrideErrorLog); // TODO: We'll later report these
152191

153192
// The only exporter setting we currently _allow_ to change is the AgentUri, but if that does change,
@@ -159,7 +198,7 @@ private bool UpdateSettings(
159198
var newRawExporterSettings = ExporterSettings.Raw.CreateUpdatedFromManualConfig(
160199
currentExporter.RawSettings,
161200
manualSource,
162-
telemetry,
201+
tempTelemetry,
163202
manualSource.UseDefaultSources);
164203

165204
var isSameMutableSettings = currentMutable.Equals(newMutableSettings);
@@ -171,13 +210,33 @@ private bool UpdateSettings(
171210
return null;
172211
}
173212

213+
// we have changes, so we need to report them
214+
// First record the "default"/fallback values, then record the "new" values
215+
defaultTelemetry.CopyTo(telemetry);
216+
tempTelemetry.CopyTo(telemetry);
217+
174218
Log.Information("Notifying consumers of new settings");
175219
var updatedMutableSettings = isSameMutableSettings ? null : newMutableSettings;
176220
var updatedExporterSettings = isSameExporterSettings ? null : new ExporterSettings(newRawExporterSettings, telemetry);
177221

178222
return new SettingChanges(updatedMutableSettings, updatedExporterSettings, currentMutable, currentExporter);
179223
}
180224

225+
[MemberNotNull(nameof(_noDefaultSettingsTelemetry))]
226+
[MemberNotNull(nameof(_noDefaultSourcesSettings))]
227+
private void InitialiseNoDefaultSourceSettings()
228+
{
229+
if (_noDefaultSourcesSettings is not null
230+
&& _noDefaultSettingsTelemetry is not null)
231+
{
232+
return;
233+
}
234+
235+
var telemetry = new ConfigurationTelemetry();
236+
_noDefaultSettingsTelemetry = telemetry;
237+
_noDefaultSourcesSettings = MutableSettings.CreateWithoutDefaultSources(_tracerSettings, telemetry);
238+
}
239+
181240
private void NotifySubscribers(SettingChanges settings)
182241
{
183242
// 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
@@ -125,8 +125,6 @@ internal TracerSettings(IConfigurationSource? source, IConfigurationTelemetry te
125125
.AsBoolResult()
126126
.OverrideWith(in otelActivityListenerEnabled, ErrorLog, defaultValue: false);
127127

128-
var exporter = new ExporterSettings(source, _telemetry);
129-
130128
PeerServiceTagsEnabled = config
131129
.WithKeys(ConfigurationKeys.PeerServiceDefaultsEnabled)
132130
.AsBool(defaultValue: false);
@@ -335,66 +333,6 @@ not null when string.Equals(value, "otlp", StringComparison.OrdinalIgnoreCase) =
335333

336334
OpenTelemetryLogsEnabled = OpenTelemetryLogsEnabled && OtelLogsExporterEnabled;
337335

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

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

739740
internal bool IsRunningInCiVisibility { get; }

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

Lines changed: 2 additions & 0 deletions
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

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using System;
77
using Datadog.Trace.Configuration;
88
using Datadog.Trace.Configuration.Schema;
9+
using Datadog.Trace.Configuration.Telemetry;
910
using Datadog.Trace.Sampling;
1011
using Moq;
1112

@@ -23,7 +24,7 @@ public EmptyDatadogTracer()
2324
DefaultServiceName = "My Service Name";
2425
Settings = new TracerSettings(NullConfigurationSource.Instance);
2526
var namingSchema = new NamingSchema(SchemaVersion.V0, false, false, DefaultServiceName, null, null);
26-
PerTraceSettings = new PerTraceSettings(null, null, namingSchema, MutableSettings.CreateWithoutDefaultSources(Settings));
27+
PerTraceSettings = new PerTraceSettings(null, null, namingSchema, MutableSettings.CreateWithoutDefaultSources(Settings, new ConfigurationTelemetry()));
2728
}
2829

2930
public string DefaultServiceName { get; }

0 commit comments

Comments
 (0)