[release/7.0] [RateLimiting] Handle Timer jitter #74971
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Tagging subscribers to this area: @mangod9 Issue DetailsBackport of #74360 to release/7.0 /cc @BrennanConroy Customer ImpactTestingRiskIMPORTANT: Is this backport for a servicing release? If so and this change touches code that ships in a NuGet package, please make certain that you have added any necessary package authoring and gotten it explicitly reviewed.
|
|
approved -- new feature, significant customer feedback. |
|
@BrennanConroy Please let me know when it's ready to merge, I see you pushed a new commit (this also need a code review sign-off). |
...em.Threading.RateLimiting/src/System/Threading/RateLimiting/FixedWindowRateLimiterOptions.cs
Outdated
Show resolved
Hide resolved
....Threading.RateLimiting/src/System/Threading/RateLimiting/SlidingWindowRateLimiterOptions.cs
Outdated
Show resolved
Hide resolved
...em.Threading.RateLimiting/src/System/Threading/RateLimiting/TokenBucketRateLimiterOptions.cs
Outdated
Show resolved
Hide resolved
...em.Threading.RateLimiting/src/System/Threading/RateLimiting/FixedWindowRateLimiterOptions.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Stephen Halter <halter73@gmail.com>
|
@carlossanlop This is ready now. The failures are unrelated to the change, not sure how to track them since they're fairly poor errors :/ |
|
Yeah, I can confirm that the failures are all infra, unrelated to this. The actual test failure happened in the Networking area, and the logs are unreachable. |
ghost
locked as resolved and limited conversation to collaborators
Backport of #74360 to release/7.0
/cc @BrennanConroy
Customer Impact
Customer reported issue in #74056.
Due to "timer jitter", at load our rate limiter implementations would skip roughly every other replenish resulting in the user expected rate limit to be 75% of expectations.
Testing
Did manual testing of the customer scenario with different limits and refresh intervals and added internal metrics while testing to make sure the limit was being honored more smoothly than before and closer to the theoretical asked for limit.
Risk
Low.
The users rate limit is now less restrictive than it was in RC1 and previews. This means if they noticed it limiting more than expected they might have increased the limits to get their expected limit. After this change the limits will be respected better at load and could cause more traffic than expected due to their previously increased limits.
Since this is a new feature in 7.0 it's unlikely many people will have noticed this issue yet so it should be low risk to change now.