Merge branch 'dev' into bryn/named_runtime_channel

.changesets/fix_junior_granddad_salami_dye.md

@@ -0,0 +1,7 @@
+### Fix missing Content-Length header in subgraph requests ([Issue #6503](https://github.com/apollographql/router/issues/6503))
+
+A change in `1.59.0` caused the Router to send requests to subgraphs without a `Content-Length` header, which would cause issues with some GraphQL servers that depend on that header.
+
+This solves the underlying bug and reintroduces the `Content-Length` header.
+
+By [@nmoutschen](https://github.com/nmoutschen) in https://github.com/apollographql/router/pull/6538

.changesets/fix_tninesling_demand_control_perf.md

@@ -0,0 +1,9 @@
+### Demand control lookup optimizations ([PR #6450](https://github.com/apollographql/router/pull/6450))
+
+Demand Control can reduce router throughput due to the extra processing required for scoring. This change shifts more data to be computed at plugin initialization and consolidates lookup queries.
+
+- Cost directives for arguments are now stored in a map alongside those for field definitions
+- All precomputed directives are bundled into a struct for each field, along with that field's extended schema type. This reduces 5 individual lookups to a single lookup.
+- Response scoring was looking up each field's definition twice. This is now reduced to a single lookup.
+
+By [@tninesling](https://github.com/tninesling) in https://github.com/apollographql/router/pull/6450

.changesets/maint_renee_migrate_metrics_values.md

@@ -1,5 +1,5 @@
-### Migrate gauge metrics to OTel instruments ([PR #6476](https://github.com/apollographql/router/pull/6476))
+### Migrate various metrics to OTel instruments ([PR #6476](https://github.com/apollographql/router/pull/6476), [PR #6356](https://github.com/apollographql/router/pull/6356), [PR #6539](https://github.com/apollographql/router/pull/6539))
 
-Updates gauge metrics using the legacy `tracing::info!(value.*)` syntax to OTel instruments. 
+Various metrics using our legacy mechanism based on the `tracing` crate are migrated to OTel instruments.
 
-By [@goto-bus-stop](https://github.com/goto-bus-stop) in https://github.com/apollographql/router/pull/6476
+By [@goto-bus-stop](https://github.com/goto-bus-stop) in https://github.com/apollographql/router/pull/6476, https://github.com/apollographql/router/pull/6356, https://github.com/apollographql/router/pull/6539

Cargo.lock

@@ -174,9 +174,9 @@ checksum = "b3d1d046238990b9cf5bcde22a3fb3584ee5cf65fb2765f454ed428c7a0063da"
 
 [[package]]
 name = "apollo-compiler"
-version = "1.0.0-beta.24"
+version = "1.25.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "71153ad85c85f7aa63f0e0a5868912c220bb48e4c764556f5841d37fc17b0103"
+checksum = "2d810db7b42773a4082426bcd58658cb9b311c5b74febd255130a9dcfc13109d"
 dependencies = [
  "ahash",
  "apollo-parser",
@@ -245,9 +245,9 @@ dependencies = [
 
 [[package]]
 name = "apollo-parser"
-version = "0.8.3"
+version = "0.8.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "b64257011a999f2e22275cf7a118f651e58dc9170e11b775d435de768fad0387"
+checksum = "c8f05cbc7da3c2e3bb2f86e985aad5f72571d2e2cd26faf8caa7782131576f84"
 dependencies = [
  "memchr",
  "rowan",
@@ -471,9 +471,9 @@ dependencies = [
 
 [[package]]
 name = "apollo-smith"
-version = "0.14.0"
+version = "0.15.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "d89479524886fdbe62b124d3825879778680e0147304d1a6d32164418f8089a2"
+checksum = "95a04f64d4d4f0048201967b8064257de212ab240fd74e2b153691b39b0e8721"
 dependencies = [
  "apollo-compiler",
  "apollo-parser",
@@ -500,9 +500,9 @@ checksum = "69f7f8c3906b62b754cd5326047894316021dcfe5a194c8ea52bdd94934a3457"
 
 [[package]]
 name = "ariadne"
-version = "0.4.1"
+version = "0.5.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "44055e597c674aef7cb903b2b9f6e4cba1277ed0d2d61dae7cd52d7ffa81f8e2"
+checksum = "31beedec3ce83ae6da3a79592b3d8d7afd146a5b15bb9bb940279aced60faa89"
 dependencies = [
  "concolor",
  "unicode-width",
@@ -4042,15 +4042,6 @@ version = "2.7.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "78ca9ab1a0babb1e7d5695e3530886289c18cf2f87ec19a575a0abdce112e3a3"
 
-[[package]]
-name = "memoffset"
-version = "0.9.1"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "488016bfae457b036d996092f6cb448677611ce4449e970ceaf42695203f218a"
-dependencies = [
- "autocfg",
-]
-
 [[package]]
 name = "memory-stats"
 version = "1.2.0"
@@ -5585,13 +5576,12 @@ dependencies = [
 
 [[package]]
 name = "rowan"
-version = "0.15.15"
+version = "0.16.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "32a58fa8a7ccff2aec4f39cc45bf5f985cec7125ab271cf681c279fd00192b49"
+checksum = "417a3a9f582e349834051b8a10c8d71ca88da4211e4093528e36b9845f6b5f21"
 dependencies = [
  "countme",
  "hashbrown 0.14.5",
- "memoffset",
  "rustc-hash",
  "text-size",
 ]

Cargo.toml

@@ -49,9 +49,9 @@ debug = 1
 # Dependencies used in more than one place are specified here in order to keep versions in sync:
 # https://doc.rust-lang.org/cargo/reference/workspaces.html#the-dependencies-table
 [workspace.dependencies]
-apollo-compiler = "=1.0.0-beta.24"
-apollo-parser = "0.8.3"
-apollo-smith = "0.14.0"
+apollo-compiler = "1.25.0"
+apollo-parser = "0.8.4"
+apollo-smith = "0.15.0"
 async-trait = "0.1.77"
 hex = { version = "0.4.3", features = ["serde"] }
 http = "0.2.11"

apollo-federation/src/operation/tests/mod.rs

@@ -1,5 +1,6 @@
 use apollo_compiler::collections::IndexSet;
 use apollo_compiler::name;
+use apollo_compiler::parser::Parser;
 use apollo_compiler::schema::Schema;
 use apollo_compiler::ExecutableDocument;
 
@@ -29,8 +30,9 @@ macro_rules! assert_normalized {
 pub(super) fn parse_schema_and_operation(
     schema_and_operation: &str,
 ) -> (ValidFederationSchema, ExecutableDocument) {
-    let (schema, executable_document) =
-        apollo_compiler::parse_mixed_validate(schema_and_operation, "document.graphql").unwrap();
+    let (schema, executable_document) = Parser::new()
+        .parse_mixed_validate(schema_and_operation, "document.graphql")
+        .unwrap();
     let executable_document = executable_document.into_inner();
     let schema = ValidFederationSchema::new(schema).unwrap();
     (schema, executable_document)

apollo-federation/src/query_graph/path_tree.rs

@@ -563,6 +563,7 @@ where
 mod tests {
     use std::sync::Arc;
 
+    use apollo_compiler::parser::Parser;
     use apollo_compiler::ExecutableDocument;
     use petgraph::stable_graph::NodeIndex;
     use petgraph::visit::EdgeRef;
@@ -585,9 +586,9 @@ mod tests {
     fn parse_schema_and_operation(
         schema_and_operation: &str,
     ) -> (ValidFederationSchema, ExecutableDocument) {
-        let (schema, executable_document) =
-            apollo_compiler::parse_mixed_validate(schema_and_operation, "document.graphql")
-                .unwrap();
+        let (schema, executable_document) = Parser::new()
+            .parse_mixed_validate(schema_and_operation, "document.graphql")
+            .unwrap();
         let executable_document = executable_document.into_inner();
         let schema = ValidFederationSchema::new(schema).unwrap();
         (schema, executable_document)

apollo-federation/src/subgraph/mod.rs

@@ -119,7 +119,7 @@ impl Subgraph {
                     .schema_definition
                     .make_mut()
                     .directives
-                    .push(defaults.applied_link_directive().into());
+                    .push(defaults.applied_link_directive());
                 defaults
             }
         };
@@ -136,7 +136,7 @@ impl Subgraph {
                     .schema_definition
                     .make_mut()
                     .directives
-                    .push(defaults.applied_link_directive().into());
+                    .push(defaults.applied_link_directive());
                 defaults
             }
         };

apollo-router/src/axum_factory/listeners.rs

@@ -224,6 +224,19 @@ pub(super) fn serve_router_on_listen_addr(
     all_connections_stopped_sender: mpsc::Sender<()>,
 ) -> (impl Future<Output = Listener>, oneshot::Sender<()>) {
     let (shutdown_sender, shutdown_receiver) = oneshot::channel::<()>();
+
+    let meter = meter_provider().meter("apollo/router");
+    let total_session_count_instrument = meter
+        .u64_observable_gauge("apollo_router_session_count_total")
+        .with_description("Number of currently connected clients")
+        .with_callback(move |gauge| {
+            gauge.observe(
+                TOTAL_SESSION_COUNT.load(Ordering::Relaxed),
+                &[KeyValue::new("listener", address.to_string())],
+            );
+        })
+        .init();
+
     // this server reproduces most of hyper::server::Server's behaviour
     // we select over the stop_listen_receiver channel and the listener's
     // accept future. If the channel received something or the sender
@@ -232,18 +245,6 @@ pub(super) fn serve_router_on_listen_addr(
     let server = async move {
         tokio::pin!(shutdown_receiver);
 
-        let _total_session_count_instrument = meter_provider()
-            .meter("apollo/router")
-            .u64_observable_gauge("apollo_router_session_count_total")
-            .with_description("Number of currently connected clients")
-            .with_callback(move |gauge| {
-                gauge.observe(
-                    TOTAL_SESSION_COUNT.load(Ordering::Relaxed),
-                    &[KeyValue::new("listener", address.to_string())],
-                );
-            })
-            .init();
-
         let connection_shutdown = Arc::new(Notify::new());
 
         loop {
@@ -263,13 +264,20 @@ pub(super) fn serve_router_on_listen_addr(
                                 MAX_FILE_HANDLES_WARN.store(false, Ordering::SeqCst);
                             }
 
-                            // We only want to recognise sessions if we are the main graphql port.
-                            let _guard = main_graphql_port.then(TotalSessionCountGuard::start);
+                            // The session count instrument must be kept alive as long as any
+                            // request is in flight. So its lifetime is not related to the server
+                            // itself. The simplest way to do this is to hold onto a reference for
+                            // the duration of every request.
+                            let session_count_instrument = total_session_count_instrument.clone();
+                            // We only want to count sessions if we are the main graphql port.
+                            let session_count_guard = main_graphql_port.then(TotalSessionCountGuard::start);
 
                             let mut http_config = http_config.clone();
                             tokio::task::spawn(async move {
                                 // this sender must be moved into the session to track that it is still running
                                 let _connection_stop_signal = connection_stop_signal;
+                                let _session_count_instrument = session_count_instrument;
+                                let _session_count_guard = session_count_guard;
 
                                 match res {
                                     NetworkStream::Tcp(stream) => {

apollo-router/src/axum_factory/tests.rs

@@ -61,6 +61,7 @@ use crate::graphql;
 use crate::http_server_factory::HttpServerFactory;
 use crate::http_server_factory::HttpServerHandle;
 use crate::json_ext::Path;
+use crate::metrics::FutureMetricsExt as _;
 use crate::plugin::test::MockSubgraph;
 use crate::query_planner::QueryPlannerService;
 use crate::router_factory::create_plugins;
@@ -2351,3 +2352,95 @@ async fn test_supergraph_timeout() {
         })
     );
 }
+
+/// There are two session count gauges:
+/// - apollo_router_session_count_total, the number of open client connections
+/// - apollo_router_session_count_active, the number of in-flight HTTP requests
+///
+/// To test them, we use two hyper clients. Each client has its own connection pool, so by manually
+/// sending requests and completing responses, and creating and dropping clients, we can control
+/// the amount of open connections and the amount of in-flight requests.
+///
+/// XXX(@goto-bus-stop): this only tests the `session_count_total` metric right now. The
+/// `session_count_active` metric is reported from inside an axum router, so its lifetime is
+/// actually a little shorter than the full request/response cycle, in a way that is not easy to
+/// test from the outside. To test it we could use a custom inner service (passed to
+/// `init_with_config`) where we can control the progress the inner service makes.
+/// For now, I've tested the `session_count_active` metric manually and confirmed its value makes
+/// sense...
+#[tokio::test]
+async fn it_reports_session_count_metric() {
+    let configuration = Configuration::fake_builder().build().unwrap();
+
+    async {
+        let (server, _client) = init_with_config(
+            router::service::empty().await,
+            Arc::new(configuration),
+            MultiMap::new(),
+        )
+        .await
+        .unwrap();
+
+        let url = server
+            .graphql_listen_address()
+            .as_ref()
+            .unwrap()
+            .to_string();
+
+        let make_request = || {
+            http::Request::builder()
+                .uri(&url)
+                .body(hyper::Body::from(r#"{ "query": "{ me }" }"#))
+                .unwrap()
+        };
+
+        let client = hyper::Client::new();
+        // Create a second client that does not reuse the same connection pool.
+        let second_client = hyper::Client::new();
+
+        let first_response = client.request(make_request()).await.unwrap();
+
+        assert_gauge!(
+            "apollo_router_session_count_total",
+            1,
+            "listener" = url.clone()
+        );
+
+        let second_response = second_client.request(make_request()).await.unwrap();
+
+        // Both requests are in-flight
+        assert_gauge!(
+            "apollo_router_session_count_total",
+            2,
+            "listener" = url.clone()
+        );
+
+        _ = hyper::body::to_bytes(first_response.into_body()).await;
+
+        // Connection is still open in the pool even though the request is complete.
+        assert_gauge!(
+            "apollo_router_session_count_total",
+            2,
+            "listener" = url.clone()
+        );
+
+        _ = hyper::body::to_bytes(second_response.into_body()).await;
+
+        drop(client);
+        drop(second_client);
+
+        // XXX(@goto-bus-stop): Not ideal, but we would probably have to drop down to very
+        // low-level hyper primitives to control the shutdown of connections to the required
+        // extent. 100ms is a long time so I hope it's not flaky.
+        tokio::time::sleep(Duration::from_millis(100)).await;
+
+        // All connections are closed
+        assert_gauge!(
+            "apollo_router_session_count_total",
+            0,
+            "listener" = url.clone()
+        );
+    }
+    .with_metrics()
+    .await;
+}

apollo-router/src/error.rs

@@ -343,7 +343,7 @@ impl IntoGraphQLErrors for FederationErrorBridge {
     }
 }
 
-impl IntoGraphQLErrors for Vec<apollo_compiler::execution::GraphQLError> {
+impl IntoGraphQLErrors for Vec<apollo_compiler::response::GraphQLError> {
     fn into_graphql_errors(self) -> Result<Vec<Error>, Self> {
         Ok(self
             .into_iter()
@@ -550,7 +550,7 @@ impl IntoGraphQLErrors for ParseErrors {
 /// Collection of schema validation errors.
 #[derive(Debug, Clone, Serialize, Deserialize)]
 pub(crate) struct ValidationErrors {
-    pub(crate) errors: Vec<apollo_compiler::execution::GraphQLError>,
+    pub(crate) errors: Vec<apollo_compiler::response::GraphQLError>,
 }
 
 impl ValidationErrors {

apollo-router/src/graphql/mod.rs

@@ -7,8 +7,8 @@ mod visitor;
 use std::fmt;
 use std::pin::Pin;
 
-use apollo_compiler::execution::GraphQLError as CompilerExecutionError;
-use apollo_compiler::execution::ResponseDataPathElement;
+use apollo_compiler::response::GraphQLError as CompilerExecutionError;
+use apollo_compiler::response::ResponseDataPathSegment;
 use futures::Stream;
 use heck::ToShoutySnakeCase;
 pub use request::Request;
@@ -233,10 +233,10 @@ impl From<CompilerExecutionError> for Error {
             let elements = path
                 .into_iter()
                 .map(|element| match element {
-                    ResponseDataPathElement::Field(name) => {
+                    ResponseDataPathSegment::Field(name) => {
                         JsonPathElement::Key(name.as_str().to_owned(), None)
                     }
-                    ResponseDataPathElement::ListIndex(i) => JsonPathElement::Index(i),
+                    ResponseDataPathSegment::ListIndex(i) => JsonPathElement::Index(i),
                 })
                 .collect();
             Some(Path(elements))