-
-
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
Fix resource group overriding #656
Fix resource group overriding #656
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:pr656 Want to verify the new version? Run it locally: docker run -d -p 8999:80 --name promitor-agent-scraper-pr656 \
--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:pr656 You can find a CI version of our Helm chart on hub.helm.sh |
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 awesome, thanks!
Just one thing - Do we have a dedicated test where both the general and metric-specific resource group is defined? We might want to add that one and expect the metric-specific one.
We might also want to verify that we have a test where we have a general one configured and only assignes that.
@tomkerkhove so all of the tests for the deserializers test both with a resource group and without a resource group specified. They're all setup as Theories with two sets of test data. You can maybe correct me on this, but as far as I understand it, the way the code works is like this:
So I think the place we'd really want to test is the scrapers. The issue I can see with that is that currently I think the way they're implemented they make a call directly to the Azure API, meaning we'd need to do some refactoring to make them unit testable. I think it would be sensible to do that, I'm just not sure it's a great idea to do that refactoring as part of this set of changes. Is that correct, and does that make sense? |
I don't think this is true, but I'm not 100% sure. What makes you think this is how it works?
This check is just based on if the metric-specific one is specified, otherwise use global value.
Integration tests will help us assure this, but I don't have unit tests for this on my radar. Would be interested in contributions for this post v1.0 but would prefer to hold this off for now indeed. If you are up for this or want to track this, feel free to open an issue for it. |
@tomkerkhove I was thinking of promitor/src/Promitor.Core.Scraping/Configuration/Providers/MetricsDeclarationProvider.cs Line 34 in 321a3ac
I don't think it really matters to your original question anyway, although it's be good for my benefit to know if I'm understanding it right or not. |
No you are correct, I forgot about that 😅 Sorry! |
@adamconnelly Can you change your commits here as well or sign the CLA with @adam-resdiary? Careful: Just merged master |
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:pr656 Want to verify the new version? Run it locally: docker run -d -p 8999:80 --name promitor-agent-scraper-pr656 \
--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:pr656 You can find a CI version of our Helm chart on hub.helm.sh |
Yeah sure. Sorry - just forgot. I'll do it later today when I've got a minute. |
Thank you! |
The ability to override the resource group per metric was broken for all resource types other than the Generic resource type. This commit: - Fixes the tests for all resource types to test for this by removing the `Ignore()` method calls for the `ResourceGroupName` property and added an extra assert to check for the resource group. - Alters the deserializers to inherit directly from `MetricDeserializer` rather than `GenericAzureMetricDeserializer`. They weren't actually using any of the functionality from the generic type. - Fixes a few Scraper implementations to actually use the overridden group name. Fixes tomkerkhove#655
be06bc1
to
222e5a6
Compare
@tomkerkhove that should be it sorted. I pulled upstream master first and then just rebased against that, so it should be up to date now. |
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:pr656 Want to verify the new version? Run it locally: docker run -d -p 8999:80 --name promitor-agent-scraper-pr656 \
--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:pr656 You can find a CI version of our Helm chart on hub.helm.sh |
The ability to override the resource group per metric was broken for all resource types other than the Generic resource type.
This commit:
Ignore()
method calls for theResourceGroupName
property and added an extra assert to check for the resource group.MetricDeserializer
rather thanGenericAzureMetricDeserializer
. They weren't actually using any of the functionality from the generic type.Fixes #655