fix bug: throttledRateLimiter is not once per minute, but five times #7156
Conversation
|
|
|
hi @devilcoolyue! are you able to sign the CLA above? #7156 (comment) |
hello @trask ! I just signed the CLA |
devilcoolyue
force-pushed
the
fix-bug-throttling-logger
branch
from
4fe326f to
bb3168d
Compare
devilcoolyue
force-pushed
the
fix-bug-throttling-logger
branch
from
bb3168d to
d8c65d1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #7156 +/- ##
============================================
+ Coverage 89.85% 89.87% +0.01%
- Complexity 6622 6624 +2
============================================
Files 740 740
Lines 20007 20007
Branches 1968 1968
============================================
+ Hits 17978 17982 +4
+ Misses 1439 1437 -2
+ Partials 590 588 -2 ☔ View full report in Codecov by Sentry. |
| @@ -41,7 +41,8 @@ public ThrottlingLogger(Logger delegate) { | |||
| this.fastRateLimiter = | |||
| new RateLimiter(RATE_LIMIT / rateTimeUnit.toSeconds(1), RATE_LIMIT, clock); | |||
| this.throttledRateLimiter = | |||
| new RateLimiter(RATE_LIMIT / rateTimeUnit.toSeconds(1), THROTTLED_RATE_LIMIT, clock); | |||
| new RateLimiter( | |||
| THROTTLED_RATE_LIMIT / rateTimeUnit.toSeconds(1), THROTTLED_RATE_LIMIT, clock); | |||
There was a problem hiding this comment.
The throttledRateLimiter is currently accruing 5 credits / 60 seconds, with a max balance of 1 credits.
This changes it to accrue 1 credit / 60 seconds, with a max balance of 1 credits.
Meanwhile, we have fastRateLimiter set to accrue 5 credits / 60 seconds, with a max balance of 5 credits.
fastRateLimiteris used to determine whether to log during normal operation.throttledRateLimiteris used to determine whether to log when throttled, which occurs when rate of logging exceedsfastRateLimiter.
The problem you're fixing is that throttledRateLimiter accrues credits too fast. I.e. despite the fact that it has a max balance of 1, it is still accruing new credits at a rate of 5 / 60 = .083 credits / second, or 1 credit every 12 seconds. So a noisy logger logging every 12 seconds (or more frequent) will be able to 5 logs per minute, when it should only be 1 log per minute.
Is that right? If so, could you adjust ThrottlingLoggerTest with a demonstration of how the current logic fails but this change fixes it?
cc @jkwatson who wrote this initial implementation
There was a problem hiding this comment.
Looks like this was just a typo. Yes, agree it needs a test, which was obviously missing from the first implementation!
After I stopped the otel collector, the program injected with agnet logged errors. The error logs were printed very frequently. I saw that the source code had already limited the flow of printing logs, but it was not as expected once a minute, but still five times a minute. Then I checked the code and found that there was an error in the parameter assignment. After changing it, it ran OK.
Thanks for reviewing the PR! I appreciate your time and any feedback you can provide.
application-log.txt