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

updating prometheus metrics names according to naming best practices #1910

Merged

Conversation

noqcks
Copy link
Contributor

@noqcks noqcks commented Jan 16, 2018

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

Special notes for your reviewer:

Release note:

Updated Nginx metrics names according to Prometheus best practices

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 16, 2018
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 16, 2018
@aledbf
Copy link
Member

aledbf commented Jan 16, 2018

@noqcks thank you for doing this

@aledbf
Copy link
Member

aledbf commented Jan 16, 2018

@noqcks please sign the CLA

@noqcks
Copy link
Contributor Author

noqcks commented Jan 16, 2018

Just signed CLA. Commenting to update.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 16, 2018
@coveralls
Copy link

Coverage Status

Coverage increased (+0.05%) to 36.691% when pulling b55fb6c on noqcks:adding-prometheus-metrics-best-practices into a1afc41 on kubernetes:master.

@aledbf
Copy link
Member

aledbf commented Jan 17, 2018

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, 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 /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 17, 2018
@aledbf aledbf merged commit 74451e6 into kubernetes:master Jan 17, 2018
@aledbf
Copy link
Member

aledbf commented Jan 17, 2018

@noqcks thanks!

@discordianfish
Copy link
Contributor

Only counters should have _total suffixes, not gauges. Also keep in mind that changing these metrics might break alerts and dashboards, so these changes should be kept to a minimum.

@brancz: Would it be possible to have a tag or something for "includes metrics" that could be added to PRs that should get reviewed by someone with prometheus experience?

@brancz
Copy link
Member

brancz commented Jan 17, 2018

As this repo is under the Kubernetes org, we can simply use @kubernetes/sig-instrumentation-pr-reviews to tag the team that is aware of changes that involve instrumentation. Please do encourage anyone working on instrumentation related things to tag those.

Otherwise what @discordianfish said regarding counter/gauge naming is correct, and we should oblige to it.

@k8s-ci-robot k8s-ci-robot added the sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. label Jan 17, 2018
aledbf added a commit that referenced this pull request Jan 17, 2018
@aledbf
Copy link
Member

aledbf commented Jan 17, 2018

@discordianfish @brancz can you send a PR to fix counter/gauge naming issue?

aledbf pushed a commit to aledbf/ingress-nginx that referenced this pull request Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make prometheus metric names follow best practices
7 participants