Skip to content
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

[receiver/azuremonitor] Add parameter to overwrite azure's max record size #32380

Merged
merged 3 commits into from
Jul 11, 2024

Conversation

cmergenthaler
Copy link
Contributor

@cmergenthaler cmergenthaler commented Apr 15, 2024

Description:
When having lots of different records of one dimension in the metric, azure by default only returns 10 of them. This setting adds the possibility to overwrite the default and specify a custom number in the config of the receiver.

Fixes #32165

Testing:
Tested fetching metrics with different configs. Do we need a unit test for this?

Documentation:
Added parameter to README

@nslaughter
Copy link
Contributor

nslaughter commented Apr 16, 2024

Adding flexibility to support more use cases generally makes sense to me. Looking forward to see this touched up to pass CI. Let me know if I can help with anything.

@cmergenthaler
Copy link
Contributor Author

Adding flexibility to support more use cases generally makes sense to me. Looking forward to see this touched up to pass CI. Let me know if I can help with anything.

Have rebased the branch, now CI passed

Copy link
Contributor

github-actions bot commented May 8, 2024

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

@cmergenthaler
Copy link
Contributor Author

@codeboten Can I get a review for this feature please?

Copy link
Contributor

github-actions bot commented Jun 1, 2024

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

@cmergenthaler
Copy link
Contributor Author

@nslaughter @codeboten Anyone here to get this merged?

Copy link
Member

@crobert-1 crobert-1 left a comment

Choose a reason for hiding this comment

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

Could we add some tests here to ensure this config option is applied in the expected way? (To show that it does properly limit the max?)

@cmergenthaler
Copy link
Contributor Author

cmergenthaler commented Jun 13, 2024

Could we add some tests here to ensure this config option is applied in the expected way? (To show that it does properly limit the max?)

Sure, I can add tests but I'm not sure if the tests verify what you are asking for here. The top parameter is applied to the Azure API and the API limits the max (so outside the receiver itself) and since we are not having tests with explicit Azure API calls here, it is hard to verify that the parameter is actually limiting the returned results. What I can test though is if the parameter is applied to the API call correctly. Would that be okay for you?

@crobert-1
Copy link
Member

Could we add some tests here to ensure this config option is applied in the expected way? (To show that it does properly limit the max?)

Sure, I can add tests but I'm not sure if the tests verify what you are asking for here. The top parameter is applied to the Azure API and the API limits the max (so outside the receiver itself) and since we are not having tests with explicit Azure API calls here, it is hard to verify that the parameter is actually limiting the returned results. What I can test though is if the parameter is applied to the API call correctly. Would that be okay for you?

That's a fair point. If you're able to verify the argument is being passed to the API properly that's enough for me. It's a pretty simple change here passing in the value, so I'm not too worried about it.

Copy link
Contributor

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

@github-actions github-actions bot added the Stale label Jun 28, 2024
@github-actions github-actions bot removed the Stale label Jul 9, 2024
@crobert-1 crobert-1 added the ready to merge Code review completed; ready to merge by maintainers label Jul 9, 2024
@codeboten codeboten merged commit 9a3e1de into open-telemetry:main Jul 11, 2024
161 checks passed
@github-actions github-actions bot added this to the next release milestone Jul 11, 2024
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.

[receiver/azuremonitor] Add the ability to scrape more than 10 records
5 participants