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

Introduce promlint to guarantee metrics follow Prometheus best practices #86477

Merged
merged 2 commits into from May 25, 2020

Conversation

RainbowMango
Copy link
Member

What type of PR is this?
/kind feature

What this PR does / why we need it:
Introduce the Prometheus tool promlint to guarantee metrics follow best practices.

Which issue(s) this PR fixes:
Follow up: #85173 (comment)

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/apiserver area/cloudprovider area/dependency Issues or PRs related to dependency changes area/kubectl sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Dec 20, 2019
@k8s-ci-robot k8s-ci-robot requested review from cheftako, dims and a team December 20, 2019 10:16
@RainbowMango
Copy link
Member Author

/cc @brancz
@coffeepac

Please take a look.

@brancz
Copy link
Member

brancz commented Dec 20, 2019

Wow that was super fast! Very awesome!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 20, 2019
@RainbowMango
Copy link
Member Author

Then, please @liggitt review dependency changes.
/assign @liggitt

go.mod Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2020
@RainbowMango
Copy link
Member Author

@brancz
I think we should introduce promlint as soon as possible to avoiding new metrics to violate the practice. How about we push forward by replicate the checks by ourselves as per @liggitt 's suggestion?

By the way, I don't know how to do with a binary for this...

@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 19, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brancz, RainbowMango

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel 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 May 19, 2020
@RainbowMango
Copy link
Member Author

/test pull-kubernetes-verify

@RainbowMango
Copy link
Member Author

/test pull-kubernetes-e2e-gce-device-plugin-gpu
/test pull-kubernetes-e2e-gce
/test pull-kubernetes-e2e-kind
/test pull-kubernetes-e2e-kind-ipv6

@RainbowMango
Copy link
Member Author

@brancz Please take a look again.
In favor of #90582, we don't need to import Prometheus anymore.

/priority important-soon
/milestone v1.19

@k8s-ci-robot k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label May 20, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone May 20, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label May 20, 2020
@RainbowMango
Copy link
Member Author

/cc @coffeepac @ehashman @logicalhan
Please take a look, #85173 still waiting for this.

@cblecker cblecker removed the request for review from a team May 24, 2020 21:48
@brancz
Copy link
Member

brancz commented May 25, 2020

Really awesome! lgtm 👍

Once in place, could we open issues for each of these violations? (maybe bundled per component)

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2020
@RainbowMango
Copy link
Member Author

/test pull-kubernetes-conformance-kind-ga-only-parallel

Once in place, could we open issues for each of these violations? (maybe bundled per component)

Sure. I'll clean up all legacy metrics which violate the rules once then.

@k8s-ci-robot k8s-ci-robot merged commit d66bbd8 into kubernetes:master May 25, 2020
@RainbowMango RainbowMango deleted the pr_introduce_promlint branch May 25, 2020 10:54
@dims dims removed their request for review April 7, 2022 01:40
@dims
Copy link
Member

dims commented Aug 23, 2022

test ... ignore me!

@RainbowMango
Copy link
Member Author

I caught you @dims :) :) :)

@dims
Copy link
Member

dims commented Aug 23, 2022

LOL @RainbowMango

@dims
Copy link
Member

dims commented Aug 23, 2022

just so you know... i have this PR in my dashboard ( https://gubernator.k8s.io/pr ) and i am trying to see if a new event helps my dashboard pull latest status (and drop it from my queue since this is already merged! )

:)

@RainbowMango
Copy link
Member Author

Good to know. I never heard of the dashboard before. But seems this PR in my dashboard too with the status Needs Review.

@dims
Copy link
Member

dims commented Sep 19, 2022

dang! i can't get rid of this :)

@RainbowMango
Copy link
Member Author

neither can I.
But I guess there is hidden button named ACK at the end of the item. Maybe you can get rid of this by clicking the button, and you might need to see the button first, e.g. unassign some people to shorten the item.
miracles~

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. area/apiserver area/cloudprovider area/dependency Issues or PRs related to dependency changes area/kubectl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants