RobustPerception / azure_metrics_exporter

Azure metrics exporter for Prometheus
Apache License 2.0
133 stars 69 forks source link

Metrics name, tags & help improvements #86

Open rturowicz opened 4 years ago

rturowicz commented 4 years ago

New metrics tags added:

Resource name added to Target, ResourceGroup, and ResourceTag. When set, value is used as metrics name prefix (helpful when many resources from different subscriptions are monitored by one Prometheus instance).

brian-brazil commented 4 years ago

Resource name added to Target, ResourceGroup, and ResourceTag. When set, value is used as metrics name prefix

This is an anti-pattern, see https://www.robustperception.io/target-labels-not-metric-name-prefixes

subscription id - helpful, when metrics from different subscriptions are monitored by one Prometheus instance

This should be a target label as an exporter only handles one subscription, and also it's a secret so should not be exposed as that'd be a security vulnerability.

resource type - helpful, when metrics are filtered by tags

Can you explain more about what this is, and if this information is already available in the current metrics?

rturowicz commented 4 years ago

Resource name added to Target, ResourceGroup, and ResourceTag. When set, value is used as metrics name prefix

This is an anti-pattern, see https://www.robustperception.io/target-labels-not-metric-name-prefixes

subscription id - helpful, when metrics from different subscriptions are monitored by one Prometheus instance

This should be a target label as an exporter only handles one subscription, and also it's a secret so should not be exposed as that'd be a security vulnerability.

thanks for the advises, prefix removed in favor of custom labels

resource type - helpful, when metrics are filtered by tags

Can you explain more about what this is, and if this information is already available in the current metrics?

let's assume resource_tags config as follows:

resource_tags:
  - resource_tag_name: "monitoring"
    resource_tag_value: "enabled"
    aggregations:
      - Minimum
      - Maximum
      - Average
    metrics:
      - name: "cpu_percent"

and pg & ms sql databases with monitoring: enabled tag, then the metrics are:

cpu_percent_percent_average{resource_group="rt-main-group",resource_name="rt-test-pg"} 0.32
cpu_percent_percent_average{resource_group="rt-main-group",resource_name="rt-test-sql",sub_resource_name="rt-test-db"} 0

it's not explicitly marked which metric is for pg and which for ms sql, something like this will be more clear:

cpu_percent_percent_average{resource_group="rt-main-group",resource_name="rt-test-pg",resource_type="Microsoft.DBforPostgreSQL/servers"} 0.26
cpu_percent_percent_average{resource_group="rt-main-group",resource_name="rt-test-sql",resource_type="Microsoft.Sql/servers",sub_resource_name="rt-test-db"} 0
brian-brazil commented 4 years ago

it's not explicitly marked which metric is for pg and which for ms sql, something like this will be more clear:

That sounds plausible, is this something that belongs on every single metric? It is technically a breaking change.

I also see you added a labels field, this has similar issues to the others you already removed from the PR.

goya commented 4 years ago

haha seems we have the same requirements! i have 3 PR's that do the same thing!