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

Add annotation metrics #1495

Closed
wants to merge 5 commits into from
Closed

Conversation

sepich
Copy link

@sepich sepich commented Jun 4, 2021

What this PR does / why we need it:
This is continuation of #1041:
Provides a metric which exposes annotations for that particular API kind. By default it is disabled, but user could specify annotation keys to expose per-each API kind.

kube_namespace_labels{namespace="name"} 1

Sorry for large PR size, but this feature affects almost all families.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #941

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 4, 2021
@k8s-ci-robot
Copy link
Contributor

Welcome @sepich!

It looks like this is your first PR to kubernetes/kube-state-metrics 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kube-state-metrics has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sepich
To complete the pull request process, please assign brancz after the PR has been reviewed.
You can assign the PR to them by writing /assign @brancz in a comment when ready.

The full list of commands accepted by this bot can be found 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 requested review from brancz and mrueg June 4, 2021 14:23
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Jun 4, 2021
@lilic
Copy link
Member

lilic commented Jun 7, 2021

Hey, did you see there is #1468. Can you explain the diff between this one and that one, thanks!

@sepich
Copy link
Author

sepich commented Jun 7, 2021

Sorry, i haven't seen #1468 as it not linked to the issue.
I've checked the implementation and the difference is:

  • consider starting ksm with:
    --metric-labels-allowlist=pods=[team] --metric-annotations-allowlist=namespaces=[channel]
    In 1468 for each API type (family) metrics like this would be created
kube_<kind>_labels{namespace="ns",<kind>="name"} 1
kube_<kind>_annotations{namespace="ns",<kind>="name"} 1

But only kube_pod_labels and kube_namespace_annotations would have useful additional metric labels.
In my case only these metrics would be exposed:

kube_pod_labels{namespace="...",pod="...",label_team="..."} 1
kube_namespace_annotations{namespace="ns",annotation_channel="..."} 1

(So, no kube_deployment_labels for example)
Which is more in line with --metric-allowlist usage.
(In case starting ksm without --metric-labels-allowlist/--metric-annotations-allowlist cli arguments set, this PR would reduce unneeded cardinality. Idea is from #941 (comment)

By default this would be all disabled and users could just enable annotations they are interested in.

  • this PR has test cases for both Labels and Annotations, that's why its size is bigger

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2021
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 7, 2021
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 13, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2021
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2021
@dgrisonnet
Copy link
Member

Since @sylr started looking into this feature first, #1468 supersedes this PR.

That said, both of you could collaborate to make this feature better. Since you have invested some time to look into this feature @sepich, your opinion can be very valuable to improve #1468.

@k8s-ci-robot
Copy link
Contributor

@sepich: PR needs rebase.

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.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 19, 2021
@mrueg
Copy link
Member

mrueg commented Oct 27, 2021

#1468 has been merged, so closing this one out. Feel free to reopen or create a new one if any changes are missing!

@mrueg mrueg closed this Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

annotation metrics, what is the future direction?
5 participants