[receiver/azuremonitorreceiver] feat: multi subscriptions support and automatic discovery #37167

celian-garcia · 2025-01-13T14:08:59Z

Recreated from a diffferent fork. Context: #36467

Description

This PR allows to discover and scrape all the subscriptions located in the tenant.
I'm adding also at the same time the ability to give multiple subscriptions as it's free and can also be desired.

This is a part of this PR: #29593 that has been split for readability.
Indeed in a next PR, I'll propose the usage of the getBatch of metrics Azure API. Because if your tenant contains a lot of subscription, you can face rate limitations.

It contains also a little refactor of the tests and the "backdoor" used to mock the API. Now the tests are using the way provided by Azure: the fake API. https://github.com/Azure/azure-sdk-for-go/tree/main/sdk/samples/fakes
The "backdoor" is now only the client options. So no weird interfaces nor weird constructor for the Azure clients inside the scraper.go file.

Link to tracking issue

Fixes #36612

Testing

Documentation

Thanks to the tests refactoring, closes #31264 (at best it fixes it, at worst this makes it obsolete), relates also #38639

github-actions · 2025-01-29T05:20:51Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

receiver/azuremonitorreceiver/config.go

receiver/azuremonitorreceiver/scraper.go

github-actions · 2025-02-25T05:20:49Z

This PR was marked stale due to lack of activity. It will be closed in 14 days.

celian-garcia · 2025-03-08T14:43:37Z

ping @andrzej-stencel @jmacd ready for review/merge 🚀

atoulme · 2025-03-09T06:22:42Z

receiver/azuremonitorreceiver/internal/metadata/metrics.go

@@ -232,6 +224,11 @@ func (mb *MetricsBuilder) AddDataPoint(
 	dp.SetStartTimestamp(mb.startTime)
 	dp.SetTimestamp(ts)
 	dp.SetDoubleValue(val)
+
+	if mb.resourceAttributesSettings.AzureMonitorSubscriptionID.Enabled {
+		dp.Attributes().PutStr("azuremonitor.subscription_id", subscriptionID)


maybe do this at the resource level? looks like maybe that's not possible, but the resourceAttributesSettings indicates it should be set on the resource, not the data point

Yeah I struggled a bit with that part, as it looks like if I want to keep this way to enabled/disable subscription, with one subscription per resource, I'd have to emit several bunch of metrics per row. (call several time Emit) Is that something feasible or done by another receiver at your knowledge? I'm not sure of the consequences 🫤 -- I'm checking twice right now and this might be the purpose of EmitForResource I will investigate in that direction.

Otherwise I can also rename this "resource" config for subscriptionID to be "datapoint" to reflect more what it is. Which is a breaking change of the config but as in alpha that's okish. I'm more concern now of the scalability. It's probably better to have one metric resource per susbscription as if you have a lot of subscriptions, it may be a lot of metrics.

ok let me know when is good to review again

@atoulme this is ready now. The subscription id is back a resource attribute. I inspired a lot from other receivers that are using resouce builder and emit for resources. Which forced me to align a bit more the internal with the other receivers that are generated. Not an easy job but now this is done.

This is too bad that the generated code for metrics through metadata generation could not offer more customization. Ideally I would like it to generate all the code for metrics exposure, without having to define statically the metrics in the metadata.yaml.

Anyway here I have something working perfectly and more aligned with recent code so satisfying 🙂

atoulme · 2025-03-17T23:17:22Z

Please address conflicts and mark the PR as ready for review again.

celian-garcia · 2025-03-19T12:48:02Z

receiver/azuremonitorreceiver/testdata/expected_metrics/metrics_filtered.yaml

@@ -1,9 +1,6 @@
 resourceMetrics:


expected_metrics diff is not readable as I sorted by metrics name with yq, specifically to have a readable diff for me.

celian-garcia · 2025-03-19T12:48:14Z

receiver/azuremonitorreceiver/testdata/expected_metrics/metrics_golden.yaml

@@ -1,9 +1,6 @@
 resourceMetrics:


expected_metrics diff is not readable as I sorted by metrics name with yq, specifically to have a readable diff for me. This only this + an additional metric that I added to have a second subscription

celian-garcia · 2025-03-19T12:48:41Z

receiver/azuremonitorreceiver/testdata/expected_metrics/metrics_tags_golden.yaml

@@ -1,9 +1,6 @@
 resourceMetrics:


expected_metrics diff is not readable as I sorted by metrics name with yq, specifically to have a readable diff for me. This only this + an additional metric that I added to have a second subscription

celian-garcia · 2025-03-19T17:34:55Z

/label -waiting-for-author

celian-garcia · 2025-03-19T20:10:47Z

What is this ?

2025/03/19 17:42:34 receiver/azuremonitorreceiver has more than one function: "NewClientOptionsResolver,NewFactory,NewMockClientOptionsResolver"

I have to rename to unexported ?

atoulme · 2025-03-19T20:12:14Z

yes please.

celian-garcia · 2025-03-19T20:14:19Z

Okay I will 👍 . Besides this, this is ready for review.

… automatic discovery Co-authored-by: Joshua MacDonald <[email protected]> Signed-off-by: Célian Garcia <[email protected]>

atoulme · 2025-03-20T17:47:06Z

Nudging the build by merging latest main.

… cleaned because we take subID instead of resID (#38893)  #### Description This is a fix of regression introduced here #37167. We are using this little map to cleanup Azure resources that are not anymore discovered. In this case resources are now stored by subcriptions so ``resources`` should be changed to ``resources[subID]`` to have existing resources.  #### Link to tracking issue Fixes  #### Testing  #### Documentation  Signed-off-by: Célian Garcia <[email protected]>

… automatic discovery (open-telemetry#37167) Recreated from a diffferent fork. Context: open-telemetry#36467 >  > #### Description > > This PR allows to discover and scrape all the subscriptions located in the tenant. > I'm adding also at the same time the ability to give multiple subscriptions as it's free and can also be desired. > > This is a part of this PR: open-telemetry#29593 that has been split for readability. > Indeed in a next PR, I'll propose the usage of the getBatch of metrics Azure API. Because if your tenant contains a lot of subscription, you can face rate limitations. > > > It contains also a little refactor of the tests and the "backdoor" used to mock the API. Now the tests are using the way provided by Azure: the fake API. https://github.com/Azure/azure-sdk-for-go/tree/main/sdk/samples/fakes > The "backdoor" is now only the client options. So no weird interfaces nor weird constructor for the Azure clients inside the ``scraper.go`` file. > >  > #### Link to tracking issue > Fixes open-telemetry#36612 > >  > #### Testing > >  > #### Documentation > >  Thanks to the tests refactoring, closes open-telemetry#31264 (at best it fixes it, at worst this makes it obsolete), relates also open-telemetry#38639 Signed-off-by: Célian Garcia <[email protected]> Co-authored-by: Joshua MacDonald <[email protected]> Co-authored-by: Antoine Toulme <[email protected]>

… cleaned because we take subID instead of resID (open-telemetry#38893)  #### Description This is a fix of regression introduced here open-telemetry#37167. We are using this little map to cleanup Azure resources that are not anymore discovered. In this case resources are now stored by subcriptions so ``resources`` should be changed to ``resources[subID]`` to have existing resources.  #### Link to tracking issue Fixes  #### Testing  #### Documentation  Signed-off-by: Célian Garcia <[email protected]>

celian-garcia requested a review from a team as a code owner January 13, 2025 14:09

celian-garcia requested a review from songy23 January 13, 2025 14:09

github-actions bot assigned andrzej-stencel Jan 13, 2025

github-actions bot added the receiver/azuremonitor label Jan 13, 2025

github-actions bot requested a review from nslaughter January 13, 2025 14:09

celian-garcia mentioned this pull request Jan 13, 2025

REQUEST: New membership for celian-garcia open-telemetry/community#2508

Closed

6 tasks

github-actions bot added Stale and removed Stale labels Jan 29, 2025

atoulme added the waiting-for-code-owners label Feb 5, 2025

jmacd approved these changes Feb 10, 2025

View reviewed changes

receiver/azuremonitorreceiver/config.go Outdated Show resolved Hide resolved

receiver/azuremonitorreceiver/scraper.go Outdated Show resolved Hide resolved

This was referenced Feb 11, 2025

Weekly Report: 2025-02-04 - 2025-02-11 #37818

Closed

Weekly Report: 2025-02-11 - 2025-02-18 #37985

Closed

github-actions bot mentioned this pull request Feb 25, 2025

Weekly Report: 2025-02-18 - 2025-02-25 #38152

Closed

github-actions bot added the Stale label Feb 25, 2025

github-actions bot mentioned this pull request Mar 4, 2025

Weekly Report: 2025-02-25 - 2025-03-04 #38321

Closed

andrzej-stencel removed the Stale label Mar 6, 2025

celian-garcia force-pushed the feat/discovery branch 3 times, most recently from 4decd8a to d4de022 Compare March 8, 2025 14:21

atoulme reviewed Mar 9, 2025

View reviewed changes

github-actions bot mentioned this pull request Mar 11, 2025

Weekly Report: 2025-03-04 - 2025-03-11 #38503

Closed

celian-garcia mentioned this pull request Mar 14, 2025

[receiver/azuremonitor] Improve readability of unit tests mock data #38639

Open

atoulme added waiting for author and removed waiting-for-code-owners labels Mar 17, 2025

atoulme marked this pull request as draft March 17, 2025 23:17

celian-garcia mentioned this pull request Mar 18, 2025

Feature Request for Azure Monitor Metrics Data Plane Batch API Support #38651

Open

celian-garcia force-pushed the feat/discovery branch 2 times, most recently from 66bea01 to 2cfdbc7 Compare March 19, 2025 12:43

celian-garcia commented Mar 19, 2025

View reviewed changes

celian-garcia force-pushed the feat/discovery branch 3 times, most recently from 05b7d55 to 560e99d Compare March 19, 2025 17:27

celian-garcia marked this pull request as ready for review March 19, 2025 17:34

github-actions bot assigned crobert-1 Mar 19, 2025

celian-garcia force-pushed the feat/discovery branch from 560e99d to 4fc3526 Compare March 19, 2025 20:30

[receiver/azuremonitorreceiver] feat: multi subscriptions support and…

f0bf558

… automatic discovery Co-authored-by: Joshua MacDonald <[email protected]> Signed-off-by: Célian Garcia <[email protected]>

celian-garcia force-pushed the feat/discovery branch from 4fc3526 to f0bf558 Compare March 19, 2025 20:48

Merge branch 'main' into feat/discovery

86b1c74

atoulme approved these changes Mar 20, 2025

View reviewed changes

atoulme added ready to merge Code review completed; ready to merge by maintainers and removed waiting for author labels Mar 20, 2025

atoulme approved these changes Mar 20, 2025

View reviewed changes

Merge branch 'main' into feat/discovery

5d2c55a

atoulme merged commit 6ce5926 into open-telemetry:main Mar 21, 2025
171 checks passed

github-actions bot added this to the next release milestone Mar 21, 2025

celian-garcia mentioned this pull request Mar 24, 2025

[chore][receiver/azuremonitorreceiver] fix: azure resources are never cleaned because we take subID instead of resID #38893

Merged

celian-garcia mentioned this pull request Apr 15, 2025

[receiver/azuremonitor] use_batch_api should parallelize the calls by subscription #39417

Open

[receiver/azuremonitorreceiver] feat: multi subscriptions support and automatic discovery #37167

[receiver/azuremonitorreceiver] feat: multi subscriptions support and automatic discovery #37167

Uh oh!

Conversation

celian-garcia commented Jan 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Link to tracking issue

Testing

Documentation

Uh oh!

github-actions bot commented Jan 29, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Feb 25, 2025

Uh oh!

celian-garcia commented Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atoulme Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

celian-garcia Mar 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

atoulme Mar 17, 2025

Choose a reason for hiding this comment

Uh oh!

celian-garcia Mar 19, 2025

Choose a reason for hiding this comment

Uh oh!

atoulme commented Mar 17, 2025

Uh oh!

celian-garcia Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

celian-garcia Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

celian-garcia Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

celian-garcia commented Mar 19, 2025

Uh oh!

celian-garcia commented Mar 19, 2025

Uh oh!

atoulme commented Mar 19, 2025

Uh oh!

celian-garcia commented Mar 19, 2025

Uh oh!

atoulme commented Mar 20, 2025

Uh oh!

Uh oh!

Uh oh!

celian-garcia commented Jan 13, 2025 •

edited

Loading

celian-garcia commented Mar 8, 2025 •

edited

Loading

atoulme Mar 9, 2025 •

edited

Loading

celian-garcia Mar 9, 2025 •

edited

Loading

celian-garcia Mar 19, 2025 •

edited

Loading

celian-garcia Mar 19, 2025 •

edited

Loading

celian-garcia Mar 19, 2025 •

edited

Loading