-
Notifications
You must be signed in to change notification settings - Fork 2
Subgraph Timeout Configuration #478
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
✅
|
🐋 This PR was built and pushed to the following Docker images: Image Names: Platforms: Image Tags: Docker metadata{
"buildx.build.ref": "builder-e245c9ad-3ca0-4cd4-9450-56f1f68fdc6f/builder-e245c9ad-3ca0-4cd4-9450-56f1f68fdc6f0/tcg5nw4ixl9l0pt8aj4zwbdrf",
"containerimage.descriptor": {
"mediaType": "application/vnd.oci.image.index.v1+json",
"digest": "sha256:ca064b18f1364a6a8b62caa28762c68d3aca1fcce3b5ac09e8d7368fca3e0d5b",
"size": 1609
},
"containerimage.digest": "sha256:ca064b18f1364a6a8b62caa28762c68d3aca1fcce3b5ac09e8d7368fca3e0d5b",
"image.name": "ghcr.io/graphql-hive/router:pr-478,ghcr.io/graphql-hive/router:sha-3535e07"
} |
@gemini-code-assist review and summary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements timeout configuration for subgraph requests, adding support for both fixed duration timeouts and dynamic expression-based timeouts using VRL (Vector Remap Language). The implementation also restructures the traffic shaping configuration to support global defaults and per-subgraph overrides, following Apollo Router's pattern.
Key changes:
- Added timeout configuration with support for fixed durations and VRL expressions
- Restructured traffic shaping config to support
all
(global) andsubgraphs
(per-subgraph) configurations - Implemented timeout execution using
tokio::time::timeout
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
lib/router-config/src/traffic_shaping.rs | Added HTTPTimeoutConfig enum and timeout field to TrafficShapingExecutorConfig |
lib/router-config/src/lib.rs | Restructured traffic shaping config with TrafficShapingConfig wrapper |
lib/executor/src/executors/timeout.rs | New module implementing timeout logic with VRL expression support |
lib/executor/src/executors/mod.rs | Added timeout module export |
lib/executor/src/executors/map.rs | Updated executor map creation to handle per-subgraph configurations |
lib/executor/src/executors/http.rs | Integrated timeout functionality into HTTP executor |
lib/executor/src/executors/error.rs | Added RequestTimeout error variant |
lib/executor/src/executors/common.rs | Added client_request field to HttpExecutionRequest |
lib/executor/src/execution/plan.rs | Updated request creation to include client_request |
docs/README.md | Updated documentation to reflect new configuration structure |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a valuable feature for configuring timeouts on subgraph requests, both globally and on a per-subgraph basis. The implementation correctly uses tokio::time::timeout
for handling timeouts and VRL expressions for dynamic timeout values. The configuration structure is well-designed, allowing for flexible overrides. My review includes suggestions to correct the documentation examples to align with the implementation, prevent a potential panic when handling negative timeout values from VRL expressions, and an optimization for resource usage related to request deduplication maps.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
lib/executor/src/executors/timeout.rs:1
- Corrected 'type' to 'kind' to match the actual field name in OperationDetails.
use std::collections::BTreeMap;
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@gemin-code-assist review again |
@gemini-code-assist review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a powerful timeout configuration feature for subgraphs, including support for dynamic timeouts using VRL expressions. The implementation is solid, using the decorator pattern to wrap executors with timeout logic. The configuration is also well-structured, allowing for both global and per-subgraph settings, similar to Apollo Router.
My review focuses on improving the documentation to make these new features clearer to users and suggests a couple of small enhancements for better flexibility and efficiency. Overall, this is a great addition.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Implementation of Timeout in #317
This also adds
subgraphs
andall
options totraffic_shaping
as in Apollo Router. So subgraph specific configuration can be done withsubgraphs
;Apollo Router -> https://www.apollographql.com/docs/graphos/routing/performance/traffic-shaping#configuration
Notes:
tokio
'stimeout
works as expected. But let me know if this is not the right way. Also let me know if dynamic timeout expression implementation is good for you.