-
Notifications
You must be signed in to change notification settings - Fork 7
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
Add in log filter for pacing logs #497
Conversation
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.
It's great how it integrated well with log filters. I've asked implementation detail changes though. Other than that:
- This needs to land as patch, bumping the version
- This needs unit test
- This needs changelog to be updated
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.
Nicely done. Great to have this tightly integrated in the logger @Ktmi.
Overall looks good, I'm waiting to see unit tests, changelog and version being bumped before approving though.
Did you also have the chance to confirm with the latest refactor that 1.0
lockout_time worked as you'd expect? Let's exercise this in the rate limit and seeing if it's being helpful to understand the timeline where it kicked in and then stopped.
Also, final trivial related point, in the log message did you mean "Limited reached"
or "Limit reached"
?
@viniarck This one should be good to go now. The lower lockout_time works as expected.
|
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.
Nicely done, David. LGTM.
Let's ship it.
Closes #495
Summary
Adds in
RepeatMessageFilter
which caches a limited number of log messages, and prevents any repeat messages from being sent out for a short period of time. The way I had to implement this is a little janky. Prefarably the logging would be loaded and setup before kytos core is loaded, which is part of the reason why the filter exists inkytos.logging,
as I plan to move the logging code to there.Local Tests
Repeat pacing messages are now suppressed, rather than shown in the logs. Works as expected.
End-to-End Tests
Here are the E2E results: