-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
removing _total prefix from nginx guage metrics #1912
Conversation
@noqcks: Reiterating the mentions to trigger a notification: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign @brancz |
I would suggest removing |
I might actually reverse these metrics.
But, thinking more, is it valid to
|
FYI @loburm |
We need to add a note in the breaking section of the release notes. |
@noqcks you can use |
9e3123d
to
6b9993b
Compare
@noqcks please squash the commits |
@noqcks is this PR ready to be reviewed/merged? |
updating Go names to not use underscores accidental tab autocomplete 🔪
c9e0b6b
to
fae55e8
Compare
squashed commits. ready for review @aledbf |
@brancz @discordianfish please review |
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.
LGTM
@SuperQ sorry I forgot to add you in the comment :) |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aledbf, brancz, noqcks The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Awesome, that was quick! |
What this PR does / why we need it: This fixes #1509. We want to be following best practices! https://prometheus.io/docs/practices/naming/
Which issue(s) this PR fixes:
Fixes #1509 and corrects #1910
Release note:
I'm not sure if we would like to remove the
current_
prefix from the gauges if that's implied without some kind of total prefix, but I think this fixes the issues @discordianfish and @brancz had with #1910.Side note: does this need some kind of release note action to notify people using these metrics since it could break their monitoring/alerting?
tagging @kubernetes/sig-instrumentation-pr-reviews for input.