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

Renaming all prometheus metrics to comply with convention #348

Merged
merged 5 commits into from
Mar 20, 2019
Merged

Renaming all prometheus metrics to comply with convention #348

merged 5 commits into from
Mar 20, 2019

Conversation

max-rocket-internet
Copy link
Contributor

Fixes #337

The current metric names are not very informative or consistent with metric and label conventions. Specifically:

A metric name should have a (single-word) application prefix relevant to the domain the metric belongs to

So in this PR I am prepending them all with awscni_.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@micahhausler
Copy link
Member

Great call. What about the eni_max metric? We need to clearly document this in the release notes

@max-rocket-internet
Copy link
Contributor Author

Hi @micahhausler

What about the eni_max metric?

Ahh I missed that one. Fixed. I think that's them all:

$ grep -r -A2 '= prometheus.' * | grep Name
ipamd/ipamd.go-			Name: "awscni_ipamd_error_count",
ipamd/ipamd.go-			Name: "awscni_ipamd_action_inprogress",
ipamd/ipamd.go-			Name: "awscni_eni_max",
ipamd/ipamd.go-			Name: "awscni_ip_max",
ipamd/ipamd.go-			Name: "awscni_reconcile_count",
ipamd/ipamd.go-			Name: "awscni_add_ip_req_count",
ipamd/ipamd.go-			Name: "awscni_del_ip_req_count",
ipamd/datastore/data_store.go-			Name: "awscni_eni_allocated",
ipamd/datastore/data_store.go-			Name: "awscni_total_ip_addresses",
ipamd/datastore/data_store.go-			Name: "awscni_assigned_ip_addresses",
pkg/awsutils/awsutils.go-			Name: "awscni_aws_api_lantency_ms",
pkg/awsutils/awsutils.go-			Name: "awscni_aws_api_error_count",
pkg/awsutils/awsutils.go-			Name: "awscni_aws_utils_error_count",

We need to clearly document this in the release notes

True, but that's not done in a PR, right? Or you mean the changelog? OK I've added my change there. Also fixed the formatting a added the missing 1.3.2 release.

Copy link
Member

@micahhausler micahhausler left a comment

Choose a reason for hiding this comment

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

@max-rocket-internet Thanks for updating the CHANGELOG!

@mogren do we typically squash commits before merging?

LGTM

@micahhausler
Copy link
Member

@mogren Also, since this is against master, should the changelog be for 1.4?

@mogren
Copy link
Contributor

mogren commented Mar 19, 2019

Yes, we squash commits, and yes, it should be against 1.4.0.

The 1.3.x will be minor patches only, users might be using these metrics so we shouldn't change them in a patch release version.

@max-rocket-internet
Copy link
Contributor Author

it should be against 1.4.0.

OK I've changed my changelog entry to 1.4.0.

awscni_assigned_ip_addresses 46
# HELP awscni_eni_allocated The number of ENI allocated
# TYPE awscni_eni_allocated gauge
awscni_eni_allocated 4
# HELP eni_max The number of maximum ENIs can be attached to the instance
# TYPE eni_max gauge
Copy link
Member

Choose a reason for hiding this comment

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

nit: can you update this name here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a nit! Fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's more under here, called go_xxx. Should I rename those too? I think they come from the prometheus client though.

Copy link
Member

Choose a reason for hiding this comment

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

Correct, those come from the prometheus client and should stay as they are. Thanks!

Copy link
Contributor

@mogren mogren left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@mogren mogren added this to the v1.4 milestone Mar 20, 2019
@mogren mogren merged commit c7bd902 into aws:master Mar 20, 2019
@max-rocket-internet max-rocket-internet deleted the prom_metrics_rename branch March 21, 2019 08:35
rudoi pushed a commit to newrelic-forks/amazon-vpc-cni-k8s that referenced this pull request Apr 1, 2019
* Renaming all prometheus metrics to comply with convention
* Update and fix changelog
mogren pushed a commit to mogren/amazon-vpc-cni-k8s that referenced this pull request Apr 25, 2019
* Renaming all prometheus metrics to comply with convention
* Update and fix changelog
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.

Rename Prometheus metrics to be more conventional
3 participants