pkg/util/workqueue/prometheus: fix double registration #77553

s-urbaniak · 2019-05-07T15:43:39Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Currently, if workqueue metrics are registered twice, these metrics will be ignored.
This fixes it.

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

NONE

s-urbaniak · 2019-05-07T15:44:54Z

/cc @brancz as you had some thoughts on the prometheus.AlreadyRegisteredError approach
/cc @krasi-georgiev :-)

neolit123

/sig instrumentation
/priority backlog

brancz · 2019-05-07T18:33:13Z

Interesting solution, I do wonder, shouldn't these metrics be vector metrics? Then we wouldn't have this problem in the first place.

s-urbaniak · 2019-05-08T07:10:02Z

@brancz this is indeed a great idea 👍 let me play with that, now that this has a unit test.

neolit123 · 2019-05-08T13:09:39Z

pkg/util/workqueue/prometheus/prometheus.go

+	// an invalid or duplicate metric descriptor,
+	// a previously registered descriptor with the same fqdn but different labels,
+	// or inconsistent label names or help strings for the same fqdn.
+	klog.Fatalf("failed to register metric %v name %v: %v", metric, name, err)


/assign @logicalhan
as per:
#75737

klog.Fatalf() can be an anti-pattern in certain cases, but it's up to the maintainers.
i guess it's OK to check for AlreadyRegisteredError, but it is a question whether the process should hard-fail on metric registration (or log "a lot" of errors).

logicalhan

Have you verified that double registration renders the existing metrics defunct? I do not believe that that is actually the case (of course I could be wrong).

If you look in the documentation for prometheus.Desc, then you can see that descriptors are considered equal if the fully-qualified name and the labels are the same. In the case of the workqueue metrics, even if the function is invoked multiple times, each metric which is returned is therefore considered identical to the registry.

s-urbaniak · 2019-05-09T08:01:34Z

@logicalhan yes I verified this by looking the registration logic in the registry, it first checks if the collector is already registered in

kubernetes/vendor/github.com/prometheus/client_golang/prometheus/registry.go

Lines 327 to 332 in e9af72c

    
           if existing, exists := r.collectorsByID[collectorID]; exists { 
        
           	return AlreadyRegisteredError{ 
        
           		ExistingCollector: existing, 
        
           		NewCollector:      c, 
        
           	} 
        
           }

and only sets it in the registry later if the above detection did not fail:

kubernetes/vendor/github.com/prometheus/client_golang/prometheus/registry.go

Lines 339 to 347 in e9af72c

    
           // Only after all tests have passed, actually register. 
        
           r.collectorsByID[collectorID] = c 
        
           for hash := range newDescIDs { 
        
           	r.descIDs[hash] = struct{}{} 
        
           } 
        
           for name, dimHash := range newDimHashesByName { 
        
           	r.dimHashesByName[name] = dimHash 
        
           } 
        
           return nil

and used a small verification snippet https://play.golang.org/p/WsnIqXbddzP

Currently, if workqueue metrics are registered twice, these metrics will be ignored. This fixes it.

s-urbaniak · 2019-05-09T11:43:28Z

@brancz added vector metrics, where possible. The deprecated ones are not label based so I thing we have to use the AlreadyRegisteredError approach on these.

Once we have consensus on this PR, I'll address the bazel failures as they seem mechanical.

s-urbaniak · 2019-05-09T18:24:47Z

pkg/util/workqueue/prometheus/prometheus.go

