Skip to content
This repository has been archived by the owner on May 23, 2024. It is now read-only.

Fix bug of zero credit in rate limiter #614

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ChenX1993
Copy link

@ChenX1993 ChenX1993 commented Apr 24, 2023

Which problem is this PR solving?

Fix the bug that zero credit per second can still grand credit for the first request.

Short description of the changes

Additional explanation

I understand the jaeger-client-go is deprecated, and no new PR request is accepted.
I have to create a public PR (not need to merge) in order to patch in our own internal repo following our process.

@ChenX1993 ChenX1993 requested a review from a team as a code owner April 24, 2023 23:53
@ChenX1993 ChenX1993 requested review from pavolloffay and removed request for a team April 24, 2023 23:53
@@ -55,9 +55,13 @@ type ReconfigurableRateLimiter struct {

// NewRateLimiter creates a new ReconfigurableRateLimiter.
func NewRateLimiter(creditsPerSecond, maxBalance float64) *ReconfigurableRateLimiter {
balance := maxBalance
if creditsPerSecond == 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like it should be invalid condition to instantiate rate limiter with <=0 replenishment rate

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yurishkuro.
I feel 0 maybe a valid condition which should have the same effect as {const, 0}.

Supporting zero case will be useful when we want to totally mute the signal from a service endpoint when remotely controlling the sampling from collector using the perOperation strategy. (Setting the probability to 0 is not enough and we have to set the lowerbound to 0 as well).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants