Exclude gRPC health check requests from rate limiting in history & matching service #6036
Conversation
There was a problem hiding this comment.
Overall this approach makes sense.
If a method exists in the map with a value of 0 that indicates it would consume 0 tokens from a periodically filled token bucket. So therefore, don't bother trying to consume and return nil to proceed.
If a method does NOT exist in the map, token will be 0 due to go zero value semantics. However, existence check overrides token value with the default of 1. This makes the !ok check extremely critical.
Out of curiosity, I wonder what happens if the value is less than zero? I recognize that any odd handling would not be a new issue introduced by this PR.
|
Just because there's so many layers of interceptors and behavior, it might be nice to do a manual test also: set the rate limit really low and use evans or some other cli grpc tool to just run a bunch of health checks and non-health-checks and make sure it does what we want. |
Co-authored-by: Tim Deeb-Swihart <409226+tdeebswihart@users.noreply.github.com>
The value indicates how many tokens to consume per hit for a given endpoint. So any value <= 0 should be ignored. Tim's suggestion covers that now. Without it would run into some weirdness unless underlying libraries safely handle invalid values. |
Co-authored-by: David Reiss <david@temporal.io>
Co-authored-by: David Reiss <david@temporal.io>
…oral into cg_healthcheck_ratelimit
|
Manually verified using postman. |
What changed?
The rate limit interceptor will now ignore gRPC health check requests and doesn't apply any rate limiters.
Why?
Health check requests should never be rate limited.
How did you test it?
Added unit tests to assert interceptor behavior.
Potential risks
N/A
Documentation
N/A
Is hotfix candidate?
no