Update runtime model to support multiple resources per metric #666
Conversation
I've updated `MetricDefinition` to support multiple resources so that we can define a single prometheus metric that is used to store the values for multiple Azure resources. In order to do this, I've changed `MetricDefinition` so that it doesn't describe an Azure resource anymore, and created `AzureResourceDefinition` to be the base class for each resource type. I've also created a `PrometheusMetricDefinition` class to contain the details of the prometheus metric being defined. In addition, I've added a new class, `ScrapeDefinition`, that describes a single Azure resource being scraped. This is used in the places where `MetricDefinition` would have been used previously as part of scraping.
- Updated all the `MetricDefinition` implementations to inherit from `AzureResourceDefinition` instead.
I've updated the scraper implementations to work with the new MetricDefinition format. I couldn't come up with a neat way of making MetricDefinition generic without ending up with a nightmare, so for now what I've done is altered the signature of `ScrapeResourceAsync` to accept both the `ScrapeDefinition` object, as well as the `AzureResourceDefinition` cast to the correct type for the subclass.
Updated a couple of the objects used to configure the scraping to handle multiple resources per metric.
- Removed the `MetricValidator` abstract class because it doesn't add anything now that we can't cast the `MetricDefinition` to a specific resource type. - Updated the validators to validate all the resources in the metric definition.
- Updating `MetricsDeclarationBuilder` to use v1 Builder objects instead of the model objects. - Updating the tests to handle the fact that a `MetricDefinition` isn't a resource anymore.
When I was making the initial set of changes, I didn't want to make too many changes at once, so I held off on renaming the subclasses of `AzureResourceDefinition`. This change renames them all from `{Type}MetricDefinition` to `{Type}ResourceDefinition` to make it clearer what they are.
I realised that the Version property wasn't being populated on either the model object or the serialization object. I've removed it from the model object since it isn't versioned. I've also updated the v1 ConfigurationSerializer to just set the version to `v1` since it only handles the v1 format.
adamconnelly
force-pushed
the
multi-resource-metrics
branch
from
5d0dc75 to
c324ceb
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:pr666Want to verify the new version? Run it locally: docker run -d -p 8999:80 --name promitor-agent-scraper-pr666 \
--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:pr666You can find a CI version of our Helm chart on hub.helm.sh |
|
I will have a look, thanks! PS: Build failure is due to Azure DevOps, next commit it will kick in again or will find another way to sort it out. |
src/Promitor.Core.Scraping/Configuration/Model/Metrics/AzureResourceDefinition.cs
Outdated
Show resolved
Hide resolved
src/Promitor.Core.Scraping/Configuration/Model/Metrics/PrometheusMetricDefinition.cs
Show resolved
Hide resolved
src/Promitor.Core.Scraping/Configuration/Model/Metrics/PrometheusMetricDefinition.cs
Show resolved
Hide resolved
...re.Scraping/Configuration/Model/Metrics/ResourceTypes/ContainerRegistryResourceDefinition.cs
Outdated
Show resolved
Hide resolved
...re.Scraping/Configuration/Model/Metrics/ResourceTypes/ContainerRegistryResourceDefinition.cs
Outdated
Show resolved
Hide resolved
src/Promitor.Scraper.Tests.Unit/Serialization/v1/MetricsDeclaration/YamlSerializationTests.cs
Show resolved
Hide resolved
src/Promitor.Scraper.Tests.Unit/Serialization/v1/Mapping/MetricDefinitionV1MappingTests.cs
Show resolved
Hide resolved
src/Promitor.Core.Scraping/Configuration/Providers/MetricsDeclarationProvider.cs
Outdated
Show resolved
Hide resolved
src/Promitor.Core.Scraping/Configuration/Serialization/v1/Core/ConfigurationSerializer.cs
Outdated
Show resolved
Hide resolved
|
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:pr666Want to verify the new version? Run it locally: docker run -d -p 8999:80 --name promitor-agent-scraper-pr666 \
--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:pr666You can find a CI version of our Helm chart on hub.helm.sh |
|
@tomkerkhove I've addressed most of your comments, and pushed another commit with fixes for them. I've left two unresolved because I'm not completely sure what we should do about them. Also - I noticed that the Open-Api.xml file changed in the commit I've just pushed. I included it in the commit because I assumed it was just automatically generated based on the code, but I just wanted to point that out because I'm not quite sure how that works. |
|
Also - just pushed another commit to fix the build. I've just added them as fixups because I figured if you were going to squash the commits before merging anyway it didn't really matter if they had decent commit messages or not. |
|
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:pr666Want to verify the new version? Run it locally: docker run -d -p 8999:80 --name promitor-agent-scraper-pr666 \
--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:pr666You can find a CI version of our Helm chart on hub.helm.sh |
That is correct, you can ignore that one! Will go through it all now but first glance looks good! |
Works towards #513
Proposed Changes
This PR updates the runtime model to support multiple resources per metric, but without altering the v1 config format. It shouldn't cause any change in functionality, but gives us the basis to update the config to support multiple resources per metric.
@tomkerkhove I've kept this split into multiple commits because it's a pretty huge change, and I figured it would be a bit easier to follow what I was trying to do. The first commit contains the main change, and then the other commits are really just the required updates to the rest of the codebase to support that change.