-
Notifications
You must be signed in to change notification settings - Fork 39.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve rate limiter latency logging and add component-base metric #88134
Conversation
/retest |
/cc @logicalhan |
/assign @logicalhan |
fbe4d1d
to
4e78c71
Compare
@logicalhan addressed comments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
We should probably add a clock to the logger so that we can add some unit tests for this. Then we can probably verify that this thing is doing what we think it's doing.
Ah there's something wrong with it now. |
I told you I didn't test it. |
4e78c71
to
71b534e
Compare
I'll add a unit test :) |
If you add a clock to the logger, it'll make testing much easier. |
71b534e
to
6bb8ea9
Compare
6bb8ea9
to
be4e51e
Compare
be4e51e
to
2bcf99f
Compare
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
if time.Since(b.lastLogTime) > b.minLogInterval { | ||
klog.V(level).Info(message) | ||
b.lastLogTime = time.Now() | ||
func (b *throttledLogger) attemptToLog() (klog.Level, bool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a docstring on the return values. My first super quick skim, I was super confused by the -1 (it makes sense once I read the code though).
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jennybuckley, lavalamp 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:
Approvers can indicate their approval by writing |
/retest Review the full test history for this PR. Silence the bot with an |
1 similar comment
/retest Review the full test history for this PR. Silence the bot with an |
We've found that the rate limiting metric wasn't exporting any metrics, in spite of clearly seeing the metric in the disassembled binary. As it turns out, the rest_client_rate_limiter_duration_seconds metric has been added as part of the logging improvements, but it appears to have been accidentally forgotten to be registered. kubernetes#88134
We've found that the rate limiting metric wasn't exporting any metrics, in spite of clearly seeing the metric in the disassembled binary. As it turns out, the rest_client_rate_limiter_duration_seconds metric has been added as part of the logging improvements, but it appears to have been accidentally forgotten to be registered. kubernetes/kubernetes#88134 Kubernetes-commit: da9ffb8458d49b2f5710e1acd640b436d7163b3e
We've found that the rate limiting metric wasn't exporting any metrics, in spite of clearly seeing the metric in the disassembled binary. As it turns out, the rest_client_rate_limiter_duration_seconds metric has been added as part of the logging improvements, but it appears to have been accidentally forgotten to be registered. kubernetes#88134
…!893) Fix rest_client_rate_limiter_duration_seconds not registered Fix rest_client_rate_limiter_duration_seconds not registered We've found that the rate limiting metric wasn't exporting any metrics, in spite of clearly seeing the metric in the disassembled binary. As it turns out, the rest_client_rate_limiter_duration_seconds metric has been added as part of the logging improvements, but it appears to have been accidentally forgotten to be registered. kubernetes#88134
What type of PR is this?
/sig api-machinery
/priority important-soon
/kind feature
/cc @lavalamp
What this PR does / why we need it:
Follow up to #87740
Adds a more severe and infrequent log message as suggested in #87740 (comment)
Also adds a new client-go metric to track the rate limiter latency across all invocations.
Does this PR introduce a user-facing change?: