chore: Add integration tests for Open Telemetry #276
Conversation
o0Ignition0o
changed the title
|
Brace yourselves, it's macro time. |
apollo-router-core/src/query_planner/router_bridge_query_planner.rs
Outdated
Show resolved
Hide resolved
| [ | ||
| "otel.name", | ||
| { | ||
| "Debug": "POST /graphql" |
There was a problem hiding this comment.
why is there a Debug key? What does it mean in that context?
There was a problem hiding this comment.
https://docs.rs/reqwest-tracing/latest/src/reqwest_tracing/middleware.rs.html#23, not really sure tbh
There was a problem hiding this comment.
oook i see, it s the kind of visitor entry, eg: https://github.com/apollographql/router/pull/276/files/d74ca96e2159b05f8d431321deea3e5f44031ab7#diff-b6df8c2743d864c312f734e1e5a8377f49d9e6abe5b0ef9f27b94af6c962a168R91, it probably means I shouldn't gather the field.name()
|
missing a lot of docs and explanation, but the functionality is there |
…correctly sorted" This reverts commit 9aa0c52.
…e sure the dag walks the children the right way
…show the team about that it s super cool
| members = [ | ||
| "apollo-router", | ||
| "apollo-router-core", | ||
| "apollo-router-benchmarks", | ||
| "xtask", | ||
| ] |
There was a problem hiding this comment.
- 🧶 You keep changing that and someone else keep changing it back.
- 🪛 This file is unnecessarily touched in this PR.
| request, | ||
| schema, | ||
| )?; | ||
| let Variables { variables, paths } = query_span.in_scope(|| { |
There was a problem hiding this comment.
🧶 per our ADR on telemetry, we should prefer to put this at the source instead of the caller.
| if !response.is_primary() { | ||
| return Err(FetchError::SubrequestUnexpectedPatchResponse { | ||
| service: service_name.to_owned(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧶 I'm don't understand why this is under the query_span
| @@ -23,7 +23,7 @@ impl RouterBridgeQueryPlanner { | |||
|
|
|||
| #[async_trait] | |||
| impl QueryPlanner for RouterBridgeQueryPlanner { | |||
| #[tracing::instrument(name = "plan", level = "debug", skip_all)] | |||
| #[tracing::instrument(skip_all, name = "plan", level = "debug")] | |||
There was a problem hiding this comment.
🧶 File unnecessarily touch in this PR
If possible I would like to keep the commits on main focus on their actual feature. If you want to re-order all the skip_all everywhere, you can make it on a separate PR.
| @@ -43,7 +43,7 @@ macro_rules! assert_federated_response { | |||
| }; | |||
| } | |||
|
|
|||
| #[test(tokio::test)] | |||
| #[tokio::test] | |||
There was a problem hiding this comment.
❔ Why did you remove the logger initialization for these tests?
~Hey folks, wondering if you would be open to making the `build_errors` field on `BuildErrors` `pub` ? We're using harmonizer for some internal tooling and would like access to individual composition errors.~ Exposing `iter` instead to keep `build_errors` private.
) * feat: request and response mapping for inner/connector fetch notes This change inspects subgraph requests from the inner query plan and constructs a vec of requests for the SubgraphConnector to call. It handles: * root fields (including aliased fields) * entity resolvers * fields on entities (by basically providing a stub entity resolver) For each request, we collect the info necessary for mapping the response (JSON selection transforms and key path). The response handling function applies this information to construct a single response from multiple JSON objects. --------- Co-authored-by: o0Ignition0o <jeremy.lempereur@gmail.com>
will fix #38 eventually