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

s-urbaniak · 2019-05-16T15:24:33Z

What type of PR is this?

/kind api-change
/kind bug

/kind cleanup

/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:

This removes a trailing 1 from the rate limiting name as it was confusing, and also appears in metrics.

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

openapi-controller: remove the trailing `1` character literal from the rate limiting metric `APIServiceOpenAPIAggregationControllerQueue1` and rename it to `open_api_aggregation_controller` to adhere to Prometheus best practices.

s-urbaniak · 2019-05-16T15:24:52Z

/cc @brancz @sttts

brancz · 2019-05-16T15:28:20Z

/lgtm

from instrumentation side, let's have one of the approvers confirm that this number isn't there for a reason, but it looks strange at best.

fedebongio · 2019-05-16T20:17:48Z

/assign @logicalhan

fedebongio · 2019-05-16T20:17:57Z

/assign @roycaihw

roycaihw · 2019-05-16T20:20:19Z

/hold

before we merge this, just to verify if the name is exposed in metrics and if changing it will cause any breaking change

logicalhan

Renaming a metric is equivalent to deleting a metric and creating a new one. We should probably deprecate the old metric so that if people are using it, they have some window to migrate off of it. Also, if we're going to do this, we should really just completely fix the name of the metric (i.e. no camel-casing at all).

brancz · 2019-05-17T05:23:38Z

This is renaming a single label value, not the metric name, nor the label-key. I don’t know whether deprecation is necessary here as it’s more a time series identity label rather than one you would actively query and for what it’s worth the underlying metrics were broken until @s-urbaniak recently fixed them (the PR may still be open in which case they are still broken).

logicalhan · 2019-05-17T05:43:34Z

This is renaming a single label value, not the metric name, nor the label-key. I don’t know whether deprecation is necessary here as it’s more a time series identity label rather than one you would actively query and for what it’s worth the underlying metrics were broken until Sergiusz Urbaniak recently fixed them (the PR may still be open in which case they are still broken).

It is used to construct a number of different metric names:

# HELP APIServiceOpenAPIAggregationControllerQueue1_adds Total number of adds handled by workqueue: APIServiceOpenAPIAggregationControllerQueue1
# TYPE APIServiceOpenAPIAggregationControllerQueue1_adds counter
APIServiceOpenAPIAggregationControllerQueue1_adds 2607
# HELP APIServiceOpenAPIAggregationControllerQueue1_depth Current depth of workqueue: APIServiceOpenAPIAggregationControllerQueue1
# TYPE APIServiceOpenAPIAggregationControllerQueue1_depth gauge
APIServiceOpenAPIAggregationControllerQueue1_depth 0
# HELP APIServiceOpenAPIAggregationControllerQueue1_queue_latency How long an item stays in workqueueAPIServiceOpenAPIAggregationControllerQueue1 before being requested.
# TYPE APIServiceOpenAPIAggregationControllerQueue1_queue_latency summary
APIServiceOpenAPIAggregationControllerQueue1_queue_latency{quantile="0.5"} 13
APIServiceOpenAPIAggregationControllerQueue1_queue_latency{quantile="0.9"} 17
APIServiceOpenAPIAggregationControllerQueue1_queue_latency{quantile="0.99"} 17
APIServiceOpenAPIAggregationControllerQueue1_queue_latency_sum 52987
APIServiceOpenAPIAggregationControllerQueue1_queue_latency_count 2607
# HELP APIServiceOpenAPIAggregationControllerQueue1_retries Total number of retries handled by workqueue: APIServiceOpenAPIAggregationControllerQueue1
# TYPE APIServiceOpenAPIAggregationControllerQueue1_retries counter
APIServiceOpenAPIAggregationControllerQueue1_retries 3478
# HELP APIServiceOpenAPIAggregationControllerQueue1_work_duration How long processing an item from workqueueAPIServiceOpenAPIAggregationControllerQueue1 takes.
# TYPE APIServiceOpenAPIAggregationControllerQueue1_work_duration summary
APIServiceOpenAPIAggregationControllerQueue1_work_duration{quantile="0.5"} 96023
APIServiceOpenAPIAggregationControllerQueue1_work_duration{quantile="0.9"} 140829
APIServiceOpenAPIAggregationControllerQueue1_work_duration{quantile="0.99"} 140829
APIServiceOpenAPIAggregationControllerQueue1_work_duration_sum 3.69094537e+08
APIServiceOpenAPIAggregationControllerQueue1_work_duration_count 2607

brancz · 2019-05-17T06:56:44Z

Thanks, I didn’t see that while browsing the code. That’s unfortunate. In that case I agree this should be changed to snake case and remove the trailing number. In regards to deprecation I think these metrics violate just about every guideline we have and I can’t find any usages in the popular sources. I think just changing would be ok.

s-urbaniak · 2019-05-17T07:06:05Z

@logicalhan @brancz thanks for verifying the bigger picture! It seems we have a consensus to a) convert this to snake case and b) omit deprecation. I am pushing a commit on top of this then.

s-urbaniak · 2019-05-17T07:32:45Z

fwiw I did a quick sweep and verified we have more camel-case occurences:

kubernetes/staging/src/k8s.io/kube-aggregator/pkg/apiserver/apiservice_controller.go

Line 62 in afd928b

    
           queue:             workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "APIServiceRegistrationController"),

kubernetes/pkg/controller/clusterroleaggregation/clusterroleaggregation_controller.go

Line 58 in afd928b

    
           queue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "ClusterRoleAggregator"),

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/apiserver/customresource_discovery_controller.go

Line 62 in 3b618af

    
           queue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "DiscoveryController"),

kubernetes/staging/src/k8s.io/kube-aggregator/pkg/controllers/status/available_controller.go

Lines 96 to 101 in f9e1620

    
           queue: workqueue.NewNamedRateLimitingQueue( 
        
           	// We want a fairly tight requeue time.  The controller listens to the API, but because it relies on the routability of the 
        
           	// service network, it is possible for an external, non-watchable factor to affect availability.  This keeps 
        
           	// the maximum disruption time to a minimum, but it does prevent hot loops. 
        
           	workqueue.NewItemExponentialFailureRateLimiter(5*time.Millisecond, 30*time.Second), 
        
           	"AvailableConditionController"),

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/establish/establishing_controller.go

Line 55 in 42f23bd

    
           queue:     workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "crdEstablishing"),

Finally the sample controller includes a camel case example which we probably should also fix along with a comment:

kubernetes/staging/src/k8s.io/sample-controller/controller.go

Line 110 in 9030187

    
           workqueue:         workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "Foos"),

I can grep to check if those are in general use and submit another cleanup PR fixing the above.

… limiter

sttts · 2020-06-24T13:53:55Z

/remove-lifecycle rotten

sttts · 2020-06-24T13:54:08Z

/approve

k8s-ci-robot · 2020-06-24T13:59:02Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

~~staging/src/k8s.io/kube-aggregator/OWNERS~~ [sttts]

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

dims · 2020-06-29T17:06:30Z

/test all

fejta-bot · 2020-06-29T22:38:46Z

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

Review the full test history for this PR.