pkg/collectors: Renable white-/blacklisting and HELP text #577

mxinden · 2018-11-01T14:34:57Z

What this PR does / why we need it:

Reenable feature to filter the exposition of metric families by their
name. This is now done at startup time, thereby not slowing down the
critical path.
Expose metrics grouped by their metric family and prefix each metric
family with their HELP text. In the future this can easily be extended
to also expose the TYPE text.

(- This also paved the path for sorting the exposed metrics in a
zero-cost manner if this is wanted (//CC @beorn7).)

Relates to #557

mxinden · 2018-11-01T14:35:53Z

pkg/collectors/builder.go

-	batchv1 "k8s.io/api/batch/v1"
-	batchv1beta1 "k8s.io/api/batch/v1beta1"
-	extensions "k8s.io/api/extensions/v1beta1"
+	// 	apps "k8s.io/api/apps/v1beta1"


I have done the refactor only for service.go for now. Thereby one can ignore this change.

mxinden · 2018-11-01T14:39:47Z

This pull request is still in a work-in-progress state. So far the refactoring was only applied to the service.go collector and the overall patch is missing unit- and benchmarking-tests.

I would like to welcome feedback on the overall architecture proposed here.

mxinden · 2018-11-01T14:40:39Z

Also //CC @lilic as this relates to #575.

brancz · 2018-11-01T14:51:40Z

As this is WIP and still needs a substantial amount of work, could we move forward with #575, and rebase these changes on top of that?

metalmatze

LGTM and nice job on the GenerateFunc approach.

brancz · 2018-11-01T15:28:40Z

I'm not sure I'm happy with the amount of concerns in this PR. Could we split this into multiple?

The PRs that I'm seeing in this (each individually a big improvements, but unless separated hard to review):

reintroducing whitelist/blacklist
reintroducing help text (and type?)
changing the interface to simply consume io.Writer (I do like the change)

While I'm very much in favor of just accepting io.Writer as the input parameter, do you think this is something that we could postpone to after the v1.5.0 release? If not we will need to put this through another run of benchmarking as the changes proposed are rather invasive.

beorn7 · 2018-11-02T13:04:32Z

This also paved the path for sorting the exposed metrics in a zero-cost manner if this is wanted

Yes, I would love that.

Also 👍 for HELP and TYPE.

mxinden · 2018-11-05T17:34:01Z

I'm not sure I'm happy with the amount of concerns in this PR. Could we split this into multiple?

Good point @brancz. I have split up the work into the three suggested parts.

While I'm very much in favor of just accepting io.Writer as the input parameter, do you think this is something that we could postpone to after the v1.5.0 release? If not we will need to put this through another run of benchmarking as the changes proposed are rather invasive.

Performance wise this patch includes multiple changes in the hot path that need benchmarking:

Re-enabling white- and blacklisting forces us to have a different marshal logic.
Reintroducing HELP text forces us to save slices of slices of metrics and to loop multiple times over the same slice.
Passing a io.Writer instead of returning strings might influence memory allocation in a unforeseeable way.

Thereby I would suggest including them before the first release candidate. I don't think we need to go through the same thorough testing as @beorn7 and I did before as the general architecture (caching, ...) stays the same. What do you think @brancz?

brancz · 2018-11-05T17:49:27Z

That’s fair, let’s do them and do another round of benchmarking. While we are at it, what do you think of splitting the micro benchmarks into metric rendering into buffer, rendering the metrics request and one that does the whole thing (this one is essentially what we have today)? Plus it would be a nice detail to have the benchmarks show the byte throughout in addition to the Op/s. This would make it easier to see differences when optimizing.

mxinden · 2018-11-05T18:13:50Z

splitting the micro benchmarks

@brancz what are you referring to with "micro benchmarks"? As of today we only have a single benchmark test.

brancz · 2018-11-05T18:21:05Z

That’s what I meant, I should have put the micro in quotes :)

mxinden · 2018-11-09T16:45:52Z

@brancz @ALL any comments on the structural changes in pkg/collectors/builder.go and pkg/collectors/service.go? Otherwise I would follow up to restructure the other collectors as well.

brancz · 2018-11-09T16:51:54Z

Generally I understand the scheme and I think it makes sense, but could we think a bit about how we could reduce the amount of boilerplate that brings with it? I imagine the pod collector being very hard to read once converted. It seems there is a common theme for: value metrics, label metrics, condition metrics and boolean metrics. If we can somehow abstract those further so they're essentially one liners, I think I'd be happy with the approach. I haven't looked too much into that, but it feels like that should be possible.

mxinden · 2018-11-14T13:27:47Z

@brancz would you mind taking another look? I have added all discussed changes (split benchmark, dry out collectors code, ...). If this looks good to you I will:

Rebase on top of changes introduced in Add external name/ip and load balancer ingress to service collector #571 resolving conflicts.
Refactor pod collector.
Do more in depth benchmarking looking for regressions to current master.
Refactor all other collectors.

main_test.go

Reenable feature to filter the exposition of metric families by their name. This is now done at startup time, thereby not slowing down the critical path.

mxinden · 2018-11-20T13:51:56Z

This pull request is now targeting kubernetes:performance-optimization-new. Any further suggestions? Otherwise I will follow up with the rest in future pull requests.

mxinden · 2018-11-20T15:28:13Z

Instead of having one big pull request refactoring all collectors, we will separate the changes into multiple pull requests. As we don't want to break master too much, those pull requests are going into kubernetes:performance-optimization-new for now. We hope that small pull requests will make reviews a lot simpler.

Please speak up if you have alternative suggestions.

Once we refector each of them to the new style, they will be re-introduced. Deleting: - configmap.go - configmap_test.go - cronjob.go - cronjob_test.go - daemonset.go - daemonset_test.go - deployment.go - deployment_test.go - endpoint.go - endpoint_test.go - hpa.go - hpa_test.go - job.go - job_test.go - limitrange.go - limitrange_test.go - namespace.go - namespace_test.go - node.go - node_test.go - persistentvolume.go - persistentvolume_test.go - persistentvolumeclaim.go - persistentvolumeclaim_test.go - poddisruptionbudget.go - poddisruptionbudget_test.go - replicaset.go - replicaset_test.go - replicationcontroller.go - replicationcontroller_test.go - resourcequota.go - resourcequota_test.go - secret.go - secret_test.go - statefulset.go - statefulset_test.go

mxinden · 2018-11-20T16:30:43Z

@brancz tests are green. Any further comments?

brancz

Let’s address my comment in a follow up.

/lgtm

brancz · 2018-11-20T16:48:52Z

main.go

@@ -40,6 +40,7 @@ import (
 	kcollectors "k8s.io/kube-state-metrics/pkg/collectors"
 	"k8s.io/kube-state-metrics/pkg/options"
 	"k8s.io/kube-state-metrics/pkg/version"
+	"k8s.io/kube-state-metrics/pkg/whiteblacklist"


Can we already call this allow/deny list as planned for 2.0 and just keep the flags called as they are called today?

@brancz I would prefer to do this in one go to avoid confusion. Is that also ok?

also fine by me

k8s-ci-robot · 2018-11-20T16:52:25Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~OWNERS~~ [brancz]

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 added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 1, 2018

k8s-ci-robot requested review from andyxning and zouyee November 1, 2018 14:35

mxinden commented Nov 1, 2018

View reviewed changes

mxinden force-pushed the filter-and-help branch from 2b6d84c to d72fb54 Compare November 1, 2018 14:37

metalmatze reviewed Nov 1, 2018

View reviewed changes

mxinden mentioned this pull request Nov 1, 2018

Making kube-state-metrics a library #575

Merged

mxinden force-pushed the filter-and-help branch from d72fb54 to e2c117d Compare November 5, 2018 17:27

mxinden force-pushed the filter-and-help branch from e2c117d to fc40ef3 Compare November 5, 2018 18:00

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 Nov 14, 2018

lilic reviewed Nov 15, 2018

View reviewed changes

main_test.go Outdated Show resolved Hide resolved

main_test.go Show resolved Hide resolved

main_test.go Show resolved Hide resolved

mxinden force-pushed the filter-and-help branch from 5aaa995 to 55de851 Compare November 16, 2018 10:51

mxinden mentioned this pull request Nov 19, 2018

Add kube_pod_container_status_last_terminated_reason #535

Merged

mxinden added 2 commits November 19, 2018 10:59

pkg/collectors: Renable white-/blacklisting

6b8ae5c

Reenable feature to filter the exposition of metric families by their name. This is now done at startup time, thereby not slowing down the critical path.

pkg/collectors: Reintroduce help text

ca3dd9a

mxinden changed the base branch from performance-optimizations to performance-optimization-new November 20, 2018 13:50

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 Nov 20, 2018

mxinden force-pushed the filter-and-help branch from e9b1250 to 024ea1c Compare November 20, 2018 15:24

mxinden force-pushed the filter-and-help branch 4 times, most recently from 15abaf2 to 151d337 Compare November 20, 2018 16:16

mxinden added 3 commits November 20, 2018 17:26

pkg/collectors: Refactor Pod collector

580eeca

pkg/whiteblacklist: Encapsulate white- blacklisting logic into pkg

3d9d0aa

mxinden force-pushed the filter-and-help branch from 151d337 to c49a15d Compare November 20, 2018 16:26

brancz reviewed Nov 20, 2018

View reviewed changes

k8s-ci-robot assigned brancz Nov 20, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 20, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 20, 2018

k8s-ci-robot merged commit 484b53a into kubernetes:performance-optimization-new Nov 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/collectors: Renable white-/blacklisting and HELP text #577

pkg/collectors: Renable white-/blacklisting and HELP text #577

mxinden commented Nov 1, 2018

mxinden Nov 1, 2018

mxinden commented Nov 1, 2018

mxinden commented Nov 1, 2018

brancz commented Nov 1, 2018

metalmatze left a comment

brancz commented Nov 1, 2018

beorn7 commented Nov 2, 2018

mxinden commented Nov 5, 2018

brancz commented Nov 5, 2018 •

edited

Loading

mxinden commented Nov 5, 2018

brancz commented Nov 5, 2018

mxinden commented Nov 9, 2018

brancz commented Nov 9, 2018

mxinden commented Nov 14, 2018

mxinden commented Nov 20, 2018

mxinden commented Nov 20, 2018

mxinden commented Nov 20, 2018

brancz left a comment

brancz Nov 20, 2018

mxinden Nov 27, 2018

brancz Nov 27, 2018

k8s-ci-robot commented Nov 20, 2018

pkg/collectors: Renable white-/blacklisting and HELP text #577

pkg/collectors: Renable white-/blacklisting and HELP text #577

Conversation

mxinden commented Nov 1, 2018

mxinden Nov 1, 2018

Choose a reason for hiding this comment

mxinden commented Nov 1, 2018

mxinden commented Nov 1, 2018

brancz commented Nov 1, 2018

metalmatze left a comment

Choose a reason for hiding this comment

brancz commented Nov 1, 2018

beorn7 commented Nov 2, 2018

mxinden commented Nov 5, 2018

brancz commented Nov 5, 2018 • edited Loading

mxinden commented Nov 5, 2018

brancz commented Nov 5, 2018

mxinden commented Nov 9, 2018

brancz commented Nov 9, 2018

mxinden commented Nov 14, 2018

mxinden commented Nov 20, 2018

mxinden commented Nov 20, 2018

mxinden commented Nov 20, 2018

brancz left a comment

Choose a reason for hiding this comment

brancz Nov 20, 2018

Choose a reason for hiding this comment

mxinden Nov 27, 2018

Choose a reason for hiding this comment

brancz Nov 27, 2018

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 20, 2018

brancz commented Nov 5, 2018 •

edited

Loading