receiver/azuremonitor - Added maximumResourcesPerBatch config

[suvaidkhan]

Description

Added a new config maximum_resources_per_batch with a default and max value of 50

Link to tracking issue

Fixes #40112

Testing

Added UTs for config validation tests

Documentation

Added details about the config in receiver/azuremonitorreceiver/README.md

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: suvaidkhan / name: Suvaid (768990b, 6bec990, 52e0973, 1a864d6, 39a013c, f975004, c8c4dc2, e5d81d6, a43e72c, 7686447)

[celian-garcia]

I'm curious, would you be a user of that config field?
Because I mentioned in the issue the possibility to configure that by resource type. If this is something that could be interesting for you maybe you could implement it instead of a global field WDYT?

Also @andrewegel @ahurtaud I would like your opinion on that, since you are in the origins of that idea. #40078 (comment)

[celian-garcia]

Suggested change

	errInvalidMaxResPerBatch = errors.New(`"MaximumResourcesPerBatch" is invalid`)
	errInvalidMaxResPerBatch = errors.New(`"MaximumResourcesPerBatch" is invalid. It should be between 1 and 50`)

[ahurtaud]

I'm curious, would you be a user of that config field? Because I mentioned in the issue the possibility to configure that by resource type. If this is something that could be interesting for you maybe you could implement it instead of a global field WDYT?

Also @andrewegel @ahurtaud I would like your opinion on that, since you are in the origins of that idea. #40078 (comment)

I think it is fine, even more that we dont have much configuration per "resource type".
I would advise users of this feature, if it is require to fine-tune, to create multiple receiver instances for the specifics resources.

something like:

receivers:
  azuremonitor/default:
    use_batch_api: true
    maximum_resources_per_batch: 50
    services:
    - Microsoft.Compute/virtualMachines
    - blahblah
  azuremonitor/throttled:
    use_batch_api: true
    maximum_resources_per_batch: 10
    services:
    - Microsoft.Network/loadBalancers   

[celian-garcia]

I'm curious, would you be a user of that config field? Because I mentioned in the issue the possibility to configure that by resource type. If this is something that could be interesting for you maybe you could implement it instead of a global field WDYT?
Also @andrewegel @ahurtaud I would like your opinion on that, since you are in the origins of that idea. #40078 (comment)

I think it is fine, even more that we dont have much configuration per "resource type". I would advise users of this feature, if it is require to fine-tune, to create multiple receiver instances for the specifics resources.

something like:

receivers:
  azuremonitor/default:
    use_batch_api: true
    maximum_resources_per_batch: 50
    services:
    - Microsoft.Compute/virtualMachines
    - blahblah
  azuremonitor/throttled:
    use_batch_api: true
    maximum_resources_per_batch: 10
    services:
    - Microsoft.Network/loadBalancers   

Having config by resource type is actually something that we do now,

with split dimensions

receivers:
  azuremonitor:
    dimensions:
      enabled: true
      overrides:
        "Microsoft.Network/azureFirewalls":
          # Real example of an Azure limitation here:
          # Dimensions exposed are Reason, Status, Protocol,
          # but when selecting Protocol in the filters, it returns nothing.
          # Note here that the metric display name is ``Network rules hit count`` but it's programmatic value is ``NetworkRuleHit``
          # Ref: https://learn.microsoft.com/en-us/azure/azure-monitor/reference/supported-metrics/microsoft-network-azurefirewalls-metrics
          "NetworkRuleHit": [Reason, Status]

With metrics aggregations specifications

receivers:
  azuremonitor:
    resource_groups:
      - ${resource_groups}
    services:
      - Microsoft.EventHub/namespaces
      - Microsoft.AAD/DomainServices # scraper will fetch all metrics from this namespace since there are no limits under the "metrics" option
    metrics:
      "microsoft.eventhub/namespaces": # scraper will fetch only the metrics listed below:
        IncomingMessages: [total]     # metric IncomingMessages with aggregation "Total"
        NamespaceCpuUsage: [*]        # metric NamespaceCpuUsage with all known aggregations

So to align in that direction, for consistency for users, it might be better to have it here as well. I don't know if it's shocking or not and in which measure this feature will be used. We are not so that's why I was asking to people that would use it ^^.

But yeah, knowing that there is an easy solution and that the feature will probably used in limited conditions, let's not overthink and do it like you said @ahurtaud

[andrewegel]

