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

Reference closure scoped family generator #1240

Conversation

jw-s
Copy link
Contributor

@jw-s jw-s commented Sep 25, 2020

Signed-off-by: Joel Whittaker-Smith jdws.dev@gmail.com

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 #1239

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 25, 2020
@jw-s jw-s force-pushed the bugfix/fix-filter-label-function-closure branch 2 times, most recently from bf93cd5 to 4328e3c Compare September 28, 2020 07:02
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Sep 28, 2020
@lilic lilic changed the base branch from master to release-2.0 September 28, 2020 07:57
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 28, 2020
@lilic
Copy link
Member

lilic commented Sep 28, 2020

Hey, thanks for this! Will test this out. I changed the base to release-2.0 as per our release process. Do you mind rebasing with the release-2.0 branch as well, thanks!

There are some failing unit tests it seems, do you mind fixing those:

--- FAIL: TestFullScrapeCycle (1.01s)
main_test.go:379: expected:

    kube_pod_labels{namespace="default",pod="pod0"} 1, but got:
    
    kube_pod_labels{pod="pod0",namespace="default"} 1

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and this does fix the bug, thanks again! Just few things left from my previous comment after that we can merge 🎉

@jw-s jw-s force-pushed the bugfix/fix-filter-label-function-closure branch from 4328e3c to 860e88c Compare September 28, 2020 08:29
@jw-s
Copy link
Contributor Author

jw-s commented Sep 28, 2020

Hey, thanks for this! Will test this out. I changed the base to release-2.0 as per our release process. Do you mind rebasing with the release-2.0 branch as well, thanks!

There are some failing unit tests it seems, do you mind fixing those:

--- FAIL: TestFullScrapeCycle (1.01s)
main_test.go:379: expected:

    kube_pod_labels{namespace="default",pod="pod0"} 1, but got:
    
    kube_pod_labels{pod="pod0",namespace="default"} 1

Seems like a flaky test. It's passing now...

@jw-s jw-s requested a review from lilic September 28, 2020 08:44
@lilic
Copy link
Member

lilic commented Sep 28, 2020

Do you mind just for now only including only your patch, so the last commit in this PR against release-2.0, we can do the rest of the 2.0 merges from master as not all are relevant for the release-2.0 branch. Hope that makes sense? Thanks!

@jw-s jw-s force-pushed the bugfix/fix-filter-label-function-closure branch from 860e88c to 0c4f95d Compare September 28, 2020 11:55
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 28, 2020
Signed-off-by: Joel Whittaker-Smith <jdws.dev@gmail.com>
@jw-s jw-s force-pushed the bugfix/fix-filter-label-function-closure branch from 0c4f95d to a112553 Compare September 28, 2020 11:58
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 28, 2020
@jw-s
Copy link
Contributor Author

jw-s commented Sep 28, 2020

@lilic done but the flaky test is failing again, it's weird as the order shouldn't matter since we sort the strings...

@lilic
Copy link
Member

lilic commented Sep 29, 2020

Seems like now the tests failed two times in a row, will rerun, if it passes will open an issue to look into it.

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Thanks again!

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 29, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jw-s, lilic

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 Sep 29, 2020
@k8s-ci-robot k8s-ci-robot merged commit 8772ad3 into kubernetes:release-2.0 Sep 29, 2020
@jimmyseto
Copy link
Contributor

@jw-s - thanks for this PR!

@jw-s @lilic - in testing the latest off the release-2.0 branch, i'm able to confirm that we now get tags/labels for most of the metrics. but, i notice that the kube_*_label metrics still don't have them. is this an expected change for v2.0 to reduce/limit cardinality?

@lilic
Copy link
Member

lilic commented Oct 1, 2020

@jimmyseto yes, this is expected. https://github.com/kubernetes/kube-state-metrics/releases/tag/v2.0.0-alpha let me know if its not clear from the changelog, this is the PR that introduces a flag to control this #1125 and issue #1047.

Help in testing the new flag is always appreciated!

@brancz to double-check as he helped that effort.

@jimmyseto
Copy link
Contributor

jimmyseto commented Oct 1, 2020

@jimmyseto yes, this is expected. https://github.com/kubernetes/kube-state-metrics/releases/tag/v2.0.0-alpha let me know if its not clear from the changelog, this is the PR that introduces a flag to control this #1125 and issue #1047.

Help in testing the new flag is always appreciated!

@brancz to double-check as he helped that effort.

thanks for the pointer, @lilic. i tried using the flag, but it doesn't seem to be working. or, perhaps, i'm not using it correctly. here's an example:

kubectl describe pod kube-state-metrics-756fb7d894-x6f22
...
Labels: app.kubernetes.io/name=kube-state-metrics
app.kubernetes.io/version=2.0.0-alpha
pod-template-hash=3129638450
testlabel=foobar
...
Containers:
kube-state-metrics:
...
Args:
--resources=pods
--labels-allow-list=kube_pod_labels=[testlabel]
...

curl -s -X GET http://kube-state-metrics.kube-system.svc.cluster.local:8080/metrics |grep pod_labels
...
kube_pod_labels 1
...

also, with the new flag, is it possible to use wildcards to get all labels for a given metric?

@jw-s
Copy link
Contributor Author

jw-s commented Oct 1, 2020

@jimmyseto labels are prefixed with label_ for labels and annotation_ for annotations. I can confirm it's working with --labels-allow-list=kube_pod_labels=[namespace, label_app] for example.
Wildcards are not currently supported for labels. By default all non _labels/_annotations metrics include all their labels.

@jw-s jw-s deleted the bugfix/fix-filter-label-function-closure branch October 1, 2020 18:39
@jimmyseto
Copy link
Contributor

@jimmyseto labels are prefixed with label_ for labels and annotation_ for annotations. I can confirm it's working with --labels-allow-list=kube_pod_labels=[namespace, label_app] for example.
Wildcards are not currently supported for labels. By default all non _labels/_annotations metrics include all their labels.

thanks @jw-s . that did the trick. forgot about the prefix.

i do think it would be helpful to support wildcards. i can see scenarios in which users may just want to scrape all labels for a particular k8s resource, not knowing exactly what they are looking for. are there plans to add support for this?

@lilic
Copy link
Member

lilic commented Oct 2, 2020

i do think it would be helpful to support wildcards. i can see scenarios in which users may just want to scrape all labels for a particular k8s resource, not knowing exactly what they are looking for. are there plans to add support for this?

@jimmyseto sounds good, feel free to open an issue for it so we can discuss there! 🎉 Thanks!

@jimmyseto
Copy link
Contributor

i do think it would be helpful to support wildcards. i can see scenarios in which users may just want to scrape all labels for a particular k8s resource, not knowing exactly what they are looking for. are there plans to add support for this?

@jimmyseto sounds good, feel free to open an issue for it so we can discuss there! 🎉 Thanks!

will do! thanks!

@jimmyseto
Copy link
Contributor

i do think it would be helpful to support wildcards. i can see scenarios in which users may just want to scrape all labels for a particular k8s resource, not knowing exactly what they are looking for. are there plans to add support for this?

@jimmyseto sounds good, feel free to open an issue for it so we can discuss there! 🎉 Thanks!

will do! thanks!

created #1246

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tags/labels missing for daemonset, deployment, and replicaset metrics
4 participants