Pr 4 #4
Pr 4 #4
Conversation
Refactor the rate limiting configuration to use tokio's `watch` channels instead of `mpsc` channels.
Previously, the limiter Arc was being cloned instead of downgraded inside the spawned async task, preventing the Arc from being properly deallocated when no longer in use. This change downgrades the limiter Arc before passing it to the async task to ensure memory is managed correctly.
- Changed various rate limit key representations to use the common `RateLimitKey` enum. - Updated configuration functions to return `Vec` instead of `HashMap`.
| for (name, config) in config.rate_limiting_configs { | ||
| let Some(limiter) = create_limiter(config) else { | ||
| for (name, config) in updates.borrow_and_update().iter() { | ||
| let Some(limiter) = create_limiter(*config) else { |
There was a problem hiding this comment.
⚡ Suggestion
Ensure that the create_limiter function handles the case where config might be invalid or not as expected, to avoid potential runtime errors.
| Self { | ||
| config_path, | ||
| rate_limit_sender: rate_limit_sender.into(), | ||
| rate_limit_sender, |
There was a problem hiding this comment.
⚡ Suggestion
Ensure that the rate_limit_sender is properly validated or checked for errors when being passed to the ConfigWatcher constructor to avoid potential runtime issues.
| } | ||
|
|
||
| RedisRateLimiter::runtime(global_config, rx) | ||
| RedisRateLimiter::runtime(global_config, rate_limit_rx) |
There was a problem hiding this comment.
⚡ Suggestion
Ensure that the rate_limit_rx variable is properly handled in case of errors during runtime.
| @@ -57,7 +56,12 @@ pub(super) async fn new_gateway(config: Option<engine_v2::VersionedConfig>) -> O | |||
| .into_iter() | |||
| .map(|(k, v)| { | |||
| ( | |||
| k.to_string(), | |||
| match k { | |||
There was a problem hiding this comment.
⚡ Suggestion
Consider handling the case where k does not match any of the expected variants. This will help prevent potential runtime errors if an unexpected value is encountered.
| @@ -4,7 +4,10 @@ use grafbase_telemetry::{ | |||
| gql_response_status::{GraphqlResponseStatus, SubgraphResponseStatus}, | |||
| span::{GqlRecorderSpanExt, GRAFBASE_TARGET}, | |||
| }; | |||
| use runtime::fetch::{FetchRequest, FetchResponse}; | |||
| use runtime::{ | |||
| fetch::{FetchRequest, FetchResponse}, | |||
There was a problem hiding this comment.
⚡ Suggestion
Consider ensuring that the RateLimitKey is properly defined and imported to avoid potential runtime errors.
✨
Description by Cal
PR Description
This PR introduces a new rate limiting feature by refactoring the existing rate limiting context and updating the related components to use a new
RateLimitKeyenum. It replaces the previousRateLimiterContextwith a more flexible approach usingRateLimitKeyfor both global and subgraph rate limits.Key Issues
None
Files Changed
File: /cli/crates/federated-dev/src/dev/gateway_nanny.rs
Updated rate limiting logic to use `watch` channel instead of `mpsc`.File: /engine/crates/engine-v2/config/src/lib.rs
Removed unused constant `GLOBAL_RATE_LIMIT_KEY`.File: /engine/crates/engine-v2/src/engine.rs
Updated rate limiting checks to use the new `RateLimitKey` enum.File: /engine/crates/runtime-local/src/rate_limiting/in_memory/key_based.rs
Refactored rate limiting context to use `RateLimitKey`.File: /gateway/crates/federated-server/src/config.rs
Updated rate limit configuration to use `RateLimitKey`.Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.
Type of change