Minor IConfigurationSource clean up (#7327)

## Summary of changes

- Remove unused methods in `IConfigurationSource`
- Add a test for the recording of "error" telemetry in configuration
sources

## Reason for change

I'm working on some things for configuration, and needed to do a bit of
cleanup first. Nothing major here.

## Implementation details

- Delete unused `CompositeConfigurationSource.Insert()` method
- Delete unused `IConfigurationSource.IsPresent` method
- Add a test that we record "invalid" errors, and that the final
telemetry value is the "real" value

## Test coverage

Covered by existing + a new one

Author: andrewlock

tracer/src/Datadog.Trace/Configuration/ConfigurationSources/CompositeConfigurationSource.cs

@@ -10,11 +10,7 @@
 using System.Linq;
 using Datadog.Trace.Configuration.ConfigurationSources.Telemetry;
 using Datadog.Trace.Configuration.Telemetry;
-using Datadog.Trace.ExtensionMethods;
 using Datadog.Trace.SourceGenerators;
-using Datadog.Trace.Telemetry;
-using Datadog.Trace.Telemetry.Metrics;
-using Datadog.Trace.Util;
 
 namespace Datadog.Trace.Configuration
 {
@@ -41,34 +37,18 @@ public CompositeConfigurationSource(IEnumerable<IConfigurationSource> sources)
         /// <param name="source">The configuration source to add.</param>
         public void Add(IConfigurationSource source)
         {
-            if (source == null) { ThrowHelper.ThrowArgumentNullException(nameof(source)); }
+            if (source == null!) { ThrowHelper.ThrowArgumentNullException(nameof(source)); }
 
             _sources.Add(source);
         }
 
-        /// <summary>
-        /// Inserts an element into the <see cref="Datadog.Trace.Configuration.CompositeConfigurationSource"/> at the specified index.
-        /// </summary>
-        /// <param name="index">The zero-based index at which <paramref name="item"/> should be inserted.</param>
-        /// <param name="item">The configuration source to insert.</param>
-        public void Insert(int index, IConfigurationSource item)
-        {
-            if (item == null) { ThrowHelper.ThrowArgumentNullException(nameof(item)); }
-
-            _sources.Insert(index, item);
-        }
-
         /// <inheritdoc />
         IEnumerator<IConfigurationSource> IEnumerable<IConfigurationSource>.GetEnumerator() => _sources.GetEnumerator();
 
         /// <inheritdoc />
         [PublicApi]
         IEnumerator IEnumerable.GetEnumerator() => _sources.GetEnumerator();
 
-        /// <inheritdoc />
-        public bool IsPresent(string key)
-            => _sources.Select(source => source.IsPresent(key)).FirstOrDefault(value => value);
-
         /// <inheritdoc />
         public ConfigurationResult<string> GetString(string key, IConfigurationTelemetry telemetry, Func<string, bool>? validator, bool recordValue)
             => _sources

tracer/src/Datadog.Trace/Configuration/ConfigurationSources/DictionaryObjectConfigurationSource.cs

@@ -33,8 +33,6 @@ public DictionaryObjectConfigurationSource(IReadOnlyDictionary<string, object?>
     protected virtual bool TryGetValue(string key, out object? value)
         => Dictionary.TryGetValue(key, out value);
 
-    public bool IsPresent(string key) => Dictionary.ContainsKey(key);
-
     public ConfigurationResult<string> GetString(string key, IConfigurationTelemetry telemetry, Func<string, bool>? validator, bool recordValue)
     {
         if (TryGetValue(key, out var objValue) && objValue is not null)

tracer/src/Datadog.Trace/Configuration/ConfigurationSources/IConfigurationSource.cs

@@ -18,13 +18,6 @@ namespace Datadog.Trace.Configuration;
 /// </summary>
 public interface IConfigurationSource
 {
-    /// <summary>
-    /// Gets whether the specified key is present in the source.
-    /// </summary>
-    /// <param name="key">The key that identifies the setting.</param>
-    /// <returns><c>true</c> if the key is present in the source, false otherwise.</returns>
-    bool IsPresent(string key);
-
     /// <summary>
     /// Gets the <see cref="string"/> value of
     /// the setting with the specified key.

tracer/src/Datadog.Trace/Configuration/ConfigurationSources/JsonConfigurationSource.cs

@@ -96,14 +96,6 @@ internal static JsonConfigurationSource FromFile(string filename, ConfigurationO
                        : token.Value<T>();
         }
 
-        /// <inheritdoc />
-        public bool IsPresent(string key)
-        {
-            JToken? token = SelectToken(key);
-
-            return token is not null;
-        }
-
         /// <inheritdoc />
         public ConfigurationResult<string> GetString(string key, IConfigurationTelemetry telemetry, Func<string, bool>? validator, bool recordValue)
         {

tracer/src/Datadog.Trace/Configuration/ConfigurationSources/NullConfigurationSource.cs

@@ -16,8 +16,6 @@ internal class NullConfigurationSource : IConfigurationSource
 {
     public static readonly NullConfigurationSource Instance = new();
 
-    public bool IsPresent(string key) => false;
-
     public ConfigurationResult<string> GetString(string key, IConfigurationTelemetry telemetry, Func<string, bool>? validator, bool recordValue)
         => ConfigurationResult<string>.NotFound();
 

tracer/src/Datadog.Trace/Configuration/ConfigurationSources/StringConfigurationSource.cs

@@ -114,14 +114,6 @@ internal abstract class StringConfigurationSource : IConfigurationSource
 
         protected abstract string? GetString(string key);
 
-        /// <inheritdoc />
-        public bool IsPresent(string key)
-        {
-            var value = GetString(key);
-
-            return value is not null;
-        }
-
         /// <inheritdoc />
         public ConfigurationResult<string> GetString(string key, IConfigurationTelemetry telemetry, Func<string, bool>? validator, bool recordValue)
         {

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

@@ -66,6 +66,7 @@ private static bool TryHandleResult<T>(
         bool recordValue,
         Func<DefaultResult<T>>? getDefaultValue,
         [NotNullIfNotNull(nameof(getDefaultValue))] out T? value)
+        where T : notnull
     {
         if (result is { Result: { } ddResult, IsValid: true })
         {
@@ -85,11 +86,7 @@ private static bool TryHandleResult<T>(
         var defaultValue = getDefaultValue();
         RecordTelemetry(telemetry, key, recordValue, defaultValue);
 
-        // The compiler complains about this, because technically you _could_ call it as `TryHandleResult<int?>` (for example)
-        // in which case Func<DefaultResult<T>> _could_ return a `null` value, so the `[NotNullIfNotNull]` annotation
-        // would be wrong. In practice, we control the calls to this method, and we know that T is always non-null
-        // so it's safe to use the dammit here.
-        value = defaultValue.Result!;
+        value = defaultValue.Result;
         return true;
     }
 
@@ -186,6 +183,7 @@ public string AsString(string defaultValue, Func<string, bool>? validator)
         // We have to use different methods for class/struct when we _don't_ have a null value, because NRTs don't work properly otherwise
         [return: NotNullIfNotNull(nameof(getDefaultValue))]
         public T GetAs<T>(Func<DefaultResult<T>> getDefaultValue, Func<T, bool>? validator, Func<string, ParsingResult<T>> converter)
+            where T : notnull
         {
             var result = GetAs(validator, converter);
             return TryHandleResult(Telemetry, Key, result, recordValue: true, getDefaultValue, out var value)

tracer/test/Datadog.Trace.Tests/Configuration/CompositeConfigurationSourceTests.cs

@@ -5,7 +5,9 @@
 
 using System.Collections.Generic;
 using System.Collections.Specialized;
+using System.Linq;
 using Datadog.Trace.Configuration;
+using Datadog.Trace.Configuration.ConfigurationSources.Telemetry;
 using Datadog.Trace.Configuration.Telemetry;
 using Datadog.Trace.Telemetry;
 using FluentAssertions;
@@ -137,7 +139,7 @@ public void AttemptsToGrabStringFromEverySource(string key)
     {
         var telemetry = new StubTelemetry();
         var actual = _source.GetString(key, telemetry, validator: null, recordValue: true);
-        telemetry.Accesses[key].Should().Be(1);
+        telemetry.GetInstanceCount(key).Should().Be(1);
     }
 
     [Theory]
@@ -160,7 +162,7 @@ public void AttemptsToGrabIntFromEverySource(string key)
     {
         var telemetry = new StubTelemetry();
         var actual = _source.GetInt32(key, telemetry, validator: null);
-        telemetry.Accesses[key].Should().Be(1);
+        telemetry.GetInstanceCount(key).Should().Be(1);
     }
 
     [Theory]
@@ -183,7 +185,7 @@ public void AttemptsToGrabDoubleFromEverySource(string key)
     {
         var telemetry = new StubTelemetry();
         var actual = _source.GetDouble(key, telemetry, validator: null);
-        telemetry.Accesses[key].Should().Be(1);
+        telemetry.GetInstanceCount(key).Should().Be(1);
     }
 
     [Theory]
@@ -206,7 +208,7 @@ public void AttemptsToGrabBoolFromEverySource(string key)
     {
         var telemetry = new StubTelemetry();
         var actual = _source.GetBool(key, telemetry, validator: null);
-        telemetry.Accesses[key].Should().Be(1);
+        telemetry.GetInstanceCount(key).Should().Be(1);
     }
 
     [Theory]
@@ -229,30 +231,105 @@ public void AttemptsToGrabDictionaryFromEverySource(string key)
     {
         var telemetry = new StubTelemetry();
         var actual = _source.GetDictionary(key, telemetry, validator: null);
-        telemetry.Accesses[key].Should().Be(1);
+        telemetry.GetInstanceCount(key).Should().Be(1);
+    }
+
+    [Fact]
+    public void Telemetry_WhenMissingDoesNotRecordTelemetry()
+    {
+        var telemetry = new StubTelemetry();
+        const string key = "int_value";
+        var source = new CompositeConfigurationSource()
+        {
+            new NameValueConfigurationSource(new(), ConfigurationOrigins.Calculated),
+            new NameValueConfigurationSource(new() { { "something_else", "456" } }, ConfigurationOrigins.EnvVars),
+            new NameValueConfigurationSource(new(), ConfigurationOrigins.AppConfig),
+        };
+
+        // not present
+        var actual = source.GetInt32(key, telemetry, validator: null);
+        actual.Should().Be(ConfigurationResult<int>.NotFound());
+
+        // final telemetry value should be the "real" value
+        telemetry.Telemetry.Should().BeEmpty();
+    }
+
+    [Fact]
+    public void Telemetry_WhenErrorRecordsTelemetry()
+    {
+        var telemetry = new StubTelemetry();
+        const string key = "int_value";
+        var source = new CompositeConfigurationSource()
+        {
+            new NameValueConfigurationSource(new(), ConfigurationOrigins.Calculated),
+            new NameValueConfigurationSource(new() { { key, "not_an_int" } }, ConfigurationOrigins.DdConfig),
+            new NameValueConfigurationSource(new(), ConfigurationOrigins.AppConfig),
+        };
+
+        // no valid value
+        var actual = source.GetInt32(key, telemetry, validator: null);
+        actual.Should().Be(ConfigurationResult<int>.NotFound());
+
+        // only telemetry value should be the error
+        telemetry.Telemetry.Should()
+                 .ContainSingle()
+                 .Which.Should()
+                 .BeEquivalentTo(new ConfigurationTelemetryTests.ConfigDto(key, "not_an_int", ConfigurationOrigins.DdConfig, true, TelemetryErrorCode.ParsingInt32Error));
+    }
+
+    [Fact]
+    public void RecordsTelemetry_WhenPresentInMultipleSources()
+    {
+        var telemetry = new StubTelemetry();
+        const string key = "int_value";
+        var source = new CompositeConfigurationSource()
+        {
+            new NameValueConfigurationSource(new(), ConfigurationOrigins.Calculated),
+            new NameValueConfigurationSource(new() { { key, "not_an_int" } }, ConfigurationOrigins.DdConfig),
+            new NameValueConfigurationSource(new() { { key, "123" } }, ConfigurationOrigins.Code),
+            new NameValueConfigurationSource(new(), ConfigurationOrigins.AppConfig),
+            new NameValueConfigurationSource(new() { { key, "not_an_int" } }, ConfigurationOrigins.RemoteConfig),
+            new NameValueConfigurationSource(new() { { key, "456" } }, ConfigurationOrigins.EnvVars),
+        };
+
+        // first wins
+        var expected = 123;
+        var actual = source.GetInt32(key, telemetry, validator: null);
+        actual.Should().Be(ConfigurationResult<int>.Valid(expected));
+
+        // final telemetry value should be the "real" value
+        telemetry.Telemetry.Last().Value.Should().Be(expected);
+
+        // telemetry records the first error, and then the successful value
+        telemetry.Telemetry.Should()
+                 .BeEquivalentTo(
+                  [
+                      new ConfigurationTelemetryTests.ConfigDto(key, "not_an_int", ConfigurationOrigins.DdConfig, true, TelemetryErrorCode.ParsingInt32Error),
+                      new ConfigurationTelemetryTests.ConfigDto(key, 123, ConfigurationOrigins.Code, true, null),
+                  ]);
     }
 
     internal class StubTelemetry : IConfigurationTelemetry
     {
-        public Dictionary<string, int> Accesses { get; } = new();
+        public List<ConfigurationTelemetryTests.ConfigDto> Telemetry { get; } = new();
 
         public void Record(string key, string value, bool recordValue, ConfigurationOrigins origin, TelemetryErrorCode? error = null)
-            => IncrementAccess(key);
+            => Telemetry.Add(new(key, value, origin, recordValue, error));
 
         public void Record(string key, bool value, ConfigurationOrigins origin, TelemetryErrorCode? error = null)
-            => IncrementAccess(key);
+            => Telemetry.Add(new(key, value, origin, recordValue: true, error));
 
         public void Record(string key, double value, ConfigurationOrigins origin, TelemetryErrorCode? error = null)
-            => IncrementAccess(key);
+            => Telemetry.Add(new(key, value, origin, recordValue: true, error));
 
         public void Record(string key, int value, ConfigurationOrigins origin, TelemetryErrorCode? error = null)
-            => IncrementAccess(key);
+            => Telemetry.Add(new(key, value, origin, recordValue: true, error));
 
         public void Record(string key, double? value, ConfigurationOrigins origin, TelemetryErrorCode? error = null)
-            => IncrementAccess(key);
+            => Telemetry.Add(new(key, value, origin, recordValue: true, error));
 
         public void Record(string key, int? value, ConfigurationOrigins origin, TelemetryErrorCode? error = null)
-            => IncrementAccess(key);
+            => Telemetry.Add(new(key, value, origin, recordValue: true, error));
 
         public ICollection<ConfigurationKeyValue> GetData() => null;
 
@@ -264,16 +341,7 @@ public void SetErrorOnCurrentEntry(string key, TelemetryErrorCode error)
         {
         }
 
-        private void IncrementAccess(string key)
-        {
-            if (Accesses.TryGetValue(key, out var i))
-            {
-                Accesses[key] = i + 1;
-            }
-            else
-            {
-                Accesses[key] = 1;
-            }
-        }
+        public int GetInstanceCount(string key)
+            => Telemetry.Count(x => x.Name == key);
     }
 }