Skip to content

Commit ce13f33

Browse files
andrewlockandrewlock
committed
"Fix" ordering of recorded telemetry
1 parent 3e35d3a commit ce13f33

File tree

2 files changed

+62
-15
lines changed

2 files changed

+62
-15
lines changed

‎tracer/src/Datadog.Trace/Configuration/ConfigurationSources/Telemetry/ConfigurationBuilder.cs

Lines changed: 43 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -120,24 +120,31 @@ public string AsString(string defaultValue, Func<string, bool>? validator)
120120
[return: NotNullIfNotNull(nameof(defaultValue))]
121121
private string? AsString(string? defaultValue, Func<string, bool>? validator, bool recordValue)
122122
{
123+
// pre-record the default value, so it's in the "correct" place in the stack
124+
if (defaultValue is not null)
125+
{
126+
Telemetry.Record(Key, defaultValue, recordValue, ConfigurationOrigins.Default);
127+
}
128+
123129
var result = GetStringResult(validator, converter: null, recordValue);
124130
if (result is { Result: { } ddResult, IsValid: true })
125131
{
126132
return ddResult;
127133
}
128134

129-
if (defaultValue is null)
135+
if (defaultValue is not null && result.IsPresent)
130136
{
131-
return null;
137+
// re-record telemetry because we found an invalid value in sources which clobbered it
138+
Telemetry.Record(Key, defaultValue, recordValue, ConfigurationOrigins.Default);
132139
}
133140

134-
Telemetry.Record(Key, defaultValue, recordValue, ConfigurationOrigins.Default);
135141
return defaultValue;
136142
}
137143

