opentelemetry-collector-contrib icon indicating copy to clipboard operation
opentelemetry-collector-contrib copied to clipboard

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

Open cmergenthaler opened this issue 10 months ago • 9 comments

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.

Link to tracking Issue: #32165

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

Documentation: Added parameter to README

cmergenthaler avatar Apr 15 '24 11:04 cmergenthaler

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.

nslaughter avatar Apr 16 '24 20:04 nslaughter

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

cmergenthaler avatar Apr 17 '24 12:04 cmergenthaler

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

github-actions[bot] avatar May 08 '24 05:05 github-actions[bot]

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

cmergenthaler avatar May 08 '24 05:05 cmergenthaler

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

github-actions[bot] avatar Jun 01 '24 05:06 github-actions[bot]

@nslaughter @codeboten Anyone here to get this merged?

cmergenthaler avatar Jun 05 '24 06:06 cmergenthaler

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?

cmergenthaler avatar Jun 13 '24 06:06 cmergenthaler

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.

crobert-1 avatar Jun 13 '24 16:06 crobert-1

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

github-actions[bot] avatar Jun 28 '24 05:06 github-actions[bot]