diff --git a/.changesets/config_apollo_telemetry_config_rename.md b/.changesets/config_apollo_telemetry_config_rename.md index 577bc62df36..c766a1f7f7f 100644 --- a/.changesets/config_apollo_telemetry_config_rename.md +++ b/.changesets/config_apollo_telemetry_config_rename.md @@ -1,9 +1,9 @@ ### ([#5807](https://github.com/apollographql/router/pull/5807)) All known issues related to the new Apollo usage report generation have been resolved so we are renaming some experimental options to be non-experimental. -* experimental_apollo_metrics_generation_mode is now apollo_metrics_generation_mode -* telemetry.apollo.experimental_apollo_signature_normalization_algorithm is now telemetry.apollo.signature_normalization_algorithm -* telemetry.apollo.experimental_apollo_metrics_reference_mode is now telemetry.apollo.metrics_reference_mode +* `experimental_apollo_metrics_generation_mode` is now `apollo_metrics_generation_mode` +* `telemetry.apollo.experimental_apollo_signature_normalization_algorithm` is now `telemetry.apollo.signature_normalization_algorithm` +* `telemetry.apollo.experimental_apollo_metrics_reference_mode` has been removed since the Rust implementation has been the default for a few versions and it is generating reports identical to the router-bridge implementation Previous configuration will warn but still work. diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests.rs b/apollo-federation/tests/query_plan/build_query_plan_tests.rs index 8f256843e2e..216e5253c09 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests.rs @@ -542,75 +542,19 @@ fn key_where_at_external_is_not_at_top_level_of_selection_of_requires() { } "#, @r###" - QueryPlan { - Sequence { - Fetch(service: "A") { - { - u { - __typename - k1 { - id - } - } - } - }, - Flatten(path: "u") { - Fetch(service: "B") { - { - ... on U { - __typename - k1 { - id - } - } - } => - { - ... on U { - k2 - } - } - }, - }, - Flatten(path: "u") { - Fetch(service: "C") { - { - ... on U { - __typename - k2 - } - } => - { - ... on U { - v { - v - } - } - } - }, - }, - Flatten(path: "u") { - Fetch(service: "B") { - { - ... on U { - __typename - v { - v - } - k1 { - id - } - } - } => - { - ... on U { - f - } - } - }, - }, - }, + QueryPlan { + Fetch(service: "A") { + { + u { + __typename + k1 { + id + } + } } - "### + }, + } + "### ); } diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/interface_object.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/interface_object.rs index 5a99022d4a7..68285e6cddb 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/interface_object.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/interface_object.rs @@ -59,35 +59,25 @@ fn can_use_a_key_on_an_interface_object_type() { "#, @r###" - QueryPlan { - Sequence { - Fetch(service: "S1") { - { - iFromS1 { - __typename - id - x - } - } - }, - Flatten(path: "iFromS1") { - Fetch(service: "S2") { - { - ... on I { - __typename - id - } - } => - { - ... on I { - y - } - } - }, - }, - }, + QueryPlan { + Fetch(service: "S1") { + { + iFromS1 { + __typename + x + ... on A { + __typename + id + } + ... on B { + __typename + id + } } - "### + } + }, + } + "### ); } @@ -247,49 +237,34 @@ fn does_not_rely_on_an_interface_object_directly_if_a_specific_implementation_is "#, @r###" - QueryPlan { - Sequence { - Fetch(service: "S2") { - { - iFromS2 { - __typename - id - } - } - }, - Flatten(path: "iFromS2") { - Fetch(service: "S1") { - { - ... on I { - __typename - id - } - } => - { - ... on I { - __typename - } - } - }, - }, - Flatten(path: "iFromS2") { - Fetch(service: "S2") { - { - ... on A { - __typename - id - } - } => - { - ... on I { - y - } - } - }, - }, - }, + QueryPlan { + Sequence { + Fetch(service: "S2") { + { + iFromS2 { + __typename + id + } } - "### + }, + Flatten(path: "iFromS2") { + Fetch(service: "S1") { + { + ... on I { + __typename + id + } + } => + { + ... on I { + __typename + } + } + }, + }, + }, + } + "### ); } @@ -314,37 +289,20 @@ fn can_use_a_key_on_an_interface_object_type_even_for_a_concrete_implementation( "#, @r###" - QueryPlan { - Sequence { - Fetch(service: "S1") { - { - iFromS1 { - __typename - ... on A { - __typename - id - } - } - } - }, - Flatten(path: "iFromS1") { - Fetch(service: "S2") { - { - ... on A { - __typename - id - } - } => - { - ... on I { - y - } - } - }, - }, - }, - } - "### + QueryPlan { + Fetch(service: "S1") { + { + iFromS1 { + __typename + ... on A { + __typename + id + } + } + } + }, + } + "### ); let fetch_nodes = find_fetch_nodes_for_subgraph("S2", &plan); @@ -467,52 +425,37 @@ fn it_avoids_buffering_interface_object_results_that_may_have_to_be_filtered_wit "#, @r###" - QueryPlan { - Sequence { - Fetch(service: "S1") { - { - everything { - __typename - id - } + QueryPlan { + Sequence { + Fetch(service: "S1") { + { + everything { + __typename + id + } + } + }, + Flatten(path: "everything.@") { + Fetch(service: "S2") { + { + ... on I { + __typename + id } - }, - Flatten(path: "everything.@") { - Fetch(service: "S2") { - { - ... on I { - __typename - id - } - } => - { - ... on I { - __typename - ... on A { - a - } - } - } - }, - }, - Flatten(path: "everything.@") { - Fetch(service: "S1") { - { - ... on A { - __typename - id - } - } => - { - ... on I { - expansiveField - } + } => + { + ... on I { + __typename + ... on A { + a } - }, - }, + } + } }, - } - "### + }, + }, + } + "### ); } @@ -562,53 +505,38 @@ fn it_handles_requires_on_concrete_type_of_field_provided_by_interface_object() "#, @r###" - QueryPlan { - Sequence { - Fetch(service: "S2") { - { - i { - __typename - ... on A { - __typename - id - } - } + QueryPlan { + Sequence { + Fetch(service: "S2") { + { + i { + __typename + ... on A { + __typename + id } - }, - Flatten(path: "i") { - Fetch(service: "S1") { - { - ... on A { - __typename - id - } - } => - { - ... on I { - x - } - } - }, - }, - Flatten(path: "i") { - Fetch(service: "S2") { - { - ... on A { - __typename - x - id - } - } => - { - ... on A { - y - } - } - }, - }, + } + } + }, + Flatten(path: "i") { + Fetch(service: "S2") { + { + ... on A { + __typename + x + id + } + } => + { + ... on A { + y + } + } }, - } - "### + }, + }, + } + "### ); } @@ -663,55 +591,42 @@ fn it_handles_interface_object_in_nested_entity() { "#, @r###" - QueryPlan { - Sequence { - Fetch(service: "S2") { - { - i { - __typename - id - } + QueryPlan { + Sequence { + Fetch(service: "S2") { + { + i { + __typename + ... on A { + __typename + id } - }, - Flatten(path: "i") { - Fetch(service: "S1") { - { - ... on I { - __typename - id - } - } => - { - ... on I { - t { - relatedIs { - __typename - id - } - } - } - } - }, - }, - Flatten(path: "i.t.relatedIs.@") { - Fetch(service: "S2") { - { - ... on I { - __typename - id - } - } => - { - ... on I { - __typename - a - } - } - }, - }, + ... on B { + __typename + id + } + } + } + }, + Flatten(path: "i.t.relatedIs.@") { + Fetch(service: "S2") { + { + ... on I { + __typename + id + } + } => + { + ... on I { + __typename + a + } + } }, - } - "### + }, + }, + } + "### ); } @@ -782,55 +697,41 @@ fn it_handles_interface_object_input_rewrites_when_cloning_dependency_graph() { "#, @r###" - QueryPlan { - Sequence { - Fetch(service: "S1") { - { - i { - __typename - i1 - i2 { - __typename - t1 - } - } + QueryPlan { + Sequence { + Fetch(service: "S1") { + { + i { + __typename + i2 { + __typename + t1 } - }, - Parallel { - Flatten(path: "i") { - Fetch(service: "S2") { - { - ... on I { - __typename - i1 - } - } => - { - ... on I { - i3 - } - } - }, - }, - Flatten(path: "i.i2") { - Fetch(service: "S3") { - { - ... on T { - __typename - t1 - } - } => - { - ... on T { - __typename - t2 - } - } - }, - }, - }, + ... on U { + __typename + i1 + } + } + } + }, + Flatten(path: "i.i2") { + Fetch(service: "S4") { + { + ... on T { + __typename + t1 + } + } => + { + ... on T { + __typename + t2 + } + } }, - } - "### + }, + }, + } + "### ); } diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/requires/include_skip.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/requires/include_skip.rs index b981592f684..c91050acee2 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/requires/include_skip.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/requires/include_skip.rs @@ -31,38 +31,40 @@ fn it_handles_a_simple_at_requires_triggered_within_a_conditional() { } "#, @r###" - QueryPlan { - Include(if: $test) { - Sequence { - Fetch(service: "Subgraph1") { - { - t { - __typename - id - a - } - } - }, - Flatten(path: "t") { - Fetch(service: "Subgraph2") { - { - ... on T { - __typename - id - a - } - } => - { - ... on T { - b - } - } - }, - }, - }, + QueryPlan { + Include(if: $test) { + Sequence { + Fetch(service: "Subgraph1") { + { + t { + __typename + id + ... on T { + a + } + } + } + }, + Flatten(path: "t") { + Fetch(service: "Subgraph2") { + { + ... on T { + __typename + id + a + } + } => + { + ... on T { + b + } + } }, - } - "### + }, + }, + }, + } + "### ); } @@ -180,63 +182,69 @@ fn it_handles_an_at_requires_where_multiple_conditional_are_involved() { } "#, @r###" - QueryPlan { - Include(if: $test1) { - Sequence { - Fetch(service: "Subgraph1") { + QueryPlan { + Include(if: $test1) { + Sequence { + Fetch(service: "Subgraph1") { + { + a { + __typename + idA + } + } + }, + Include(if: $test2) { + Sequence { + Flatten(path: "a") { + Fetch(service: "Subgraph2") { { - a { + ... on A { __typename idA } - } - }, - Include(if: $test2) { - Sequence { - Flatten(path: "a") { - Fetch(service: "Subgraph2") { - { - ... on A { - __typename - idA - } - } => - { - ... on A { - b { - __typename - idB - required - } - } - } - }, - }, - Flatten(path: "a.b.@") { - Fetch(service: "Subgraph3") { - { + } => + { + ... on A { + b { + __typename + idB + ... on B { ... on B { ... on B { - __typename - idB required } } - } => - { - ... on B { - ... on B { - c - } - } } - }, - }, + } + } } }, - } + }, + Flatten(path: "a.b.@") { + Fetch(service: "Subgraph3") { + { + ... on B { + ... on B { + __typename + idB + required + } + } + } => + { + ... on B { + ... on B { + c + } + } + } + }, + }, }, - } - "### + }, + }, + }, + } + "### ); } diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/subscriptions.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/subscriptions.rs index 67c3367c315..454acffca35 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/subscriptions.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/subscriptions.rs @@ -189,20 +189,40 @@ fn trying_to_use_defer_with_a_subcription_results_in_an_error() { // This is just a placeholder. We expect the planner to return an Err, which is then // unwrapped. @r###" - QueryPlan { - Subscription { - Primary: { - Fetch(service: "subgraphA") { - { - onNewUser { - id - name - } + QueryPlan { + Subscription { + Primary: { + Fetch(service: "SubgraphA") { + { + onNewUser { + __typename + id + name } } }, - } }, - "### + Rest: { + Sequence { + Flatten(path: "onNewUser") { + Fetch(service: "SubgraphB") { + { + ... on User { + __typename + id + } + } => + { + ... on User { + address + } + } + }, + }, + }, + }, + }, + } + "### ); } diff --git a/apollo-router/src/apollo_studio_interop/mod.rs b/apollo-router/src/apollo_studio_interop/mod.rs index 16b1a2f9eef..9aeaee61f6e 100644 --- a/apollo-router/src/apollo_studio_interop/mod.rs +++ b/apollo-router/src/apollo_studio_interop/mod.rs @@ -169,65 +169,6 @@ pub(crate) struct ComparableUsageReporting { pub(crate) result: UsageReporting, } -/// Enum specifying the result of a comparison. -#[derive(Debug)] -pub(crate) enum UsageReportingComparisonResult { - /// The UsageReporting instances are the same - Equal, - /// The stats_report_key in the UsageReporting instances are different - StatsReportKeyNotEqual, - /// The referenced_fields in the UsageReporting instances are different. When comparing referenced - /// fields, we ignore the ordering of field names. - ReferencedFieldsNotEqual, - /// Both the stats_report_key and referenced_fields in the UsageReporting instances are different. - BothNotEqual, -} - -impl ComparableUsageReporting { - /// Compare this to another UsageReporting. - pub(crate) fn compare(&self, other: &UsageReporting) -> UsageReportingComparisonResult { - let sig_equal = self.result.stats_report_key == other.stats_report_key; - let refs_equal = self.compare_referenced_fields(&other.referenced_fields_by_type); - match (sig_equal, refs_equal) { - (true, true) => UsageReportingComparisonResult::Equal, - (false, true) => UsageReportingComparisonResult::StatsReportKeyNotEqual, - (true, false) => UsageReportingComparisonResult::ReferencedFieldsNotEqual, - (false, false) => UsageReportingComparisonResult::BothNotEqual, - } - } - - fn compare_referenced_fields( - &self, - other_ref_fields: &HashMap, - ) -> bool { - let self_ref_fields = &self.result.referenced_fields_by_type; - if self_ref_fields.len() != other_ref_fields.len() { - return false; - } - - for (name, self_refs) in self_ref_fields.iter() { - let maybe_other_refs = other_ref_fields.get(name); - if let Some(other_refs) = maybe_other_refs { - if self_refs.is_interface != other_refs.is_interface { - return false; - } - - let self_field_names_set: HashSet<_> = - self_refs.field_names.clone().into_iter().collect(); - let other_field_names_set: HashSet<_> = - other_refs.field_names.clone().into_iter().collect(); - if self_field_names_set != other_field_names_set { - return false; - } - } else { - return false; - } - } - - true - } -} - /// Generate a ComparableUsageReporting containing the stats_report_key (a normalized version of the operation signature) /// and referenced fields of an operation. The document used to generate the signature and for the references can be /// different to handle cases where the operation has been filtered, but we want to keep the same signature. diff --git a/apollo-router/src/apollo_studio_interop/tests.rs b/apollo-router/src/apollo_studio_interop/tests.rs index 34f457a1967..466ca301b0b 100644 --- a/apollo-router/src/apollo_studio_interop/tests.rs +++ b/apollo-router/src/apollo_studio_interop/tests.rs @@ -765,213 +765,3 @@ async fn test_enums_from_response_fragments() { let generated = enums_from_response(query_str, op_name, schema_str, response_str); assert_enums_from_response!(&generated); } - -#[test(tokio::test)] -async fn test_compare() { - let source = ComparableUsageReporting { - result: UsageReporting { - stats_report_key: "# -\n{basicResponseQuery{field1 field2}}".into(), - referenced_fields_by_type: HashMap::from([ - ( - "Query".into(), - ReferencedFieldsForType { - field_names: vec!["basicResponseQuery".into()], - is_interface: false, - }, - ), - ( - "SomeResponse".into(), - ReferencedFieldsForType { - field_names: vec!["field1".into(), "field2".into()], - is_interface: false, - }, - ), - ]), - }, - }; - - // Same signature and ref fields should match - assert!(matches!( - source.compare(&UsageReporting { - stats_report_key: source.result.stats_report_key.clone(), - referenced_fields_by_type: source.result.referenced_fields_by_type.clone(), - }), - UsageReportingComparisonResult::Equal - )); - - // Reordered signature should not match - assert!(matches!( - source.compare(&UsageReporting { - stats_report_key: "# -\n{basicResponseQuery{field2 field1}}".into(), - referenced_fields_by_type: source.result.referenced_fields_by_type.clone(), - }), - UsageReportingComparisonResult::StatsReportKeyNotEqual - )); - - // Different signature should not match - assert!(matches!( - source.compare(&UsageReporting { - stats_report_key: "# NamedQuery\nquery NamedQuery {basicResponseQuery{field1 field2}}" - .into(), - referenced_fields_by_type: source.result.referenced_fields_by_type.clone(), - }), - UsageReportingComparisonResult::StatsReportKeyNotEqual - )); - - // Reordered parent type should match - assert!(matches!( - source.compare(&UsageReporting { - stats_report_key: source.result.stats_report_key.clone(), - referenced_fields_by_type: HashMap::from([ - ( - "Query".into(), - ReferencedFieldsForType { - field_names: vec!["basicResponseQuery".into()], - is_interface: false, - }, - ), - ( - "SomeResponse".into(), - ReferencedFieldsForType { - field_names: vec!["field1".into(), "field2".into()], - is_interface: false, - }, - ), - ]) - }), - UsageReportingComparisonResult::Equal - )); - - // Reordered fields should match - assert!(matches!( - source.compare(&UsageReporting { - stats_report_key: source.result.stats_report_key.clone(), - referenced_fields_by_type: HashMap::from([ - ( - "Query".into(), - ReferencedFieldsForType { - field_names: vec!["basicResponseQuery".into()], - is_interface: false, - }, - ), - ( - "SomeResponse".into(), - ReferencedFieldsForType { - field_names: vec!["field2".into(), "field1".into()], - is_interface: false, - }, - ), - ]) - }), - UsageReportingComparisonResult::Equal - )); - - // Added parent type should not match - assert!(matches!( - source.compare(&UsageReporting { - stats_report_key: source.result.stats_report_key.clone(), - referenced_fields_by_type: HashMap::from([ - ( - "Query".into(), - ReferencedFieldsForType { - field_names: vec!["basicResponseQuery".into()], - is_interface: false, - }, - ), - ( - "SomeResponse".into(), - ReferencedFieldsForType { - field_names: vec!["field1".into(), "field2".into()], - is_interface: false, - }, - ), - ( - "OtherType".into(), - ReferencedFieldsForType { - field_names: vec!["otherField".into()], - is_interface: false, - }, - ), - ]) - }), - UsageReportingComparisonResult::ReferencedFieldsNotEqual - )); - - // Added field should not match - assert!(matches!( - source.compare(&UsageReporting { - stats_report_key: source.result.stats_report_key.clone(), - referenced_fields_by_type: HashMap::from([ - ( - "Query".into(), - ReferencedFieldsForType { - field_names: vec!["basicResponseQuery".into()], - is_interface: false, - }, - ), - ( - "SomeResponse".into(), - ReferencedFieldsForType { - field_names: vec!["field1".into(), "field2".into(), "field3".into()], - is_interface: false, - }, - ), - ]) - }), - UsageReportingComparisonResult::ReferencedFieldsNotEqual - )); - - // Missing parent type should not match - assert!(matches!( - source.compare(&UsageReporting { - stats_report_key: source.result.stats_report_key.clone(), - referenced_fields_by_type: HashMap::from([( - "Query".into(), - ReferencedFieldsForType { - field_names: vec!["basicResponseQuery".into()], - is_interface: false, - }, - ),]) - }), - UsageReportingComparisonResult::ReferencedFieldsNotEqual - )); - - // Missing field should not match - assert!(matches!( - source.compare(&UsageReporting { - stats_report_key: source.result.stats_report_key.clone(), - referenced_fields_by_type: HashMap::from([ - ( - "Query".into(), - ReferencedFieldsForType { - field_names: vec!["basicResponseQuery".into()], - is_interface: false, - }, - ), - ( - "SomeResponse".into(), - ReferencedFieldsForType { - field_names: vec!["field1".into()], - is_interface: false, - }, - ), - ]) - }), - UsageReportingComparisonResult::ReferencedFieldsNotEqual - )); - - // Both different should not match - assert!(matches!( - source.compare(&UsageReporting { - stats_report_key: "# -\n{basicResponseQuery{field2 field1}}".into(), - referenced_fields_by_type: HashMap::from([( - "Query".into(), - ReferencedFieldsForType { - field_names: vec!["basicResponseQuery".into()], - is_interface: false, - }, - ),]) - }), - UsageReportingComparisonResult::BothNotEqual - )); -} diff --git a/apollo-router/src/configuration/migrations/0027-apollo_telemetry_experimental.yaml b/apollo-router/src/configuration/migrations/0027-apollo_telemetry_experimental.yaml index 3d0326e70de..02ee640c4a2 100644 --- a/apollo-router/src/configuration/migrations/0027-apollo_telemetry_experimental.yaml +++ b/apollo-router/src/configuration/migrations/0027-apollo_telemetry_experimental.yaml @@ -1,8 +1,7 @@ description: Some Apollo telemetry options are no longer experimental actions: - - type: move - from: experimental_apollo_metrics_generation_mode - to: apollo_metrics_generation_mode + - type: delete + path: experimental_apollo_metrics_generation_mode - type: move from: telemetry.apollo.experimental_apollo_signature_normalization_algorithm to: telemetry.apollo.signature_normalization_algorithm diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index e285698ee1f..c9055eca1a8 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -50,8 +50,6 @@ use crate::plugin::plugins; use crate::plugins::subscription::SubscriptionConfig; use crate::plugins::subscription::APOLLO_SUBSCRIPTION_PLUGIN; use crate::plugins::subscription::APOLLO_SUBSCRIPTION_PLUGIN_NAME; -use crate::plugins::telemetry::config::ApolloMetricsReferenceMode; -use crate::plugins::telemetry::config::ApolloSignatureNormalizationAlgorithm; use crate::uplink::UplinkConfig; use crate::ApolloRouterError; @@ -161,10 +159,6 @@ pub struct Configuration { #[serde(default)] pub(crate) experimental_chaos: Chaos, - /// Set the Apollo usage report signature and referenced field generation implementation to use. - #[serde(default)] - pub(crate) apollo_metrics_generation_mode: ApolloMetricsGenerationMode, - /// Set the query planner implementation to use. #[serde(default)] pub(crate) experimental_query_planner_mode: QueryPlannerMode, @@ -200,21 +194,6 @@ impl PartialEq for Configuration { } } -/// Apollo usage report signature and referenced field generation modes. -#[derive(Clone, PartialEq, Eq, Default, Derivative, Serialize, Deserialize, JsonSchema)] -#[derivative(Debug)] -#[serde(rename_all = "lowercase")] -pub(crate) enum ApolloMetricsGenerationMode { - /// Use the new Rust-based implementation. - #[default] - New, - /// Use the old JavaScript-based implementation. - Legacy, - /// Use Rust-based and Javascript-based implementations side by side, logging warnings if the - /// implementations disagree. - Both, -} - /// Query planner modes. #[derive(Clone, PartialEq, Eq, Default, Derivative, Serialize, Deserialize, JsonSchema)] #[derivative(Debug)] @@ -271,7 +250,6 @@ impl<'de> serde::Deserialize<'de> for Configuration { experimental_chaos: Chaos, batching: Batching, experimental_type_conditioned_fetching: bool, - apollo_metrics_generation_mode: ApolloMetricsGenerationMode, experimental_query_planner_mode: QueryPlannerMode, } let ad_hoc: AdHocConfiguration = serde::Deserialize::deserialize(deserializer)?; @@ -291,7 +269,6 @@ impl<'de> serde::Deserialize<'de> for Configuration { persisted_queries: ad_hoc.persisted_queries, limits: ad_hoc.limits, experimental_chaos: ad_hoc.experimental_chaos, - apollo_metrics_generation_mode: ad_hoc.apollo_metrics_generation_mode, experimental_type_conditioned_fetching: ad_hoc.experimental_type_conditioned_fetching, experimental_query_planner_mode: ad_hoc.experimental_query_planner_mode, plugins: ad_hoc.plugins, @@ -339,7 +316,6 @@ impl Configuration { uplink: Option, experimental_type_conditioned_fetching: Option, batching: Option, - apollo_metrics_generation_mode: Option, experimental_query_planner_mode: Option, ) -> Result { let notify = Self::notify(&apollo_plugins)?; @@ -355,7 +331,6 @@ impl Configuration { persisted_queries: persisted_query.unwrap_or_default(), limits: operation_limits.unwrap_or_default(), experimental_chaos: chaos.unwrap_or_default(), - apollo_metrics_generation_mode: apollo_metrics_generation_mode.unwrap_or_default(), experimental_query_planner_mode: experimental_query_planner_mode.unwrap_or_default(), plugins: UserPlugins { plugins: Some(plugins), @@ -458,7 +433,6 @@ impl Configuration { uplink: Option, batching: Option, experimental_type_conditioned_fetching: Option, - apollo_metrics_generation_mode: Option, experimental_query_planner_mode: Option, ) -> Result { let configuration = Self { @@ -470,7 +444,6 @@ impl Configuration { cors: cors.unwrap_or_default(), limits: operation_limits.unwrap_or_default(), experimental_chaos: chaos.unwrap_or_default(), - apollo_metrics_generation_mode: apollo_metrics_generation_mode.unwrap_or_default(), experimental_query_planner_mode: experimental_query_planner_mode.unwrap_or_default(), plugins: UserPlugins { plugins: Some(plugins), @@ -572,51 +545,6 @@ impl Configuration { } } - if self.experimental_query_planner_mode == QueryPlannerMode::New - && self.apollo_metrics_generation_mode != ApolloMetricsGenerationMode::New - { - return Err(ConfigurationError::InvalidConfiguration { - message: "`experimental_query_planner_mode: new` requires `apollo_metrics_generation_mode: new`", - error: "either change to some other query planner mode, or change to new metrics generation".into() - }); - } - - let apollo_telemetry_config = match self.apollo_plugins.plugins.get("telemetry") { - Some(telemetry_config) => { - match serde_json::from_value::( - telemetry_config.clone(), - ) { - Ok(conf) => Some(conf.apollo), - _ => None, - } - } - _ => None, - }; - - if let Some(config) = apollo_telemetry_config { - if matches!( - config.signature_normalization_algorithm, - ApolloSignatureNormalizationAlgorithm::Enhanced - ) && self.apollo_metrics_generation_mode != ApolloMetricsGenerationMode::New - { - return Err(ConfigurationError::InvalidConfiguration { - message: "`signature_normalization_algorithm: enhanced` requires `apollo_metrics_generation_mode: new`", - error: "either change to the legacy signature normalization mode, or change to new metrics generation".into() - }); - } - - if matches!( - config.metrics_reference_mode, - ApolloMetricsReferenceMode::Extended - ) && self.apollo_metrics_generation_mode != ApolloMetricsGenerationMode::New - { - return Err(ConfigurationError::InvalidConfiguration { - message: "`metrics_reference_mode: extended` requires `apollo_metrics_generation_mode: new`", - error: "either change to the standard reference generation mode, or change to new metrics generation".into() - }); - }; - } - Ok(self) } } diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap index 456799deb5c..d1bbc8e2561 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap +++ b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__schema_generation.snap @@ -113,32 +113,6 @@ expression: "&schema" ], "type": "string" }, - "ApolloMetricsGenerationMode": { - "description": "Apollo usage report signature and referenced field generation modes.", - "oneOf": [ - { - "description": "Use the new Rust-based implementation.", - "enum": [ - "new" - ], - "type": "string" - }, - { - "description": "Use the old JavaScript-based implementation.", - "enum": [ - "legacy" - ], - "type": "string" - }, - { - "description": "Use Rust-based and Javascript-based implementations side by side, logging warnings if the implementations disagree.", - "enum": [ - "both" - ], - "type": "string" - } - ] - }, "ApolloMetricsReferenceMode": { "description": "Apollo usage report reference generation modes.", "oneOf": [ @@ -8279,10 +8253,6 @@ expression: "&schema" }, "description": "The configuration for the router.\n\nCan be created through `serde::Deserialize` from various formats, or inline in Rust code with `serde_json::json!` and `serde_json::from_value`.", "properties": { - "apollo_metrics_generation_mode": { - "$ref": "#/definitions/ApolloMetricsGenerationMode", - "description": "#/definitions/ApolloMetricsGenerationMode" - }, "apq": { "$ref": "#/definitions/Apq", "description": "#/definitions/Apq" diff --git a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__upgrade_old_configuration@apollo_telemetry_experimental.router.yaml.snap b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__upgrade_old_configuration@apollo_telemetry_experimental.router.yaml.snap index 7b6d107fa4a..f9c70e95aad 100644 --- a/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__upgrade_old_configuration@apollo_telemetry_experimental.router.yaml.snap +++ b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__upgrade_old_configuration@apollo_telemetry_experimental.router.yaml.snap @@ -7,4 +7,3 @@ telemetry: apollo: signature_normalization_algorithm: enhanced metrics_reference_mode: extended -apollo_metrics_generation_mode: new diff --git a/apollo-router/src/configuration/tests.rs b/apollo-router/src/configuration/tests.rs index 721f80c26b3..d1288df2151 100644 --- a/apollo-router/src/configuration/tests.rs +++ b/apollo-router/src/configuration/tests.rs @@ -1148,51 +1148,3 @@ fn it_prevents_reuse_and_generate_query_fragments_simultaneously() { assert!(conf.supergraph.generate_query_fragments); assert_eq!(conf.supergraph.reuse_query_fragments, Some(false)); } - -#[test] -fn it_requires_rust_apollo_metrics_generation_for_enhanced_signature_normalization() { - let mut plugins_config = serde_json::Map::new(); - plugins_config.insert( - "telemetry".to_string(), - serde_json::json! {{ - "apollo": { - "signature_normalization_algorithm": "enhanced" - } - }}, - ); - - let error = Configuration::builder() - .apollo_metrics_generation_mode(ApolloMetricsGenerationMode::Both) - .apollo_plugins(plugins_config) - .build() - .expect_err("Must have an error because we have conflicting config options"); - - assert_eq!( - error.to_string(), - String::from("`signature_normalization_algorithm: enhanced` requires `apollo_metrics_generation_mode: new`: either change to the legacy signature normalization mode, or change to new metrics generation") - ); -} - -#[test] -fn it_requires_rust_apollo_metrics_generation_for_extended_references() { - let mut plugins_config = serde_json::Map::new(); - plugins_config.insert( - "telemetry".to_string(), - serde_json::json! {{ - "apollo": { - "metrics_reference_mode": "extended" - } - }}, - ); - - let error = Configuration::builder() - .apollo_metrics_generation_mode(ApolloMetricsGenerationMode::Both) - .apollo_plugins(plugins_config) - .build() - .expect_err("Must have an error because we have conflicting config options"); - - assert_eq!( - error.to_string(), - String::from("`metrics_reference_mode: extended` requires `apollo_metrics_generation_mode: new`: either change to the standard reference generation mode, or change to new metrics generation") - ); -} diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index 5923cd8769e..6f0ab174cf4 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -28,8 +28,6 @@ use tower::Service; use super::PlanNode; use super::QueryKey; use crate::apollo_studio_interop::generate_usage_reporting; -use crate::apollo_studio_interop::UsageReportingComparisonResult; -use crate::configuration::ApolloMetricsGenerationMode; use crate::configuration::QueryPlannerMode; use crate::error::PlanErrors; use crate::error::QueryPlannerError; @@ -297,8 +295,6 @@ impl PlannerMode { let plan = result?; // Dummy value overwritten below in `BrigeQueryPlanner::plan` - // `Configuration::validate` ensures that we only take this path - // when we also have `ApolloMetricsGenerationMode::New`` let usage_reporting = UsageReporting { stats_report_key: Default::default(), referenced_fields_by_type: Default::default(), @@ -530,23 +526,6 @@ impl BridgeQueryPlanner { ) .await?; - // the `statsReportKey` field should match the original query instead of the filtered query, to index them all under the same query - let operation_signature = if matches!( - self.configuration.apollo_metrics_generation_mode, - ApolloMetricsGenerationMode::Legacy | ApolloMetricsGenerationMode::Both - ) && original_query != filtered_query - { - Some( - self.planner - .js_for_api_schema_and_introspection_and_operation_signature() - .operation_signature(original_query.clone(), operation.clone()) - .await - .map_err(QueryPlannerError::RouterBridgeError)?, - ) - } else { - None - }; - match plan_success { PlanSuccess { data: @@ -556,108 +535,32 @@ impl BridgeQueryPlanner { }, mut usage_reporting, } => { - if let Some(sig) = operation_signature { - usage_reporting.stats_report_key = sig; - } + // If the query is filtered, we want to generate the signature using the original query and generate the + // reference using the filtered query. To do this, we need to re-parse the original query here. + let signature_doc = if original_query != filtered_query { + Query::parse_document( + &original_query, + operation.clone().as_deref(), + &self.schema, + &self.configuration, + ) + .unwrap_or(doc.clone()) + } else { + doc.clone() + }; - if matches!( - self.configuration.apollo_metrics_generation_mode, - ApolloMetricsGenerationMode::New | ApolloMetricsGenerationMode::Both - ) { - // If the query is filtered, we want to generate the signature using the original query and generate the - // reference using the filtered query. To do this, we need to re-parse the original query here. - let signature_doc = if original_query != filtered_query { - Query::parse_document( - &original_query, - operation.clone().as_deref(), - &self.schema, - &self.configuration, - ) - .unwrap_or(doc.clone()) - } else { - doc.clone() - }; - - let generated_usage_reporting = generate_usage_reporting( - &signature_doc.executable, - &doc.executable, - &operation, - self.schema.supergraph_schema(), - &self.signature_normalization_algorithm, - ); - - // Ignore comparison if the operation name is an empty string since there is a known issue where - // router behaviour is incorrect in that case, and it also generates incorrect usage reports. - // https://github.com/apollographql/router/issues/4837 - let is_empty_operation_name = operation.map_or(false, |s| s.is_empty()); - let is_in_both_metrics_mode = matches!( - self.configuration.apollo_metrics_generation_mode, - ApolloMetricsGenerationMode::Both - ); - if !is_empty_operation_name && is_in_both_metrics_mode { - let comparison_result = generated_usage_reporting.compare(&usage_reporting); - - if matches!( - comparison_result, - UsageReportingComparisonResult::StatsReportKeyNotEqual - | UsageReportingComparisonResult::BothNotEqual - ) { - u64_counter!( - "apollo.router.operations.telemetry.studio.signature", - "The match status of the Apollo reporting signature generated by the JS implementation vs the Rust implementation", - 1, - "generation.is_matched" = false - ); - tracing::debug!( - "Different signatures generated between router and router-bridge.\nQuery:\n{}\nRouter:\n{}\nRouter Bridge:\n{}", - filtered_query, - generated_usage_reporting.result.stats_report_key, - usage_reporting.stats_report_key, - ); - } else { - u64_counter!( - "apollo.router.operations.telemetry.studio.signature", - "The match status of the Apollo reporting signature generated by the JS implementation vs the Rust implementation", - 1, - "generation.is_matched" = true - ); - } + let generated_usage_reporting = generate_usage_reporting( + &signature_doc.executable, + &doc.executable, + &operation, + self.schema.supergraph_schema(), + &self.signature_normalization_algorithm, + ); - if matches!( - comparison_result, - UsageReportingComparisonResult::ReferencedFieldsNotEqual - | UsageReportingComparisonResult::BothNotEqual - ) { - u64_counter!( - "apollo.router.operations.telemetry.studio.references", - "The match status of the Apollo reporting references generated by the JS implementation vs the Rust implementation", - 1, - "generation.is_matched" = false - ); - tracing::debug!( - "Different referenced fields generated between router and router-bridge.\nQuery:\n{}\nRouter:\n{:?}\nRouter Bridge:\n{:?}", - filtered_query, - generated_usage_reporting.result.referenced_fields_by_type, - usage_reporting.referenced_fields_by_type, - ); - } else { - u64_counter!( - "apollo.router.operations.telemetry.studio.references", - "The match status of the Apollo reporting references generated by the JS implementation vs the Rust implementation", - 1, - "generation.is_matched" = true - ); - } - } else if matches!( - self.configuration.apollo_metrics_generation_mode, - ApolloMetricsGenerationMode::New - ) { - usage_reporting.stats_report_key = - generated_usage_reporting.result.stats_report_key; - usage_reporting.referenced_fields_by_type = - generated_usage_reporting.result.referenced_fields_by_type; - } - } + usage_reporting.stats_report_key = + generated_usage_reporting.result.stats_report_key; + usage_reporting.referenced_fields_by_type = + generated_usage_reporting.result.referenced_fields_by_type; Ok(QueryPlannerContent::Plan { plan: Arc::new(super::QueryPlan { @@ -677,13 +580,9 @@ impl BridgeQueryPlanner { query_plan: QueryPlan { node: None }, .. }, - mut usage_reporting, + usage_reporting, } => { failfast_debug!("empty query plan"); - if let Some(sig) = operation_signature { - usage_reporting.stats_report_key = sig; - } - Err(QueryPlannerError::EmptyPlan(usage_reporting)) } } diff --git a/apollo-router/src/query_planner/caching_query_planner.rs b/apollo-router/src/query_planner/caching_query_planner.rs index 20b2f753422..7715868d528 100644 --- a/apollo-router/src/query_planner/caching_query_planner.rs +++ b/apollo-router/src/query_planner/caching_query_planner.rs @@ -546,7 +546,7 @@ where } } - // This will be overridden when running in ApolloMetricsGenerationMode::New mode + // This will be overridden by the Rust usage reporting implementation if let Some(QueryPlannerContent::Plan { plan, .. }) = &content { context.extensions().with_lock(|mut lock| { lock.insert::>(plan.usage_reporting.clone()) diff --git a/apollo-router/tests/integration/fixtures/query_planner_redis_config_update_query_planner_mode.router.yaml b/apollo-router/tests/integration/fixtures/query_planner_redis_config_update_query_planner_mode.router.yaml index b2ba0ded854..195306ed62c 100644 --- a/apollo-router/tests/integration/fixtures/query_planner_redis_config_update_query_planner_mode.router.yaml +++ b/apollo-router/tests/integration/fixtures/query_planner_redis_config_update_query_planner_mode.router.yaml @@ -8,5 +8,4 @@ supergraph: - redis://localhost:6379 ttl: 10s -experimental_query_planner_mode: new -apollo_metrics_generation_mode: new \ No newline at end of file +experimental_query_planner_mode: new \ No newline at end of file