Skip to content

Conversation

@ardatan
Copy link
Member

@ardatan ardatan commented Oct 31, 2025

Removed pool_idle_timeout_seconds from traffic_shaping, instead use pool_idle_timeout with duration format.

traffic_shaping:
-  pool_idle_timeout_seconds: 30
+  pool_idle_timeout: 30s

Related documentation changes PR -> graphql-hive/console#7207

Created some helper functions to deduplicate the code to compile and resolve VRL expressions

@gemini-code-assist

This comment was marked as outdated.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a 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 new Expression type to centralize VRL expression handling, which is a great improvement for maintainability. The new type encapsulates VRL compilation and execution, removing duplicated logic from various parts of the executor. My review focuses on performance improvements for this new type, in line with the repository's style guide.

@github-actions
Copy link

github-actions bot commented Oct 31, 2025

k6-benchmark results

     ✓ response code was 200
     ✓ no graphql errors
     ✓ valid response structure

     █ setup

     checks.........................: 100.00% ✓ 201687      ✗ 0    
     data_received..................: 5.9 GB  196 MB/s
     data_sent......................: 79 MB   2.6 MB/s
     http_req_blocked...............: avg=3.57µs   min=701ns   med=1.86µs  max=11.52ms  p(90)=2.72µs  p(95)=3.13µs  
     http_req_connecting............: avg=659ns    min=0s      med=0s      max=1.8ms    p(90)=0s      p(95)=0s      
     http_req_duration..............: avg=21.81ms  min=2.2ms   med=20.86ms max=88.74ms  p(90)=29.81ms p(95)=33.27ms 
       { expected_response:true }...: avg=21.81ms  min=2.2ms   med=20.86ms max=88.74ms  p(90)=29.81ms p(95)=33.27ms 
     http_req_failed................: 0.00%   ✓ 0           ✗ 67249
     http_req_receiving.............: avg=132.89µs min=25.59µs med=41.19µs max=47.12ms  p(90)=86.31µs p(95)=391.18µs
     http_req_sending...............: avg=25.8µs   min=5.63µs  med=11.09µs max=28.17ms  p(90)=16.93µs p(95)=27.48µs 
     http_req_tls_handshaking.......: avg=0s       min=0s      med=0s      max=0s       p(90)=0s      p(95)=0s      
     http_req_waiting...............: avg=21.65ms  min=2.14ms  med=20.72ms max=88.64ms  p(90)=29.57ms p(95)=32.98ms 
     http_reqs......................: 67249   2236.224049/s
     iteration_duration.............: avg=22.3ms   min=5.43ms  med=21.21ms max=221.63ms p(90)=30.27ms p(95)=33.8ms  
     iterations.....................: 67229   2235.558991/s
     vus............................: 50      min=50        max=50 
     vus_max........................: 50      min=50        max=50 

@github-actions
Copy link

github-actions bot commented Oct 31, 2025

🐋 This PR was built and pushed to the following Docker images:

Image Names: ghcr.io/graphql-hive/router

Platforms: linux/amd64,linux/arm64

Image Tags: ghcr.io/graphql-hive/router:pr-540 ghcr.io/graphql-hive/router:sha-a7217d0

Docker metadata
{
"buildx.build.ref": "builder-0c044f54-0e2b-44ec-bfea-e82a686617ee/builder-0c044f54-0e2b-44ec-bfea-e82a686617ee0/y48xbq6zugd2ihjm2u5orc7zd",
"containerimage.descriptor": {
  "mediaType": "application/vnd.oci.image.index.v1+json",
  "digest": "sha256:bddf9468aac1eed8ed60e9aafe2961dd07f839d788b19fcc1f45309bcf028c53",
  "size": 1609
},
"containerimage.digest": "sha256:bddf9468aac1eed8ed60e9aafe2961dd07f839d788b19fcc1f45309bcf028c53",
"image.name": "ghcr.io/graphql-hive/router:pr-540,ghcr.io/graphql-hive/router:sha-a7217d0"
}

Comment on lines 22 to 23
static VRL_FUNCTIONS: Lazy<Vec<Box<dyn Function>>> = Lazy::new(vrl_build_functions);
static VRL_TIMEZONE: Lazy<VrlTimeZone> = Lazy::new(VrlTimeZone::default);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not do it this way, where we centralize the function building.
Yeah, right now we pass all functions that vrl provides, but if we ever want to add one made by us or filter some, for certain expressions, we would have to opt out and, we won't be able to do it without basically reverting your changes

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just didn't want to copy/paste the same logic again like we did 3 different places before. How do you think I should do that instead?

Copy link
Contributor

@kamilkisiela kamilkisiela Oct 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can create a function and reuse it. If we ever want to opt-out, we either slightly refactor it or duplicate.

if you move this logic to the config crate then we’re not able to do those small adjustments in future

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks better now?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's much better now. Thanks

If we ever want to use different set of function, we can now do:

compile_expression(expression, Some(fns))

and add pre-built functions anywhere in code, where it makes sense.

@ardatan ardatan changed the title New Expression type to handle VRL in one place Shared utilities to handle VRL expressions Oct 31, 2025
/// An expression that must evaluate to a boolean. If true, the label will be applied.
expression: String,
},
Expression { expression: String },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

original comment is missing

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reverted 👍

indexmap = "2.10.0"
bumpalo = "3.19.0"
once_cell = "1.21.3"
schemars = "1.0.4"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you sure we need schemars in executor?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed 👍

@ardatan ardatan force-pushed the primitive-expression branch from 2644a55 to 128b10a Compare November 3, 2025 10:40
@ardatan ardatan enabled auto-merge (squash) November 3, 2025 11:30
@ardatan ardatan force-pushed the primitive-expression branch from 128b10a to 766bd0e Compare November 3, 2025 11:30
@ardatan ardatan changed the title Shared utilities to handle VRL expressions Shared utilities to handle VRL expressions & some other improvements Nov 3, 2025
@dotansimha dotansimha force-pushed the main branch 4 times, most recently from 4cffb36 to c709bec Compare November 4, 2025 11:27
@github-actions
Copy link

github-actions bot commented Nov 5, 2025

🐋 This PR was built and pushed to the following Docker images:

Image Names: ghcr.io/graphql-hive/router

Platforms: linux/amd64,linux/arm64

Image Tags: ghcr.io/graphql-hive/router:pr-540 ghcr.io/graphql-hive/router:sha-a9357dd

Docker metadata
{
"buildx.build.ref": "builder-f7eef0ad-4ea7-46e8-ad04-461b9f8e33ef/builder-f7eef0ad-4ea7-46e8-ad04-461b9f8e33ef0/v2peugjk24d19970a7k2gnro3",
"containerimage.descriptor": {
  "mediaType": "application/vnd.oci.image.index.v1+json",
  "digest": "sha256:0c9322a3636b23031dc38dca705e9acdf0fc6e4fb7ea569863c0c2853506ef6f",
  "size": 1609
},
"containerimage.digest": "sha256:0c9322a3636b23031dc38dca705e9acdf0fc6e4fb7ea569863c0c2853506ef6f",
"image.name": "ghcr.io/graphql-hive/router:pr-540,ghcr.io/graphql-hive/router:sha-a9357dd"
}

@ardatan ardatan force-pushed the primitive-expression branch from 046cc07 to 8650809 Compare November 5, 2025 23:25
@ardatan ardatan merged commit 5dab3d3 into main Nov 21, 2025
20 checks passed
@ardatan ardatan deleted the primitive-expression branch November 21, 2025 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants