-
-
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 better configuration deserialization & validation at startup #592
Comments
This issue is stale because it has been open with no activity. Remove |
@tomkerkhove I'm interested in working on this. I've posted a PR with an outline of what I'm thinking: #793. I've also mocked up some tests locally to verify quickly that we should be able to do this without too much difficulty. If you have some time, any chance you can take a look at the PR and let me know if you think I'm going in the right direction, and if you've got any suggestions? |
I certainly will, thank you very much! |
@tomkerkhove sorry about the delayed response - I spent some time over the holidays and got this mostly working. I still need to go through and tidy things up, and maybe tweak the startup code of promitor to make things a bit nicer to read (at the moment I've just hooked things into the main validation code, and it ends up printing a big stack trace out, which is a bit ugly). Anyhow, since this is another one of these changes that affects a massive chunk of the code-base, I want to try to split it into a few PRs to make it a bit more manageable to review. At the same time, I don't really want to put up PRs for merging back into the main Promitor repo with partially working code, so I was going to suggest getting you to review a few PRs against my own fork, and then once it's complete I can create another PR to merge all my changes into your repo. Does that sound ok? If so, the first (and hopefully biggest) PR is here: adamconnelly#1. It should give you a decent idea of the approach I've taken. |
Awesome, thanks Adam! I'll go through that PR asap, thanks. |
@tomkerkhove here's the next PR when you have some time: adamconnelly#2. |
@tomkerkhove here's a PR converting all the resource deserializers over: adamconnelly#3. It would be great if you could check that the fields are correct in terms of being required or optional. Especially for the following resources that I'm not really sure about:
I tried to guess based on the docs on promitor.io, but I might have got some of them wrong. |
@tomkerkhove here's a PR hooking the changes up properly so we can see it working end to end: adamconnelly#4. There's still other things to do like offering suggestions, adding validation of cron expressions, along with a few other changes you asked me to make in my initial PR, but it shows that the concept works. |
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 tomkerkhove#592
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 tomkerkhove#592
Also: - Fixed the metrics definition docs to indicate that 'labels' is optional. Part of tomkerkhove#592
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 tomkerkhove#592
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 tomkerkhove#592
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 tomkerkhove#592
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 tomkerkhove#592
* 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
Provide better configuration deserialization & validation at startup.
An example of this came up in #591 where everything seemed ok but in really there was a deserialization issue which was not blocked given we don't have mandatory field validation.
Configuration was:
Which should resolve in something along the lines of:
Note that
azureMetricConfiguration.aggregation.type
isnone
since the configuration hasType
rather thantype
. We should not ignore this.The text was updated successfully, but these errors were encountered: