-
Notifications
You must be signed in to change notification settings - Fork 2k
enhancement(http_client source): allow VRL in query parameters #22706
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
enhancement(http_client source): allow VRL in query parameters #22706
Conversation
cb79433 to
834d166
Compare
ff535bb to
4407fff
Compare
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.
Copilot reviewed 5 out of 6 changed files in this pull request and generated no comments.
Files not reviewed (1)
- website/cue/reference/components/sources/base/http_client.cue: Language not supported
Comments suppressed due to low confidence (3)
src/sources/http_client/client.rs:398
- [nitpick] Consider renaming 'process_query_value' to 'evaluate_query_parameter' to more clearly indicate that the function evaluates dynamic VRL expressions.
fn process_query_value(value: &QueryParameterValue) -> QueryParameterValue {
src/sources/http_client/client.rs:241
- If VRL expression compilation fails, the warning is logged and the parameter is silently returned unevaluated. Consider whether it is preferable to fallback to the original input or signal a compilation failure to prevent unexpected behavior.
if let Ok(value) = Runtime::default().resolve(&mut target, &program, &timezone) {
src/sources/http_client/tests.rs:231
- Consider adding a test case for scenarios where VRL expression compilation fails to ensure the system gracefully handles such errors.
/// VRL query parameters should be parsed correctly
d07ca7f to
83fbf80
Compare
c402043 to
e753fac
Compare
e753fac to
637d427
Compare
pront
left a comment
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.
A few small comments. But LGTM!
src/http.rs
Outdated
| rename = "type" | ||
| )] | ||
| param_type: ParamType, |
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.
| rename = "type" | |
| )] | |
| param_type: ParamType, | |
| )] | |
| r#type: ParamType, |
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.
Changes made in 37952e8
|
Hm I see that the CI is failing here: but I'm not sure in this case if it can actually be a const function |
Could be a false positive. You can add an allow block to exclude it from this check. |
|
Thanks, this is a useful feature! |
Summary
This PR adds support for VRL in query parameters within the http client source.
This is useful for making GET requests with dynamic parameters e.g. a
start_timeparameter which can help to filter extraneous data (e.g. only get "active" events based on the current timestamp, or events from the last hour)e.g.
Which will resolve to
https://endpoint.com?timestamp=1749291799121Closes #14621, #22988
Change Type
Is this a breaking change?
How did you test this PR?
Tests have been added, but otherwise we've been running this in a Kubernetes environment with Helm.
Does this PR include user facing changes?
Checklist
make check-allis a good command to run locally. This check isdefined here. Some of these
checks might not be relevant to your PR. For Rust changes, at the very least you should run:
cargo fmt --allcargo clippy --workspace --all-targets -- -D warningscargo nextest run --workspace(alternatively, you can runcargo test --all)References