decouple workqueue metrics from prometheus #33792

caesarxuchao · 2016-09-29T21:52:36Z

What this PR does / why we need it:
We want to include the workqueue in client-go, but do not want to having to import Prometheus. This PR decouples the workqueue from prometheus.

Which issue this PR fixes (optional, in fixes #<issue number>(, #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Partially address #33497
User requested for workqueue in client-go: kubernetes/client-go#4 (comment)

Special notes for your reviewer:

Release note:

The implicit registration of Prometheus metrics for workqueue has been removed, and a plug-able interface was added. If you were using workqueue in your own binaries and want these metrics, add the following to your imports in the main package: "k8s.io/pkg/util/workqueue/prometheus".

This change is

k8s-ci-robot · 2016-09-29T22:27:53Z

Jenkins GCI GCE e2e failed for commit 1e8c071e1910525b6651d888f876a4bda9283ac4. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

krousey · 2016-09-29T23:00:33Z

The new public types you're introducing lack documentation, so it's hard to try to infer what they're supposed to be. Like PartialDefaultMetrics. Why is it partial, and defaulted? Also you seem to have exposed a factory variable that no one outside of the queue package can effectively use.

It would be a lot simpler to just have a private interface variable that defaulted to a no-op, and a public registration function. Other than the registration function, and the interface's methods, everything else should be private. e.g.

var metricsProvider MetricsProvider = &noopProvider{}

func SetProvider(p MetricsProvider) { // set once }

It also seems like the recording of queue and processing start times are being conflated with the metrics provider here. They should be separated out because one is how you calculate metrics, the other is how you expose and record them.

caesarxuchao · 2016-09-29T23:40:53Z

Sorry for the confusion, I'll add the comments.

Other than the registration function, and the interface's methods, everything else should be private.

I thought of a better pattern, similar to what you asked for. I'll update the code.

caesarxuchao · 2016-09-30T05:28:39Z

@krousey I changed the pattern to similar to what you suggested. I also added comments. Hopefully it's easier to read now.

It also seems like the recording of queue and processing start times are being conflated with the metrics provider here. They should be separated out because one is how you calculate metrics, the other is how you expose and record them.

The new version should have addressed this comment, if I understood you correctly.

k8s-ci-robot · 2016-09-30T05:57:35Z

Jenkins verification failed for commit 21b46bf. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

krousey · 2016-09-30T16:24:02Z

pkg/util/workqueue/metrics.go

 type queueMetrics interface {
 	add(item t)
 	get(item t)
 	done(item t)
 }

+// GaugeMetric that represents a single numerical value that can


// GaugeMetric represents ...

krousey · 2016-09-30T16:24:42Z

pkg/util/workqueue/metrics.go

 type queueMetrics interface {
 	add(item t)
 	get(item t)
 	done(item t)
 }

+// GaugeMetric that represents a single numerical value that can
+// arbitrarily go up and down.
+// Its methods are a subset of prometheus.Gauge.


No need to call out prometheus here. Other implementations could exist.

krousey · 2016-09-30T16:25:04Z

pkg/util/workqueue/metrics.go

 )

+// You need to import the prometheus package to create and register prometheus


Maybe make this more generic and not prometheus specific.

krousey · 2016-09-30T16:40:48Z

pkg/util/workqueue/metrics.go

+	Dec()
+}
+
+type noopGaugeMetric struct{}


Your no-ops can be condensed to one type:

type noopMetric struct{} func (noopMetric) Inc() {} func (noopMetric) Dec() {} func (noopMetric) Observe(float64) {}

krousey · 2016-09-30T16:42:07Z

pkg/util/workqueue/metrics.go

-		return ret
-	}
+// DepthMetricProvider creates DepthMetric.
+type DepthMetricProvider interface {


These should be condensed to the types of metrics they provide, not what the metrics are used for. i.e.

GaugeMetricProvider
ConterMetricProvider
SummaryMetricProvider

I condensed them all to a MetricsProvider interface.

krousey · 2016-09-30T16:43:00Z

pkg/util/workqueue/metrics.go

-		addTimes:             map[t]time.Time{},
-		processingStartTimes: map[t]time.Time{},
-	}
+type noopDepthMetricProvider struct{}


These noop providers can also be condensed to one type.

krousey · 2016-09-30T16:44:56Z

pkg/util/workqueue/prometheus/prometheus.go

