Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add const labels to metrics #64

Merged
merged 4 commits into from
Sep 22, 2022

Conversation

bergmannf
Copy link
Contributor

This PR adds const labels to metrics:

This is a newer version of #49, but I couldn't reopen the old PR, due to having force-pushed additional commits.

@bergmannf bergmannf force-pushed the add-aws-account-id-to-metrics branch 2 times, most recently from 6a18e99 to 1871d7b Compare September 13, 2022 07:54
route53.go Outdated
HostedZonesPerAccountUsage: prometheus.NewDesc(prometheus.BuildFQName(namespace, "", "route53_hostedzonesperaccount_total"), "Number of Resource records", []string{}, nil),
LastUpdateTime: prometheus.NewDesc(prometheus.BuildFQName(namespace, "", "route53_last_updated_timestamp_seconds"), "Last time, the route53 metrics were sucessfully updated", []string{}, nil),
RecordsPerHostedZoneQuota: prometheus.NewDesc(prometheus.BuildFQName(namespace, "", "route53_recordsperhostedzone_quota"), "Quota for maximum number of records in a Route53 hosted zone", []string{"hostedzoneid", "hostedzonename"}, constLabels),
RecordsPerHostedZoneUsage: prometheus.NewDesc(prometheus.BuildFQName(namespace, "", "route53_recordsperhostedzone_total"), "Number of Resource records", []string{"hostedzoneid", "hostedzonename"}, constLabels),
Copy link
Contributor

@mrWinston mrWinston Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RecordsPerHostedZones acutally do have their own quota code: L-E209CC9F

I think, we should include that as well

@janboll
Copy link
Contributor

janboll commented Sep 20, 2022

Please also check this ongoing discussion, cause it seems related: #62 (comment)
Our intention is to add a metric, that holds the labels, as done i.e. in kube-state-metrics. I think this would be a good addition to it, WDYT?

@bergmannf
Copy link
Contributor Author

@janboll thanks for the pointer - I think we can live with that solution - it would make the app-interface queries a bit more complex, but I think that shouldn't be a big issue, as we only have to get one working and the rest is 90% copy-paste.

So if that is the approach you will use, I will update our static labels to use the same approach - I certainly don't want 2 divergent solutions to the same problem in this codebase.

@mrWinston WDYT?

@bergmannf
Copy link
Contributor Author

After trying to migrate the metrics to use a single metric for constant labels, I am not so sure this works well for the metrics in the privatelink exporters.
The problem I mainly see is, that the metrics do not have a common identifier like the RDS metrics do (dbinstance_indentifier), but instead some even have no real identifier at all:

  • VpcPerRegion
  • HostedZonesPerAccount
  • TransitGateways

This makes writing the group expression quite annoying, as we either have to rename the (primary) identifier to id and add this to the constant label as well - or relabel the metrics - but I feel that might make it a lot more complex than the current solution.
So maybe it would be easier to stick to the current approach as the metrics are missing a common identifier that they could share with the constant metric.

This will help in quickly identifying which account requires action, if
an alerts pops up.
This can later be used for automation purposes.
@AlexVulaj
Copy link

Seconding @bergmannf 's point above - the lack of a common ID for our metrics would make writing the group expressions grow in complexity for what I don't see is much gain. I'm personally in favor with the current approach for our needs here.

For example, in the following metrics there is no common id in the labels that we could group our metrics on easily. We could add all of the IDs to the labels for every metric, but this would lead to empty labels in multiple places (which I'm not sure is posssible).

aws_resources_exporter_vpc_interfacevpcendpointspervpc_usage{aws_account_id="644306948063",aws_region="eu-central-1",quota_code="L-29B6F2EB",service_code="vpc",vpcid="vpc-e6e3758c"} 0

aws_resources_exporter_vpc_routesperroutetable_usage{aws_account_id="644306948063",aws_region="us-east-1",quota_code="L-93826ACB",routetableid="rtb-00d603cf395eb905e",service_code="vpc",vpcid="vpc-088317c5566cbd781"} 1

@bergmannf
Copy link
Contributor Author

@janboll So I think the privatelink metrics are better of with the constant labels being duplicated between the metrics.
Not having a common ID to merge on is no issue for RDS so I think using the k8s approach there is a good idea to save some space, but here it would make the metrics for constant labels too complex.

Would also be great - if this is fine for you - to get this PR reviewed, because @AlexVulaj would like to start working on the app-interface updates to the prometheusrules.

@janboll
Copy link
Contributor

janboll commented Sep 22, 2022

Your argument makes sense! Also the account ID is somewhat of a special case. I added some review comments to get this merged.

pkg/util.go Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@janboll janboll merged commit 0309210 into app-sre:master Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants