diff --git a/changelog/content/experimental/unreleased.md b/changelog/content/experimental/unreleased.md index 9f33a370c..bbac4cfc5 100644 --- a/changelog/content/experimental/unreleased.md +++ b/changelog/content/experimental/unreleased.md @@ -7,4 +7,5 @@ version: - {{% tag added %}} New validation rule to ensure at least one resource or resource collection is configured to scrape - {{% tag removed %}} Support for Swagger 2.0 ([deprecation notice](https://changelog.promitor.io/#swagger-2-0)) -- {{% tag removed %}} Support for Swagger UI 2.0 ([deprecation notice](https://changelog.promitor.io/#swagger-ui-2-0)) \ No newline at end of file +- {{% tag removed %}} Support for Swagger UI 2.0 ([deprecation notice](https://changelog.promitor.io/#swagger-ui-2-0)) +- {{% tag added %}} Provide suggestions when unknown fields are found in the metrics config. [#1105](https://github.com/tomkerkhove/promitor/issues/1105). \ No newline at end of file diff --git a/src/Promitor.Core.Scraping/Configuration/Providers/MetricsDeclarationProvider.cs b/src/Promitor.Core.Scraping/Configuration/Providers/MetricsDeclarationProvider.cs index acffde6be..c9ab8c4c8 100644 --- a/src/Promitor.Core.Scraping/Configuration/Providers/MetricsDeclarationProvider.cs +++ b/src/Promitor.Core.Scraping/Configuration/Providers/MetricsDeclarationProvider.cs @@ -40,7 +40,7 @@ public virtual MetricsDeclaration Get(bool applyDefaults = false, IErrorReporter var config = _configurationSerializer.Deserialize(rawMetricsDeclaration, errorReporter); - if (applyDefaults) + if (!errorReporter.HasErrors && applyDefaults) { foreach (var metric in config.Metrics) { diff --git a/src/Promitor.Core.Scraping/Configuration/Serialization/DeserializationContext.cs b/src/Promitor.Core.Scraping/Configuration/Serialization/DeserializationContext.cs index b62580aa9..64934bde3 100644 --- a/src/Promitor.Core.Scraping/Configuration/Serialization/DeserializationContext.cs +++ b/src/Promitor.Core.Scraping/Configuration/Serialization/DeserializationContext.cs @@ -1,5 +1,6 @@ using System.Collections.Generic; using System.Linq; +using Fastenshtein; namespace Promitor.Core.Scraping.Configuration.Serialization { @@ -8,6 +9,13 @@ namespace Promitor.Core.Scraping.Configuration.Serialization /// public class DeserializationContext { + /// + /// The max edit distance for us to consider a field as a suggestion + /// between a field name entered by a user, and the field names that + /// have been configured for the object. + /// + private const int MaxSuggestionDistance = 3; + private readonly ISet _ignoredFields; private readonly Dictionary> _fields; @@ -57,5 +65,21 @@ public bool TryGetField(string fieldName, out FieldDeserializationContext + /// Returns a list of fields that are close enough to + /// for us to suggest them as alternatives. + /// + /// + /// The name of a field that doesn't exist that we want to find suggestions for. + /// + public IReadOnlyCollection GetSuggestions(string fieldName) + { + return _fields + .Select(field => field.Key) + .Union(_ignoredFields) + .Where(suggestion => Levenshtein.Distance(suggestion, fieldName) < MaxSuggestionDistance) + .ToList(); + } } } diff --git a/src/Promitor.Core.Scraping/Configuration/Serialization/Deserializer.cs b/src/Promitor.Core.Scraping/Configuration/Serialization/Deserializer.cs index 3c5e9f3e8..97001b34c 100644 --- a/src/Promitor.Core.Scraping/Configuration/Serialization/Deserializer.cs +++ b/src/Promitor.Core.Scraping/Configuration/Serialization/Deserializer.cs @@ -45,7 +45,8 @@ public virtual TObject Deserialize(YamlMappingNode node, IErrorReporter errorRep } else { - errorReporter.ReportWarning(child.Key, $"Unknown field '{child.Key}'."); + var warningMessage = GetUnknownFieldWarningMessage(deserializationContext, child.Key.ToString()); + errorReporter.ReportWarning(child.Key, warningMessage); } } @@ -158,5 +159,28 @@ private static object GetFieldValue( return null; } + + /// + /// Gets the warning message to use when an invalid field name is found + /// in the configuration. + /// + /// The deserialization context. + /// The unknown field. + private static string GetUnknownFieldWarningMessage(DeserializationContext deserializationContext, string fieldName) + { + var message = $"Unknown field '{fieldName}'."; + var suggestions = deserializationContext + .GetSuggestions(fieldName) + .Select(suggestion => $"'{suggestion}'") + .ToList(); + + if (suggestions.Any()) + { + var formattedSuggestions = string.Join(", ", suggestions); + message += $" Did you mean {formattedSuggestions}?"; + } + + return message; + } } } \ No newline at end of file diff --git a/src/Promitor.Core.Scraping/Configuration/Serialization/v1/Core/V1Deserializer.cs b/src/Promitor.Core.Scraping/Configuration/Serialization/v1/Core/V1Deserializer.cs index 401e7857f..12fd06f13 100644 --- a/src/Promitor.Core.Scraping/Configuration/Serialization/v1/Core/V1Deserializer.cs +++ b/src/Promitor.Core.Scraping/Configuration/Serialization/v1/Core/V1Deserializer.cs @@ -8,8 +8,6 @@ namespace Promitor.Core.Scraping.Configuration.Serialization.v1.Core { public class V1Deserializer : Deserializer { - private readonly IDeserializer _azureMetadataDeserializer; - private readonly IDeserializer _defaultsDeserializer; private readonly IDeserializer _metricsDeserializer; public V1Deserializer(IDeserializer azureMetadataDeserializer, @@ -17,70 +15,36 @@ public V1Deserializer(IDeserializer azureMetadataDeserializer, IDeserializer metricsDeserializer, ILogger logger) : base(logger) { - _azureMetadataDeserializer = azureMetadataDeserializer; - _defaultsDeserializer = defaultsDeserializer; _metricsDeserializer = metricsDeserializer; - } - - public override MetricsDeclarationV1 Deserialize(YamlMappingNode rootNode, IErrorReporter errorReporter) - { - ValidateVersion(rootNode); - - var azureMetadata = DeserializeAzureMetadata(rootNode, errorReporter); - var metricDefaults = DeserializeMetricDefaults(rootNode, errorReporter); - var metrics = DeserializeMetrics(rootNode, errorReporter); - - return new MetricsDeclarationV1 - { - Version = SpecVersion.v1.ToString(), - AzureMetadata = azureMetadata, - MetricDefaults = metricDefaults, - Metrics = metrics - }; - } - - private static void ValidateVersion(YamlMappingNode rootNode) - { - var versionFound = rootNode.Children.TryGetValue("version", out var versionNode); - if (!versionFound) - { - throw new System.Exception("No 'version' element was found in the metrics config"); - } - if (versionNode.ToString() != SpecVersion.v1.ToString()) - { - throw new System.Exception($"A 'version' element with a value of '{SpecVersion.v1}' was expected but the value '{versionNode}' was found"); - } + Map(definition => definition.Version) + .IsRequired() + .MapUsing(GetVersion); + Map(definition => definition.AzureMetadata) + .IsRequired() + .MapUsingDeserializer(azureMetadataDeserializer); + Map(definition => definition.MetricDefaults) + .IsRequired() + .MapUsingDeserializer(defaultsDeserializer); + Map(definition => definition.Metrics) + .IsRequired() + .MapUsing(DeserializeMetrics); } - private AzureMetadataV1 DeserializeAzureMetadata(YamlMappingNode rootNode, IErrorReporter errorReporter) + private static object GetVersion(string value, KeyValuePair node, IErrorReporter errorReporter) { - if (rootNode.Children.TryGetValue("azureMetadata", out var azureMetadataNode)) + if (value != SpecVersion.v1.ToString()) { - return _azureMetadataDeserializer.Deserialize((YamlMappingNode)azureMetadataNode, errorReporter); + errorReporter.ReportError(node.Value, $"A 'version' element with a value of '{SpecVersion.v1}' was expected but the value '{value}' was found"); } - return null; + return SpecVersion.v1.ToString(); } - private MetricDefaultsV1 DeserializeMetricDefaults(YamlMappingNode rootNode, IErrorReporter errorReporter) + private IReadOnlyCollection DeserializeMetrics( + string value, KeyValuePair nodePair, IErrorReporter errorReporter) { - if (rootNode.Children.TryGetValue("metricDefaults", out var defaultsNode)) - { - return _defaultsDeserializer.Deserialize((YamlMappingNode)defaultsNode, errorReporter); - } - - return null; - } - - private IReadOnlyCollection DeserializeMetrics(YamlMappingNode rootNode, IErrorReporter errorReporter) - { - if (rootNode.Children.TryGetValue("metrics", out var metricsNode)) - { - return _metricsDeserializer.Deserialize((YamlSequenceNode)metricsNode, errorReporter); - } - - return null; + return _metricsDeserializer.Deserialize((YamlSequenceNode)nodePair.Value, errorReporter); } } } diff --git a/src/Promitor.Core.Scraping/Promitor.Core.Scraping.csproj b/src/Promitor.Core.Scraping/Promitor.Core.Scraping.csproj index b4fb1d4ad..ad8857d9b 100644 --- a/src/Promitor.Core.Scraping/Promitor.Core.Scraping.csproj +++ b/src/Promitor.Core.Scraping/Promitor.Core.Scraping.csproj @@ -1,4 +1,4 @@ - + netcoreapp3.1 @@ -16,6 +16,7 @@ + diff --git a/src/Promitor.Tests.Unit/Promitor.Tests.Unit.csproj b/src/Promitor.Tests.Unit/Promitor.Tests.Unit.csproj index 84cbfe0fe..d2efaca15 100644 --- a/src/Promitor.Tests.Unit/Promitor.Tests.Unit.csproj +++ b/src/Promitor.Tests.Unit/Promitor.Tests.Unit.csproj @@ -27,6 +27,7 @@ + diff --git a/src/Promitor.Tests.Unit/Serialization/DeserializationContextTests.cs b/src/Promitor.Tests.Unit/Serialization/DeserializationContextTests.cs new file mode 100644 index 000000000..19e91b7c4 --- /dev/null +++ b/src/Promitor.Tests.Unit/Serialization/DeserializationContextTests.cs @@ -0,0 +1,102 @@ +using System.Collections.Generic; +using System.Reflection; +using Promitor.Core.Scraping.Configuration.Serialization; +using Xunit; + +namespace Promitor.Tests.Unit.Serialization +{ + public class DeserializationContextTests + { + [Fact] + public void GetSuggestions_NoFieldsConfigured_Empty() + { + // Arrange + var context = new DeserializationContext( + new HashSet(), new List()); + + // Act + var suggestions = context.GetSuggestions("job"); + + // Assert + Assert.Empty(suggestions); + } + + [Fact] + public void GetSuggestions_CloseEnoughSuggestion_ReturnsSuggestion() + { + // Arrange + var fields = new List + { + CreateField(typeof(Person).GetProperty(nameof(Person.Name))), + CreateField(typeof(Person).GetProperty(nameof(Person.Age))), + CreateField(typeof(Person).GetProperty(nameof(Person.Country))), + CreateField(typeof(Person).GetProperty(nameof(Person.County))) + }; + var context = new DeserializationContext(new HashSet(), fields); + + // Act + var suggestions = context.GetSuggestions("nmae"); + + // Assert + Assert.Collection(suggestions, suggestion => Assert.Equal("name", suggestion)); + } + + [Fact] + public void GetSuggestions_CloseEnoughSuggestion_ReturnsAllSuggestions() + { + // Arrange + var fields = new List + { + CreateField(typeof(Person).GetProperty(nameof(Person.Name))), + CreateField(typeof(Person).GetProperty(nameof(Person.Age))), + CreateField(typeof(Person).GetProperty(nameof(Person.Country))), + CreateField(typeof(Person).GetProperty(nameof(Person.County))) + }; + var context = new DeserializationContext(new HashSet(), fields); + + // Act + var suggestions = context.GetSuggestions("count"); + + // Assert + Assert.Collection(suggestions, + suggestion => Assert.Equal("country", suggestion), + suggestion => Assert.Equal("county", suggestion)); + } + + [Fact] + public void GetSuggestions_IgnoredFields_IncludesIgnoredFieldsInSuggestions() + { + // Arrange + var fields = new List + { + CreateField(typeof(Person).GetProperty(nameof(Person.Name))), + CreateField(typeof(Person).GetProperty(nameof(Person.Age))), + CreateField(typeof(Person).GetProperty(nameof(Person.Country))), + CreateField(typeof(Person).GetProperty(nameof(Person.County))) + }; + var ignoredFields = new HashSet { "named" }; + var context = new DeserializationContext(ignoredFields, fields); + + // Act + var suggestions = context.GetSuggestions("nam"); + + // Assert + Assert.Collection(suggestions, + suggestion => Assert.Equal("name", suggestion), + suggestion => Assert.Equal("named", suggestion)); + } + + private FieldDeserializationInfo CreateField(PropertyInfo propertyInfo) + { + return new FieldDeserializationInfo(propertyInfo, false, null, null, null); + } + + public class Person + { + public string Name { get; set; } + public int Age { get; set; } + public string Country { get; set; } + public string County { get; set; } + } + } +} \ No newline at end of file diff --git a/src/Promitor.Tests.Unit/Serialization/DeserializerTests/ValidationTests.cs b/src/Promitor.Tests.Unit/Serialization/DeserializerTests/ValidationTests.cs index ab267d885..6ef54ea09 100644 --- a/src/Promitor.Tests.Unit/Serialization/DeserializerTests/ValidationTests.cs +++ b/src/Promitor.Tests.Unit/Serialization/DeserializerTests/ValidationTests.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Linq; using Microsoft.Extensions.Logging.Abstractions; using Moq; @@ -153,11 +153,42 @@ public void Deserialize_CalledMultipleTimes_DoesNotReusePreviousState() r => r.ReportError(It.IsAny(), It.Is(s => s.Contains("name")))); } + [Fact] + public void Deserialize_UnknownField_ReturnsSuggestion() + { + // Arrange + var node = YamlUtils.CreateYamlNode(@"nmae: Promitor"); + + // Act + _deserializer.Deserialize(node, _errorReporter.Object); + + // Assert + var nameTagNode = node.Children.Single(c => c.Key.ToString() == "nmae").Key; + + _errorReporter.Verify(r => r.ReportWarning(nameTagNode, "Unknown field 'nmae'. Did you mean 'name'?")); + } + + [Fact] + public void Deserialize_UnknownField_ReturnsMultipleSuggestions() + { + // Arrange + var node = YamlUtils.CreateYamlNode(@"dat: Monday"); + + // Act + _deserializer.Deserialize(node, _errorReporter.Object); + + // Assert + var nameTagNode = node.Children.Single(c => c.Key.ToString() == "dat").Key; + + _errorReporter.Verify(r => r.ReportWarning(nameTagNode, "Unknown field 'dat'. Did you mean 'day', 'date'?")); + } + private class TestConfigObject { public string Name { get; set; } public int Age { get; set; } public DayOfWeek Day { get; set; } + public string Date { get; set; } public TimeSpan Interval { get; set; } } @@ -168,6 +199,7 @@ public TestDeserializer() : base(NullLogger.Instance) Map(t => t.Name).IsRequired(); Map(t => t.Age); Map(t => t.Day); + Map(t => t.Date); Map(t => t.Interval); IgnoreField("customField"); } diff --git a/src/Promitor.Tests.Unit/Serialization/v1/Core/V1DeserializerTests.cs b/src/Promitor.Tests.Unit/Serialization/v1/Core/V1DeserializerTests.cs index a3e4c97d5..749c48dfc 100644 --- a/src/Promitor.Tests.Unit/Serialization/v1/Core/V1DeserializerTests.cs +++ b/src/Promitor.Tests.Unit/Serialization/v1/Core/V1DeserializerTests.cs @@ -1,6 +1,6 @@ -using System; -using System.Collections.Generic; +using System.Collections.Generic; using System.ComponentModel; +using System.Linq; using Microsoft.Extensions.Logging.Abstractions; using Moq; using Promitor.Core.Scraping.Configuration.Serialization; @@ -34,16 +34,18 @@ public V1DeserializerTests() } [Fact] - public void Deserialize_NoVersionSpecified_ThrowsException() + public void Deserialize_NoVersionSpecified_ReportsError() { // Arrange - var yamlNode = YamlUtils.CreateYamlNode("azureMetadata:"); - - // Act - var exception = Assert.Throws(() => _deserializer.Deserialize(yamlNode, _errorReporter.Object)); - - // Assert - Assert.Equal("No 'version' element was found in the metrics config", exception.Message); + var node = YamlUtils.CreateYamlNode( +@"azureMetadata: + tenantId: '123'"); + + // Act / Assert + YamlAssert.ReportsErrorForProperty( + _deserializer, + node, + "version"); } [Fact] @@ -60,16 +62,20 @@ public void Deserialize_VersionSpecified_SetsCorrectVersion() } [Fact] - public void Deserialize_WrongVersionSpecified_ThrowsException() + public void Deserialize_WrongVersionSpecified_ReportsError() { // Arrange var yamlNode = YamlUtils.CreateYamlNode("version: v2"); + var versionNode = yamlNode.Children + .FirstOrDefault(c => c.Key.ToString() == "version") + .Value; // Act - var exception = Assert.Throws(() => _deserializer.Deserialize(yamlNode, _errorReporter.Object)); + _deserializer.Deserialize(yamlNode, _errorReporter.Object); // Assert - Assert.Equal("A 'version' element with a value of 'v1' was expected but the value 'v2' was found", exception.Message); + _errorReporter.Verify(r => r.ReportError( + versionNode, "A 'version' element with a value of 'v1' was expected but the value 'v2' was found")); } [Fact] @@ -83,7 +89,7 @@ public void Deserialize_AzureMetadata_UsesMetadataDeserializer() var yamlNode = YamlUtils.CreateYamlNode(config); var azureMetadata = new AzureMetadataV1(); _metadataDeserializer.Setup( - d => d.Deserialize(It.IsAny(), It.IsAny())).Returns(azureMetadata); + d => d.DeserializeObject(It.IsAny(), It.IsAny())).Returns(azureMetadata); // Act var declaration = _deserializer.Deserialize(yamlNode, _errorReporter.Object); @@ -98,7 +104,7 @@ public void Deserialize_AzureMetadataNotSupplied_SetsMetadataNull() // Arrange var yamlNode = YamlUtils.CreateYamlNode("version: v1"); _metadataDeserializer.Setup( - d => d.Deserialize(It.IsAny(), It.IsAny())).Returns(new AzureMetadataV1()); + d => d.DeserializeObject(It.IsAny(), It.IsAny())).Returns(new AzureMetadataV1()); // Act var declaration = _deserializer.Deserialize(yamlNode, _errorReporter.Object); @@ -119,7 +125,7 @@ public void Deserialize_MetricDefaults_UsesDefaultsDeserializer() var yamlNode = YamlUtils.CreateYamlNode(config); var metricDefaults = new MetricDefaultsV1(); _defaultsDeserializer.Setup( - d => d.Deserialize(It.IsAny(), It.IsAny())).Returns(metricDefaults); + d => d.DeserializeObject(It.IsAny(), It.IsAny())).Returns(metricDefaults); // Act var declaration = _deserializer.Deserialize(yamlNode, _errorReporter.Object); @@ -136,7 +142,7 @@ public void Deserialize_MetricDefaultsNotSupplied_SetsDefaultsNull() @"version: v1"; var yamlNode = YamlUtils.CreateYamlNode(config); _defaultsDeserializer.Setup( - d => d.Deserialize(It.IsAny(), It.IsAny())).Returns(new MetricDefaultsV1()); + d => d.DeserializeObject(It.IsAny(), It.IsAny())).Returns(new MetricDefaultsV1()); // Act var declaration = _deserializer.Deserialize(yamlNode, _errorReporter.Object);