Fix resource group overriding

[adamconnelly]

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 #655

[CLAassistant]

All committers have signed the CLA.

[promitor-bot]

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

[tomkerkhove]

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.

[adamconnelly]

@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:

• The model gets deserialized with the ResourceGroupName set to null if it's not specified.
• There's then a bit of code that runs on startup that populates the resource group if it's null.
• Lastly, the scrapers have logic in them to decide whether to use the global resource group or the specific group.

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?

[tomkerkhove]

• There's then a bit of code that runs on startup that populates the resource group if it's null.

I don't think this is true, but I'm not 100% sure. What makes you think this is how it works?

Lastly, the scrapers have logic in them to decide whether to use the global resource group or the specific group.

This check is just based on if the metric-specific one is specified, otherwise use global value.

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?

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.

[adamconnelly]

@tomkerkhove I was thinking of

promitor/src/Promitor.Core.Scraping/Configuration/Providers/MetricsDeclarationProvider.cs

Line 34 in 321a3ac

if (string.IsNullOrWhiteSpace(metric.ResourceGroupName))

, although maybe I've misunderstood that.

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.

[tomkerkhove]

No you are correct, I forgot about that 😅 Sorry!

[tomkerkhove]

@adamconnelly Can you change your commits here as well or sign the CLA with @adam-resdiary?

Careful: Just merged master

[promitor-bot]

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

[adamconnelly]

Yeah sure. Sorry - just forgot. I'll do it later today when I've got a minute.

[tomkerkhove]

Thank you!

[adamconnelly]

@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.

[promitor-bot]

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