+// prometheus metrics. To use this package, you just have to import it.
+
+func init() {
+	workqueue.DefaultMetricsFactory.SetProviders(


Calling a method on a type whose definition is private to another package. Not that it won't work, it just feels wrong. Like if I wanted to search godoc.org for the documentation on SetProviders, I wouldn't find it.

Thanks for the explanation.

krousey · 2016-09-30T16:58:19Z

pkg/util/workqueue/metrics.go

@@ -151,3 +188,63 @@ func (m *defaultRetryMetrics) retry() {

 	m.retries.Inc()
 }
+
+type metricsFactory struct {


I think it might be easiest for me to just show what I meant by not needing a factory variable exposed.

var metricsFactory = struct { counterMetricProvider CounterMetricProvider gaugeMetricProvider GaugeMetricProvider summaryMetricProvider SummaryMetricProvider setProviders sync.Once }{ counterMetricProvider: noopProvider{}, gaugeMetricProvider: noopProvider{}, summaryMetricProvider: noopProvider{}, } func newQueueMetrics(name string) queueMetrics { var ret *defaultQueueMetrics if len(name) == 0 { return ret } return &defaultQueueMetrics{ depth: metricsFactory.gaugeMetricProvider.NewGuageMetric(name, "depth"), adds: metricsFactory.counterMetricProvider.NewCounterMetric(name, "adds"), latency: metricsFactory.summaryMetricProvider.NewSummaryMetric(name, "latency"), workDuration: metricsFactory.summaryMetricProvider.NewSummaryMetric(name, "duration"), addTimes: map[t]time.Time{}, processingStartTimes: map[t]time.Time{}, } } func newRetryMetrics(name string) retryMetrics { var ret *defaultRetryMetrics if len(name) == 0 { return ret } return &defaultRetryMetrics{ retries: metricsFactory.counterMetricProvider.NewCounterMetric(name, "retries"), } } // SetProviders sets the metrics providers of the metricsFactory. func SetProviders( counterMetricProvider CounterMetricProvider, gaugeMetricProvider GaugeMetricProvider, summaryMetricProvider SummaryMetricProvider, ) { metricsFactory.setProviders.Do(func() { metricsFactory.counterMetricProvider = counterMetricProvider metricsFactory.gaugeMetricProvider = gaugeMetricProvider metricsFactory.summaryMetricProvider = summaryMetricProvider }) }

krousey · 2016-09-30T16:58:51Z

pkg/util/workqueue/prometheus/prometheus.go

+	)
+}
+
+type prometheusDepthMetricProvider struct{}


These can also be condensed into one struct with multiple methods.

krousey · 2016-09-30T17:00:46Z

federation/pkg/federation-controller/replicaset/replicasetcontroller.go

@@ -43,6 +43,7 @@ import (
 	"k8s.io/kubernetes/pkg/util/flowcontrol"
 	"k8s.io/kubernetes/pkg/util/wait"
 	"k8s.io/kubernetes/pkg/util/workqueue"
+	_ "k8s.io/kubernetes/pkg/util/workqueue/prometheus"


I think you try to put this import in package main instead of in these libraries.

The new github review UI is confusing. I definitely didn't type "Done" to this comment..

Anyway, do you mean importing it in pkg/controller? I'd rather keep this line next to the workqueue import line for easier tracking.

I meant importing it in the relevant main packages in cmd/... Like the throttle metrics are done. Perhaps we should consolidate these metrics implementations into one package. Just import that one package and you get prometheus everywhere.

Another PR of course.

k8s-ci-robot · 2016-09-30T22:26:53Z

Jenkins Kubemark GCE e2e failed for commit b094d6c. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-ci-robot · 2016-09-30T22:27:53Z

Jenkins GCI GKE smoke e2e failed for commit b094d6c. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

caesarxuchao · 2016-10-01T03:45:38Z

@k8s-bot unit test this, issue #33838

k8s-ci-robot · 2016-10-01T04:08:54Z

Jenkins unit/integration failed for commit b094d6c. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

krousey · 2016-10-03T17:10:41Z

federation/pkg/federation-controller/replicaset/replicasetcontroller.go

@@ -43,6 +43,7 @@ import (
 	"k8s.io/kubernetes/pkg/util/flowcontrol"
 	"k8s.io/kubernetes/pkg/util/wait"
 	"k8s.io/kubernetes/pkg/util/workqueue"
+	_ "k8s.io/kubernetes/pkg/util/workqueue/prometheus"


I meant importing it in the relevant main packages in cmd/... Like the throttle metrics are done. Perhaps we should consolidate these metrics implementations into one package. Just import that one package and you get prometheus everywhere.

Another PR of course.

krousey · 2016-10-03T17:14:51Z

pkg/util/workqueue/metrics.go

+var metricsFactory = struct {
+	metricsProvider MetricsProvider
+	setProviders    sync.Once
+}{}


You should initialize metricsProvider here to noopMetricsProvider.

caesarxuchao · 2016-10-03T18:05:08Z

I centralized the imports. I removed the import in the scheduler because it's just using workqueue.Parallelize(), so it does not need the metrics. PTAL. Thanks.

k8s-ci-robot · 2016-10-03T18:25:47Z

Jenkins GCE e2e failed for commit feb0d1d. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

caesarxuchao · 2016-10-03T18:37:23Z

@k8s-bot e2e test this, issue #33867

k8s-ci-robot · 2016-10-03T19:16:30Z

Jenkins GKE smoke e2e failed for commit feb0d1d. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

caesarxuchao · 2016-10-03T19:41:27Z

@k8s-bot gce e2e test this

caesarxuchao · 2016-10-03T19:42:15Z

@krousey can I get the lgtm? Thanks.

caesarxuchao · 2016-10-05T18:40:29Z

@lavalamp could you apply the label for this one? I think @krousey is not available.

krousey

sorry... was going through email and this was at the bottom of it... until you bumped it that is

caesarxuchao · 2016-10-05T19:38:16Z

Thanks @krousey. I just added a release note (copied from yours), let me know if you have comments on that.

k8s-ci-robot · 2016-10-06T00:08:12Z

Jenkins GCE Node e2e failed for commit feb0d1d. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

caesarxuchao · 2016-10-06T00:22:54Z

@k8s-bot node e2e test this, issue #32430

k8s-github-robot · 2016-10-06T05:10:11Z

Automatic merge from submit-queue

caesarxuchao added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Sep 29, 2016

caesarxuchao assigned krousey Sep 29, 2016

googlebot added the cla: yes label Sep 29, 2016

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

caesarxuchao force-pushed the decouple-workqueue-prometheus branch from 1e8c071 to 21b46bf Compare September 30, 2016 05:25

krousey reviewed Sep 30, 2016

View reviewed changes

caesarxuchao force-pushed the decouple-workqueue-prometheus branch 3 times, most recently from f4864f1 to b094d6c Compare September 30, 2016 22:10

krousey reviewed Oct 3, 2016

View reviewed changes

decouple workqueue metrics from prometheus

feb0d1d

caesarxuchao force-pushed the decouple-workqueue-prometheus branch from b094d6c to feb0d1d Compare October 3, 2016 18:02

krousey approved these changes Oct 3, 2016

View reviewed changes

krousey approved these changes Oct 5, 2016

View reviewed changes

krousey added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 5, 2016

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

k8s-github-robot merged commit 45e5719 into kubernetes:master Oct 6, 2016

		)

		// You need to import the prometheus package to create and register prometheus

decouple workqueue metrics from prometheus #33792

decouple workqueue metrics from prometheus #33792

Conversation

caesarxuchao commented Sep 29, 2016 • edited Loading

k8s-ci-robot commented Sep 29, 2016

krousey commented Sep 29, 2016

caesarxuchao commented Sep 29, 2016

caesarxuchao commented Sep 30, 2016

k8s-ci-robot commented Sep 30, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krousey Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krousey Sep 30, 2016 • edited Loading

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

krousey Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 30, 2016

k8s-ci-robot commented Sep 30, 2016

caesarxuchao commented Oct 1, 2016

k8s-ci-robot commented Oct 1, 2016

krousey Oct 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caesarxuchao commented Oct 3, 2016

k8s-ci-robot commented Oct 3, 2016

caesarxuchao commented Oct 3, 2016

k8s-ci-robot commented Oct 3, 2016

caesarxuchao commented Oct 3, 2016

caesarxuchao commented Oct 3, 2016

caesarxuchao commented Oct 5, 2016

krousey left a comment

Choose a reason for hiding this comment

caesarxuchao commented Oct 5, 2016

k8s-ci-robot commented Oct 6, 2016

caesarxuchao commented Oct 6, 2016

k8s-github-robot commented Oct 6, 2016

caesarxuchao commented Sep 29, 2016 •

edited

Loading

krousey Sep 30, 2016 •

edited

Loading

krousey Sep 30, 2016 •

edited

Loading

krousey Oct 3, 2016 •

edited

Loading

krousey Oct 3, 2016 •

edited

Loading