138144
[return: NotNullIfNotNull(nameof(getDefaultValue))]
139145
private string? AsString(Func<string>? getDefaultValue, Func<string, bool>? validator, Func<string, ParsingResult<string>>? converter, bool recordValue)
140146
{
147+
// We don't "pre-record" the default because it's expensive to create
141148
var result = GetStringResult(validator, converter, recordValue);
142149
if (result is { Result: { } ddResult, IsValid: true })
143150
{
@@ -161,6 +168,10 @@ public string AsString(string defaultValue, Func<string, bool>? validator)
161168
public T GetAs<T>(DefaultResult<T> defaultValue, Func<T, bool>? validator, Func<string, ParsingResult<T>> converter)
162169
where T : notnull
163170
{
171+
// Ideally we would like to pre-record the default telemetry here so it's in the correct place
172+
// in the stack, but the GetAs<T> behaviour of the JsonConfigurationSource is problematic, as it
173+
// adds a telemetry result but still returns NotFound, so we can't use NotFound as the indicator
174+
// of whether we need to re-record the telemetry or not
164175
var result = GetAs(validator, converter);
165176
if (result is { Result: { } ddResult, IsValid: true })
166177
{
@@ -174,6 +185,7 @@ public T GetAs<T>(DefaultResult<T> defaultValue, Func<T, bool>? validator, Func<
174185
public T GetAs<T>(Func<DefaultResult<T>> getDefaultValue, Func<T, bool>? validator, Func<string, ParsingResult<T>> converter)
175186
where T : notnull
176187
{
188+
// We don't "pre-record" the default because it's expensive to create
177189
var result = GetAs(validator, converter);
178190
if (result is { Result: { } ddResult, IsValid: true })
179191
{
@@ -218,24 +230,30 @@ public bool AsBool(bool defaultValue, Func<bool, bool>? validator)
218230
[return: NotNullIfNotNull(nameof(defaultValue))]
219231
public bool? AsBool(bool? defaultValue, Func<bool, bool>? validator, Func<string, ParsingResult<bool>>? converter)
220232
{
233+
// pre-record the default value, so it's in the "correct" place in the stack
234+
if (defaultValue.HasValue)
235+
{
236+
Telemetry.Record(Key, defaultValue.Value, ConfigurationOrigins.Default);
237+
}
238+
221239
var result = GetBoolResult(validator, converter: null);
222240
if (result is { Result: { } ddResult, IsValid: true })
223241
{
224242
return ddResult;
225243
}
226244

227-
if (defaultValue is not { } value)
245+
if (defaultValue is { } value && result.IsPresent)
228246
{
229-
return null;
247+
Telemetry.Record(Key, value, ConfigurationOrigins.Default);
230248
}
231249

232-
Telemetry.Record(Key, value, ConfigurationOrigins.Default);
233-
return value;
250+
return defaultValue;
234251
}
235252

236253
[return: NotNullIfNotNull(nameof(getDefaultValue))] // This doesn't work with nullables, but it still expresses intent
237254
public bool? AsBool(Func<bool>? getDefaultValue, Func<bool, bool>? validator, Func<string, ParsingResult<bool>>? converter)
238255
{
256+
// We don't "pre-record" the default because it's expensive to create
239257
var result = GetBoolResult(validator, converter);
240258
if (result is { Result: { } ddResult, IsValid: true })
241259
{
@@ -268,19 +286,24 @@ public bool AsBool(bool defaultValue, Func<bool, bool>? validator)
268286
[return: NotNullIfNotNull(nameof(defaultValue))] // This doesn't work with nullables, but it still expresses intent
269287
public int? AsInt32(int? defaultValue, Func<int, bool>? validator, Func<string, ParsingResult<int>>? converter)
270288
{
289+
// pre-record the default value, so it's in the "correct" place in the stack
290+
if (defaultValue.HasValue)
291+
{
292+
Telemetry.Record(Key, defaultValue.Value, ConfigurationOrigins.Default);
293+
}
294+
271295
var result = GetInt32Result(validator, converter);
272296
if (result is { Result: { } ddResult, IsValid: true })
273297
{
274298
return ddResult;
275299
}
276300

277-
if (defaultValue is not { } value)
301+
if (defaultValue is { } value && result.IsPresent)
278302
{
279-
return null;
303+
Telemetry.Record(Key, value, ConfigurationOrigins.Default);
280304
}
281305

282-
Telemetry.Record(Key, value, ConfigurationOrigins.Default);
283-
return value;
306+
return defaultValue;
284307
}
285308

286309
// ****************
@@ -299,19 +322,24 @@ public bool AsBool(bool defaultValue, Func<bool, bool>? validator)
299322
[return: NotNullIfNotNull(nameof(defaultValue))]
300323
public double? AsDouble(double? defaultValue, Func<double, bool>? validator, Func<string, ParsingResult<double>>? converter)
301324
{
325+
// pre-record the default value, so it's in the "correct" place in the stack
326+
if (defaultValue.HasValue)
327+
{
328+
Telemetry.Record(Key, defaultValue.Value, ConfigurationOrigins.Default);
329+
}
330+
302331
var result = GetDoubleResult(validator, converter);
303332
if (result is { Result: { } ddResult, IsValid: true })
304333
{
305334
return ddResult;
306335
}
307336

308-
if (defaultValue is not { } value)
337+
if (defaultValue is { } value && result.IsPresent)
309338
{
310-
return null;
339+
Telemetry.Record(Key, value, ConfigurationOrigins.Default);
311340
}
312341

313-
Telemetry.Record(Key, value, ConfigurationOrigins.Default);
314-
return value;
342+
return defaultValue;
315343
}
316344

317345
// ****************

‎tracer/test/Datadog.Trace.Tests/Configuration/Telemetry/ConfigurationBuilderTests.cs

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -158,11 +158,13 @@ public void RecordsTelemetryCorrectly(object value)
158158
{
159159
"" => new List<Entry>
160160
{
161+
Entry.String(Key, Default, ConfigurationOrigins.Default, error: null),
161162
Entry.String(Key, string.Empty, ConfigurationOrigins.Code, error: TelemetryErrorCode.FailedValidation),
162163
Entry.String(Key, Default, ConfigurationOrigins.Default, error: null),
163164
},
164165
string s => new List<Entry>
165166
{
167+
Entry.String(Key, Default, ConfigurationOrigins.Default, error: null),
166168
Entry.String(Key, s, ConfigurationOrigins.Code, error: null),
167169
},
168170
null => new()
@@ -171,6 +173,7 @@ public void RecordsTelemetryCorrectly(object value)
171173
},
172174
{ } i => new List<Entry>
173175
{
176+
Entry.String(Key, Default, ConfigurationOrigins.Default, error: null),
174177
Entry.String(Key, Convert.ToString(i, CultureInfo.InvariantCulture), ConfigurationOrigins.Code, error: null),
175178
},
176179
};
@@ -210,10 +213,12 @@ public void RecordsTelemetryCorrectly(object value)
210213
{
211214
true or "True" or "true" => new List<Entry>()
212215
{
216+
Entry.Bool(Key, true, ConfigurationOrigins.Default, error: null),
213217
Entry.Bool(Key, true, ConfigurationOrigins.Code, error: null),
214218
},
215219
false or "False" or "false" => new()
216220
{
221+
Entry.Bool(Key, true, ConfigurationOrigins.Default, error: null),
217222
Entry.Bool(Key, false, ConfigurationOrigins.Code, TelemetryErrorCode.FailedValidation),
218223
Entry.Bool(Key, true, ConfigurationOrigins.Default, error: null),
219224
},
@@ -223,6 +228,7 @@ public void RecordsTelemetryCorrectly(object value)
223228
},
224229
string x1 => new()
225230
{
231+
Entry.Bool(Key, true, ConfigurationOrigins.Default, error: null),
226232
Entry.String(Key, x1, ConfigurationOrigins.Code, TelemetryErrorCode.JsonBooleanError),
227233
},
228234
_ => throw new InvalidOperationException("Unexpected value " + value),
@@ -258,23 +264,28 @@ public void RecordsTelemetryCorrectly(object value)
258264
{
259265
int i and > 0 => new List<Entry>
260266
{
267+
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
261268
Entry.Number(Key, i, ConfigurationOrigins.Code, error: null),
262269
},
263270
"123" => new List<Entry> // Note the implicit conversion, but not for 23.3!
264271
{
272+
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
265273
Entry.Number(Key, 123, ConfigurationOrigins.Code, error: null),
266274
},
267275
int i => new List<Entry>
268276
{
277+
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
269278
Entry.Number(Key, i, ConfigurationOrigins.Code, error: TelemetryErrorCode.FailedValidation),
270279
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
271280
},
272281
double d and > 0 => new List<Entry> // Note the implicit conversion!
273282
{
283+
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
274284
Entry.Number(Key, (int)d, ConfigurationOrigins.Code, error: null),
275285
},
276286
double d => new List<Entry> // Note the implicit conversion!
277287
{
288+
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
278289
Entry.Number(Key, (int)d, ConfigurationOrigins.Code, error: TelemetryErrorCode.FailedValidation),
279290
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
280291
},
@@ -284,6 +295,7 @@ public void RecordsTelemetryCorrectly(object value)
284295
},
285296
string x1 => new()
286297
{
298+
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
287299
Entry.String(Key, x1, ConfigurationOrigins.Code, TelemetryErrorCode.JsonInt32Error),
288300
},
289301
_ => throw new InvalidOperationException("Unexpected value " + value),
@@ -321,28 +333,34 @@ public void RecordsTelemetryCorrectly(object value)
321333
{
322334
int i and > 0 => new List<Entry>
323335
{
336+
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
324337
Entry.Number(Key, (double)i, ConfigurationOrigins.Code, error: null),
325338
},
326339
int i => new List<Entry>
327340
{
341+
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
328342
Entry.Number(Key, (double)i, ConfigurationOrigins.Code, error: TelemetryErrorCode.FailedValidation),
329343
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
330344
},
331345
double d and > 0 => new List<Entry>
332346
{
347+
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
333348
Entry.Number(Key, d, ConfigurationOrigins.Code, error: null),
334349
},
335350
double d => new List<Entry> // Note the implicit conversion!
336351
{
352+
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
337353
Entry.Number(Key, d, ConfigurationOrigins.Code, error: TelemetryErrorCode.FailedValidation),
338354
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
339355
},
340356
string s when TryParse(s, out var d) && d > 0 => new List<Entry>
341357
{
358+
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
342359
Entry.Number(Key, d, ConfigurationOrigins.Code, error: null),
343360
},
344361
string s when TryParse(s, out var d) => new List<Entry>
345362
{
363+
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
346364
Entry.Number(Key, d, ConfigurationOrigins.Code, error: TelemetryErrorCode.FailedValidation),
347365
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
348366
},
@@ -352,6 +370,7 @@ public void RecordsTelemetryCorrectly(object value)
352370
},
353371
string x => new()
354372
{
373+
Entry.Number(Key, Default, ConfigurationOrigins.Default, error: null),
355374
Entry.String(Key, x, ConfigurationOrigins.Code, TelemetryErrorCode.JsonDoubleError),
356375
},
357376
_ => throw new InvalidOperationException("Unexpected value " + value),

0 commit comments

Comments
 (0)