+	klog.Fatalf("failed to register metric %v name %v: %v", metric, name, err)
+	return nil
+}
+
 func (prometheusMetricsProvider) NewDeprecatedDepthMetric(name string) workqueue.GaugeMetric {
 	depth := prometheus.NewGauge(prometheus.GaugeOpts{


nevermind, as discussed with @logicalhan we can go with simple singletons here and can omit mustRegister at all 👍 I will adapt the PR accordingly.

@logicalhan @brancz unfortunately declaring those deprecated metrics as singletons doesn't seem to work, as the subsystem is dynamic (sorry when I discussed this i was blind).

To omit the ugly mustRegister method, I added another commit which completely removes the deprecated metrics for good. @brancz the question is, if we can do this at this point.

logicalhan

Thanks for taking the time on this!

logicalhan · 2019-05-09T20:17:08Z

pkg/util/workqueue/prometheus/prometheus.go

+	klog.Fatalf("failed to register metric %v name %v: %v", metric, name, err)
+	return nil
+}
+
 func (prometheusMetricsProvider) NewDeprecatedDepthMetric(name string) workqueue.GaugeMetric {
 	depth := prometheus.NewGauge(prometheus.GaugeOpts{


This deletes deprecated metrics and simplifies registration.

s-urbaniak · 2019-05-15T15:29:38Z

/test pull-kubernetes-dependencies

brancz · 2019-05-16T11:54:32Z

/retest

brancz · 2019-05-16T15:17:49Z

Thanks for cleaning this up!

/lgtm

logicalhan

/lgtm

Looks good to me as well, thanks so much for fixing this!

s-urbaniak · 2019-07-15T06:55:27Z

ping @logicalhan @brancz Not sure what Not mergeable. Must be in milestone v1.15. means. I guess this is just missing some label?

brancz · 2019-07-15T07:15:16Z

I think that was an outdated status. This now just needs an approver.

cc @smarterclayton

s-urbaniak · 2019-07-22T17:35:37Z

ping @smarterclayton for review and approval :-)

smarterclayton · 2019-07-22T17:50:22Z

/approve

k8s-ci-robot · 2019-07-22T17:51:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: s-urbaniak, smarterclayton

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/OWNERS~~ [smarterclayton]
~~staging/src/k8s.io/client-go/OWNERS~~ [smarterclayton]

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

fejta-bot · 2019-07-22T21:29:24Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

fejta-bot · 2019-07-23T00:17:24Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot requested review from lavalamp and thockin May 7, 2019 15:45

s-urbaniak force-pushed the fix-76956 branch from 51a71b6 to 24494f4 Compare May 7, 2019 15:47

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label May 7, 2019

neolit123 reviewed May 7, 2019

View reviewed changes

neolit123 reviewed May 8, 2019

View reviewed changes

k8s-ci-robot assigned logicalhan May 8, 2019

logicalhan reviewed May 9, 2019

View reviewed changes

s-urbaniak mentioned this pull request May 9, 2019

duplicate metrics collector registration attempted #76956

Closed

pkg/util/workqueue/prometheus: fix double registration

0e291d1

Currently, if workqueue metrics are registered twice, these metrics will be ignored. This fixes it.

s-urbaniak force-pushed the fix-76956 branch from 24494f4 to 0e291d1 Compare May 9, 2019 11:41

s-urbaniak commented May 9, 2019

View reviewed changes

logicalhan reviewed May 9, 2019

View reviewed changes

k8s-ci-robot added the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label May 13, 2019

s-urbaniak force-pushed the fix-76956 branch 2 times, most recently from 6a26efd to d019b06 Compare May 14, 2019 10:54

pkg/util/workqueue: delete deprecated metrics

4532cfd

This deletes deprecated metrics and simplifies registration.

s-urbaniak force-pushed the fix-76956 branch from d019b06 to 4532cfd Compare May 15, 2019 10:27

k8s-ci-robot assigned brancz May 16, 2019

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

logicalhan approved these changes May 16, 2019

View reviewed changes

logicalhan mentioned this pull request May 17, 2019

kube-aggregator/openapi/controller: remove trailing 1 in failure ratelimiter #77979

Merged

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

k8s-ci-robot merged commit f101466 into kubernetes:master Jul 23, 2019

s-urbaniak deleted the fix-76956 branch August 15, 2019 08:54

thxCode mentioned this pull request Nov 7, 2019

Monitoring - some graphs are missing in k8s 1.16 cluster rancher/rancher#23731

Closed

swapnilgm mentioned this pull request Jan 30, 2020

Updated dependecies to refer k8s 1.16.0 gardener/machine-controller-manager#398

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/util/workqueue/prometheus: fix double registration #77553

pkg/util/workqueue/prometheus: fix double registration #77553

s-urbaniak commented May 7, 2019

s-urbaniak commented May 7, 2019

neolit123 left a comment

brancz commented May 7, 2019

s-urbaniak commented May 8, 2019

neolit123 May 8, 2019

logicalhan left a comment

s-urbaniak commented May 9, 2019

s-urbaniak commented May 9, 2019

s-urbaniak May 9, 2019

logicalhan May 9, 2019

s-urbaniak May 13, 2019

logicalhan left a comment

logicalhan May 9, 2019

s-urbaniak commented May 15, 2019

brancz commented May 16, 2019

brancz commented May 16, 2019

logicalhan left a comment

s-urbaniak commented Jul 15, 2019

brancz commented Jul 15, 2019

s-urbaniak commented Jul 22, 2019

smarterclayton commented Jul 22, 2019

k8s-ci-robot commented Jul 22, 2019

fejta-bot commented Jul 22, 2019

fejta-bot commented Jul 23, 2019

pkg/util/workqueue/prometheus: fix double registration #77553

pkg/util/workqueue/prometheus: fix double registration #77553

Conversation

s-urbaniak commented May 7, 2019

s-urbaniak commented May 7, 2019

neolit123 left a comment

Choose a reason for hiding this comment

brancz commented May 7, 2019

s-urbaniak commented May 8, 2019

neolit123 May 8, 2019

Choose a reason for hiding this comment

logicalhan left a comment

Choose a reason for hiding this comment

s-urbaniak commented May 9, 2019

s-urbaniak commented May 9, 2019

s-urbaniak May 9, 2019

Choose a reason for hiding this comment

logicalhan May 9, 2019

Choose a reason for hiding this comment

s-urbaniak May 13, 2019

Choose a reason for hiding this comment

logicalhan left a comment

Choose a reason for hiding this comment

logicalhan May 9, 2019

Choose a reason for hiding this comment

s-urbaniak commented May 15, 2019

brancz commented May 16, 2019

brancz commented May 16, 2019

logicalhan left a comment

Choose a reason for hiding this comment

s-urbaniak commented Jul 15, 2019

brancz commented Jul 15, 2019

s-urbaniak commented Jul 22, 2019

smarterclayton commented Jul 22, 2019

k8s-ci-robot commented Jul 22, 2019

fejta-bot commented Jul 22, 2019

fejta-bot commented Jul 23, 2019