-
-
Notifications
You must be signed in to change notification settings - Fork 92
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Provide possible suggestions for unknown fields #1106
Provide possible suggestions for unknown fields #1106
Conversation
Docker image for this PR was built and is available on Docker Hub. You can pull it locally via the CLI: docker pull tomkerkhove/promitor-agent-scraper-ci:pr1106-linux Want to verify the new version? Run it locally: docker run -d -p 8999:80 --name promitor-agent-scraper-pr1106-linux \
--env PROMITOR_AUTH_APPID='<azure-ad-app-id>' \
--env-file C:/Promitor/az-mon-auth.creds \
--volume C:/Promitor/metrics-declaration.yaml:/config/metrics-declaration.yaml \
--volume C:/Promitor/runtime-config.yaml:/config/runtime.yaml \
tomkerkhove/promitor-agent-scraper-ci:pr1106-linux You can find a CI version of our Helm chart on hub.helm.sh |
5c94fbc
to
788bf77
Compare
Docker image for this PR was built and is available on Docker Hub. You can pull it locally via the CLI: docker pull tomkerkhove/promitor-agent-scraper-ci:pr1106-linux Want to verify the new version? Run it locally: docker run -d -p 8999:80 --name promitor-agent-scraper-pr1106-linux \
--env PROMITOR_AUTH_APPID='<azure-ad-app-id>' \
--env-file C:/Promitor/az-mon-auth.creds \
--volume C:/Promitor/metrics-declaration.yaml:/config/metrics-declaration.yaml \
--volume C:/Promitor/runtime-config.yaml:/config/runtime.yaml \
tomkerkhove/promitor-agent-scraper-ci:pr1106-linux You can find a CI version of our Helm chart on hub.helm.sh |
I've updated the deserialization to provide a list of suggestions for unknown field names. This uses the Fastenshtein library to calculate the edit distance between the field name entered by a user and the possible fields. I've chosen to return fields if they have an edit distance less than 3. This is fairly arbitrary and just based on some simple testing. I wanted to provide suggestions, but not provide ones that were completely different to the name entered. Also: - Updated the V1Deserializer to use the `Map()` syntax for mapping rather than doing everything manually. The main reason for this was so that the fields are added as suggestions, although it also removes some unnecessary code. - Altered MetricsDeclarationProvider to not set the defaults if validating the metrics fails. The reason I've done this is that if the metric defaults couldn't be deserialized, we end up with a NullReferenceException. Fixes tomkerkhove#1105
788bf77
to
b877cf8
Compare
Docker image for this PR was built and is available on Docker Hub. You can pull it locally via the CLI: docker pull tomkerkhove/promitor-agent-scraper-ci:pr1106-linux Want to verify the new version? Run it locally: docker run -d -p 8999:80 --name promitor-agent-scraper-pr1106-linux \
--env PROMITOR_AUTH_APPID='<azure-ad-app-id>' \
--env-file C:/Promitor/az-mon-auth.creds \
--volume C:/Promitor/metrics-declaration.yaml:/config/metrics-declaration.yaml \
--volume C:/Promitor/runtime-config.yaml:/config/runtime.yaml \
tomkerkhove/promitor-agent-scraper-ci:pr1106-linux You can find a CI version of our Helm chart on hub.helm.sh |
src/Promitor.Core.Scraping/Configuration/Serialization/Deserializer.cs
Outdated
Show resolved
Hide resolved
@@ -8,79 +8,43 @@ namespace Promitor.Core.Scraping.Configuration.Serialization.v1.Core | |||
{ | |||
public class V1Deserializer : Deserializer<MetricsDeclarationV1> | |||
{ | |||
private readonly IDeserializer<AzureMetadataV1> _azureMetadataDeserializer; | |||
private readonly IDeserializer<MetricDefaultsV1> _defaultsDeserializer; | |||
private readonly IDeserializer<MetricDefinitionV1> _metricsDeserializer; | |||
|
|||
public V1Deserializer(IDeserializer<AzureMetadataV1> azureMetadataDeserializer, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks a lot better, thanks 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - I think we can simplify it even more by adding support for deserializing YamlSequenceNode
objects, but I didn't want to change too much at once (or have to figure out how to do that 😄).
@@ -34,16 +34,18 @@ public V1DeserializerTests() | |||
} | |||
|
|||
[Fact] | |||
public void Deserialize_NoVersionSpecified_ThrowsException() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if these are not specified and you start up the app? Does it still block it from proceeding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - in all cases it blocks the app from starting properly. The version is actually a bit of an odd one - it won't even get as far as the deserializer if the version is missing because we don't know what version of the config to deserialize!
Anyhow, here's examples of the output in each case.
- No version: https://gist.github.com/adamconnelly/99ec2264cf8e796a54f40d0bd6b3df22
- Metrics missing: https://gist.github.com/adamconnelly/1c402535d4eca8f07fc3d9e7662af6b6
- Defaults missing: https://gist.github.com/adamconnelly/c8016c675e81190cd3d81e8b9479a292
- Azure metadata missing: https://gist.github.com/adamconnelly/d14d422bc8ee3cbf9b38cda3482308be
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks!
Docker image for this PR was built and is available on Docker Hub. You can pull it locally via the CLI: docker pull tomkerkhove/promitor-agent-scraper-ci:pr1106-linux Want to verify the new version? Run it locally: docker run -d -p 8999:80 --name promitor-agent-scraper-pr1106-linux \
--env PROMITOR_AUTH_APPID='<azure-ad-app-id>' \
--env-file C:/Promitor/az-mon-auth.creds \
--volume C:/Promitor/metrics-declaration.yaml:/config/metrics-declaration.yaml \
--volume C:/Promitor/runtime-config.yaml:/config/runtime.yaml \
tomkerkhove/promitor-agent-scraper-ci:pr1106-linux You can find a CI version of our Helm chart on hub.helm.sh |
I've updated the deserialization to provide a list of suggestions for unknown field names. This uses the Fastenshtein library to calculate the edit distance between the field name entered by a user and the possible fields.
I've chosen to return fields if they have an edit distance less than 3. This is fairly arbitrary and just based on some simple testing. I wanted to provide suggestions, but not provide ones that were completely different to the name entered.
Also:
Map()
syntax for mapping rather than doing everything manually. The main reason for this was so that the fields are added as suggestions, although it also removes some unnecessary code.Fixes #1105
If we have the following config:
It produces the following error messages: