add reflector metrics #48224

deads2k · 2017-06-28T20:31:26Z

This adds metrics (optionally prometheus) to reflectors so that you can see when one reflector is behaving poorly and just how poorly its doing.

@eparis

Adds metrics for checking reflector health.

deads2k · 2017-06-29T18:47:43Z

@smarterclayton my metrics

smarterclayton · 2017-06-29T21:44:34Z

staging/src/k8s.io/client-go/tools/cache/reflector.go

@@ -344,6 +347,12 @@ func (r *Reflector) watchHandler(w watch.Interface, resourceVersion *string, err
 	// Stopping the watcher should be idempotent and if we return from this function there's no way
 	// we're coming back in with the same watch interface.
 	defer w.Stop()
+	// update metrics
+	defer func() {
+		r.metrics.numberOfItemsInWatch.Observe(float64(eventCount))


Same metric twice?

Same metric twice?

just to be extra sure.

Why do you say it's the same metric twice? I don't see other reference to numberOfItemsInWatch.

smarterclayton · 2017-06-29T21:47:19Z

@DirectXMan12 can you @ other sig-instrumentation folks who review consistency on prom metrics?

DirectXMan12

comments inline. Needs some naming changes, and you're silently swallowing errors.

DirectXMan12 · 2017-06-30T16:20:41Z

pkg/util/reflector/prometheus/prometheus.go

+		Name:      "lists",
+		Help:      "Total number of API lists done by the reflector: " + name,
+	})
+	prometheus.Register(lists)


this needs to be MustRegister, otherwise you're silently discarding the error

this needs to be MustRegister, otherwise you're silently discarding the error

doing

DirectXMan12 · 2017-06-30T16:24:32Z

pkg/util/reflector/prometheus/prometheus.go

+}
+
+func (_ prometheusMetricsProvider) NewListDurationMetric(name string) cache.SummaryMetric {
+	listDuration := prometheus.NewSummary(prometheus.SummaryOpts{


I'd add a note on why you chose summary, just in case we decide to revisit the decision later.

I'd add a note on why you chose summary, just can case we decide to revisit the decision later.

I want the averages and percentile analysis.

DirectXMan12 · 2017-06-30T16:25:00Z

pkg/util/reflector/prometheus/prometheus.go

+func (_ prometheusMetricsProvider) NewWatchesMetric(name string) cache.CounterMetric {
+	watches := prometheus.NewCounter(prometheus.CounterOpts{
+		Subsystem: name,
+		Name:      "watches",


if this is actually a counter, the name should have _total on the end.

ditto for the other counters.

if this is actually a counter, the name should have _total on the end.

doing

DirectXMan12 · 2017-06-30T16:26:24Z

pkg/util/reflector/prometheus/prometheus.go

+}
+
+func (_ prometheusMetricsProvider) NewLastResourceVersionMetric(name string) cache.GaugeMetric {
+	rv := prometheus.NewGauge(prometheus.GaugeOpts{


this... is a bit of a strange case. Shouldn't it be a counter? Can the last resource version go down?

Can the last resource version go down?

Maybe. You can imagine cases like etcd backup restoration or a list against a latent etcd.

DirectXMan12 · 2017-06-30T16:30:29Z

pkg/util/reflector/prometheus/prometheus.go

+
+func (_ prometheusMetricsProvider) NewListsMetric(name string) cache.CounterMetric {
+	lists := prometheus.NewCounter(prometheus.CounterOpts{
+		Subsystem: name,


You might want to consider making this a label, so that we can do aggregations or bulk queries in the dashboard (e.g. see lists done by all reflectors) more easily

You might want to consider making this a label, so that we can do aggregations or bulk queries in the dashboard (e.g. see lists done by all reflectors) more easily

Maybe later. I'm not yet convinced that all reflectors are the same (go into the same larger aggregation bucket"

Hrm, I would agree that these are almost always labels - each of these is a series.

deads2k · 2017-06-30T19:56:04Z

@DirectXMan12 comments addressed

caesarxuchao

A few nits, otherwise lgtm.

caesarxuchao · 2017-06-30T21:22:28Z

pkg/util/reflector/prometheus/prometheus.go

+	"github.com/prometheus/client_golang/prometheus"
+)
+
+// Package prometheus sets the cache DefaultMetricsFactory to produce


Move to package declaration to be a package doc.

Move to package declaration to be a package doc.

done

caesarxuchao · 2017-06-30T22:01:14Z

staging/src/k8s.io/client-go/tools/cache/reflector.go

@@ -344,6 +347,12 @@ func (r *Reflector) watchHandler(w watch.Interface, resourceVersion *string, err
 	// Stopping the watcher should be idempotent and if we return from this function there's no way
 	// we're coming back in with the same watch interface.
 	defer w.Stop()
+	// update metrics
+	defer func() {
+		r.metrics.numberOfItemsInWatch.Observe(float64(eventCount))


Why do you say it's the same metric twice? I don't see other reference to numberOfItemsInWatch.

caesarxuchao · 2017-06-30T22:04:03Z

staging/src/k8s.io/client-go/tools/cache/reflector_metrics.go

+	"time"
+)
+
+// This file provides abstractions for setting the provider (e.g., prometheus)


Move it to above the package declaration?

Move it to above the package declaration?

done

deads2k · 2017-07-03T17:57:37Z

comments addressed.

caesarxuchao · 2017-07-05T18:20:24Z

/lgtm

k8s-github-robot · 2017-07-05T18:20:35Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: caesarxuchao, deads2k
We suggest the following additional approver: lavalamp

Assign the PR to them by writing /assign @lavalamp in a comment when ready.

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kube-apiserver/OWNERS~~ [caesarxuchao,deads2k]
~~cmd/kube-controller-manager/OWNERS~~ [deads2k]
federation/cmd/federation-controller-manager/OWNERS
pkg/OWNERS
plugin/pkg/admission/OWNERS
~~staging/src/k8s.io/apiserver/pkg/storage/OWNERS~~ [deads2k]
~~staging/src/k8s.io/client-go/OWNERS~~ [caesarxuchao,deads2k]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

caesarxuchao · 2017-07-05T18:20:57Z

Could you add a release note?

deads2k · 2017-07-05T18:24:05Z

Could you add a release note?

done

deads2k · 2017-07-05T18:24:27Z

external packages are just loading the prometheus package to enable

smarterclayton · 2017-07-05T18:37:33Z

pkg/util/reflector/prometheus/prometheus.go

+func (_ prometheusMetricsProvider) NewWatchDurationMetric(name string) cache.SummaryMetric {
+	watchDuration := prometheus.NewSummary(prometheus.SummaryOpts{
+		Subsystem: name,
+		Name:      "watch_duration",


What unit is this duration

What unit is this duration

microseconds.

Prometheus convention is to do seconds, and to include that in the name i.e. watch_duration_seconds. Same for list duration.

https://prometheus.io/docs/practices/naming/

deads2k · 2017-07-24T20:37:49Z

@DirectXMan12 had a bad rebase. See if its good now.

deads2k · 2017-07-24T21:02:23Z

/retest

DirectXMan12 · 2017-07-24T21:22:45Z

looks good 👍

deads2k · 2017-07-24T22:08:38Z

/lint

k8s-ci-robot

@deads2k: 18 warnings.

In response to this:

/lint

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 · 2017-07-24T22:09:59Z

pkg/util/reflector/prometheus/prometheus.go

+}
+
+// use summary to get averages and percentiles
+func (_ prometheusMetricsProvider) NewWatchDurationMetric(name string) cache.SummaryMetric {


Golint naming: receiver name should not be an underscore. More info.

k8s-ci-robot · 2017-07-24T22:09:59Z

pkg/util/reflector/prometheus/prometheus.go

+
+// Package prometheus sets the cache DefaultMetricsFactory to produce
+// prometheus metrics. To use this package, you just have to import it.
+


Golint comments: package comment is detached; there should be no blank lines between it and the package statement. More info.

k8s-ci-robot · 2017-07-24T22:09:59Z

pkg/util/reflector/prometheus/prometheus.go

+}
+
+// use summary to get averages and percentiles
+func (_ prometheusMetricsProvider) NewItemsInWatchMetric(name string) cache.SummaryMetric {


Golint naming: receiver name should not be an underscore. More info.

k8s-ci-robot · 2017-07-24T22:09:59Z

pkg/util/reflector/prometheus/prometheus.go

+	return itemsPerWatch.WithLabelValues(name)
+}
+
+func (_ prometheusMetricsProvider) NewLastResourceVersionMetric(name string) cache.GaugeMetric {


Golint naming: receiver name should not be an underscore. More info.

k8s-ci-robot · 2017-07-24T22:09:59Z

pkg/util/reflector/prometheus/prometheus.go

+
+type prometheusMetricsProvider struct{}
+
+func (_ prometheusMetricsProvider) NewListsMetric(name string) cache.CounterMetric {


Golint naming: receiver name should not be an underscore. More info.

k8s-ci-robot · 2017-07-24T22:10:00Z

staging/src/k8s.io/client-go/tools/cache/reflector_metrics.go

+
+type noopMetricsProvider struct{}
+
+func (_ noopMetricsProvider) NewListsMetric(name string) CounterMetric         { return noopMetric{} }


Golint naming: receiver name should not be an underscore. More info.

k8s-ci-robot · 2017-07-24T22:10:00Z

staging/src/k8s.io/client-go/tools/cache/reflector_metrics.go

+
+func (_ noopMetricsProvider) NewListsMetric(name string) CounterMetric         { return noopMetric{} }
+func (_ noopMetricsProvider) NewListDurationMetric(name string) SummaryMetric  { return noopMetric{} }
+func (_ noopMetricsProvider) NewItemsInListMetric(name string) SummaryMetric   { return noopMetric{} }


Golint naming: receiver name should not be an underscore. More info.

k8s-ci-robot · 2017-07-24T22:10:00Z

staging/src/k8s.io/client-go/tools/cache/reflector_metrics.go

+// This file provides abstractions for setting the provider (e.g., prometheus)
+// of metrics.
+
+package cache


Golint comments: should have a package comment, unless it's in another file for this package. More info.

k8s-ci-robot · 2017-07-24T22:10:00Z

staging/src/k8s.io/client-go/tools/cache/reflector_metrics.go

+type noopMetricsProvider struct{}
+
+func (_ noopMetricsProvider) NewListsMetric(name string) CounterMetric         { return noopMetric{} }
+func (_ noopMetricsProvider) NewListDurationMetric(name string) SummaryMetric  { return noopMetric{} }


Golint naming: receiver name should not be an underscore. More info.

k8s-ci-robot · 2017-07-24T22:10:00Z

staging/src/k8s.io/client-go/tools/cache/reflector_metrics.go

+func (_ noopMetricsProvider) NewItemsInListMetric(name string) SummaryMetric   { return noopMetric{} }
+func (_ noopMetricsProvider) NewWatchesMetric(name string) CounterMetric       { return noopMetric{} }
+func (_ noopMetricsProvider) NewShortWatchesMetric(name string) CounterMetric  { return noopMetric{} }
+func (_ noopMetricsProvider) NewWatchDurationMetric(name string) SummaryMetric { return noopMetric{} }


Golint naming: receiver name should not be an underscore. More info.

deads2k · 2017-07-25T12:08:00Z

updated for linting, applied tags based on previous comments.

deads2k · 2017-07-25T12:27:38Z

/test pull-kubernetes-kubemark-e2e-gce

k8s-github-robot · 2017-07-25T18:06:46Z

Automatic merge from submit-queue (batch tested with PRs 48224, 45431, 45946, 48775, 49396)

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 28, 2017

k8s-github-robot assigned caesarxuchao and david-mcmahon Jun 28, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Jun 28, 2017

deads2k force-pushed the controller-11-reflectormetrics branch from c136353 to 6b17f51 Compare June 29, 2017 14:16

david-mcmahon removed their assignment Jun 29, 2017

smarterclayton reviewed Jun 29, 2017

View reviewed changes

smarterclayton added the sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. label Jun 29, 2017

deads2k force-pushed the controller-11-reflectormetrics branch from 6b17f51 to 4ee204c Compare June 30, 2017 11:52

DirectXMan12 suggested changes Jun 30, 2017

View reviewed changes

deads2k force-pushed the controller-11-reflectormetrics branch from 4ee204c to f3fb169 Compare June 30, 2017 19:55

caesarxuchao reviewed Jun 30, 2017

View reviewed changes

deads2k force-pushed the controller-11-reflectormetrics branch 2 times, most recently from 4bbf0d3 to 53efd42 Compare July 3, 2017 14:15

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 5, 2017

k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jul 5, 2017

k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jul 5, 2017

DirectXMan12 approved these changes Jul 5, 2017

View reviewed changes

smarterclayton reviewed Jul 5, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 8, 2017

deads2k force-pushed the controller-11-reflectormetrics branch from 53efd42 to 40174c1 Compare July 24, 2017 19:34

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 24, 2017

deads2k force-pushed the controller-11-reflectormetrics branch 3 times, most recently from fed2ba3 to 32644ab Compare July 24, 2017 20:37

k8s-ci-robot reviewed Jul 24, 2017

View reviewed changes

deads2k force-pushed the controller-11-reflectormetrics branch from 32644ab to b506a0f Compare July 25, 2017 12:06

add reflector metrics

151d396

deads2k force-pushed the controller-11-reflectormetrics branch from b506a0f to 151d396 Compare July 25, 2017 13:01

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2017

deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2017

k8s-github-robot merged commit 9c3d0e8 into kubernetes:master Jul 25, 2017

deads2k deleted the controller-11-reflectormetrics branch August 3, 2017 20:12

FUSAKLA mentioned this pull request Mar 14, 2018

Provide metrics for kubernetes sd errors prometheus/prometheus#3876

Closed


		// Package prometheus sets the cache DefaultMetricsFactory to produce
		// prometheus metrics. To use this package, you just have to import it.


		type prometheusMetricsProvider struct{}

		func (_ prometheusMetricsProvider) NewListsMetric(name string) cache.CounterMetric {


		type noopMetricsProvider struct{}

		func (_ noopMetricsProvider) NewListsMetric(name string) CounterMetric { return noopMetric{} }

add reflector metrics #48224

add reflector metrics #48224

Conversation

deads2k commented Jun 28, 2017 • edited

deads2k commented Jun 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jun 29, 2017

DirectXMan12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 Jun 30, 2017 • edited

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

Choose a reason for hiding this comment

deads2k commented Jun 30, 2017

caesarxuchao 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

deads2k commented Jul 3, 2017

caesarxuchao commented Jul 5, 2017

k8s-github-robot commented Jul 5, 2017

caesarxuchao commented Jul 5, 2017

deads2k commented Jul 5, 2017

deads2k commented Jul 5, 2017

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

deads2k commented Jul 24, 2017

deads2k commented Jul 24, 2017

DirectXMan12 commented Jul 24, 2017

deads2k commented Jul 24, 2017

k8s-ci-robot 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jul 25, 2017

deads2k commented Jul 25, 2017

k8s-github-robot commented Jul 25, 2017

deads2k commented Jun 28, 2017 •

edited

DirectXMan12 Jun 30, 2017 •

edited