feat: add support for dynamic rate limit configurations with hot reload by devslovecoffee · Pull Request #3 · CallstackAI/grafbase

devslovecoffee · 2024-08-02T06:49:37Z

✨ Description by Cal

PR Description

This PR introduces dynamic rate limiting configurations with hot reload functionality, allowing updates to rate limits without server restarts. It includes changes to various files to support this feature, including the addition of a new hot_reload module.

Diagrams of code changes

sequenceDiagram
    participant User
    participant GatewayNanny
    participant RateLimiter
    participant ConfigWatcher

    User->>GatewayNanny: Request to update configuration
    GatewayNanny->>ConfigWatcher: Watch for configuration changes
    ConfigWatcher->>RateLimiter: Send updated rate limiting configurations
    RateLimiter->>GatewayNanny: Apply new rate limits
    GatewayNanny->>User: Configuration updated successfully

Key Issues

None

Files Changed

File: /Cargo.lock

Added `notify` dependency for file watching.

File: /cli/crates/federated-dev/src/dev/gateway_nanny.rs

Updated `new_gateway` function to include a channel for rate limit updates.

File: /engine/crates/integration-tests/src/federation/builder/test_runtime.rs

Modified `TestRuntime` to include a channel for rate limit updates.

File: /engine/crates/runtime-local/src/rate_limiting/in_memory/key_based.rs

Refactored `InMemoryRateLimiter` to support dynamic updates via a channel.

File: /engine/crates/runtime-local/src/rate_limiting/redis.rs

Updated `RedisRateLimiter` to accept dynamic rate limit configurations.

File: /engine/crates/runtime/src/rate_limiting.rs

Changed `KeyedRateLimitConfig` to use `String` instead of `&str`.

File: /gateway/crates/federated-server/src/config/hot_reload.rs

Added a new module for watching configuration files and reloading rate limits.

File: /gateway/crates/federated-server/src/config/rate_limit.rs

Updated `GraphRateLimit` struct to derive `Copy`.

File: /gateway/crates/federated-server/src/server.rs

Modified `serve` function to handle hot reload configuration.

File: /gateway/crates/gateway-binary/src/args.rs

Added methods to handle configuration path and hot reload flag.

File: /gateway/crates/gateway-binary/src/main.rs

Updated main function to pass server configuration including hot reload settings.

- Introduce dynamic rate limiting configurations - Incorporate hot reload functionality to update rate limits without a server restart - Allow configuration of hot reload of rate limits in `gateway` server setup

Description

Please include a summary of the change and which issue is fixed.
Please also include relevant motivation and context.

Type of change

- Introduce dynamic rate limiting configurations - Incorporate hot reload functionality to update rate limits without a server restart - Allow configuration of hot reload of rate limits in `gateway` server setup

devslovecoffee · 2024-08-02T08:12:32Z

gateway/crates/federated-server/src/server.rs

-    config: Config,
-    fetch_method: GraphFetchMethod,
-    otel_tracing: Option<OtelTracing>,
+    ServerConfig {


⚡ Suggestion
Ensure that the config_hot_reload functionality is properly validated and handled in the serve function to avoid unexpected behavior during runtime. Consider adding checks or logging to confirm that hot reload is functioning as intended.

devslovecoffee · 2024-08-02T08:12:33Z

gateway/crates/gateway-binary/src/args/lambda.rs

        }
    }

+    fn hot_reload(&self) -> bool {


⚡ Suggestion
Consider implementing the hot_reload method to actually check for configuration changes instead of returning a static false. This will make the feature functional and allow for dynamic updates as intended.

devslovecoffee · 2024-08-02T08:12:34Z

engine/crates/runtime-local/src/rate_limiting/redis.rs

        let Some(key) = context.key() else { return Ok(()) };

-        let Some(config) = self.subgraph_limits.get(key) else {
+        let Some(config) = self.limits.borrow().get(key).copied() else {


⚡ Suggestion
Consider handling the case where self.limits.borrow() might fail, to avoid potential panics.

devslovecoffee · 2024-08-02T08:12:35Z

gateway/crates/federated-server/src/config/hot_reload.rs

+}
+
+impl RateLimitSender {
+    fn send(&self, data: RateLimitData) -> crate::Result<()> {


⚡ Suggestion
Consider using expect instead of unwrap for error handling in the send method to provide more context in case of failure.

devslovecoffee · 2024-08-02T08:12:35Z

gateway/crates/gateway-binary/src/args.rs


    fn config(&self) -> anyhow::Result<Config>;

+    fn config_path(&self) -> Option<&Path>;


⚡ Suggestion
Ensure that the config_path method is implemented in a way that it returns a valid path or None, to avoid potential runtime errors when accessing the configuration.

devslovecoffee · 2024-08-02T08:12:37Z

gateway/crates/gateway-binary/src/main.rs

-        federated_server::serve(args.listen_address(), config, args.fetch_method()?, otel_tracing).await?;
+        federated_server::serve(ServerConfig {
+            listen_addr: args.listen_address(),
+            config,


⚡ Suggestion
Ensure that the config variable is properly initialized and contains valid data before passing it to federated_server::serve. This will help avoid potential issues during runtime.

devslovecoffee · 2024-08-02T08:12:38Z

gateway/crates/gateway-binary/src/main.rs

        tracing::info!(target: GRAFBASE_TARGET, "Grafbase Gateway {crate_version}");

-        federated_server::serve(args.listen_address(), config, args.fetch_method()?, otel_tracing).await?;
+        federated_server::serve(ServerConfig {


⚡ Suggestion
Consider validating the args parameters before using them to ensure they meet expected formats or constraints, especially for config_path and listen_addr. This can prevent runtime errors.

devslovecoffee · 2024-08-02T08:12:39Z

gateway/crates/federated-server/src/server/gateway.rs

-            RedisRateLimiter::runtime(global_config, rate_limiting_configs)
+            match config_path {
+                Some(path) if config_hot_reload => {
+                    hot_reload::ConfigWatcher::new(path, tx).watch()?;


⚡ Suggestion
Consider handling the case where hot_reload::ConfigWatcher::new(path, tx).watch()? fails, to prevent the application from crashing.

devslovecoffee · 2024-08-02T08:12:39Z

gateway/crates/federated-server/src/server/gateway.rs

+            let (tx, rx) = mpsc::channel(100);
+
+            match config_path {
+                Some(path) if config_hot_reload => {


🐛 Bug
The config_path and config_hot_reload variables are checked for Some(path) and true, respectively, but there is no handling for the case where config_path is None or config_hot_reload is false. This could lead to unexpected behavior if the configuration is not set correctly.

Suggested change

Some(path) if config_hot_reload => {

if config_hot_reload { return Err(crate::Error::InternalError("Hot reload is enabled but config_path is not set".into())); }

devslovecoffee · 2024-08-02T08:12:40Z

gateway/crates/federated-server/src/server/gateway.rs

    pub timeout: Option<Duration>,
    pub entity_caching: EntityCachingConfig,
+    pub config_hot_reload: bool,
+    pub config_path: Option<PathBuf>,


⚡ Suggestion
Ensure that the config_path is validated before being used to avoid potential runtime errors if the path is invalid.

feat: add support for dynamic rate limit configurations with hot reload

b3eca9c

- Introduce dynamic rate limiting configurations - Incorporate hot reload functionality to update rate limits without a server restart - Allow configuration of hot reload of rate limits in `gateway` server setup

devslovecoffee commented Aug 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

feat: add support for dynamic rate limit configurations with hot reload#3

feat: add support for dynamic rate limit configurations with hot reload#3
devslovecoffee wants to merge 1 commit intopr3-basefrom
pr-3

devslovecoffee commented Aug 2, 2024 •

edited

Loading

Uh oh!

devslovecoffee Aug 2, 2024

Uh oh!

devslovecoffee Aug 2, 2024

Uh oh!

devslovecoffee Aug 2, 2024

Uh oh!

devslovecoffee Aug 2, 2024

Uh oh!

devslovecoffee Aug 2, 2024

Uh oh!

devslovecoffee Aug 2, 2024

Uh oh!

devslovecoffee Aug 2, 2024

Uh oh!

devslovecoffee Aug 2, 2024

Uh oh!

devslovecoffee Aug 2, 2024

Uh oh!

devslovecoffee Aug 2, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		fn config(&self) -> anyhow::Result<Config>;

		fn config_path(&self) -> Option<&Path>;

	Some(path) if config_hot_reload => {
	if config_hot_reload { return Err(crate::Error::InternalError("Hot reload is enabled but config_path is not set".into())); }

Comments

Conversation

devslovecoffee commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description by Cal

PR Description

Key Issues

Files Changed

Description

Type of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

devslovecoffee commented Aug 2, 2024 •

edited

Loading