Add apiserver metric for number of requests dropped by 'max-inflight-requests' filters. #58340
Conversation
gmarek
added
the
release-note-none
k8s-ci-robot
added
do-not-merge/release-note-label-needed
| }, | ||
| ) | ||
| // DroppedMutatingRequests is a number of mutating requests dropped with 'Try again later' reponse | ||
| DroppedMutatingRequests = prometheus.NewCounter( |
There was a problem hiding this comment.
Why can't we have a single metric with label mutating set to true or false?
I think it would be much cleaner when looking into it.
There was a problem hiding this comment.
I was considering this, but I'm not sure how prometheus is implemented. It might have a lock per variable, not variable:label-set pair, which would make two variables slightly more efficient. If that's the case we gain a bit on contention by paying a bit on cleanness of code (but also not much), which is why I chose two variables approach.
There was a problem hiding this comment.
Personally I don't think it just cleanness of the code. If I don't know the metrics (and I will forget about it in few months probably), as soon as I will find the first one I will stop looking further. So i think it may be misleading for people using them.
That said, raising the lock contention here is a good point. But I'm not convinced how much effect this lock contention would have. Do we have some measurements? My personal opinion is that due to how apiserver is structures, those channels for tokens for max inflight requests, etc., the increase cost of lock contention will be negligible.
There was a problem hiding this comment.
I'll take your word on that.
gmarek
added
the
release-note-none
k8s-ci-robot
removed
the
release-note-none
timothysc
added
sig/scalability
| @@ -106,6 +107,12 @@ func WithMaxInFlightLimit( | |||
| } | |||
| } | |||
| metrics.Record(r, requestInfo, "", http.StatusTooManyRequests, 0, 0) | |||
| // We need to split this data between buckets used for throttling. | |||
There was a problem hiding this comment.
move to the top as droppedRequestBucket
There was a problem hiding this comment.
or shouldn't we add all this logic to metrics.Record?
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gmarek, wojtek-t Associated issue requirement bypassed by: wojtek-t The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
k8s-ci-robot
added
the
approved
|
Automatic merge from submit-queue (batch tested with PRs 58446, 58459, 58340). If you want to cherry-pick this change to another branch, please follow the instructions here. |
…-#58340-#58342-upstream-release-1.8 Automatic merge from submit-queue. Automated cherry pick of #58340: Add apiserver metric for number of requests dropped by #58342: Add a metric to track usage of inflight request limit. Cherry pick of #58340 #58342 on release-1.8. #58340: Add apiserver metric for number of requests dropped by #58342: Add a metric to track usage of inflight request limit. ```release-note Add apiserver metric for current inflight-request usage and number of requests dropped because of inflight limit. ```
k8s-ci-robot
added
release-note
…58342-upstream-release-1.9 Automatic merge from submit-queue. Automated cherry pick of #58340: Add apiserver metric for number of requests dropped by #58342: Add a metric to track usage of inflight request limit. Cherry pick of #58340 #58342 on release-1.9. #58340: Add apiserver metric for number of requests dropped by #58342: Add a metric to track usage of inflight request limit. ```release-note Add apiserver metric for current inflight-request usage and number of requests dropped because of inflight limit. ```
Useful for figuring out on which dimension master is overloaded.
cc @sttts @lavalamp @deads2k @timothysc @hulkholden