✨ Expose client-go metrics #233

DirectXMan12 · 2018-12-04T01:57:11Z

This exposes the client-go metrics (client, workqueue, reflector)
metrics in the controller-runtime registry.

It also fixes a typo in the tests, and fixes our Gopkg.toml to prune some unnecessary requires (since I had to do a dep ensure anyway, and it was taking forever without the changes).

k8s-ci-robot · 2018-12-04T01:57:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12

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~~ [DirectXMan12]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

DirectXMan12 · 2018-12-04T01:57:20Z

/kind feature

DirectXMan12 · 2018-12-04T01:59:53Z

@brancz btw, this first is why I'm hesitant to call the current client-go setup easily usable.

brancz

I agree that this is quite a mouthful. Nonetheless the way you’re using the metrics registry here it could just as easily be injected and then this could just be a package in client-go.

brancz · 2018-12-04T07:46:14Z

pkg/metrics/client_go_adapter.go

+
+// registerClientMetrics sets up the client latency metrics from client-go
+func registerClientMetrics() {
+	var ()


This looks like a leftover

droot

Code looks good to me. I have couple of questions.

droot · 2018-12-04T17:37:24Z

pkg/metrics/client_go_adapter.go

+
+// this section contains adapters, implementations, and other sundry organic, artisinally
+// hand-crafted syntax trees required to convince client-go that it actually wants to let
+// someone use its metrics.


droot · 2018-12-04T17:38:52Z

pkg/metrics/client_go_adapter.go

+
+// NB(directxman12): these are changed to MustRegister from Register.  It's not clear why they weren't
+// MustRegister in the first place, except maybe to not bring down the controller if the metrics fail
+// to register (which shouldn't happen unless there's a duplicate metric).


droot · 2018-12-04T17:42:34Z

pkg/metrics/client_go_adapter.go

+
+// registerWorkQueueMetrics sets up workqueue (other reconcile) metrics
+func registerWorkqueueMetrics() {
+	workqueuemetrics.SetProvider(workqueueMetricsProvider{})


Does this means metrics from multiple controllers will get combined ?

no, if you look at how the provider is implemented, there's a label for the controller name

droot · 2018-12-04T17:44:10Z

pkg/metrics/client_go_adapter.go

@@ -0,0 +1,266 @@
+/*
+Copyright 2016 The Kubernetes Authors.


s/2016/2018 (someone should build github suggestion bot for this :))

droot · 2018-12-04T17:48:14Z

pkg/metrics/client_go_adapter.go

+	})
+	Registry.MustRegister(retries)
+	return retries
+}


These workqueue metrics have some overlap with the existing metrics. Shall we remove the higher level controller_runtime metrics ?

There's things that look like duplicates, but it's not clear to me if they actually are completely duplicate. Lets evaluate a bit.

I think we want to kill QueueLength in favor of depth, although the fact that the client-go metrics don't expose queue names as labels is strange to me. The other two I think we want to keep.

@brancz do you have any insight on the subsystem vs label decision there?

on this subject: kubernetes/kubernetes#71165

DirectXMan12

ack, will on the comments, will fix a bit later today

DirectXMan12 · 2018-12-04T19:16:02Z

pkg/metrics/client_go_adapter.go

+
+// registerClientMetrics sets up the client latency metrics from client-go
+func registerClientMetrics() {
+	var ()


DirectXMan12 · 2018-12-04T19:19:46Z

pkg/metrics/client_go_adapter.go

@@ -0,0 +1,266 @@
+/*
+Copyright 2016 The Kubernetes Authors.


DirectXMan12 · 2018-12-04T19:20:58Z

pkg/metrics/client_go_adapter.go

+
+// registerWorkQueueMetrics sets up workqueue (other reconcile) metrics
+func registerWorkqueueMetrics() {
+	workqueuemetrics.SetProvider(workqueueMetricsProvider{})


no, if you look at how the provider is implemented, there's a label for the controller name

DirectXMan12 · 2018-12-04T19:22:15Z

pkg/metrics/client_go_adapter.go

+	})
+	Registry.MustRegister(retries)
+	return retries
+}


There's things that look like duplicates, but it's not clear to me if they actually are completely duplicate. Lets evaluate a bit.

DirectXMan12 · 2018-12-07T17:52:15Z

I agree that this is quite a mouthful. Nonetheless the way you’re using the metrics registry here it could just as easily be injected and then this could just be a package in client-go.

Oh sure. I think this particular model is just super clunky ;-) Injected registry + refactor in client-go would definitely address this.

brancz · 2018-12-08T18:17:47Z

Do you think you could take that on as a follow up, to contribute that to client-go? This is the second time I am reviewing a PR that implements more or less the same thing twice (first time was Prometheus implementing these metrics itself for its Kubernetes service discovery). I imagine many more want to do the same thing. Preferably even first in client-go and then move forward with this PR, but I understand if you first want to make progress with this.

DirectXMan12 · 2018-12-19T01:29:26Z

I can take a follow up to move some of this in client-go. I'd prefer to get this merged first, then I'll fix up client-go a bit, and then port it back over (it makes it easier since we might not immediately pull in the latest client-go release).

This exposes the client-go metrics (client, workqueue, reflector) metrics in the controller-runtime registry.

This marks tests that have a "// TODO: write this" or similar as pending tests, so that we can keep track of them.

The QueueLength metric is now a duplicate of the workqueue "depth" metric, so it's no longer needed.

mengqiy

A few comments.
Other LGTM.

mengqiy · 2019-01-10T23:45:34Z

pkg/metrics/client_go_adapter.go

+
+	longestRunning = prometheus.NewGaugeVec(prometheus.GaugeOpts{
+		Subsystem: workQueueSubsystem,
+		Name:      "longest_running_processor_microseconds",


microseconds?
It seems this is not following the convention.

yeah, but the current code requires it atm. it's fixed in future releases

mengqiy · 2019-01-10T23:51:19Z

pkg/metrics/client_go_adapter.go

+	return retries.WithLabelValues(name)
+}
+
+func (workqueueMetricsProvider) NewLongestRunningProcessorMicrosecondsMetric(name string) workqueuemetrics.SettableGaugeMetric {


Microseconds?

the current code requires it atm (see above)

mengqiy · 2019-01-11T18:28:07Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 4, 2018

k8s-ci-robot requested review from droot and pwittrock December 4, 2018 01:57

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 4, 2018

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 4, 2018

DirectXMan12 force-pushed the features/client-go-metrics branch from 250f34b to d06e3fb Compare December 4, 2018 01:59

brancz reviewed Dec 4, 2018

View reviewed changes

droot reviewed Dec 4, 2018

View reviewed changes

DirectXMan12 commented Dec 4, 2018

View reviewed changes

DirectXMan12 force-pushed the features/client-go-metrics branch from 1dfbd22 to cd73474 Compare December 4, 2018 22:07

DirectXMan12 changed the title ~~✨ Expose client-go metrics~~ ⚠️ Expose client-go metrics Dec 5, 2018

DirectXMan12 changed the title ~~⚠️ Expose client-go metrics~~ ✨ Expose client-go metrics Dec 5, 2018

DirectXMan12 force-pushed the features/client-go-metrics branch from cd73474 to 4c17676 Compare December 5, 2018 19:03

DirectXMan12 changed the title ~~✨ Expose client-go metrics~~ ✨ Expose client-go metrics Dec 6, 2018

DirectXMan12 force-pushed the features/client-go-metrics branch 2 times, most recently from 96b2df6 to a696eb8 Compare January 10, 2019 22:06

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 10, 2019

DirectXMan12 added 3 commits January 10, 2019 14:34

Expose client-go metrics

05643fc

This exposes the client-go metrics (client, workqueue, reflector) metrics in the controller-runtime registry.

🏃 Mark pending tests as pending

b3610e8

This marks tests that have a "// TODO: write this" or similar as pending tests, so that we can keep track of them.

⚠️ Remove duplicate QueueLength metric

ed6a18c

The QueueLength metric is now a duplicate of the workqueue "depth" metric, so it's no longer needed.

DirectXMan12 force-pushed the features/client-go-metrics branch from a696eb8 to ed6a18c Compare January 10, 2019 22:34

mengqiy approved these changes Jan 10, 2019

View reviewed changes

k8s-ci-robot assigned mengqiy Jan 11, 2019

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

k8s-ci-robot merged commit c043856 into kubernetes-sigs:master Jan 11, 2019

DirectXMan12 deleted the features/client-go-metrics branch January 14, 2019 21:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Expose client-go metrics #233

✨ Expose client-go metrics #233

DirectXMan12 commented Dec 4, 2018

k8s-ci-robot commented Dec 4, 2018

DirectXMan12 commented Dec 4, 2018

DirectXMan12 commented Dec 4, 2018

brancz left a comment

brancz Dec 4, 2018

DirectXMan12 Dec 4, 2018

droot left a comment

droot Dec 4, 2018

droot Dec 4, 2018

droot Dec 4, 2018

DirectXMan12 Dec 4, 2018

droot Dec 4, 2018

DirectXMan12 Dec 4, 2018

droot Dec 4, 2018

DirectXMan12 Dec 4, 2018

DirectXMan12 Dec 4, 2018 •

edited

Loading

DirectXMan12 Dec 7, 2018

DirectXMan12 left a comment

DirectXMan12 Dec 4, 2018

DirectXMan12 Dec 4, 2018

DirectXMan12 Dec 4, 2018

DirectXMan12 Dec 4, 2018

DirectXMan12 commented Dec 7, 2018 •

edited

Loading

brancz commented Dec 8, 2018

DirectXMan12 commented Dec 19, 2018

mengqiy left a comment

mengqiy Jan 10, 2019

DirectXMan12 Jan 11, 2019

mengqiy Jan 10, 2019

DirectXMan12 Jan 11, 2019

mengqiy commented Jan 11, 2019

✨ Expose client-go metrics #233

✨ Expose client-go metrics #233

Conversation

DirectXMan12 commented Dec 4, 2018

k8s-ci-robot commented Dec 4, 2018

DirectXMan12 commented Dec 4, 2018

DirectXMan12 commented Dec 4, 2018

brancz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droot 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

DirectXMan12 Dec 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 commented Dec 7, 2018 • edited Loading

brancz commented Dec 8, 2018

DirectXMan12 commented Dec 19, 2018

mengqiy 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

mengqiy commented Jan 11, 2019

DirectXMan12 Dec 4, 2018 •

edited

Loading

DirectXMan12 commented Dec 7, 2018 •

edited

Loading