Simplify StreamRateLimiter and make its stats more flexible#44145
Merged
ravenblackx merged 1 commit intoenvoyproxy:mainfrom Mar 31, 2026
Merged
Simplify StreamRateLimiter and make its stats more flexible#44145ravenblackx merged 1 commit intoenvoyproxy:mainfrom
ravenblackx merged 1 commit intoenvoyproxy:mainfrom
Conversation
Signed-off-by: Raven Black <ravenblack@dropbox.com>
ravenblackx
requested review from
KBaichoo,
mattklein123,
tonya11en and
yanavlasov
as code owners
Contributor
Author
|
Picking @tonya11en to review since you helped me simplify the fair token bucket and this is going to be part of the same thing. (Mostly I just wanted the more usable stats, but the clunky constructor kept bothering me as well.) |
Contributor
Author
|
/retest |
tonya11en
approved these changes
Mar 31, 2026
TAOXUY
pushed a commit
to TAOXUY/envoy
that referenced
this pull request
Apr 1, 2026
…xy#44145) Commit Message: Simplify StreamRateLimiter and make its stats more flexible Additional Description: The constructor of StreamRateLimiter only conditionally uses two of its mandatory input arguments (if a TokenBucket is provided those args are ignored). Instead of doing that, it makes more sense to have the TokenBucket be mandatory, and provide a helper function to convert those other two args into a TokenBucket argument - that way clients that want to use the TokenBucket method aren't forced to provide those unused args. And the stats callback of StreamRateLimiter exposes "there is stuff in the buffer" where it's just as easy to provide "there is this much stuff in the buffer" - doing the latter allows the client to construct more valuable stats. A test setup function that was never called was also removed. Risk Level: Very low - existing behavior is effectively unchanged. (External extensions using StreamRateLimiter will need to update to the new API.) Testing: Existing test coverage covers this just as well as it did the old version. Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Raven Black <ravenblack@dropbox.com> Signed-off-by: Xuyang Tao <taoxuy@google.com>
nshipilov
pushed a commit
to nshipilov/envoy
that referenced
this pull request
Apr 13, 2026
…xy#44145) Commit Message: Simplify StreamRateLimiter and make its stats more flexible Additional Description: The constructor of StreamRateLimiter only conditionally uses two of its mandatory input arguments (if a TokenBucket is provided those args are ignored). Instead of doing that, it makes more sense to have the TokenBucket be mandatory, and provide a helper function to convert those other two args into a TokenBucket argument - that way clients that want to use the TokenBucket method aren't forced to provide those unused args. And the stats callback of StreamRateLimiter exposes "there is stuff in the buffer" where it's just as easy to provide "there is this much stuff in the buffer" - doing the latter allows the client to construct more valuable stats. A test setup function that was never called was also removed. Risk Level: Very low - existing behavior is effectively unchanged. (External extensions using StreamRateLimiter will need to update to the new API.) Testing: Existing test coverage covers this just as well as it did the old version. Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Raven Black <ravenblack@dropbox.com> Signed-off-by: Nick Shipilov <nick.shipilov.n@gmail.com>
phlax
pushed a commit
that referenced
this pull request
Apr 13, 2026
Relnote to accompany the change made in #44145, in case of external filters using the API. Signed-off-by: Raven Black <ravenblack@dropbox.com>
krinkinmu
pushed a commit
to grnmeira/envoy
that referenced
this pull request
Apr 20, 2026
…xy#44145) Commit Message: Simplify StreamRateLimiter and make its stats more flexible Additional Description: The constructor of StreamRateLimiter only conditionally uses two of its mandatory input arguments (if a TokenBucket is provided those args are ignored). Instead of doing that, it makes more sense to have the TokenBucket be mandatory, and provide a helper function to convert those other two args into a TokenBucket argument - that way clients that want to use the TokenBucket method aren't forced to provide those unused args. And the stats callback of StreamRateLimiter exposes "there is stuff in the buffer" where it's just as easy to provide "there is this much stuff in the buffer" - doing the latter allows the client to construct more valuable stats. A test setup function that was never called was also removed. Risk Level: Very low - existing behavior is effectively unchanged. (External extensions using StreamRateLimiter will need to update to the new API.) Testing: Existing test coverage covers this just as well as it did the old version. Docs Changes: n/a Release Notes: n/a Platform Specific Features: n/a Signed-off-by: Raven Black <ravenblack@dropbox.com>
krinkinmu
pushed a commit
to grnmeira/envoy
that referenced
this pull request
Apr 20, 2026
Relnote to accompany the change made in envoyproxy#44145, in case of external filters using the API. Signed-off-by: Raven Black <ravenblack@dropbox.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit Message: Simplify StreamRateLimiter and make its stats more flexible
Additional Description: The constructor of StreamRateLimiter only conditionally uses two of its mandatory input arguments (if a TokenBucket is provided those args are ignored). Instead of doing that, it makes more sense to have the TokenBucket be mandatory, and provide a helper function to convert those other two args into a TokenBucket argument - that way clients that want to use the TokenBucket method aren't forced to provide those unused args.
And the stats callback of StreamRateLimiter exposes "there is stuff in the buffer" where it's just as easy to provide "there is this much stuff in the buffer" - doing the latter allows the client to construct more valuable stats.
A test setup function that was never called was also removed.
Risk Level: Very low - existing behavior is effectively unchanged. (External extensions using StreamRateLimiter will need to update to the new API.)
Testing: Existing test coverage covers this just as well as it did the old version.
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a