Coprocessor hook for connectors request/response stages#8869
Coprocessor hook for connectors request/response stages#8869andrewmcgivery merged 19 commits intodevfrom
Conversation
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 2 changed, 0 removedBuild ID: 9b4ba4156a72ca4a0aeab67b URL: https://www.apollographql.com/docs/deploy-preview/9b4ba4156a72ca4a0aeab67b |
This comment has been minimized.
This comment has been minimized.
… id passing to response, proper body modification in response
| tracing::debug!(?payload, "externalized output"); | ||
| let start = Instant::now(); | ||
| let co_processor_result = payload.call(http_client, &coprocessor_url).await; | ||
| *executed = true; |
There was a problem hiding this comment.
I'd have to trace through what this is used for. If the request is aborted do we record anything?
There was a problem hiding this comment.
Not sure, this is the same logic that exists on the other lifecycle stages so I replicated what was already there. 😅
There was a problem hiding this comment.
worth a comment on what executed is for so that we don't accidentally remove it; here's what's in coprocessor/supergraph.rs:
// Indicate the stage was executed to raise execution metric on parent
the parent bit is important for future developers
There was a problem hiding this comment.
oh, in reply to @BrynCooke: if the request errors out we emit an error; if the request executes (ie, that bool above stays true), we record a metric (whether it succeeded or failed), but I don't see a metric emitted specifically for coprocessor request failures (probably because they get the same visibility normal request errors get? not sure)
|
I did some duplicate work here but this works for both subgraphs and connectors as well as the top level router http hook: #8874 |
| register_plugin!( | ||
| register_private_plugin!( |
There was a problem hiding this comment.
what's behind teh change to a private plugin?
There was a problem hiding this comment.
The connector tower layers are currently only hooked up to the private plugin interface, not the public plugin interface.
My assumption (though I 100% could be wrong here) is that coprocessors are a "private"/internal plugin anyways so this shouldn't have any impact... but again, could be wrong there.
There is some ask from customers to move the connector tower layers to the public interface but that would be a larger undertaking so if possible, I'd prefer to do that in a separate PR.
| impl Plugin for CoprocessorPlugin<HTTPClientService> { | ||
| impl PluginPrivate for CoprocessorPlugin<HTTPClientService> { |
There was a problem hiding this comment.
why the switch to private?
| coprocessor: | ||
| url: http://localhost:3007 | ||
| connector: | ||
| all: |
There was a problem hiding this comment.
not sure I see why we'd want an all layer between the req/res? eg, SupergraphStage just has req/res without the all layer--any reason why we shouldn't keep that same structure?
There was a problem hiding this comment.
looking at the integration tests, it looks like this might be a way to have connector-specific overrides? noticing structural similarities to how subgraph is handled
There was a problem hiding this comment.
You should be comparing this entire PR to subgraph, not supergraph. The connectors_request layer is equivalent to the subgraph layer. If you look at subgraph coprocessors, they currently have all and I believe the idea was to eventually allow for per-subgraph configuration, but that was never implemented.
But yes, the idea here is that we're leaving open the ability to override for specific connector sources, but currently are not implementing that in this PR. (all connectors config has this pattern of all and the ability to override per source).
| #[serde(default)] | ||
| pub(super) struct ConnectorStages { | ||
| #[serde(default)] | ||
| pub(super) all: ConnectorStage, |
There was a problem hiding this comment.
code reference for the all layer mentioned in the changeset comment; this could reflect the SupergraphStage struct's fields to keep the same structure, unless there's a reason it should be different
There was a problem hiding this comment.
Per above, you should be comparing this entire PR to subgraph, not supergraph. The connectors_request layer is equivalent to the subgraph layer.
| /// What information is passed to a connector request stage | ||
| #[derive(Clone, Debug, Default, Deserialize, PartialEq, JsonSchema)] | ||
| #[serde(default, deny_unknown_fields)] | ||
| pub(super) struct ConnectorRequestConf { |
There was a problem hiding this comment.
no sdl? what's the reason for excluding it? cf. the supergraph coprocessor's req/res handling
There was a problem hiding this comment.
Per above, you should be comparing this entire PR to subgraph, not supergraph. The connectors_request layer is equivalent to the subgraph layer.
| // unwrap is safe here because validate_coprocessor_output made sure control is available | ||
| let control = co_processor_output.control.expect("validated above; qed"); |
There was a problem hiding this comment.
I know we do this elsewhere, but we shouldn't do it here; the "validation above" might change in the future without someone meaning to make this panic, but we need to guard against future ourselves and emit an error here
There was a problem hiding this comment.
So this is a direct copy/paste of what the other coprocessor layers are already doing, including subgraph.
Given this is meant to be as close to subgraph as possible, unless we're also refactoring the other stages (which I do not want to take ownership for in this PR), I'd prefer to go with being consistent with that the other stages are doing here.
There was a problem hiding this comment.
(Though I will say in principle I agree 100% with not using unwrap! 😅)
| mapped_response: MappedResponse::Error { | ||
| error: runtime_error, | ||
| key: request.key, | ||
| problems: Vec::new(), |
There was a problem hiding this comment.
not totally sure I know what problems there would be in this case, but would problems ever lead to a break?
There was a problem hiding this comment.
This is a connectors structure... problems are "mapping problems" with our connectors mapping language that get exposed to the mapping playground or the debugging panel on the embedded sandbox.
In this case, there is no mapping happening, so no problems to report!
| return Ok(ControlFlow::Break(res)); | ||
| } | ||
|
|
||
| // Continue flow - apply modifications |
There was a problem hiding this comment.
what are the modifications? this section seemed a bit mysterious to me; it looks similar to how we do coprocessor/supergraph.rs, but with some extra flavor; might be nice to flesh out this comment to give the reader more direction in what to expect
| // Tests for context key deletion functionality | ||
| } | ||
|
|
||
| fn create_test_connector() -> Arc<Connector> { |
There was a problem hiding this comment.
can we put the connector tests in a mod block or something to make it more readable?
|
|
||
| Ok(()) | ||
| } | ||
|
|
There was a problem hiding this comment.
might be good to have a test showing that a proper graphql error comes back to the client if the connectors coprocessor fails
aaronArinder
left a comment
There was a problem hiding this comment.
panic still scares me, but alas; lgtm!
pragl
left a comment
There was a problem hiding this comment.
Docs LGTM! I made a handful of suggestions for your consideration based on our style guide, but none of them are substantive, so I'm going ahead and approving.
Co-authored-by: Parker <parker.ragland@apollographql.com>
abernix
left a comment
There was a problem hiding this comment.
Post-merge: Can tick the checklist on the PR description?
You can now configure a coprocessor hook for the
ConnectorRequestandConnectorResponsestages of the Router lifecycle.Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
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. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
A lot of (if not most) features benefit from built-in observability and
debug-level logs. Please read this guidance on metrics best-practices. ↩Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