[receiver/azuremonitor] Return concrete types

[nslaughter]

Component(s)

receiver/azuremonitor

Describe the issue you're reporting

This was raised by @michalpristas as a tangential comment to changes in #30224. I believe that the instance of this receiver's code more idiomatic approach of returning concrete types will make our code more maintainable - likely highlighting where we benefit from the layer of abstraction in interfaces and... where we don't.

[github-actions]

Pinging code owners:

• receiver/azuremonitor: @altuner @codeboten

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[crobert-1]

Sounds like a good idea to me, removing needs triage.

[github-actions]

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

• receiver/azuremonitor: @nslaughter @codeboten

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[michalpristas]

This also sounds like a good first issue, do we have a label like that?

[led0nk]

@michalpristas
Just to make sure i understood the comment from PR #30224, we're talking about silently failing here:

opentelemetry-collector-contrib/receiver/azuremonitorreceiver/scraper.go

Lines 167 to 170 in 9396cd8

	s.armClientOptions = s.getArmClientOptions()
	s.clientResources = s.getArmClient()
	s.clientMetricsDefinitions = s.getMetricsDefinitionsClient()
	s.clientMetricsValues = s.GetMetricsValuesClient()

is this correct?

[github-actions]

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

• receiver/azuremonitor: @nslaughter @codeboten

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[michalpristas]

@led0nk sorry for very late reply
i was referring to this line
client, _ := s.armClientFunc(s.cfg.SubscriptionID, s.cred, s.armClientOptions)

where we ignore error but we return client, that may be nil, but there's no check for nil client going forward

[led0nk]

@led0nk sorry for very late reply i was referring to this line client, _ := s.armClientFunc(s.cfg.SubscriptionID, s.cred, s.armClientOptions)

where we ignore error but we return client, that may be nil, but there's no check for nil client going forward

@michalpristas no worries

Okay i got that, but shouldn't we implement this for those functions also? Because if something fails there, we wouldn't notice either, that's why i was referring to those lines. What do you think?

opentelemetry-collector-contrib/receiver/azuremonitorreceiver/scraper.go

Lines 146 to 149 in 9396cd8

	func (s *azureScraper) getMetricsDefinitionsClient() metricsDefinitionsClientInterface {
	client, _ := s.armMonitorDefinitionsClientFunc(s.cfg.SubscriptionID, s.cred, s.armClientOptions)
	return client
	}

opentelemetry-collector-contrib/receiver/azuremonitorreceiver/scraper.go

Lines 157 to 160 in 9396cd8

	func (s *azureScraper) GetMetricsValuesClient() metricsValuesClient {
	client, _ := s.armMonitorMetricsClientFunc(s.cfg.SubscriptionID, s.cred, s.armClientOptions)
	return client
	}

[led0nk]

@michalpristas while i was working on this issue i found out, that this seems to be already resolved, so i guess we can close the issue, wdyt?

opentelemetry-collector-contrib/receiver/azuremonitorreceiver/scraper.go

Lines 143 to 190 in 282a911

	func (s *azureScraper) getArmClient() (armClient, error) {
	client, err := s.armClientFunc(s.cfg.SubscriptionID, s.cred, s.armClientOptions)
	return client, err
	}
	type metricsDefinitionsClientInterface interface {
	NewListPager(resourceURI string, options *armmonitor.MetricDefinitionsClientListOptions) *runtime.Pager[armmonitor.MetricDefinitionsClientListResponse]
	}
	func (s *azureScraper) getMetricsDefinitionsClient() (metricsDefinitionsClientInterface, error) {
	client, err := s.armMonitorDefinitionsClientFunc(s.cfg.SubscriptionID, s.cred, s.armClientOptions)
	return client, err
	}
	type metricsValuesClient interface {
	List(ctx context.Context, resourceURI string, options *armmonitor.MetricsClientListOptions) (
	armmonitor.MetricsClientListResponse, error,
	)
	}
	func (s *azureScraper) GetMetricsValuesClient() (metricsValuesClient, error) {
	client, err := s.armMonitorMetricsClientFunc(s.cfg.SubscriptionID, s.cred, s.armClientOptions)
	return client, err
	}
	func (s *azureScraper) start(_ context.Context, _ component.Host) (err error) {
	if err = s.loadCredentials(); err != nil {
	return err
	}
	s.armClientOptions = s.getArmClientOptions()
	s.clientResources, err = s.getArmClient()
	if err != nil {
	return err
	}
	s.clientMetricsDefinitions, err = s.getMetricsDefinitionsClient()
	if err != nil {
	return err
	}
	s.clientMetricsValues, err = s.GetMetricsValuesClient()
	if err != nil {
	return err
	}
	s.resources = map[string]*azureResource{}
	return
	}

[michalpristas]

this was part of the problem, other part was that getArmClient returns armClient that is an interface while Func returns a client,
there's no really a benefit of returning an interface. this issue was about replacing this with concrete type implementation armresources.Client. It should be small refactor. if you evaluate that it is problematic we can close the issue

[github-actions]

This issue has been inactive for 60 days. It will be closed in 60 days if there is no activity. To ping code owners by adding a component label, see Adding Labels via Comments, or if you are unsure of which component this issue relates to, please ping @open-telemetry/collector-contrib-triagers. If this issue is still relevant, please ping the code owners or leave a comment explaining why it is still relevant. Otherwise, please close it.

Pinging code owners:

• receiver/azuremonitor: @nslaughter @codeboten

See Adding Labels via Comments if you do not have permissions to add labels yourself.

[led0nk]

@michalpristas
there was a pr to solve this, but it went to inactive state.
I'm not sure if this is needed anymore ?

[celian-garcia]

Hello, reading the different comments of this issue. I realized that I may have partially resolved it in this PR.
This PR is unrelated, but as I had to create yet a new azure client for subscriptions fetch, I took the opportunity to simplify the way the clients are mocked. Using the right fake API from Azure. Doing so I realized that the way to pass a client by using a function was obsolete as fake just provide client ctor options that can be reused.

So in the end it removes the passed func ctor for different clients instead we pass client ctor options and cancel the need of specific interfaces.
WDYT ? https://github.com/open-telemetry/opentelemetry-collector-contrib/pull/36467/files#diff-4334a6e38c5c4180507005cd07d1d9cf6a39cc4e2f5dffa31693a47d10b642b6L120-L167