fix: Makes response codes rate limited as well as prints a message when it is hit #6701
Conversation
docs/operate-and-deploy/installation/server-config/config-reference.md
Outdated
Show resolved
Hide resolved
Co-authored-by: Jim Galasyn <jim.galasyn@confluent.io>
There was a problem hiding this comment.
Thanks @AlanConfluent -- LGTM! The usual comments and questions inline.
| A comma-separated list of HTTP response codes to skip during server | ||
| request logging. This is useful for ignoring certain 4XX errors that you | ||
| might not want to show up in the logs. | ||
| A list of `path:rate_limit` pairs, to limit the rate of server request |
There was a problem hiding this comment.
Clarify the units of the rate limit (and also for the other config below). Appears to be number of messages per second?
There was a problem hiding this comment.
I clarified it's qps and even explicitly write out 10 logs per second as an example.
| logging. This is useful for limiting certain 4XX errors that you | ||
| might not want to blow up in the logs. | ||
| This setting enables seeing the logs when the request rate is low | ||
| and dropping them when they go over the threshold. |
There was a problem hiding this comment.
Mention that a message will be logged (at most once every five seconds) if the threshold is hit? (And same for the other config below.)
ksqldb-rest-app/src/main/java/io/confluent/ksql/api/server/LoggingRateLimiter.java
Show resolved
Hide resolved
| if (rateLimitedPaths.containsKey(path)) { | ||
| final double rateLimit = rateLimitedPaths.get(path); | ||
| rateLimiters.computeIfAbsent(path, (k) -> rateLimiterFactory.apply(rateLimit)); | ||
| return rateLimiters.get(path).tryAcquire(); | ||
| rateLimitersByPath.computeIfAbsent(path, (k) -> rateLimiterFactory.apply(rateLimit)); |
There was a problem hiding this comment.
Would it be better to initialize the rate limiters up front, rather than calling computeIfAbsent on each request? I don't have a sense of how large this optimization is, feels minor but I also feel it can't hurt. Feel free to disagree.
There was a problem hiding this comment.
Yeah, you're right. The data is static and not large, so why not just make an immutable map instead on initialization?
I was originally thinking it might be dynamic, but it's not currently.
ksqldb-rest-app/src/main/java/io/confluent/ksql/api/server/LoggingRateLimiter.java
Show resolved
Hide resolved
| logging. This is useful for limiting certain 4XX errors that you | ||
| might not want to blow up in the logs. | ||
| This setting enables seeing the logs when the request rate is low | ||
| and dropping them when they go over the threshold. | ||
|
|
||
| ### ksql.logging.server.rate.limited.request.paths |
There was a problem hiding this comment.
We're currently logging for internal endpoints (used in server-to-server communication for multi-node clusters), right? Would it make sense to disable logging for those by default? (I don't feel strongly one way or the other -- you have more context than I do about when/how often those endpoints are hit.)
There was a problem hiding this comment.
Yes, currently we log all requests, regardless of internal/external status. I think this could be useful if we're trying to trace pull queries falling back on standbys after failures. We can see if this ends up being useful and potentially remove internal if not.
AlanConfluent
force-pushed
the
all_ratelimited_logging
branch
from
f93c781
to
1c0ec7a
Compare
Description
Makes response codes logging rate limited too. Also prints a message when rate limit is hit.
Testing done
Unit tests and manual testing
Reviewer checklist