Skip to content

[receiver/azuremonitorreceiver] feat: add subscription name resource attribute #39029

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

Conversation

celian-garcia
Copy link
Member

Description

The subscription name is often more speaking for people playing with metrics, than the subscription id which is an uuid.
Hence this PR.

Link to tracking issue

Linked with #38895. I prefer to have a separate PR for the add of subscription name in resource attributes to have one PR by feature.

Testing

Added unit tests + new mock API to mock the new get by subscription calls.

Documentation

@github-actions github-actions bot requested a review from nslaughter March 28, 2025 07:41
@celian-garcia celian-garcia force-pushed the azuremonitorreceiver/add-subscription-name branch 4 times, most recently from b06e2ac to bae186c Compare March 28, 2025 08:54
@celian-garcia celian-garcia marked this pull request as ready for review March 28, 2025 10:40
@celian-garcia celian-garcia requested a review from a team as a code owner March 28, 2025 10:40
@celian-garcia celian-garcia force-pushed the azuremonitorreceiver/add-subscription-name branch from bae186c to 165023e Compare March 31, 2025 09:50
@celian-garcia celian-garcia requested a review from dehaansa March 31, 2025 09:51
@celian-garcia
Copy link
Member Author

@dehaansa, from your comments I updated so the GET subscription by ID is only done if the subscription name resource attribute is enabled in the config.
I'm also providing a list of the Azure API calls done in the README.md, how many times, and in which conditions.
WDYT?

Copy link
Contributor

@dehaansa dehaansa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with a documentation nit. Will give codeowners a few days to respond before trying to move forward to merge.

```yaml
conditions:
- discover_subscriptions is true
cardinality: once
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a paged API, right? So it may be more than once depending on the number of subscriptions. Same for other List endpoints.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true yes, I updated with that precision. Unfortunately I didn't find the page size in the API documentation or client codes. This is a mystery that Azure seems to keep secret deep in the backend ^^'

@celian-garcia celian-garcia force-pushed the azuremonitorreceiver/add-subscription-name branch from 2b0ca13 to f4ab492 Compare April 1, 2025 10:15
@dehaansa dehaansa added ready to merge Code review completed; ready to merge by maintainers and removed waiting-for-code-owners labels Apr 2, 2025
@dehaansa
Copy link
Contributor

dehaansa commented Apr 2, 2025

I need to do a better job checking the codeowners file before marking the waiting-for-code-owners label, oops!

@celian-garcia
Copy link
Member Author

I need to do a better job checking the codeowners file before marking the waiting-for-code-owners label, oops!

Aah my bad 😁 I thought it was meaning code owners with bigger rights. Btw the other owner of that receiver stopped answering months ago. I think he's not working on that at all anymore.

@celian-garcia
Copy link
Member Author

I think the Prometheus tests failing are flaky ones. It seems not related. I tried to update branch but it didn't change. It's not urgent anyway, I can wait fo a fix.

@dehaansa
Copy link
Contributor

dehaansa commented Apr 2, 2025

Just confirming, the prometheus-compliance-tests are broken until prometheus/compliance#160 is resolved. I attempted to disable the workflow at the repository level but it may not have worked right, looking into it.

@atoulme atoulme merged commit 9a743c6 into open-telemetry:main Apr 7, 2025
171 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 7, 2025
dmathieu pushed a commit to dmathieu/opentelemetry-collector-contrib that referenced this pull request Apr 8, 2025
…attribute (open-telemetry#39029)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
The subscription name is often more speaking for people playing with
metrics, than the subscription id which is an uuid.
Hence this PR. 

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Linked with open-telemetry#38895. I prefer to have a separate PR for the add of
subscription name in resource attributes to have one PR by feature.

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Added unit tests + new mock API to mock the new get by subscription
calls.

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

Signed-off-by: Célian Garcia <[email protected]>
Co-authored-by: Antoine Toulme <[email protected]>
LucianoGiannotti pushed a commit to LucianoGiannotti/opentelemetry-collector-contrib that referenced this pull request Apr 9, 2025
…attribute (open-telemetry#39029)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
The subscription name is often more speaking for people playing with
metrics, than the subscription id which is an uuid.
Hence this PR. 

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Linked with open-telemetry#38895. I prefer to have a separate PR for the add of
subscription name in resource attributes to have one PR by feature.

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Added unit tests + new mock API to mock the new get by subscription
calls.

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

Signed-off-by: Célian Garcia <[email protected]>
Co-authored-by: Antoine Toulme <[email protected]>
Fiery-Fenix pushed a commit to Fiery-Fenix/opentelemetry-collector-contrib that referenced this pull request Apr 24, 2025
…attribute (open-telemetry#39029)

<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
#### Description
The subscription name is often more speaking for people playing with
metrics, than the subscription id which is an uuid.
Hence this PR. 

<!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. -->
#### Link to tracking issue
Linked with open-telemetry#38895. I prefer to have a separate PR for the add of
subscription name in resource attributes to have one PR by feature.

<!--Describe what testing was performed and which tests were added.-->
#### Testing
Added unit tests + new mock API to mock the new get by subscription
calls.

<!--Describe the documentation added.-->
#### Documentation

<!--Please delete paragraphs that you did not use before submitting.-->

Signed-off-by: Célian Garcia <[email protected]>
Co-authored-by: Antoine Toulme <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready to merge Code review completed; ready to merge by maintainers receiver/azuremonitor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants