Adding initial EndpointSlice metrics. #83257

robscott · 2019-09-27T19:44:51Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR adds some initial EndpointSlice metrics that cover the number of endpoints added or removed per sync, the number of endpoints in each slice, the total number of endpoints desired, and the total number of changes to endpoint slices, grouped by operation.

Special notes for your reviewer:
I'm very open to feedback and ideas for improving/changing these metrics. They've been helpful for me during some EndpointSlice scale testing but I'm interested in any other ideas.

Does this PR introduce a user-facing change?:

Adding initial EndpointSlice metrics.

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

Enhancement Issue: EndpointSlice API enhancements#752

/sig network
/cc @bowei @freehan
/priority important-longterm

freehan

This PR need more discussion.

freehan · 2019-10-03T00:08:04Z

pkg/controller/endpointslice/endpointslice_controller.go

@@ -186,6 +187,8 @@ func (c *Controller) Run(workers int, stopCh <-chan struct{}) {
 		return
 	}

+	endpointslicemetrics.RegisterMetrics()


Is it common practice to do it in Run?

I believe this can be done in init()?

I'm not 100% sure if this works in init(), I was just going off of the few examples I found in other controllers (https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/volume/persistentvolume/pv_controller_base.go#L302 and https://github.com/kubernetes/kubernetes/blob/master/pkg/controller/volume/attachdetach/attach_detach_controller.go#L363)

Their code sample registers in the init(). I'm going to assume either way it fine (threadsafe)

https://godoc.org/github.com/prometheus/client_golang/prometheus

MustRegister grabs the Registry lock:
https://github.com/prometheus/client_golang/blob/master/prometheus/registry.go#L277

Moved it to init, I think it does make more sense there.

freehan · 2019-10-03T00:11:54Z

pkg/controller/endpointslice/metrics/metrics.go

+			Help:           "Number of endpoints added on each Service sync",
+			StabilityLevel: metrics.ALPHA,
+		},
+		[]string{"service_name", "service_namespace"},


I do not think you can use service namespace/name as the labels since they are unbounded in a cluster. This may cause high memory usage for Prometheus collector

bowei · 2019-09-27T21:16:49Z

pkg/controller/endpointslice/metrics/metrics.go

+			Help:           "Number of endpoints added on each Service sync",
+			StabilityLevel: metrics.ALPHA,
+		},
+		[]string{"service_name", "service_namespace"},


We have to be careful of any dimensions that can vary based on # of a class of objects: services, namespaces.

bowei · 2019-10-08T17:31:38Z

pkg/controller/endpointslice/metrics/metrics.go

+			Help:           "The number of EndpointSlice changes",
+			StabilityLevel: metrics.ALPHA,
+		},
+		[]string{"service_name", "service_namespace", "operation"},


Generally speaking, you cannot put user input-derived fields in a metric, as the # of metrics will potentially grow w/out bound, not to mention if they are stored in internal systems, they would constitute PIIl.

robscott · 2019-10-09T20:37:44Z

@bowei and @freehan Thanks for the feedback! I've made some big changes in response to that. The metrics are different now in hopes that they'll be more useful globally without relying on labels. There's also a new efficiency metric that shows the efficiency of endpoint distribution (essentially expected slices / actual slices) with 1 representing ideal allocation and anything less than that being less than ideal.

pkg/controller/endpointslice/metrics/cache.go

freehan · 2019-10-14T20:29:21Z

pkg/controller/endpointslice/metrics/cache.go

+
+// UpdateServicePortCache updates a ServicePortCache in the global cache for a
+// given Service and updates the corresponding metrics.
+func (c *Cache) UpdateServicePortCache(serviceNN types.NamespacedName, spCache ServicePortCache) {


It is worth comment on the detail of input.

I added comments describing the parameters here, wasn't quite sure how to format them though.

pkg/controller/endpointslice/metrics/cache.go

robscott · 2019-10-16T17:08:36Z

/retest

robscott · 2019-10-16T18:16:04Z

/retest

robscott · 2019-10-17T00:05:45Z

@bowei I reworked the ServicePortCache interface a bit so it's not just passing a map around. Not quite sure this is what you were suggesting, but I think it's at least a bit closer.

bowei · 2019-10-16T17:54:47Z

pkg/controller/endpointslice/endpointslice_controller.go

@@ -186,6 +187,8 @@ func (c *Controller) Run(workers int, stopCh <-chan struct{}) {
 		return
 	}

+	endpointslicemetrics.RegisterMetrics()


Their code sample registers in the init(). I'm going to assume either way it fine (threadsafe)

https://godoc.org/github.com/prometheus/client_golang/prometheus

bowei · 2019-10-16T17:56:58Z

pkg/controller/endpointslice/endpointslice_controller.go

@@ -186,6 +187,8 @@ func (c *Controller) Run(workers int, stopCh <-chan struct{}) {
 		return
 	}

+	endpointslicemetrics.RegisterMetrics()


MustRegister grabs the Registry lock:
https://github.com/prometheus/client_golang/blob/master/prometheus/registry.go#L277

pkg/controller/endpointslice/metrics/cache.go

robscott · 2019-10-17T03:08:09Z

/retest

robscott · 2019-10-17T06:40:37Z

/retest

pkg/controller/endpointslice/metrics/cache.go

robscott · 2019-10-24T02:25:02Z

/retest

robscott · 2019-10-24T16:52:35Z

/retest

robscott · 2019-10-24T17:34:06Z

/retest

robscott · 2019-10-24T18:31:17Z

/retest

freehan

/lgtm
/approve

robscott · 2019-11-04T22:33:43Z

/retest

robscott · 2019-11-04T22:34:48Z

/verify-owners

robscott · 2019-11-05T18:34:29Z

/verify-owners

robscott · 2019-11-05T18:43:47Z

/approve

k8s-ci-robot · 2019-11-05T18:44:35Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, robscott

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:

~~pkg/controller/endpointslice/OWNERS~~ [freehan]
~~pkg/controller/util/endpoint/OWNERS~~ [freehan]

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 requested a review from bowei September 27, 2019 19:44

k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 27, 2019

k8s-ci-robot requested a review from freehan September 27, 2019 19:44

freehan reviewed Oct 3, 2019

View reviewed changes

robscott force-pushed the endpointslice-metrics branch from 5ad4354 to 1b07419 Compare October 8, 2019 17:28

bowei reviewed Oct 8, 2019

View reviewed changes

robscott force-pushed the endpointslice-metrics branch from 1b07419 to be23b14 Compare October 9, 2019 18:54

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 9, 2019

robscott force-pushed the endpointslice-metrics branch 2 times, most recently from ef89228 to cd03db4 Compare October 9, 2019 20:09

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 9, 2019

robscott force-pushed the endpointslice-metrics branch from cd03db4 to 46e19fc Compare October 9, 2019 21:10

freehan reviewed Oct 14, 2019

View reviewed changes

robscott force-pushed the endpointslice-metrics branch from 46e19fc to aa78d94 Compare October 16, 2019 00:41

robscott force-pushed the endpointslice-metrics branch from aa78d94 to e1ff9d6 Compare October 16, 2019 22:40

bowei reviewed Oct 17, 2019

View reviewed changes

robscott force-pushed the endpointslice-metrics branch from e1ff9d6 to 48775e9 Compare October 17, 2019 00:47

bowei reviewed Oct 21, 2019

View reviewed changes

Adding initial EndpointSlice metrics.

724b142

robscott force-pushed the endpointslice-metrics branch from 48775e9 to 724b142 Compare October 24, 2019 00:33

freehan reviewed Oct 29, 2019

View reviewed changes

k8s-ci-robot assigned freehan Oct 29, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2019

robscott mentioned this pull request Oct 29, 2019

Adding owners file for Endpoint controller utils #84527

Merged

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

k8s-ci-robot merged commit 0c32aa8 into kubernetes:master Nov 5, 2019

k8s-ci-robot added this to the v1.17 milestone Nov 5, 2019

robscott deleted the endpointslice-metrics branch March 11, 2021 04:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding initial EndpointSlice metrics. #83257

Adding initial EndpointSlice metrics. #83257

robscott commented Sep 27, 2019

freehan left a comment

freehan Oct 3, 2019

robscott Oct 9, 2019

bowei Oct 16, 2019

bowei Oct 16, 2019

robscott Oct 17, 2019

freehan Oct 3, 2019

bowei Sep 27, 2019

bowei Oct 8, 2019

robscott commented Oct 9, 2019 •

edited

Loading

freehan Oct 14, 2019

robscott Oct 16, 2019

robscott commented Oct 16, 2019

robscott commented Oct 16, 2019

robscott commented Oct 17, 2019

bowei Oct 16, 2019

bowei Oct 16, 2019

robscott commented Oct 17, 2019

robscott commented Oct 17, 2019

robscott commented Oct 24, 2019

robscott commented Oct 24, 2019

robscott commented Oct 24, 2019

robscott commented Oct 24, 2019

freehan left a comment

robscott commented Nov 4, 2019

robscott commented Nov 4, 2019

robscott commented Nov 5, 2019

robscott commented Nov 5, 2019

k8s-ci-robot commented Nov 5, 2019

Adding initial EndpointSlice metrics. #83257

Adding initial EndpointSlice metrics. #83257

Conversation

robscott commented Sep 27, 2019

freehan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented Oct 9, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented Oct 16, 2019

robscott commented Oct 16, 2019

robscott commented Oct 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robscott commented Oct 17, 2019

robscott commented Oct 17, 2019

robscott commented Oct 24, 2019

robscott commented Oct 24, 2019

robscott commented Oct 24, 2019

robscott commented Oct 24, 2019

freehan left a comment

Choose a reason for hiding this comment

robscott commented Nov 4, 2019

robscott commented Nov 4, 2019

robscott commented Nov 5, 2019

robscott commented Nov 5, 2019

k8s-ci-robot commented Nov 5, 2019

robscott commented Oct 9, 2019 •

edited

Loading