Add tower for use with http/graphql clients #2067

dotdat · 2024-08-21T23:33:18Z

This PR originally started by looking at how we might use clients (studio, graphql, http) that we could construct at the top of a command and then pass through, fully-formed to the place in which it is used.
However, during this discovery, I found a few requirements that needed to be addressed:

Any HTTP client created needs to respect the global --accept-invalid-hostnames and --accept-invalid-certs flags.
This is why we construct and re-use a reqwest::Client and reqwest::ClientBuilder in our config. When calling get_reqwest_client and get_reqwest_client_builder, this configuration is pre-filled.

HTTP/GraphQL clients aren't uniform throughout rover commands
There are a number of use cases that a client might perform:

download a binary
perform introspection
contact Studio
health-check the router
Each one of these use cases may attach their own headers, timeouts, or retry logic, which is why we tend to construct our clients at the last possible moment.

reqwest supports a few nice things out of the box, such as proxies
There is a hazard in swapping out reqwest for something like hyper, as we are not entirely aware whether rover users are using any of these hidden features.

Given that, there are some improvements we could be make to our client ecosystem that makes the construction of these clients against their specific use cases more transparent / testable / modifiable in the future.

If we look at the use cases laid out currently, we can see that our client construction and use case has a few knobs that generally get tuned:

retries, whether to do so and also max elapsed time to retry
timeouts
header injection

With that, a tool like tower allows us to perform these types of tasks in a more declarative and composable way than passing around a ClientBuilder or manipulating headers ad hoc.

Initially, reqwest looked incompatible with tower based on its API. However, tower-reqwest seems to fill in that gap, by providing a way to wrap a client as a Service and apply tower layers to the client.

In this PR, I centralize http requests around http types and bytes for a robust body type.

In addition, GraphQL and Studio clients can be reduced to tower layers that perform their original tasks.
One note here is that the GraphQL layer omits internal retries and instead relies on an HttpService that provides retries (or not) as all retry logic is currently based on HTTP reasoning.

loshz

Overall LGTM! Code is clean and very easy to follow. My only ask would be to add comments and tests to the rover-graphql and rover-http crates.

loshz · 2024-09-03T14:13:53Z

crates/rover-client/src/operations/graph/introspect/types.rs

@@ -2,6 +2,7 @@ use std::collections::HashMap;

 use crate::operations::graph::introspect::runner::graph_introspect_query;

+#[cfg(test)]


Is this supposed to be here?

Yep. This is a little weird, but this is used only in tests. I had removed the whole thing previously as it wasn't used in the released binary, but then it ended up breaking the tests.

loshz · 2024-09-03T14:15:45Z

crates/rover-client/src/operations/subgraph/introspect/types.rs

-pub struct SubgraphIntrospectInput {
-    pub headers: HashMap<String, String>,
-}
+pub struct SubgraphIntrospectInput {}


Could this just be a unit struct?

Suggested change

pub struct SubgraphIntrospectInput {}

pub struct SubgraphIntrospectInput;

This could be, but usually I defer to using an empty struct unless it's specifically a marker / empty by design. In this case, we're just not expecting anything as input, but could be expanded to require something later.
All said and done, they're exactly the same in practice it's just a personal preference here.

loshz · 2024-09-03T14:19:45Z

crates/rover-client/src/service/introspection.rs