I've updated #40078 (comment) ; TL;DR: I don't think a maximum_resources_per_batch config option is the way to go, instead, try to catch the QueryThrottledException, and then recursively break down the resource list by half into two separate requests until the problematic resource(s) are found; While its not a fix for problematic resources (in our case, it was a single LB with 250 plus Health Probes on it which increased the Cardinality of the DipAvailability metric beyond what Azure's APIs will allow to be returned) it does prevent the problem of one resource blocking the scraping of other resources, which IMO in the telemetry world is a good thing. I would up writing my own full implementation using inspiration from the receiver, but my code is as follows:

func (r *metricReceiver) tryScrapeLocationResourceType(
	ctx context.Context,
	client *azmetrics.Client,
	resourceType string,
	metricNames []string,
	opts azmetrics.QueryResourcesOptions,
	resourceSet []string) ([]azmetrics.QueryResourcesResponse, error) {

	responses := make([]azmetrics.QueryResourcesResponse, 0)
	var errs error
	resourceSetQueue := list.New()
	resourceSetQueue.PushBack(resourceSet)
	for resourceSetQueue.Len() > 0 {
		currentSet := resourceSetQueue.Remove(resourceSetQueue.Front()).([]string)
		response, err := client.QueryResources(
			ctx,
			r.cfg.SubscriptionID,
			resourceType,
			metricNames,
			azmetrics.ResourceIDList{ResourceIDs: currentSet},
			&opts)
		if err != nil {
			var respErr *azcore.ResponseError
			if errors.As(err, &respErr) {
				// Its unclear if this is a QueryTooExpensive error, but we assume it is
				// because the error code is empty and the status code is 529.
				if respErr.StatusCode == 529 && respErr.ErrorCode == "" {
					// QueryTooExpensive, we need to retry with a smaller set of resources
					r.logger.Warn("Received 529 QueryTooExpensive based on StatusCode and ErrorCode values, breaking up currentSet by half",
						zap.String("resourceType", resourceType),
						zap.Strings("metricNames", metricNames),
						zap.String("aggregationString", *opts.Aggregation),
						zap.Stringp("filterString", opts.Filter),
						zap.Int("queueLength", resourceSetQueue.Len()),
						zap.Int("len(currentSet)", len(currentSet)),
					)
					if len(currentSet) > 1 {
						// Split the current set into two halves and retry
						mid := len(currentSet) / 2
						resourceSetQueue.PushBack(currentSet[:mid])
						resourceSetQueue.PushBack(currentSet[mid:])
					} else {
						// If we only have one resource, we can't split it further, so we log an error as we can't scrape it
						r.logger.Error("Cannot split QueryResources request resourceSet set any further",
							zap.String("resourceType", resourceType),
							zap.Strings("metricNames", metricNames),
							zap.String("aggregationString", *opts.Aggregation),
							zap.Stringp("filterString", opts.Filter),
							zap.Int("len(currentSet)", len(currentSet)),
						)
						errs = errors.Join(errs, fmt.Errorf("received 529 QueryTooExpensive for resource type %s with single resource %s", resourceType, currentSet[0]))
					}
				} else {
					r.settings.Logger.Error("azcore.ResponseError: failed to get Azure Metrics values data",
						zap.String("resourceType", resourceType),
						zap.String("Error", respErr.Error()),
						zap.Int("StatusCode", respErr.StatusCode),
						zap.String("ErrorCode", respErr.ErrorCode),
					)
					errs = errors.Join(errs, respErr)
				}
			} else {
				r.settings.Logger.Error("generic error: failed to get Azure Metrics values data", zap.String("resourceType", resourceType), zap.Error(err))
				errs = errors.Join(errs, err)
				return responses, errs
			}
		} else {
			responses = append(responses, response)
		}
	}
	return responses, errs
}

[celian-garcia]

I've updated #40078 (comment) ; TL;DR: I don't think a maximum_resources_per_batch config option is the way to go, instead, try to catch the QueryThrottledException, and then recursively break down the resource list by half into two separate requests until the problematic resource(s) are found; ...

Thanks @andrewegel ! I answered directly in the issue. This is not incompatible IMHO.

[celian-garcia]

Pretty neat! Looks completely good to me. I can't trigger the approval for workflow but I approve the code.

[andrewegel]

Correct not incompatible with this change, but I did go with a different route

[ahurtaud]

LGTM ready to merge. Thank you !

[suvaidkhan]

Hey @evan-bradley could you please review this!
Thanks.

[evan-bradley]

Suggested change

	credentials: does-not-matter
	credentials: does-not-matter