Configurable limit for metrics with multiple dimensions

[DaveOHenry]

Proposal

I was wondering why Promitor does not deliver all metrics for some of our services. Looks like the default limit (at least in the portal) is 10:

Unfortunately I didn't find any configuration option for the limit in Promitor, therefore this feature request.

Component

Scraper

Contact Details

No response

[tomkerkhove]

Ha, interesting! So you always get 10 results and nothing more?

Good point, I'll try to figure it out.

[tomkerkhove]

Ideally you get everything most probably?

[DaveOHenry]

Promitor currently only shows the metrics for 10 backend pools from an Application Gateway instance, even though there are over 30 pools in our environment.
It's most likely the default from Azure metrics api.

[tomkerkhove]

Makes sense, I'll test it that way - Thanks a lot!

[tomkerkhove]

Looks like an API parameter that we'll specify - https://docs.microsoft.com/en-us/rest/api/monitor/metrics/list#uri-parameters

[tomkerkhove]

Ok, turns out we can configure it here:

promitor/src/Promitor.Integrations.AzureMonitor/AzureMonitorClient.cs

Lines 205 to 209 in e1fdb94

	if (string.IsNullOrWhiteSpace(metricFilter) == false)
	{
	var filter = metricFilter.Replace("/", "%2F");
	metricQuery.WithOdataFilter(filter);
	}

I'll get started on this

[tomkerkhove]

I presume you don't want to configure the maximum amount or would you?

[DaveOHenry]

You mean just set the parameter to "maxint32" in the code? That would probably work, but I can imagine that there are edge cases where this is not desired.
I think it's better to leave the choice to the user and maybe expose the parameter in the configuration under "Metric Defaults" if that makes sense.

[tomkerkhove]

I was thinking the same so that's perfect!

[tomkerkhove]

So this is what it would look like:

version: v1
azureMetadata:
  tenantId: ABC
  subscriptionId: XYZ
  resourceGroupName: promitor
metricDefaults:
  aggregation:
    interval: 00:05:00
  labels:
    geo: china
    environment: dev
  scraping:
    # Every minute
    schedule: "0 * * ? * *"
metrics:
 - name: promitor_demo_servicebus_messagecount_discovered
  description: "Average percentage of memory usage on an Azure App Plan"
  resourceType: ServiceBusNamespace
  labels:
    geo: europe
    app: promitor
  azureMetricConfiguration:
    metricName: ActiveMessages
+   limit: 100 # Default: 10 000
    aggregation:
      type: Average
  resources:
  - namespace: promitor-messaging

What do you think? I didn't add it to metricDefaults for now since the new default is 10 000 unless you'd like to use that, then it would be:

version: v1
azureMetadata:
  tenantId: c8819874-9e56-4e3f-b1a8-1c0325138f27
  subscriptionId: 0f9d7fea-99e8-4768-8672-06a28514f77e
  resourceGroupName: promitor
metricDefaults:
+ limit: 100 # Default: None
  aggregation:
    interval: 00:05:00
  labels:
    geo: china
    environment: dev
  scraping:
    # Every minute
    schedule: "0 * * ? * *"
metrics:
 - name: promitor_demo_servicebus_messagecount_discovered
  description: "Average percentage of memory usage on an Azure App Plan"
  resourceType: ServiceBusNamespace
  labels:
    geo: europe
    app: promitor
  azureMetricConfiguration:
    metricName: ActiveMessages
+   limit: 1000 # Default: 10 000
    aggregation:
      type: Average
  resources:
  - namespace: promitor-messaging

[DaveOHenry]

This is an awesome improvement 😃 I'm always a little concerned with changing default values, but in this case it totally makes sense, because most users certainly expect Promitor to return all metrics.

[tomkerkhove]

Yes, that's the only reason why it's going up indeed.

Do you want the metric defaults or not perse?

[DaveOHenry]

A configurable limit in "metricDefaults" doesn't hurt, but I think most people will certainly never change this limit (myself included) 😉