+            .map(|result| match result {
+                Ok(data) => Q::map_response(data),
+                Err(err) => {
+                    if err.to_string().contains("Cannot query field") {


Is there any way to assert a concrete type here as opposed to a strcmp? Or do we not own this error?

This comes from the errors field in a GraphQL response, so the error is a little-bit free form and produced by the subgraph's server implementation. There is probably a way to be more specific about this since we kinda know that the error case exists, but I left it alone here mostly because I think it belongs to a separate type of PR to classify that type of error.

aaronArinder

in favor! approving to clear the way; though, tests look like they might be failing (not sure if it's ephemeral--haven't looked) and we might want to figure out whether this should be its own release or what makes sense for it to ship with

dotdat · 2024-09-09T15:24:11Z

I have removed the touchpoints for this with the existing rover crates (rover, rover-client, sputnik) as to provide the tooling outright, but to be more cautious about how it is implemented through the rover stack.

@glasser

## 🐛 Fixes - **Avoid misleading warning when `--output` is not specified - @glasser #2100** In the release of v0.26.1 logic was added to disable the output flag if the Federation version was less than 2.9, however this was being printed even when the `--output` flag was not supplied. This has been corrected. - **Improve `--graph-ref` option - @glasser #2101** In the release of v0.26.0 the `--graph-ref` option was added to `supergraph compose` as well as `rover dev`. However, the behaviour when `--graph-ref` was used in conjunction with `--config` did not work as documented. This is now fixed. Furthermore, both `rover dev` and `supergraph compose`, when using only the `--graph-ref` option, respect the graph ref's Federation version. - **Further improve `--graph-ref` option - @glasser #2105** Improves on the above by fixing some corner cases that prevented #2101 from working as intended ## 🛠 Maintenance - **Update `eslint` to v9.10.0 - @jonathanrainer #2106** - **Update `concurrently` to v9.0.0 - @jonathanrainer #2108** - **Update `manylinux` CI Docker Image to v2024.09.09 - @jonathanrainer #2110** - **Update Rust to v1.81.0 - @jonathanrainer #2107** - **Pass GitHub Tag to GitHub Actions Workflow - @glasser #2109** - **Add `tower` for use with HTTP/GraphQL clients - @dotdat #2067** ## 📚 Documentation - **Fix Glossary links - @Meschreiber @pnodet #2114**

@glasser

- **Avoid misleading warning when `--output` is not specified - @glasser In the release of v0.26.1 logic was added to disable the output flag if the Federation version was less than 2.9, however this was being printed even when the `--output` flag was not supplied. This has been corrected. - **Improve `--graph-ref` option - @glasser #2101** In the release of v0.26.0 the `--graph-ref` option was added to `supergraph compose` as well as `rover dev`. However, the behaviour when `--graph-ref` was used in conjunction with `--config` did not work as documented. This is now fixed. Furthermore, both `rover dev` and `supergraph compose`, when using only the `--graph-ref` option, respect the graph ref's Federation version. - **Further improve `--graph-ref` option - @glasser #2105** Improves on the above by fixing some corner cases that prevented #2101 from working as intended - **Update `eslint` to v9.10.0 - @jonathanrainer #2106** - **Update `concurrently` to v9.0.0 - @jonathanrainer #2108** - **Update `manylinux` CI Docker Image to v2024.09.09 - @jonathanrainer - **Update Rust to v1.81.0 - @jonathanrainer #2107** - **Pass GitHub Tag to GitHub Actions Workflow - @glasser #2109** - **Add `tower` for use with HTTP/GraphQL clients - @dotdat #2067** - **Fix Glossary links - @Meschreiber @pnodet #2114**

dotdat force-pushed the dotdat/http-client-refactor branch 2 times, most recently from 07d3389 to b7f87eb Compare August 29, 2024 15:41

dotdat marked this pull request as ready for review August 29, 2024 19:46

dotdat requested a review from a team as a code owner August 29, 2024 19:46

jonathanrainer added this to the v0.27.0 milestone Aug 30, 2024

dotdat force-pushed the dotdat/http-client-refactor branch from f4fd8c6 to 0f5de57 Compare September 3, 2024 14:10

loshz reviewed Sep 3, 2024

View reviewed changes

aaronArinder approved these changes Sep 3, 2024

View reviewed changes

aaronArinder force-pushed the dotdat/http-client-refactor branch from 0f5de57 to c61e7c6 Compare September 3, 2024 18:54

dotdat force-pushed the dotdat/http-client-refactor branch 3 times, most recently from 7ccc5c6 to 8b5d98c Compare September 5, 2024 17:33

dotdat changed the title ~~Use tower for http/graphql clients~~ Add tower for use with http/graphql clients Sep 5, 2024

dotdat force-pushed the dotdat/http-client-refactor branch from 5d7cefe to a12fe25 Compare September 6, 2024 00:35

dotdat added 16 commits September 9, 2024 10:55

Use tower for http/graphql clients

b0994a1

Use reqwest over hyper for rover-http

02dc027

Update

b546488

Update

738dd70

Update

4065fa8

Fix tests

b657208

Update

705c80f

Update

2939d94

Update

35f35d2

Add tests for rover-http

23ee5a5

Add tests for retry

b1ed843

Update retry

dfc6e64

Add tests for ExtendHeaders

93fe968

Add tests for rover-graphql

a899caa

Add tests for rover-studio

cdec76d

Add docs to rover-http

b990294

dotdat added 8 commits September 9, 2024 10:56

Add docs for rover-graphql

efc80ec

Add docs to rover-studio

8a71d36

Fix JSON error rename

570fb9a

Remove tower-reqwest

ad45f84

Update

9d33112

Revert mainline changes

c50e43c

Revert xtask

bfca298

Fix rover-graphql tests

e5445cc

dotdat force-pushed the dotdat/http-client-refactor branch from 8abe179 to e5445cc Compare September 9, 2024 14:59

Fix merge conflict

50d630e

dotdat enabled auto-merge (squash) September 9, 2024 15:24

dotdat merged commit e3bd09a into main Sep 9, 2024
22 checks passed

dotdat deleted the dotdat/http-client-refactor branch September 9, 2024 15:27

jonathanrainer modified the milestones: v0.27.0, v0.26.2 Sep 10, 2024

jonathanrainer added the maintenance 🛠️ label Sep 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tower for use with http/graphql clients #2067

Add tower for use with http/graphql clients #2067

dotdat commented Aug 21, 2024 •

edited

Loading

loshz left a comment

loshz Sep 3, 2024

dotdat Sep 3, 2024

loshz Sep 3, 2024

dotdat Sep 3, 2024

loshz Sep 3, 2024

dotdat Sep 3, 2024

aaronArinder left a comment

dotdat commented Sep 9, 2024

		@@ -2,6 +2,7 @@ use std::collections::HashMap;

		use crate::operations::graph::introspect::runner::graph_introspect_query;

		#[cfg(test)]

	pub struct SubgraphIntrospectInput {}
	pub struct SubgraphIntrospectInput;

Add tower for use with http/graphql clients #2067

Add tower for use with http/graphql clients #2067

Conversation

dotdat commented Aug 21, 2024 • edited Loading

loshz left a comment

Choose a reason for hiding this comment

loshz Sep 3, 2024

Choose a reason for hiding this comment

dotdat Sep 3, 2024

Choose a reason for hiding this comment

loshz Sep 3, 2024

Choose a reason for hiding this comment

dotdat Sep 3, 2024

Choose a reason for hiding this comment

loshz Sep 3, 2024

Choose a reason for hiding this comment

dotdat Sep 3, 2024

Choose a reason for hiding this comment

aaronArinder left a comment

Choose a reason for hiding this comment

dotdat commented Sep 9, 2024

dotdat commented Aug 21, 2024 •

edited

Loading