-
Notifications
You must be signed in to change notification settings - Fork 748
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
Conversation
Great call. What about the |
Ahh I missed that one. Fixed. I think that's them all:
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 |
There was a problem hiding this 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
@mogren Also, since this is against master, should the changelog be for 1.4? |
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. |
OK I've changed my changelog entry to |
docs/troubleshooting.md
Outdated
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a nit! Fixed.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM
* Renaming all prometheus metrics to comply with convention * Update and fix changelog
* Renaming all prometheus metrics to comply with convention * Update and fix changelog
Fixes #337
The current metric names are not very informative or consistent with metric and label conventions. Specifically:
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.