diff --git a/.changesets/fix_bnjjj_fix_880.md b/.changesets/fix_bnjjj_fix_880.md new file mode 100644 index 0000000000..3c9f0edc78 --- /dev/null +++ b/.changesets/fix_bnjjj_fix_880.md @@ -0,0 +1,53 @@ +### Fix telemetry instrumentation using supergraph query selector ([PR #6324](https://github.com/apollographql/router/pull/6324)) + +Query selector was raising error logs like `this is a bug and should not happen`. It's now fixed. +Now this configuration will work properly: +```yaml title=router.yaml +telemetry: + exporters: + metrics: + common: + views: + # Define a custom view because operation limits are different than the default latency-oriented view of OpenTelemetry + - name: oplimits.* + aggregation: + histogram: + buckets: + - 0 + - 5 + - 10 + - 25 + - 50 + - 100 + - 500 + - 1000 + instrumentation: + instruments: + supergraph: + oplimits.aliases: + value: + query: aliases + type: histogram + unit: number + description: "Aliases for an operation" + oplimits.depth: + value: + query: depth + type: histogram + unit: number + description: "Depth for an operation" + oplimits.height: + value: + query: height + type: histogram + unit: number + description: "Height for an operation" + oplimits.root_fields: + value: + query: root_fields + type: histogram + unit: number + description: "Root fields for an operation" +``` + +By [@bnjjj](https://github.com/bnjjj) in https://github.com/apollographql/router/pull/6324 \ No newline at end of file diff --git a/.changesets/fix_renee_max_evaluated_plans_rs.md b/.changesets/fix_renee_max_evaluated_plans_rs.md new file mode 100644 index 0000000000..67f7b8d0f6 --- /dev/null +++ b/.changesets/fix_renee_max_evaluated_plans_rs.md @@ -0,0 +1,7 @@ +### fix: propagate evaluated plans limit and paths limit to the native query planner ([PR #6316](https://github.com/apollographql/router/pull/6316)) + +Two experimental query planning complexity limiting options now work with the native query planner: +- `supergraph.query_planning.experimental_plans_limit` +- `supergraph.query_planning.experimental_paths_limit` + +By [@goto-bus-stop](https://github.com/goto-bus-stop) in https://github.com/apollographql/router/pull/6316 \ No newline at end of file diff --git a/.changesets/helm_host_configuration.md b/.changesets/helm_host_configuration.md deleted file mode 100644 index e26bdc6dd1..0000000000 --- a/.changesets/helm_host_configuration.md +++ /dev/null @@ -1,5 +0,0 @@ -### Allow configuring host via Helm template for virtual service ([PR #5545](https://github.com/apollographql/router/pull/5795)) - -When deploying via Helm, you can now configure hosts in `virtualservice.yaml` as a single host or a range of hosts. This is helpful when different hosts could be used within a cluster. - -By [@nicksephora](https://github.com/nicksephora) in https://github.com/apollographql/router/pull/5545 diff --git a/.circleci/config.yml b/.circleci/config.yml index 126c85a25d..50d08f1d50 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -1,6 +1,6 @@ version: 2.1 -# Cache key bump: 2 +# Cache key bump: 3 # These "CircleCI Orbs" are reusable bits of configuration that can be shared # across projects. See https://circleci.com/orbs/ for more information. diff --git a/CHANGELOG.md b/CHANGELOG.md index bd46bf9f90..679f5dfdd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -133,6 +133,12 @@ experimental_query_planner_mode: new By [@clenfest](https://github.com/clenfest), [@TylerBloom](https://github.com/TylerBloom) in https://github.com/apollographql/router/pull/6310 +### Allow configuring host via Helm template for virtual service ([PR #5545](https://github.com/apollographql/router/pull/5795)) + +When deploying via Helm, you can now configure hosts in `virtualservice.yaml` as a single host or a range of hosts. This is helpful when different hosts could be used within a cluster. + +By [@nicksephora](https://github.com/nicksephora) in https://github.com/apollographql/router/pull/5545 + ## 🐛 Fixes ### Remove noisy demand control logs ([PR #6192](https://github.com/apollographql/router/pull/6192)) diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index 2df5ad8c62..50371fd5dd 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -5,6 +5,7 @@ use std::io::BufReader; use std::iter; use std::net::IpAddr; use std::net::SocketAddr; +use std::num::NonZeroU32; use std::num::NonZeroUsize; use std::str::FromStr; use std::sync::Arc; @@ -416,16 +417,31 @@ impl Configuration { pub(crate) fn rust_query_planner_config( &self, ) -> apollo_federation::query_plan::query_planner::QueryPlannerConfig { - apollo_federation::query_plan::query_planner::QueryPlannerConfig { + use apollo_federation::query_plan::query_planner::QueryPlanIncrementalDeliveryConfig; + use apollo_federation::query_plan::query_planner::QueryPlannerConfig; + use apollo_federation::query_plan::query_planner::QueryPlannerDebugConfig; + + let max_evaluated_plans = self + .supergraph + .query_planning + .experimental_plans_limit + // Fails if experimental_plans_limit is zero; use our default. + .and_then(NonZeroU32::new) + .unwrap_or(NonZeroU32::new(10_000).expect("it is not zero")); + + QueryPlannerConfig { reuse_query_fragments: self.supergraph.reuse_query_fragments.unwrap_or(true), subgraph_graphql_validation: false, generate_query_fragments: self.supergraph.generate_query_fragments, - incremental_delivery: - apollo_federation::query_plan::query_planner::QueryPlanIncrementalDeliveryConfig { - enable_defer: self.supergraph.defer_support, - }, + incremental_delivery: QueryPlanIncrementalDeliveryConfig { + enable_defer: self.supergraph.defer_support, + }, type_conditioned_fetching: self.experimental_type_conditioned_fetching, - debug: Default::default(), + debug: QueryPlannerDebugConfig { + bypass_planner_for_single_subgraph: false, + max_evaluated_plans, + paths_limit: self.supergraph.query_planning.experimental_paths_limit, + }, } } } diff --git a/apollo-router/src/plugins/telemetry/config_new/instruments.rs b/apollo-router/src/plugins/telemetry/config_new/instruments.rs index 341f84ad35..88760f1f1c 100644 --- a/apollo-router/src/plugins/telemetry/config_new/instruments.rs +++ b/apollo-router/src/plugins/telemetry/config_new/instruments.rs @@ -1446,7 +1446,7 @@ where }) } None => { - ::tracing::error!("cannot convert static instrument into a counter, this is an error; please fill an issue on GitHub"); + failfast_debug!("cannot convert static instrument into a counter, this is an error; please fill an issue on GitHub"); } } } @@ -1501,7 +1501,7 @@ where }); } None => { - ::tracing::error!("cannot convert static instrument into a histogram, this is an error; please fill an issue on GitHub"); + failfast_debug!("cannot convert static instrument into a histogram, this is an error; please fill an issue on GitHub"); } } } diff --git a/apollo-router/src/plugins/telemetry/config_new/selectors.rs b/apollo-router/src/plugins/telemetry/config_new/selectors.rs index 1b49d9548c..3324047b2f 100644 --- a/apollo-router/src/plugins/telemetry/config_new/selectors.rs +++ b/apollo-router/src/plugins/telemetry/config_new/selectors.rs @@ -1069,25 +1069,6 @@ impl Selector for SupergraphSelector { ctx: &Context, ) -> Option { match self { - SupergraphSelector::Query { query, .. } => { - let limits_opt = ctx - .extensions() - .with_lock(|lock| lock.get::>().cloned()); - match query { - Query::Aliases => { - limits_opt.map(|limits| opentelemetry::Value::I64(limits.aliases as i64)) - } - Query::Depth => { - limits_opt.map(|limits| opentelemetry::Value::I64(limits.depth as i64)) - } - Query::Height => { - limits_opt.map(|limits| opentelemetry::Value::I64(limits.height as i64)) - } - Query::RootFields => limits_opt - .map(|limits| opentelemetry::Value::I64(limits.root_fields as i64)), - Query::String => None, - } - } SupergraphSelector::ResponseData { response_data, default, @@ -3289,12 +3270,6 @@ mod test { .unwrap(), 4.into() ); - assert_eq!( - selector - .on_response_event(&crate::graphql::Response::builder().build(), &context) - .unwrap(), - 4.into() - ); } #[test] diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index cc058b5555..dac65efe45 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -166,20 +166,7 @@ impl PlannerMode { schema: &Schema, configuration: &Configuration, ) -> Result, ServiceBuildError> { - let config = apollo_federation::query_plan::query_planner::QueryPlannerConfig { - reuse_query_fragments: configuration - .supergraph - .reuse_query_fragments - .unwrap_or(true), - subgraph_graphql_validation: false, - generate_query_fragments: configuration.supergraph.generate_query_fragments, - incremental_delivery: - apollo_federation::query_plan::query_planner::QueryPlanIncrementalDeliveryConfig { - enable_defer: configuration.supergraph.defer_support, - }, - type_conditioned_fetching: configuration.experimental_type_conditioned_fetching, - debug: Default::default(), - }; + let config = configuration.rust_query_planner_config(); let result = QueryPlanner::new(schema.federation_supergraph(), config); match &result { diff --git a/apollo-router/src/query_planner/dual_query_planner.rs b/apollo-router/src/query_planner/dual_query_planner.rs index 04d393c2cf..9e5250a447 100644 --- a/apollo-router/src/query_planner/dual_query_planner.rs +++ b/apollo-router/src/query_planner/dual_query_planner.rs @@ -14,6 +14,7 @@ use apollo_compiler::ast; use apollo_compiler::validation::Valid; use apollo_compiler::ExecutableDocument; use apollo_compiler::Name; +use apollo_compiler::Node; use apollo_federation::query_plan::query_planner::QueryPlanOptions; use apollo_federation::query_plan::query_planner::QueryPlanner; use apollo_federation::query_plan::QueryPlan; @@ -1003,6 +1004,24 @@ fn same_ast_argument(x: &ast::Argument, y: &ast::Argument) -> bool { x.name == y.name && same_ast_argument_value(&x.value, &y.value) } +fn same_ast_arguments(x: &[Node], y: &[Node]) -> bool { + vec_matches_sorted_by( + x, + y, + |a, b| a.name.cmp(&b.name), + |a, b| same_ast_argument(a, b), + ) +} + +fn same_directives(x: &ast::DirectiveList, y: &ast::DirectiveList) -> bool { + vec_matches_sorted_by( + x, + y, + |a, b| a.name.cmp(&b.name), + |a, b| a.name == b.name && same_ast_arguments(&a.arguments, &b.arguments), + ) +} + fn get_ast_selection_key( selection: &ast::Selection, fragment_map: &HashMap, @@ -1035,24 +1054,20 @@ fn same_ast_selection( (ast::Selection::Field(x), ast::Selection::Field(y)) => { x.name == y.name && x.alias == y.alias - && vec_matches_sorted_by( - &x.arguments, - &y.arguments, - |a, b| a.name.cmp(&b.name), - |a, b| same_ast_argument(a, b), - ) - && x.directives == y.directives + && same_ast_arguments(&x.arguments, &y.arguments) + && same_directives(&x.directives, &y.directives) && same_ast_selection_set_sorted(&x.selection_set, &y.selection_set, fragment_map) } (ast::Selection::FragmentSpread(x), ast::Selection::FragmentSpread(y)) => { let mapped_fragment_name = fragment_map .get(&x.fragment_name) .unwrap_or(&x.fragment_name); - *mapped_fragment_name == y.fragment_name && x.directives == y.directives + *mapped_fragment_name == y.fragment_name + && same_directives(&x.directives, &y.directives) } (ast::Selection::InlineFragment(x), ast::Selection::InlineFragment(y)) => { x.type_condition == y.type_condition - && x.directives == y.directives + && same_directives(&x.directives, &y.directives) && same_ast_selection_set_sorted(&x.selection_set, &y.selection_set, fragment_map) } _ => false, @@ -1200,6 +1215,15 @@ mod ast_comparison_tests { assert!(super::same_ast_document(&ast_x, &ast_y).is_ok()); } + #[test] + fn test_selection_directive_order() { + let op_x = r#"{ x @include(if:true) @skip(if:false) }"#; + let op_y = r#"{ x @skip(if:false) @include(if:true) }"#; + let ast_x = ast::Document::parse(op_x, "op_x").unwrap(); + let ast_y = ast::Document::parse(op_y, "op_y").unwrap(); + assert!(super::same_ast_document(&ast_x, &ast_y).is_ok()); + } + #[test] fn test_string_to_id_coercion_difference() { // JS QP coerces strings into integer for ID type, while Rust QP doesn't. diff --git a/apollo-router/tests/integration/fixtures/query_planner_max_evaluated_plans.graphql b/apollo-router/tests/integration/fixtures/query_planner_max_evaluated_plans.graphql new file mode 100644 index 0000000000..a1f388ed61 --- /dev/null +++ b/apollo-router/tests/integration/fixtures/query_planner_max_evaluated_plans.graphql @@ -0,0 +1,73 @@ +# Composed from subgraphs with hash: a9236eee956ed7fc219b2212696478159ced7eea +schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/join/v0.5", for: EXECUTION) +{ + query: Query +} + +directive @join__directive(graphs: [join__Graph!], name: String!, args: join__DirectiveArguments) repeatable on SCHEMA | OBJECT | INTERFACE | FIELD_DEFINITION + +directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE + +directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean, overrideLabel: String, contextArguments: [join__ContextArgument!]) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION + +directive @join__graph(name: String!, url: String!) on ENUM_VALUE + +directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE + +directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR + +directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION + +directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA + +input join__ContextArgument { + name: String! + type: String! + context: String! + selection: join__FieldValue! +} + +scalar join__DirectiveArguments + +scalar join__FieldSet + +scalar join__FieldValue + +enum join__Graph { + SUBGRAPH1 @join__graph(name: "Subgraph1", url: "none") + SUBGRAPH2 @join__graph(name: "Subgraph2", url: "none") +} + +scalar link__Import + +enum link__Purpose { + """ + `SECURITY` features provide metadata necessary to securely resolve fields. + """ + SECURITY + + """ + `EXECUTION` features provide metadata necessary for operation execution. + """ + EXECUTION +} + +type Query + @join__type(graph: SUBGRAPH1) + @join__type(graph: SUBGRAPH2) +{ + t: T +} + +type T + @join__type(graph: SUBGRAPH1, key: "id") + @join__type(graph: SUBGRAPH2, key: "id") +{ + id: ID! + v1: Int + v2: Int + v3: Int + v4: Int +} diff --git a/apollo-router/tests/integration/query_planner.rs b/apollo-router/tests/integration/query_planner.rs index 4b1c1ab70e..5bf0ca799c 100644 --- a/apollo-router/tests/integration/query_planner.rs +++ b/apollo-router/tests/integration/query_planner.rs @@ -3,6 +3,8 @@ use std::path::PathBuf; use crate::integration::common::graph_os_enabled; use crate::integration::IntegrationTest; +mod max_evaluated_plans; + const PROMETHEUS_METRICS_CONFIG: &str = include_str!("telemetry/fixtures/prometheus.router.yaml"); const LEGACY_QP: &str = "experimental_query_planner_mode: legacy"; const NEW_QP: &str = "experimental_query_planner_mode: new"; diff --git a/apollo-router/tests/integration/query_planner/max_evaluated_plans.rs b/apollo-router/tests/integration/query_planner/max_evaluated_plans.rs new file mode 100644 index 0000000000..d6474aa30b --- /dev/null +++ b/apollo-router/tests/integration/query_planner/max_evaluated_plans.rs @@ -0,0 +1,130 @@ +use serde_json::json; + +use crate::integration::IntegrationTest; + +fn assert_evaluated_plans(prom: &str, expected: u64) { + let line = prom + .lines() + .find(|line| line.starts_with("apollo_router_query_planning_plan_evaluated_plans_sum")) + .expect("apollo.router.query_planning.plan.evaluated_plans metric is missing"); + let (_, num) = line + .split_once(' ') + .expect("apollo.router.query_planning.plan.evaluated_plans metric has unexpected shape"); + assert_eq!(num, format!("{expected}")); +} + +#[tokio::test(flavor = "multi_thread")] +async fn reports_evaluated_plans() { + let mut router = IntegrationTest::builder() + .config( + r#" + telemetry: + exporters: + metrics: + prometheus: + enabled: true + "#, + ) + .supergraph("tests/integration/fixtures/query_planner_max_evaluated_plans.graphql") + .build() + .await; + router.start().await; + router.assert_started().await; + router + .execute_query(&json!({ + "query": r#"{ t { v1 v2 v3 v4 } }"#, + "variables": {}, + })) + .await; + + let metrics = router + .get_metrics_response() + .await + .expect("failed to fetch metrics") + .text() + .await + .expect("metrics are not text?!"); + assert_evaluated_plans(&metrics, 16); + + router.graceful_shutdown().await; +} + +#[tokio::test(flavor = "multi_thread")] +async fn does_not_exceed_max_evaluated_plans_legacy() { + let mut router = IntegrationTest::builder() + .config( + r#" + experimental_query_planner_mode: legacy + telemetry: + exporters: + metrics: + prometheus: + enabled: true + supergraph: + query_planning: + experimental_plans_limit: 4 + "#, + ) + .supergraph("tests/integration/fixtures/query_planner_max_evaluated_plans.graphql") + .build() + .await; + router.start().await; + router.assert_started().await; + router + .execute_query(&json!({ + "query": r#"{ t { v1 v2 v3 v4 } }"#, + "variables": {}, + })) + .await; + + let metrics = router + .get_metrics_response() + .await + .expect("failed to fetch metrics") + .text() + .await + .expect("metrics are not text?!"); + assert_evaluated_plans(&metrics, 4); + + router.graceful_shutdown().await; +} + +#[tokio::test(flavor = "multi_thread")] +async fn does_not_exceed_max_evaluated_plans() { + let mut router = IntegrationTest::builder() + .config( + r#" + experimental_query_planner_mode: new + telemetry: + exporters: + metrics: + prometheus: + enabled: true + supergraph: + query_planning: + experimental_plans_limit: 4 + "#, + ) + .supergraph("tests/integration/fixtures/query_planner_max_evaluated_plans.graphql") + .build() + .await; + router.start().await; + router.assert_started().await; + router + .execute_query(&json!({ + "query": r#"{ t { v1 v2 v3 v4 } }"#, + "variables": {}, + })) + .await; + + let metrics = router + .get_metrics_response() + .await + .expect("failed to fetch metrics") + .text() + .await + .expect("metrics are not text?!"); + assert_evaluated_plans(&metrics, 4); + + router.graceful_shutdown().await; +} diff --git a/apollo-router/tests/integration/telemetry/fixtures/graphql.router.yaml b/apollo-router/tests/integration/telemetry/fixtures/graphql.router.yaml index 24002e9be0..e1ff03b1c7 100644 --- a/apollo-router/tests/integration/telemetry/fixtures/graphql.router.yaml +++ b/apollo-router/tests/integration/telemetry/fixtures/graphql.router.yaml @@ -9,6 +9,31 @@ telemetry: instrumentation: instruments: + supergraph: + oplimits.aliases: + value: + query: aliases + type: histogram + unit: number + description: "Aliases for an operation" + oplimits.depth: + value: + query: depth + type: histogram + unit: number + description: "Depth for an operation" + oplimits.height: + value: + query: height + type: histogram + unit: number + description: "Height for an operation" + oplimits.root_fields: + value: + query: root_fields + type: histogram + unit: number + description: "Root fields for an operation" graphql: field.execution: true list.length: true @@ -38,7 +63,4 @@ telemetry: condition: eq: - field_name: string - - "topProducts" - - - + - "topProducts" \ No newline at end of file diff --git a/apollo-router/tests/integration/telemetry/metrics.rs b/apollo-router/tests/integration/telemetry/metrics.rs index 1ae93f2d4f..cbce509c13 100644 --- a/apollo-router/tests/integration/telemetry/metrics.rs +++ b/apollo-router/tests/integration/telemetry/metrics.rs @@ -201,6 +201,27 @@ async fn test_graphql_metrics() { router.start().await; router.assert_started().await; router.execute_default_query().await; + router + .assert_log_not_contains("this is a bug and should not happen") + .await; + router + .assert_metrics_contains( + r#"oplimits_aliases_sum{otel_scope_name="apollo/router"} 0"#, + None, + ) + .await; + router + .assert_metrics_contains( + r#"oplimits_root_fields_sum{otel_scope_name="apollo/router"} 1"#, + None, + ) + .await; + router + .assert_metrics_contains( + r#"oplimits_depth_sum{otel_scope_name="apollo/router"} 2"#, + None, + ) + .await; router .assert_metrics_contains(r#"graphql_field_list_length_sum{graphql_field_name="topProducts",graphql_field_type="Product",graphql_type_name="Query",otel_scope_name="apollo/router"} 3"#, None) .await;