Improve Promitor's metric configuration validation (#893)

* Initial implementation of new validation approach

I've altered the way that the deserializers work and are defined so that they are simpler to create, but also so that we can record validation failures during the deserialization process. This allows us to provide much better validation messages than we can if we take the approach of validating based on the deserialized objects.

In this initial change I haven't gone ahead and updated all of the deserializers because I wanted the change to be kept small. I've also not actually hooked up the validation messages in any way - instead I've just provided a stub ErrorReporter object that doesn't actually do anything yet.

Part of #592

* Converted AzureMetricConfigurationDeserializer

Converted AzureMetricConfigurationDeserializer to the new way of defining a deserializer, and added support in the base Deserializer for using another deserializer to deserialize child properties.

I had to add a new `IDeserializer` interface that isn't generic to allow us to deserialize a child property at runtime without knowing its type.

Part of #592

* Converted MetricAggregationDeserializer

Part of #592

* Converted MetricDefaultsDeserializer

Part of #592

* Converted MetricDefinitionDeserializer

Also:
- Fixed the metrics definition docs to indicate that 'labels' is optional.

Part of #592

* Converted ScrapingDeserializer

Part of #592

* Converted SecretDeserializer

Part of #592

* Converted all of the resource type deserializers

In order to do this I had to alter the type parameter in `IDeserializer<TObject>` to be covariant so that I could define the resource deserializers as their specific type (for example `IDeserializer<ContainerInstanceResourceV1>`), but still allow them to be returned as an `IDeserializer<AzureResourceDefinitionV1>` from the resource deserializer factory.

I also ended up altering a few of the newer deserializers that are using inheritance (for example, SqlServer / SqlDatabase deserializers) to inherit directly from `ResourceDeserializer<TResource>` again. The reason I did this is that the `Deserializer` class needs to know the specific type of object to construct, and I decided that the extra complication of adding more generics to get it to work wasn't worth the hassle to save one or two lines of code.

Part of #592

* Hook up the new validation to Promitor

This change implements the ErrorReporter, and connects it to Promitor's startup validation so that the errors are actually reported.

In addition:

- Report error when a deserializer cannot be found.
- Ignore the 'resources' field on metrics because we are manually deserializing it.
- Make `AzureMetricConfiguration.Dimension` optional.
- Fix a test in `ServiceBusQueueMetricsDeclarationValidationStepTests` that was asserting that validation was successful, when the test was meant to test for a validation failure.

Part of #592

* Don't reuse context when deserializing

Because the deserializers are singletons, the set of FieldContext objects was being reused when deserializing multiple nodes of the same type. This lead to a bug where if we had multiple metric definitions, and the first one to be deserialized set a required field, an error would not be reported if that required field was missing from subsequent definitions.

To solve this I've pulled the FieldContext class out of the Deserializer, and I've split it into two separate objects:

- `FieldDeserializationInfo` - contains immutable information about the field to be deserialized, and can be reused between multiple deserializations.
- `FieldDeserializationContext` - contains any information about a specific deserialization attempt, and must be created each time we deserialize a node using a deserializer.

In addition, I've also altered the way the errors / warnings are output to avoid issues I was seeing where the messages weren't always output in the correct order because of the way the logger works.

Part of #592

* Add validation improvements to changelog

Part of #592

* Refactor the validation error tests

I've added some new methods to the `YamlAssert` class to make it easier to test that errors are reported when required properties are not supplied.

I've also removed some TODOs for functionality that can be added later after this first set of validation changes are merged.

Part of #592

* Fixing code-quality issues

* fixup! Fixing code-quality issues

changelog/content/experimental/unreleased.md

@@ -7,3 +7,4 @@ version:
 
 - {{% tag added %}} Support Prometheus service discovery in Helm chart ([#861](https://github.com/tomkerkhove/promitor/issues/861))
 - {{% tag added %}} Capability to gain insights on Azure Monitor integration ([docs](http://promitor.io/operations/#azure-monitor-integration) | [#848](https://github.com/tomkerkhove/promitor/issues/848))
+- {{% tag added %}} Improve metrics configuration validation ([#592](https://github.com/tomkerkhove/promitor/issues/592))

docs/configuration/v1.x/metrics/index.md

@@ -38,7 +38,6 @@ Every metric that is being declared needs to define the following fields:
 - `description` - Description for the metric that will be exposed in the scrape
   endpoint for Prometheus.
 - `resourceType` - Defines what type of resource needs to be queried.
-- `labels` - Defines a set of custom labels to include for a given metric.
 - `azureMetricConfiguration.metricName` - The name of the metric in Azure Monitor
   to query
 - `azureMetricConfiguration.aggregation.type` - The aggregation that needs to be
@@ -57,6 +56,7 @@ Additionally, the following fields are optional:
   - ☝ *Promitor simply acts as a proxy and will not validate if it's supported or
      not, we recommend verifying that the dimension is supported in the
      [official documentation](https://docs.microsoft.com/en-us/azure/azure-monitor/platform/metrics-supported)*
+- `labels` - Defines a set of custom labels to include for a given metric.
 - `scraping.schedule` - A scraping schedule for the individual metric; overrides
   the the one specified in `metricDefaults`
 

src/Promitor.Core.Scraping/Configuration/Providers/Interfaces/IMetricsDeclarationProvider.cs

@@ -1,4 +1,5 @@
 using Promitor.Core.Scraping.Configuration.Model;
+using Promitor.Core.Scraping.Configuration.Serialization;
 
 namespace Promitor.Core.Scraping.Configuration.Providers.Interfaces
 {
@@ -9,7 +10,8 @@ public interface IMetricsDeclarationProvider
         /// </summary>
         /// <param name="applyDefaults"><c>true</c> if the provider should apply default values from top-level 
         /// configuration elements to metrics where those values aren't specified. <c>false</c> otherwise</param>
-        MetricsDeclaration Get(bool applyDefaults = false);
+        /// <param name="errorReporter">Used to report errors during the deserialization process.</param>
+        MetricsDeclaration Get(bool applyDefaults = false, IErrorReporter errorReporter = null);
 
         /// <summary>
         ///     Gets the serialized metrics declaration

src/Promitor.Core.Scraping/Configuration/Providers/MetricsDeclarationProvider.cs

@@ -19,11 +19,12 @@ public MetricsDeclarationProvider(IConfiguration configuration, ConfigurationSer
             _configuration = configuration;
         }
 
-        public virtual MetricsDeclaration Get(bool applyDefaults = false)
+        public virtual MetricsDeclaration Get(bool applyDefaults = false, IErrorReporter errorReporter = null)
         {
             var rawMetricsDeclaration = ReadRawDeclaration();
+            errorReporter ??= new ErrorReporter();
 
-            var config = _configurationSerializer.Deserialize(rawMetricsDeclaration);
+            var config = _configurationSerializer.Deserialize(rawMetricsDeclaration, errorReporter);
 
             if (applyDefaults)
             {

src/Promitor.Core.Scraping/Configuration/Serialization/ConfigurationSerializer.cs

@@ -26,17 +26,18 @@ public ConfigurationSerializer(ILogger<ConfigurationSerializer> logger, IMapper
             _v1Deserializer = v1Deserializer;
         }
 
-        public MetricsDeclaration Deserialize(string rawMetricsDeclaration)
+        public MetricsDeclaration Deserialize(string rawMetricsDeclaration, IErrorReporter errorReporter)
         {
             Guard.NotNullOrWhitespace(rawMetricsDeclaration, nameof(rawMetricsDeclaration));
+            Guard.NotNull(errorReporter, nameof(errorReporter));
 
             var input = new StringReader(rawMetricsDeclaration);
             try
             {
                 var metricsDeclarationYamlStream = new YamlStream();
                 metricsDeclarationYamlStream.Load(input);
 
-                var metricsDeclaration = InterpretYamlStream(metricsDeclarationYamlStream);
+                var metricsDeclaration = InterpretYamlStream(metricsDeclarationYamlStream, errorReporter);
 
                 return metricsDeclaration;
             }
@@ -46,7 +47,7 @@ public MetricsDeclaration Deserialize(string rawMetricsDeclaration)
             }
         }
 
-        private MetricsDeclaration InterpretYamlStream(YamlStream metricsDeclarationYamlStream)
+        private MetricsDeclaration InterpretYamlStream(YamlStream metricsDeclarationYamlStream, IErrorReporter errorReporter)
         {
             var document = metricsDeclarationYamlStream.Documents.First();
             var rootNode = (YamlMappingNode)document.RootNode;
@@ -57,7 +58,7 @@ private MetricsDeclaration InterpretYamlStream(YamlStream metricsDeclarationYaml
             switch (specVersion)
             {
                 case SpecVersion.v1:
-                    var v1Config = _v1Deserializer.Deserialize(rootNode);
+                    var v1Config = _v1Deserializer.Deserialize(rootNode, errorReporter);
 
                     return _mapper.Map<MetricsDeclaration>(v1Config);
                 default:

src/Promitor.Core.Scraping/Configuration/Serialization/DeserializationContext.cs

@@ -0,0 +1,61 @@
+using System.Collections.Generic;
+using System.Linq;
+
+namespace Promitor.Core.Scraping.Configuration.Serialization
+{
+    /// <summary>
+    /// Contains the information needed to deserialize a node.
+    /// </summary>
+    public class DeserializationContext<TObject>
+    {
+        private readonly ISet<string> _ignoredFields;
+        private readonly Dictionary<string, FieldDeserializationContext<TObject>> _fields;
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="DeserializationContext{TObject}"/> type.
+        /// </summary>
+        /// <param name="ignoredFields">The fields that should be ignored.</param>
+        /// <param name="fields">The fields that can be deserialized.</param>
+        public DeserializationContext(ISet<string> ignoredFields, IReadOnlyCollection<FieldDeserializationInfo> fields)
+        {
+            _ignoredFields = ignoredFields;
+            _fields = fields.ToDictionary(
+                fieldInfo => fieldInfo.YamlFieldName, fieldInfo => new FieldDeserializationContext<TObject>(fieldInfo));
+        }
+
+        /// <summary>
+        /// The fields that have not been set during deserialization.
+        /// </summary>
+        public IReadOnlyCollection<FieldDeserializationContext<TObject>> UnsetFields
+        {
+            get
+            {
+                return _fields
+                    .Where(f => !f.Value.HasBeenSet)
+                    .Select(f => f.Value)
+                    .ToList();
+            }
+        }
+
+        /// <summary>
+        /// Returns whether the specified field is ignored.
+        /// </summary>
+        /// <param name="fieldName">The field name.</param>
+        /// <returns>true if the field is ignored, false otherwise.</returns>
+        public bool IsIgnored(string fieldName)
+        {
+            return _ignoredFields.Contains(fieldName);
+        }
+
+        /// <summary>
+        /// Tries to find the specified field.
+        /// </summary>
+        /// <param name="fieldName">The field name.</param>
+        /// <param name="field">Set to the field if found, otherwise null.</param>
+        /// <returns>true if the field was found, false otherwise.</returns>
+        public bool TryGetField(string fieldName, out FieldDeserializationContext<TObject> field)
+        {
+            return _fields.TryGetValue(fieldName, out field);
+        }
+    }
+}

src/Promitor.Core.Scraping/Configuration/Serialization/DeserializationMessage.cs

@@ -0,0 +1,48 @@
+using YamlDotNet.RepresentationModel;
+
+namespace Promitor.Core.Scraping.Configuration.Serialization
+{
+    /// <summary>
+    /// Represents a message reported during the deserialization process.
+    /// </summary>
+    public class DeserializationMessage
+    {
+        /// <summary>
+        /// Initializes a new instance of the <see cref="DeserializationMessage"/> type.
+        /// </summary>
+        /// <param name="messageType">The type of message reported.</param>
+        /// <param name="node">The node the message is associated with.</param>
+        /// <param name="message">The message.</param>
+        public DeserializationMessage(MessageType messageType, YamlNode node, string message)
+        {
+            MessageType = messageType;
+            Node = node;
+            Message = message;
+        }
+
+        /// <summary>
+        /// Gets the type of message.
+        /// </summary>
+        public MessageType MessageType { get; }
+
+        /// <summary>
+        /// Gets the node the message has been reported against.
+        /// </summary>
+        public YamlNode Node { get; }
+
+        /// <summary>
+        /// Gets the message.
+        /// </summary>
+        public string Message { get; }
+
+        /// <summary>
+        /// Gets the message formatted for output to users.
+        /// </summary>
+        public string FormattedMessage => $"{MessageType} {Node.Start.Line}:{Node.Start.Column}: {Message}";
+
+        public override string ToString()
+        {
+            return FormattedMessage;
+        }
+    }
+}

src/Promitor.Core.Scraping/Configuration/Serialization/Deserializer.cs

@@ -1,4 +1,7 @@
-using System.Collections.Generic;
+using System;
+using System.Collections.Generic;
+using System.Linq.Expressions;
+using System.Reflection;
 using System.Runtime.Serialization;
 using GuardNet;
 using Microsoft.Extensions.Logging;
@@ -7,7 +10,11 @@
 namespace Promitor.Core.Scraping.Configuration.Serialization
 {
     public abstract class Deserializer<TObject> : IDeserializer<TObject>
+        where TObject: new()
     {
+        private readonly HashSet<string> _ignoredFields = new HashSet<string>();
+        private readonly List<FieldDeserializationInfo> _fields = new List<FieldDeserializationInfo>();
+
         protected ILogger Logger { get; }
 
         protected Deserializer(ILogger logger)
@@ -17,9 +24,47 @@ protected Deserializer(ILogger logger)
             Logger = logger;
         }
 
-        public abstract TObject Deserialize(YamlMappingNode node);
+        /// <inheritdoc />
+        public virtual TObject Deserialize(YamlMappingNode node, IErrorReporter errorReporter)
+        {
+            var result = new TObject();
+            var deserializationContext = new DeserializationContext<TObject>(_ignoredFields, _fields);
+
+            foreach (var child in node.Children)
+            {
+                if (deserializationContext.IsIgnored(child.Key.ToString()))
+                {
+                    continue;
+                }
+
+                if (deserializationContext.TryGetField(child.Key.ToString(), out var fieldContext))
+                {
+                    fieldContext.SetValue(
+                        result, GetFieldValue(child, fieldContext.DeserializationInfo, errorReporter));
+                }
+                else
+                {
+                    errorReporter.ReportWarning(child.Key, $"Unknown field '{child.Key}'.");
+                }
+            }
+
+            foreach (var unsetField in deserializationContext.UnsetFields)
+            {
+                if (unsetField.DeserializationInfo.IsRequired)
+                {
+                    errorReporter.ReportError(node, $"'{unsetField.DeserializationInfo.YamlFieldName}' is a required field but was not found.");
+                }
+                else
+                {
+                    unsetField.SetDefaultValue(result);
+                }
+            }
+
+            return result;
+        }
 
-        public List<TObject> Deserialize(YamlSequenceNode nodes)
+        /// <inheritdoc />
+        public IReadOnlyCollection<TObject> Deserialize(YamlSequenceNode nodes, IErrorReporter errorReporter)
         {
             Guard.NotNull(nodes, nameof(nodes));
 
@@ -31,11 +76,107 @@ public List<TObject> Deserialize(YamlSequenceNode nodes)
                     throw new SerializationException($"Failed parsing metrics because we couldn't cast an item to {nameof(YamlMappingNode)}");
                 }
 
-                var deserializedObject = Deserialize(metricNode);
+                var deserializedObject = Deserialize(metricNode, errorReporter);
                 deserializedObjects.Add(deserializedObject);
             }
 
             return deserializedObjects;
         }
+
+        public object DeserializeObject(YamlMappingNode node, IErrorReporter errorReporter)
+        {
+            return Deserialize(node, errorReporter);
+        }
+
+        protected void MapRequired<TReturn>(Expression<Func<TObject, TReturn>> accessorExpression, Func<string, KeyValuePair<YamlNode, YamlNode>, IErrorReporter, object> customMapperFunc = null)
+        {
+            var memberExpression = (MemberExpression)accessorExpression.Body;
+            _fields.Add(new FieldDeserializationInfo(memberExpression.Member as PropertyInfo, true, default(TReturn), customMapperFunc));
+        }
+
+        protected void MapRequired<TReturn>(
+            Expression<Func<TObject, TReturn>> accessorExpression, IDeserializer deserializer)
+            where TReturn: new()
+        {
+            var memberExpression = (MemberExpression)accessorExpression.Body;
+            _fields.Add(new FieldDeserializationInfo(
+                memberExpression.Member as PropertyInfo, true, default(TReturn), null, deserializer));
+        }
+
+        protected void MapOptional<TReturn>(
+            Expression<Func<TObject, TReturn>> accessorExpression, TReturn defaultValue = default, Func<string, KeyValuePair<YamlNode, YamlNode>, IErrorReporter, object> customMapperFunc = null)
+        {
+            var memberExpression = (MemberExpression)accessorExpression.Body;
+            _fields.Add(new FieldDeserializationInfo(memberExpression.Member as PropertyInfo, false, defaultValue, customMapperFunc));
+        }
+
+        protected void MapOptional<TReturn>(
+            Expression<Func<TObject, TReturn>> accessorExpression, IDeserializer deserializer)
+            where TReturn: new()
+        {
+            var memberExpression = (MemberExpression)accessorExpression.Body;
+            _fields.Add(new FieldDeserializationInfo(
+                memberExpression.Member as PropertyInfo, false, default(TReturn), null, deserializer));
+        }
+
+        protected void IgnoreField(string fieldName)
+        {
+            _ignoredFields.Add(fieldName);
+        }
+
+        private static object GetFieldValue(
+            KeyValuePair<YamlNode, YamlNode> fieldNodePair, FieldDeserializationInfo fieldDeserializationInfo, IErrorReporter errorReporter)
+        {
+            if (fieldDeserializationInfo.CustomMapperFunc != null)
+            {
+                return fieldDeserializationInfo.CustomMapperFunc(fieldNodePair.Value.ToString(), fieldNodePair, errorReporter);
+            }
+
+            if (fieldDeserializationInfo.Deserializer != null)
+            {
+                return fieldDeserializationInfo.Deserializer.DeserializeObject((YamlMappingNode)fieldNodePair.Value, errorReporter);
+            }
+
+            var propertyType = Nullable.GetUnderlyingType(fieldDeserializationInfo.PropertyInfo.PropertyType) ?? fieldDeserializationInfo.PropertyInfo.PropertyType;
+            if (propertyType.IsEnum)
+            {
+                var parseSucceeded = System.Enum.TryParse(
+                    propertyType, fieldNodePair.Value.ToString(), out var enumValue);
+
+                if (!parseSucceeded)
+                {
+                    errorReporter.ReportError(fieldNodePair.Value, $"'{fieldNodePair.Value}' is not a valid value for '{fieldNodePair.Key}'.");
+                }
+
+                return enumValue;
+            }
+
+            if (typeof(IDictionary<string, string>).IsAssignableFrom(propertyType))
+            {
+                return ((YamlMappingNode)fieldNodePair.Value).GetDictionary();
+            }
+
+            if (propertyType == typeof(TimeSpan))
+            {
+                var parseSucceeded = TimeSpan.TryParse(fieldNodePair.Value.ToString(), out var timeSpanValue);
+                if (!parseSucceeded)
+                {
+                    errorReporter.ReportError(fieldNodePair.Value, $"'{fieldNodePair.Value}' is not a valid value for '{fieldNodePair.Key}'. The value must be in the format 'hh:mm:ss'.");
+                }
+
+                return timeSpanValue;
+            }
+
+            try
+            {
+                return Convert.ChangeType(fieldNodePair.Value.ToString(), fieldDeserializationInfo.PropertyInfo.PropertyType);
+            }
+            catch (FormatException)
+            {
+                errorReporter.ReportError(fieldNodePair.Value, $"'{fieldNodePair.Value}' is not a valid value for '{fieldNodePair.Key}'. The value must be of type {fieldDeserializationInfo.PropertyInfo.PropertyType}.");
+            }
+
+            return null;
+        }
     }
 }