Skip to content
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

provide a rhai interface to the router service #3234

Merged
merged 29 commits into from
Sep 5, 2023

Conversation

garypen
Copy link
Contributor

@garypen garypen commented Jun 7, 2023

Adds Rhai support for the router_service.

It is now possible to interact with requests and responses at the router_service level from Rhai. The functionality is very similar to that provided for interacting with existing services, for example supergraph_service. For instance, you may map requests and responses as follows:

fn router_service(service) {
    const request_callback = Fn("process_request");
    service.map_request(request_callback);
    const response_callback = Fn("process_response");
    service.map_response(response_callback);
}

The main difference from existing services is that the router_service is dealing with HTTP Bodies, not well formatted GraphQL objects. This means that the Request.body or Response.body is not a well structured object that you may interact with, but is simply a String.

This makes it more complex to deal with Request and Response bodies with the tradeoff being that a script author has more power and can perform tasks which are just not possible within the confines of a well-formed GraphQL object.

This simple example, simply logs the bodies:

// Generate a log for each request at this stage
fn process_request(request) {
    print(`body: ${request.body}`);
}

// Generate a log for each response at this stage
fn process_response(response) {
    print(`body: ${response.body}`);
}

Fixes #2278

Checklist

Complete the checklist (and note appropriate exceptions) before a final PR is raised.

  • Changes are compatible[^1]
  • Documentation[^2] completed
  • Performance impact assessed and acceptable
  • Tests added and passing[^3]
    • Unit Tests
    • Integration Tests
    • Manual Tests

Exceptions

Note any exceptions here

Notes

[^1]. It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this.
[^2]. Configuration is an important part of many changes. Where applicable please try to document configuration examples.
[^3]. Tick whichever testing boxes are applicable. If you are adding Manual Tests:
- please document the manual testing (extensively) in the Exceptions.
- please raise a separate issue to automate the test and label it (or ask for it to be labeled) as manual test

garypen added 2 commits June 6, 2023 15:57
There's a lot of very nasty unfinished code here, but some progress at
least.
Make responses work
Figure out how to handle errors in deferred responses
Add json_parsing functions to existing json module
@garypen garypen self-assigned this Jun 7, 2023
@github-actions

This comment has been minimized.

@router-perf
Copy link

router-perf bot commented Jun 7, 2023

CI performance tests

  • const - Basic stress test that runs with a constant number of users
  • events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
  • no-graphos - Basic stress test, no GraphOS.
  • step - Basic stress test that steps up the number of users over time
  • reload - Reload test over a long period of time at a constant rate of users
  • xlarge-request - Stress test with 10 MB request payload
  • events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity
  • large-request - Stress test with a 1 MB request payload
  • events - Stress test for events with a lot of users and deduplication ENABLED
  • xxlarge-request - Stress test with 100 MB request payload

garypen added 7 commits June 7, 2023 15:50
These are minimal documentation changes to at least make it clear that
it is possible to use `router_service` from rhai.

We need much more documentation:
 - useful example (any suggestions?)
 - clearly document that router.request.body != supergraqph.request.body
 - clearly document other potential changes at router_service level...
@garypen garypen requested review from a team, Geal, BrynCooke and bnjjj June 15, 2023 11:06
@garypen garypen marked this pull request as ready for review June 15, 2023 11:07
@garypen garypen requested a review from StephenBarlow as a code owner June 15, 2023 11:07
@garypen garypen requested review from o0Ignition0o and removed request for BrynCooke June 15, 2023 11:07
garypen added 2 commits June 15, 2023 14:02
Request and Response objects are different for router_service, so
provide enough minimal documentation so that it's clear what is going
on.
Geal
Geal previously requested changes Jun 20, 2023
apollo-router/src/plugins/rhai/engine.rs Outdated Show resolved Hide resolved
apollo-router/src/plugins/rhai/mod.rs Outdated Show resolved Hide resolved
apollo-router/src/plugins/rhai/engine.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@garypen garypen left a comment

Choose a reason for hiding this comment

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

Look good. One comment which I think we should change to improve usability for rhai script authors. I think fixing up the macros should be a follow up.

apollo-router/src/plugins/rhai/engine.rs Show resolved Hide resolved
@abernix abernix removed the request for review from StephenBarlow August 14, 2023 12:39
@Geal Geal self-requested a review August 14, 2023 13:08
@Geal
Copy link
Contributor

Geal commented Aug 14, 2023

something that came up while working on #3569: I think we should work on the stream of Result<Bytes, hyper::Error> instead of only the Bytes. We would be able to align all the implementations on the response side, and the rhai script would get info on network errors

@o0Ignition0o
Copy link
Contributor

I think we should work on the stream of Result<Bytes, hyper::Error> instead of only the Bytes. We would be able to align all the implementations on the response side, and the rhai script would get info on network errors

Not sure what you mean, wouldn't those happen before the router_service?

Copy link
Contributor

@o0Ignition0o o0Ignition0o left a comment

Choose a reason for hiding this comment

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

Looks good overall, but we need a test with a defered query or a subscription to make sure the rhai APIs work as expected.

I don't see any multipart separator function in the rhai helpers so I doubt json_decoding will be used correctly in the router_response rhai service.

Not sure if this should be within the scope of this PR or done as a followup though.

apollo-router/src/plugins/rhai/engine.rs Show resolved Hide resolved
apollo-router/src/plugins/rhai/mod.rs Outdated Show resolved Hide resolved
apollo-router/src/plugins/rhai/mod.rs Show resolved Hide resolved
@Geal
Copy link
Contributor

Geal commented Aug 22, 2023

following our discussion, I removed access to the body for requests and responses in 84a3e02 and opened an issue about it in #3642

The code is only commented out, let me know if I should remove it entirely (tbh I'd be slightly annoyed at having to dig it up from git or rewriting it)

@Geal Geal dismissed their stale review August 22, 2023 13:18

fixed

@Geal Geal requested a review from o0Ignition0o August 22, 2023 13:18
@Geal Geal dismissed o0Ignition0o’s stale review August 29, 2023 10:13

adressed comments in 84a3e02

@Geal Geal requested a review from BrynCooke August 30, 2023 14:11
@Geal Geal mentioned this pull request Sep 1, 2023
6 tasks
Copy link
Contributor

@BrynCooke BrynCooke left a comment

Choose a reason for hiding this comment

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

Approved with some nits

.changesets/feat_garypen_2278_rhai_router_service.md Outdated Show resolved Hide resolved
.changesets/feat_garypen_2278_rhai_router_service.md Outdated Show resolved Hide resolved
.changesets/feat_garypen_2278_rhai_router_service.md Outdated Show resolved Hide resolved
ServiceStep::Router(shared_service.clone()),
self.block.load().scope.clone(),
) {
tracing::error!("service callback failed: {error}");
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this give a good error message? If not then maybe tell the user what they should do to to fix their code.

Copy link
Contributor

Choose a reason for hiding this comment

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

that is the same message we use on the supergraph, execution and subgraph levels. It includes the error reported by rhai (like syntax errors, etc)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Geal this hasn't been addressed, do we have a followup issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

this does not need to be addressed in this issue, as it does the same thing as other rhai services. We can open an issue to look in more detail at rhai error handling though

@Geal Geal enabled auto-merge (squash) September 5, 2023 10:23
@Geal Geal merged commit 8e86ef4 into dev Sep 5, 2023
@Geal Geal deleted the garypen/2278-rhai-router-service branch September 5, 2023 10:34
garypen added a commit that referenced this pull request Sep 12, 2023
Adds `Rhai` support for the `router_service`.

It is now possible to interact with requests and responses at the
`router_service` level from `Rhai`. The functionality is very similar to
that provided for interacting with existing services, for example
`supergraph_service`. For instance, you may map requests and responses
as follows:

```
fn router_service(service) {
    const request_callback = Fn("process_request");
    service.map_request(request_callback);
    const response_callback = Fn("process_response");
    service.map_response(response_callback);
}

```
The main difference from existing services is that the router_service is
dealing with HTTP Bodies, not well formatted GraphQL objects. This means
that the `Request.body` or `Response.body` is not a well structured
object that you may interact with, but is simply a String.

This makes it more complex to deal with Request and Response bodies with
the tradeoff being that a script author has more power and can perform
tasks which are just not possible within the confines of a well-formed
GraphQL object.

This simple example, simply logs the bodies:

```
// Generate a log for each request at this stage
fn process_request(request) {
    print(`body: ${request.body}`);
}

// Generate a log for each response at this stage
fn process_response(response) {
    print(`body: ${response.body}`);
}
```

Fixes #2278 

---------

Co-authored-by: Geoffroy Couprie <[email protected]>
Co-authored-by: Bryn Cooke <[email protected]>
@abernix abernix mentioned this pull request Sep 14, 2023
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.

provide a rhai interface to the router service
5 participants