Rate limiting fixes (config parsing, better logging) #1967
Merged
Conversation
This file contains 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
…message construction
Molter73
reviewed
Nov 20, 2024
Co-Authored-By: Mauro Ezequiel Moltrasio <[email protected]>
Molter73
approved these changes
Nov 20, 2024
| @@ -326,15 +330,25 @@ void NetworkStatusNotifier::AddConnections(::google::protobuf::RepeatedPtrField< | |||
| // the case where we send a close event for a connection that it doesn't | |||
| // know about. | |||
| // | |||
| if (!rate_limiter.Allow(delta_entry.first.container())) { | |||
| CLOG_THROTTLED(INFO, std::chrono::seconds(5)) << "Rate limiting connections reported from container " << delta_entry.first.container(); | |||
| const std::string& container = delta_entry.first.container(); | |||
There was a problem hiding this comment.
Could we use a std::string_view directly? I believe it's a trivial cast back to const std::string& so the call to Allow on the next line should still work.
Suggested change
| const std::string& container = delta_entry.first.container(); | |
| const std::string_view container = delta_entry.first.container(); |
There was a problem hiding this comment.
I didn't realise this implicit casting was a thing, I'll change it.
There was a problem hiding this comment.
Unfortunately it looks like the string_view is not able to cast back into a string (I think since it would require a copy)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
So this is embarrassing. The rate limit wasn't parsed from the config and copied over to the local runtime config object, which meant that the rate limit was set to zero and no network connections were sent from Collector.
This fixes that. It also adds some more useful logging around rate limiting.
Testing Performed
Tested locally on OCP 4 cluster.