-
Notifications
You must be signed in to change notification settings - Fork 2k
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 and ability to exclude. #1041
Conversation
Welcome @jw-s! |
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 fro your PR, it's a large PR so might take a bit to review.
Do you mind fixing the docs tests, they need regeneration, that's why the tests are failing. Thanks!
75b3eea
to
e803725
Compare
e803725
to
eed98f5
Compare
eed98f5
to
e37390d
Compare
3aaefe6
to
e6e5749
Compare
e6e5749
to
26697b5
Compare
@@ -6,3 +6,4 @@ | |||
| kube_certificatesigningrequest_condition | Gauge | `certificatesigningrequest`=<certificatesigningrequest-name> <br> `condition`=<approved\|denied> | STABLE | | |||
| kube_certificatesigningrequest_labels | Gauge | `certificatesigningrequest`=<certificatesigningrequest-name>| STABLE | | |||
| kube_certificatesigningrequest_cert_length | Gauge | `certificatesigningrequest`=<certificatesigningrequest-name>| STABLE | | |||
| kube_certificatesigningrequest_annotations | Gauge | `certificatesigningrequest`=<certificatesigningrequest-name>| EXPERIMENTAL | |
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.
Why don't you document the labels for all new metrics?
E.g. for ConfigMaps, you documented label annotation_CONFIGMAP_ANNOTATION
=<CONFIGMAP_ANNOTATION>, but for CertificateSigningRequest, you don't document all labels.
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.
Good catch, this should be consistent.
@@ -8,7 +8,7 @@ | |||
| kube_persistentvolumeclaim_resource_requests_storage_bytes | Gauge | `namespace`=<persistentvolumeclaim-namespace> <br> `persistentvolumeclaim`=<persistentvolumeclaim-name> | STABLE | | |||
| kube_persistentvolumeclaim_status_condition | Gauge | `namespace` =<persistentvolumeclaim-namespace> <br> `persistentvolumeclaim`=<persistentvolumeclaim-name> <br> `type`=<persistentvolumeclaim-condition-type> <br> `status`=<true\|false\|unknown> | EXPERIMENTAL | | |||
| kube_persistentvolumeclaim_status_phase | Gauge | `namespace`=<persistentvolumeclaim-namespace> <br> `persistentvolumeclaim`=<persistentvolumeclaim-name> <br> `phase`=<Pending\|Bound\|Lost> | STABLE | | |||
|
|||
| kube_persistentvolumeclaim_annotations | Gauge | `annotation_PERSISTENTVOLUMECLAIM_ANNOTATION`=<PERSISTENTVOLUMECLAIM_ANNOTATION> <br> `persistentvolumeclaim`=<persistentvolumeclaim-name> <br> `namespace`=<persistentvolumeclaim-namespace> | EXPERIMENTAL | | |||
Note: |
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.
Missing an empty line between the table and the line Note.
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.
Two other remarks:
-
the
--annotation-allowlist
is not intuitive to use because filtering is done on annotations already transformed to Prometheus labels.
For example, to see the annotationvolumes.kubernetes.io/controller-managed-attach-detach
, you need to pass--annotation-allowlist="volumes_kubernetes_io_controller_managed_attach_detach"
. That's neither the real K8s annotation, nor the effective prometheus label (which isannotation_volumes_kubernetes_io_controller_managed_attach_detach
). -
When annotations are filtered out, we have this kind of results:
# HELP kube_namespace_annotations Kubernetes annotations converted to Prometheus labels. # TYPE kube_namespace_annotations gauge kube_namespace_annotations 1 kube_namespace_annotations 1 kube_namespace_annotations 1 kube_namespace_annotations 1 kube_namespace_annotations 1 kube_namespace_annotations 1
I guess we should have either the other labels (
namespace
in that case) or no metric at all...
Completely agree, wanted to discuss this with everyone before doing the implementation. I feel we should allow (whitelist) default annotations i.e namespace or should we allow all metrics default labels? |
Having namespace by default included in the metric makes sense, users who don't want this can exclude this metric entirely, but not having the namespace label by default doesn't make the metric useful. cc @brancz @tariq1890 any objections? |
Only problem with this approach is it will only work for namespace scoped objects , my feeling was we include all metrics default labels. |
4d6c590
to
6a2e62d
Compare
Just pushed the changes to allow each resource's Labels by default. What do you guys think? Will also make the cli argument name more intuitive. |
@tariq1890 @brancz please also have a look |
@@ -0,0 +1,40 @@ | |||
package store |
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.
This needs a license header
@@ -0,0 +1,40 @@ | |||
package store |
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.
Can we change the name to default_labels.go
?
docs/cli-arguments.md
Outdated
@@ -27,6 +27,7 @@ $ kube-state-metrics -h | |||
Usage of ./kube-state-metrics: | |||
--add_dir_header If true, adds the file directory to the header | |||
--alsologtostderr log to standard error as well as files | |||
--annotation-allowlist string Comma-separated list of annotation labels to be exposed. This list comprises of exact annotation label names and/or regex patterns. |
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.
@@ -51,6 +53,10 @@ func TestEndpointStore(t *testing.T) { | |||
Labels: map[string]string{ | |||
"app": "foobar", | |||
}, | |||
Annotations: map[string]string{ | |||
"whitelisted": "true", |
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.
The usage of whitelist and blacklist has some rather problematic undertones. As we are trying to be rid of them for the next major release, we should stick to allowed/denied
.
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.
Agreed!
@jw-s Thank you so much for your hard work with this PR. It is really appreciated :). I've provided my first round of comments. Looks like you will need to rebase the PR as well. Sorry for the delay. |
Yes if its possible to reuse them then yes! To make it clear we want to end up with the following by default:
Hope that makes more sense now? |
f784dc7
to
2700ea5
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jw-s 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 |
65f257f
to
901150c
Compare
@jw-s is this ready for another review? |
@lilic Yeah, though e2e tests are failing and no useful output in the logs, need to check why... |
901150c
to
ad1105a
Compare
@lilic Sorry i didn't have time to check this. Would be nice to get this in before the cut. Any way i can test the e2e locally? |
@jw-s: 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. |
Yes you should be able to run them with |
@jw-s could you possibly split the adding default labels into a separate PR and we can merge as bugfix in 2.0, as those should land in 2.0 release and annotation label metrics can land in a future release. Sounds good? |
@lilic Yeah I'll work on a PR tomorrow morning and hopefully get it done before the afternoon so it can be taken in the 2.0 cut. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
@jw-s Any progress on this? |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
Rotten issues close after 30d of inactivity. Send feedback to sig-contributor-experience at kubernetes/community. |
@fejta-bot: Closed this PR. 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. |
What this PR does / why we need it:
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 annotation metrics, what is the future direction? #941