feat: SDK migration single resource scraping #2470

hkfgo · 2024-04-20T02:37:14Z

This PR implements the Azure Monitor SDK migration. It migrates away from the deprecated Azure Fluent SDK to Azure SDK for .NET. Specifically, it uses the Azure.Monitor.Query package to implement integration. Since the new SDK is essentially a different wrapper around the same REST API, I'd expect identical behavior in terms of

Billing(free through ARM)
Metric querying
Meta-level metrics to track ARM throttling
Meta-level metrics to track usage

A summary of how I did it:

Abstracted Azure Monitor integration through the IAzureMonitorClient interface.
Implemented Azure Monitor integration using the new SDK, under the IAzureMonitorClient interface
Implemented high-level control flow to use either the new client or the legacy client, depending on feature flag

Things I need help with:

If this looks like a good approach to you
I need some pointers on how to test this :( Specifically, how do you think end-to-end testing should be performed for a major PR like this? I was thinking either integration tests or building a custom branch image and deploying to our test environment. I'm not quite sure how to do either of them though. Some help would be much appreciated!

Relates to #2209

github-actions · 2024-04-20T02:37:25Z

Thank you for your contribution! 🙏 We will review it as soon as possible.

src/Promitor.Integrations.AzureMonitor/HttpPipelinePolicies/RecordArmRateLimitMetricsPolicy.cs

src/Promitor.Integrations.AzureMonitor/IAzureMonitorClient.cs

tomkerkhove · 2024-04-20T12:39:42Z

I need some pointers on how to test this :( Specifically, how do you think end-to-end testing should be performed for a major PR like this? I was thinking either integration tests or building a custom branch image and deploying to our test environment. I'm not quite sure how to do either of them though. Some help would be much appreciated!

This should be covered by the testing sweet so no worries!

tomkerkhove

Thanks for doing this, added some comments but good first step!

src/Promitor.Agents.Scraper/AzureMonitorClientFactory.cs

src/Promitor.Core.Scraping/Configuration/Model/MetricsDeclaration.cs

src/Promitor.Core.Scraping/Configuration/Serialization/v1/Core/V1Deserializer.cs

src/Promitor.Core.Scraping/Configuration/Serialization/v1/Model/MetricsDeclarationV1.cs

src/Promitor.Integrations.AzureMonitor/AzureMonitorClientLegacy.cs

src/Promitor.Integrations.AzureMonitor/AzureMonitorQueryClient.cs

tomkerkhove · 2024-04-20T12:46:26Z

src/Promitor.Integrations.AzureMonitor/HttpPipelinePolicies/RegisterPromitorAgentPolicy.cs

+            var request = message.Request;
+            string agentVersion = Version.Get();
+            var promitorUserAgent = ArmUserAgent.Generate(agentVersion, _metricSinkWriter.EnabledMetricSinks);
+            request.Headers.Remove(HttpHeader.Names.UserAgent);
+            request.Headers.Add(HttpHeader.Names.UserAgent, promitorUserAgent);


This can be moved to a dedicated method and called in respective Process methods, or not?

Same comment here, decided it made more sense to thrown an exception on the synchronous path instead.

src/Promitor.Integrations.AzureMonitor/HttpPipelinePolicies/RegisterPromitorAgentPolicy.cs

src/Promitor.Integrations.AzureMonitor/HttpPipelinePolicies/RecordArmRateLimitMetricsPolicy.cs

hkfgo · 2024-04-21T06:12:41Z

I need some pointers on how to test this :( Specifically, how do you think end-to-end testing should be performed for a major PR like this? I was thinking either integration tests or building a custom branch image and deploying to our test environment. I'm not quite sure how to do either of them though. Some help would be much appreciated!

This should be covered by the testing sweet so no worries!

Are these pipeline steps something I can repeatedly trigger on my own? I read the README more carefully and it seems like I can find my branch build under :pr{pr-id}. If I can re-trigger the CI pipeline on my own then that'd definitely cover end-to-end testing!

tomkerkhove · 2024-04-26T08:40:30Z

I need some pointers on how to test this :( Specifically, how do you think end-to-end testing should be performed for a major PR like this? I was thinking either integration tests or building a custom branch image and deploying to our test environment. I'm not quite sure how to do either of them though. Some help would be much appreciated!

This should be covered by the testing sweet so no worries!

Are these pipeline steps something I can repeatedly trigger on my own? I read the README more carefully and it seems like I can find my branch build under :pr{pr-id}. If I can re-trigger the CI pipeline on my own then that'd definitely cover end-to-end testing!

Hey, there are docs how to run things locally here: https://github.com/tomkerkhove/promitor/blob/master/CONTRIBUTING.md#net-development

The CI also runs all these integration tests automatically in case you were wondering

…ion.cs Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com>

hkfgo · 2024-05-09T07:01:08Z

Looks like all tests are passing now! The failing CodeFactor was on the giant MetricScraperFactory method to find the matching scraper. There's not much we can do I think.

Do you mind taking another look? Please ignore the log statements and modification to GitHub action for now. We can remove those before pressing the merge button

hkfgo · 2024-05-09T07:02:50Z

Also, any pointers on how to do remote debugging? I've found some online articles on remote debugging with VS Code + .NET + Kubernetes. Probably should have tried that to begin with instead of doing so many print statements..

tomkerkhove · 2024-05-16T07:25:48Z

Also, any pointers on how to do remote debugging? I've found some online articles on remote debugging with VS Code + .NET + Kubernetes. Probably should have tried that to begin with instead of doing so many print statements..

No, I always use VS to run the container locally and troubleshoot. If a running instance does not provide the insights you need, then we may be missing some logs

tomkerkhove

Requesting changes to ensure we revert the GitHub action changes, but otherwise it looks good and added some small comments.

Thanks a ton!!

.github/workflows/templates-build-push-image.yml

src/Promitor.Agents.Scraper/Scheduling/ResourcesScrapingJob.cs

src/Promitor.Core.Scraping/Configuration/Serialization/v1/Core/AzureMetadataDeserializer.cs

tomkerkhove · 2024-05-16T07:29:03Z

src/Promitor.Core.Scraping/Configuration/Serialization/v1/Core/AzureMetadataDeserializer.cs

                .MapUsing(DetermineAzureCloud);
        }

+        // TODO: validate cloud configuration in a SDK-agnostic way


Can you open an issue and link to it here please?

I used azureCloud.DetermineMetricsClientAudience() for validation, see comment above

src/Promitor.Core/Metrics/MetricAggregationType.cs

hkfgo · 2024-05-17T20:28:00Z

Also, I believe there should be two quick PRs to

Update documentation
Make useAzureMonitor flag available in the Promitor chart

Be on the look out :)

changelog/content/experimental/unreleased.md

src/Promitor.Core/Metrics/MetricAggregationTypeConverter.cs

hkfgo · 2024-05-25T05:30:54Z

Promitor documentation PR: promitor/docs#62
Promitor chart PR: promitor/charts#168

I believe they are dependencies of the next release but not this PR getting merged. I'm making this distinction because I'm waiting on merge to master to rebase and continue batch scraping work. No rush though. Thanks!

tomkerkhove

Thanks a ton for doing this and not giving up after all my comments!

hkfgo added 3 commits April 17, 2024 23:59

implement logical flow for old and legacy Azure Monitor clients

c1ee71a

implement Azure Monitor integration under new SDK

6a0d4f8

properly implement handlers to track usage and ARM rate limiting metrics

8e7ad70

hkfgo requested a review from tomkerkhove as a code owner April 20, 2024 02:37

trafico-bot bot added the 🔍 Ready for Review Pull Request is not reviewed yet label Apr 20, 2024

hkfgo commented Apr 20, 2024

View reviewed changes

src/Promitor.Integrations.AzureMonitor/HttpPipelinePolicies/RecordArmRateLimitMetricsPolicy.cs Show resolved Hide resolved

hkfgo commented Apr 20, 2024

View reviewed changes

src/Promitor.Integrations.AzureMonitor/IAzureMonitorClient.cs Show resolved Hide resolved

tomkerkhove reviewed Apr 20, 2024

View reviewed changes

hkfgo and others added 3 commits April 23, 2024 18:25

use SDK-agnostic metric aggregation model

b16c000

More SDK-agnostic refactoring to make Promitor build

92409d7

Merge branch 'master' into feat/sdk-migration-single-resource-scraping

3943e03

hkfgo and others added 15 commits April 26, 2024 15:47

log if using new SDK

ba74c67

Merge branch 'master' into feat/sdk-migration-single-resource-scraping

e3991be

more logging

3afc05f

more logging

4a3b313

use new SDK logging context for logger

91cf38e

Update src/Promitor.Core.Scraping/Configuration/Model/MetricsDeclarat…

f40ab66

…ion.cs Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com>

diff to trigger build again :(

a29807e

switch to authentication factory for metric client authentication

b676d83

github action image build

8e94aa6

Merge branch 'master' into feat/sdk-migration-single-resource-scraping

76c2abf

github action image build

d40c1d2

Update templates-build-push-image.yml

5dabb2e

handle no dimension configured case

977beaa

handle no dimension configured case

7a3239e

fix time range bug

727bed6

hkfgo added 2 commits May 8, 2024 23:36

case-insensitive dimension retrieval

81ef6dc

case-insensitive dimension retrieval

09b95b8

more clean up

67de88e

tomkerkhove requested changes May 16, 2024

View reviewed changes

trafico-bot bot added ⚠️ Changes requested Pull Request needs changes before it can be reviewed again and removed 🔍 Ready for Review Pull Request is not reviewed yet labels May 16, 2024

hkfgo added 2 commits May 17, 2024 09:11

address small comments

655964a

update change log

fd9d66e

trafico-bot bot added 🔍 Ready for Review Pull Request is not reviewed yet and removed ⚠️ Changes requested Pull Request needs changes before it can be reviewed again labels May 17, 2024

tomkerkhove reviewed May 23, 2024

View reviewed changes

changelog/content/experimental/unreleased.md Outdated Show resolved Hide resolved

src/Promitor.Core/Metrics/MetricAggregationTypeConverter.cs Show resolved Hide resolved

hkfgo added 8 commits May 24, 2024 17:12

add unit test for MetricAggregationType

ec19284

remove unused exception

e9fd780

remove unused exception

cfd55bd

remove unused exception

be67705

fix code format

1b245df

fix code format

1426a64

remove excessive logging

d169eb8

remove excessive logging

a414ae2

tomkerkhove approved these changes May 27, 2024

View reviewed changes

trafico-bot bot added ✅ Approved Pull Request has been approved and can be merged and removed 🔍 Ready for Review Pull Request is not reviewed yet labels May 27, 2024

tomkerkhove merged commit 7e288ed into tomkerkhove:master May 27, 2024
26 of 27 checks passed

trafico-bot bot added ✨ Merged Pull Request has been merged successfully and removed ✅ Approved Pull Request has been approved and can be merged labels May 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: SDK migration single resource scraping #2470

feat: SDK migration single resource scraping #2470

hkfgo commented Apr 20, 2024 •

edited by tomkerkhove

Loading

github-actions bot commented Apr 20, 2024

tomkerkhove commented Apr 20, 2024

tomkerkhove left a comment

tomkerkhove Apr 20, 2024

hkfgo May 8, 2024 •

edited

Loading

hkfgo commented Apr 21, 2024 •

edited

Loading

tomkerkhove commented Apr 26, 2024

hkfgo commented May 9, 2024

hkfgo commented May 9, 2024

tomkerkhove commented May 16, 2024

tomkerkhove left a comment

tomkerkhove May 16, 2024

hkfgo May 17, 2024

hkfgo commented May 17, 2024 •

edited

Loading

hkfgo commented May 25, 2024

tomkerkhove left a comment

feat: SDK migration single resource scraping #2470

feat: SDK migration single resource scraping #2470

Conversation

hkfgo commented Apr 20, 2024 • edited by tomkerkhove Loading

github-actions bot commented Apr 20, 2024

tomkerkhove commented Apr 20, 2024

tomkerkhove left a comment

Choose a reason for hiding this comment

tomkerkhove Apr 20, 2024

Choose a reason for hiding this comment

hkfgo May 8, 2024 • edited Loading

Choose a reason for hiding this comment

hkfgo commented Apr 21, 2024 • edited Loading

tomkerkhove commented Apr 26, 2024

hkfgo commented May 9, 2024

hkfgo commented May 9, 2024

tomkerkhove commented May 16, 2024

tomkerkhove left a comment

Choose a reason for hiding this comment

tomkerkhove May 16, 2024

Choose a reason for hiding this comment

hkfgo May 17, 2024

Choose a reason for hiding this comment

hkfgo commented May 17, 2024 • edited Loading

hkfgo commented May 25, 2024

tomkerkhove left a comment

Choose a reason for hiding this comment

hkfgo commented Apr 20, 2024 •

edited by tomkerkhove

Loading

hkfgo May 8, 2024 •

edited

Loading

hkfgo commented Apr 21, 2024 •

edited

Loading

hkfgo commented May 17, 2024 •

edited

Loading