From cf472851d5fa470ab7e5ade21b4b2e4a3cf0c765 Mon Sep 17 00:00:00 2001 From: Nick Marsh Date: Tue, 13 Aug 2024 16:11:25 +1000 Subject: [PATCH 1/7] Remove experimental label from apollo usage reporting configs --- apollo-router/src/configuration/metrics.rs | 4 +-- .../0028-apollo_telemetry_experimental.yaml | 11 ++++++ apollo-router/src/configuration/mod.rs | 35 ++++++++----------- ...nfiguration__tests__schema_generation.snap | 24 ++++++------- ...lo_telemetry_experimental.router.yaml.snap | 10 ++++++ .../apollo_telemetry_experimental.router.yaml | 6 ++++ apollo-router/src/configuration/tests.rs | 12 +++---- apollo-router/src/plugins/telemetry/apollo.rs | 10 +++--- apollo-router/src/plugins/telemetry/config.rs | 7 ++-- .../plugins/telemetry/metrics/apollo/mod.rs | 4 +-- .../src/plugins/telemetry/tracing/apollo.rs | 2 +- .../src/query_planner/bridge_query_planner.rs | 12 +++---- .../src/services/execution/service.rs | 2 +- .../src/uplink/license_enforcement.rs | 2 +- ..._test__restricted_features_via_config.snap | 2 +- .../uplink/testdata/restricted.router.yaml | 2 +- ...nfig_update_query_planner_mode.router.yaml | 2 +- docs/source/configuration/overview.mdx | 8 ++--- 18 files changed, 84 insertions(+), 71 deletions(-) create mode 100644 apollo-router/src/configuration/migrations/0028-apollo_telemetry_experimental.yaml create mode 100644 apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__upgrade_old_configuration@apollo_telemetry_experimental.router.yaml.snap create mode 100644 apollo-router/src/configuration/testdata/migrations/apollo_telemetry_experimental.router.yaml diff --git a/apollo-router/src/configuration/metrics.rs b/apollo-router/src/configuration/metrics.rs index 8cd6b56381..2a2f651dd1 100644 --- a/apollo-router/src/configuration/metrics.rs +++ b/apollo-router/src/configuration/metrics.rs @@ -387,9 +387,9 @@ impl InstrumentData { apollo.router.config.apollo_telemetry_options, "$.telemetry.apollo", opt.signature_normalization_algorithm, - "$.experimental_apollo_signature_normalization_algorithm", + "$.signature_normalization_algorithm", opt.metrics_reference_mode, - "$.experimental_apollo_metrics_reference_mode" + "$.metrics_reference_mode" ); // We need to update the entry we just made because the selected strategy is a named object in the config. diff --git a/apollo-router/src/configuration/migrations/0028-apollo_telemetry_experimental.yaml b/apollo-router/src/configuration/migrations/0028-apollo_telemetry_experimental.yaml new file mode 100644 index 0000000000..3d0326e70d --- /dev/null +++ b/apollo-router/src/configuration/migrations/0028-apollo_telemetry_experimental.yaml @@ -0,0 +1,11 @@ +description: Some Apollo telemetry options are no longer experimental +actions: + - type: move + from: experimental_apollo_metrics_generation_mode + to: apollo_metrics_generation_mode + - type: move + from: telemetry.apollo.experimental_apollo_signature_normalization_algorithm + to: telemetry.apollo.signature_normalization_algorithm + - type: move + from: telemetry.apollo.experimental_apollo_metrics_reference_mode + to: telemetry.apollo.metrics_reference_mode diff --git a/apollo-router/src/configuration/mod.rs b/apollo-router/src/configuration/mod.rs index 2559b69458..1f6e4190b6 100644 --- a/apollo-router/src/configuration/mod.rs +++ b/apollo-router/src/configuration/mod.rs @@ -163,7 +163,7 @@ pub struct Configuration { /// Set the Apollo usage report signature and referenced field generation implementation to use. #[serde(default)] - pub(crate) experimental_apollo_metrics_generation_mode: ApolloMetricsGenerationMode, + pub(crate) apollo_metrics_generation_mode: ApolloMetricsGenerationMode, /// Set the query planner implementation to use. #[serde(default)] @@ -271,7 +271,7 @@ impl<'de> serde::Deserialize<'de> for Configuration { experimental_chaos: Chaos, batching: Batching, experimental_type_conditioned_fetching: bool, - experimental_apollo_metrics_generation_mode: ApolloMetricsGenerationMode, + apollo_metrics_generation_mode: ApolloMetricsGenerationMode, experimental_query_planner_mode: QueryPlannerMode, } let ad_hoc: AdHocConfiguration = serde::Deserialize::deserialize(deserializer)?; @@ -291,8 +291,7 @@ impl<'de> serde::Deserialize<'de> for Configuration { persisted_queries: ad_hoc.persisted_queries, limits: ad_hoc.limits, experimental_chaos: ad_hoc.experimental_chaos, - experimental_apollo_metrics_generation_mode: ad_hoc - .experimental_apollo_metrics_generation_mode, + 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, @@ -340,7 +339,7 @@ impl Configuration { uplink: Option, experimental_type_conditioned_fetching: Option, batching: Option, - experimental_apollo_metrics_generation_mode: Option, + apollo_metrics_generation_mode: Option, experimental_query_planner_mode: Option, ) -> Result { let notify = Self::notify(&apollo_plugins)?; @@ -356,8 +355,7 @@ impl Configuration { persisted_queries: persisted_query.unwrap_or_default(), limits: operation_limits.unwrap_or_default(), experimental_chaos: chaos.unwrap_or_default(), - experimental_apollo_metrics_generation_mode: - experimental_apollo_metrics_generation_mode.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), @@ -460,7 +458,7 @@ impl Configuration { uplink: Option, batching: Option, experimental_type_conditioned_fetching: Option, - experimental_apollo_metrics_generation_mode: Option, + apollo_metrics_generation_mode: Option, experimental_query_planner_mode: Option, ) -> Result { let configuration = Self { @@ -472,8 +470,7 @@ impl Configuration { cors: cors.unwrap_or_default(), limits: operation_limits.unwrap_or_default(), experimental_chaos: chaos.unwrap_or_default(), - experimental_apollo_metrics_generation_mode: - experimental_apollo_metrics_generation_mode.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), @@ -576,10 +573,10 @@ impl Configuration { } if self.experimental_query_planner_mode == QueryPlannerMode::New - && self.experimental_apollo_metrics_generation_mode != ApolloMetricsGenerationMode::New + && self.apollo_metrics_generation_mode != ApolloMetricsGenerationMode::New { return Err(ConfigurationError::InvalidConfiguration { - message: "`experimental_query_planner_mode: new` requires `experimental_apollo_metrics_generation_mode: new`", + 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() }); } @@ -598,25 +595,23 @@ impl Configuration { if let Some(config) = apollo_telemetry_config { if matches!( - config.experimental_apollo_signature_normalization_algorithm, + config.signature_normalization_algorithm, ApolloSignatureNormalizationAlgorithm::Enhanced - ) && self.experimental_apollo_metrics_generation_mode - != ApolloMetricsGenerationMode::New + ) && self.apollo_metrics_generation_mode != ApolloMetricsGenerationMode::New { return Err(ConfigurationError::InvalidConfiguration { - message: "`experimental_apollo_signature_normalization_algorithm: enhanced` requires `experimental_apollo_metrics_generation_mode: new`", + 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.experimental_apollo_metrics_reference_mode, + config.metrics_reference_mode, ApolloMetricsReferenceMode::Extended - ) && self.experimental_apollo_metrics_generation_mode - != ApolloMetricsGenerationMode::New + ) && self.apollo_metrics_generation_mode != ApolloMetricsGenerationMode::New { return Err(ConfigurationError::InvalidConfiguration { - message: "`experimental_apollo_metrics_reference_mode: extended` requires `experimental_apollo_metrics_generation_mode: new`", + 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() }); }; 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 74294b07ba..6a5c204dcc 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 @@ -1664,14 +1664,6 @@ expression: "&schema" "$ref": "#/definitions/ErrorsConfiguration", "description": "#/definitions/ErrorsConfiguration" }, - "experimental_apollo_metrics_reference_mode": { - "$ref": "#/definitions/ApolloMetricsReferenceMode", - "description": "#/definitions/ApolloMetricsReferenceMode" - }, - "experimental_apollo_signature_normalization_algorithm": { - "$ref": "#/definitions/ApolloSignatureNormalizationAlgorithm", - "description": "#/definitions/ApolloSignatureNormalizationAlgorithm" - }, "experimental_local_field_metrics": { "default": false, "description": "Enable field metrics that are generated without FTV1 to be sent to Apollo Studio.", @@ -1694,6 +1686,10 @@ expression: "&schema" "$ref": "#/definitions/SamplerOption", "description": "#/definitions/SamplerOption" }, + "metrics_reference_mode": { + "$ref": "#/definitions/ApolloMetricsReferenceMode", + "description": "#/definitions/ApolloMetricsReferenceMode" + }, "send_headers": { "$ref": "#/definitions/ForwardHeaders", "description": "#/definitions/ForwardHeaders" @@ -1701,6 +1697,10 @@ expression: "&schema" "send_variable_values": { "$ref": "#/definitions/ForwardValues", "description": "#/definitions/ForwardValues" + }, + "signature_normalization_algorithm": { + "$ref": "#/definitions/ApolloSignatureNormalizationAlgorithm", + "description": "#/definitions/ApolloSignatureNormalizationAlgorithm" } }, "type": "object" @@ -8266,6 +8266,10 @@ 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" @@ -8294,10 +8298,6 @@ expression: "&schema" "$ref": "#/definitions/CSRFConfig", "description": "#/definitions/CSRFConfig" }, - "experimental_apollo_metrics_generation_mode": { - "$ref": "#/definitions/ApolloMetricsGenerationMode", - "description": "#/definitions/ApolloMetricsGenerationMode" - }, "experimental_chaos": { "$ref": "#/definitions/Chaos", "description": "#/definitions/Chaos" 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 new file mode 100644 index 0000000000..7b6d107fa4 --- /dev/null +++ b/apollo-router/src/configuration/snapshots/apollo_router__configuration__tests__upgrade_old_configuration@apollo_telemetry_experimental.router.yaml.snap @@ -0,0 +1,10 @@ +--- +source: apollo-router/src/configuration/tests.rs +expression: new_config +--- +--- +telemetry: + apollo: + signature_normalization_algorithm: enhanced + metrics_reference_mode: extended +apollo_metrics_generation_mode: new diff --git a/apollo-router/src/configuration/testdata/migrations/apollo_telemetry_experimental.router.yaml b/apollo-router/src/configuration/testdata/migrations/apollo_telemetry_experimental.router.yaml new file mode 100644 index 0000000000..1949337902 --- /dev/null +++ b/apollo-router/src/configuration/testdata/migrations/apollo_telemetry_experimental.router.yaml @@ -0,0 +1,6 @@ +experimental_apollo_metrics_generation_mode: new + +telemetry: + apollo: + experimental_apollo_signature_normalization_algorithm: enhanced + experimental_apollo_metrics_reference_mode: extended \ No newline at end of file diff --git a/apollo-router/src/configuration/tests.rs b/apollo-router/src/configuration/tests.rs index 7367da2970..721f80c26b 100644 --- a/apollo-router/src/configuration/tests.rs +++ b/apollo-router/src/configuration/tests.rs @@ -1156,20 +1156,20 @@ fn it_requires_rust_apollo_metrics_generation_for_enhanced_signature_normalizati "telemetry".to_string(), serde_json::json! {{ "apollo": { - "experimental_apollo_signature_normalization_algorithm": "enhanced" + "signature_normalization_algorithm": "enhanced" } }}, ); let error = Configuration::builder() - .experimental_apollo_metrics_generation_mode(ApolloMetricsGenerationMode::Both) + .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("`experimental_apollo_signature_normalization_algorithm: enhanced` requires `experimental_apollo_metrics_generation_mode: new`: either change to the legacy signature normalization mode, or change to new metrics generation") + 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") ); } @@ -1180,19 +1180,19 @@ fn it_requires_rust_apollo_metrics_generation_for_extended_references() { "telemetry".to_string(), serde_json::json! {{ "apollo": { - "experimental_apollo_metrics_reference_mode": "extended" + "metrics_reference_mode": "extended" } }}, ); let error = Configuration::builder() - .experimental_apollo_metrics_generation_mode(ApolloMetricsGenerationMode::Both) + .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("`experimental_apollo_metrics_reference_mode: extended` requires `experimental_apollo_metrics_generation_mode: new`: either change to the standard reference generation mode, or change to new metrics generation") + 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/plugins/telemetry/apollo.rs b/apollo-router/src/plugins/telemetry/apollo.rs index ecb8177e68..69780cbc9e 100644 --- a/apollo-router/src/plugins/telemetry/apollo.rs +++ b/apollo-router/src/plugins/telemetry/apollo.rs @@ -107,11 +107,10 @@ pub(crate) struct Config { pub(crate) errors: ErrorsConfiguration, /// Set the signature normalization algorithm to use when sending Apollo usage reports. - pub(crate) experimental_apollo_signature_normalization_algorithm: - ApolloSignatureNormalizationAlgorithm, + pub(crate) signature_normalization_algorithm: ApolloSignatureNormalizationAlgorithm, /// Set the Apollo usage report reference reporting mode to use. - pub(crate) experimental_apollo_metrics_reference_mode: ApolloMetricsReferenceMode, + pub(crate) metrics_reference_mode: ApolloMetricsReferenceMode, /// Enable field metrics that are generated without FTV1 to be sent to Apollo Studio. pub(crate) experimental_local_field_metrics: bool, @@ -215,10 +214,9 @@ impl Default for Config { send_variable_values: ForwardValues::None, batch_processor: BatchProcessorConfig::default(), errors: ErrorsConfiguration::default(), - experimental_apollo_signature_normalization_algorithm: - ApolloSignatureNormalizationAlgorithm::default(), + signature_normalization_algorithm: ApolloSignatureNormalizationAlgorithm::default(), experimental_local_field_metrics: false, - experimental_apollo_metrics_reference_mode: ApolloMetricsReferenceMode::default(), + metrics_reference_mode: ApolloMetricsReferenceMode::default(), } } } diff --git a/apollo-router/src/plugins/telemetry/config.rs b/apollo-router/src/plugins/telemetry/config.rs index 42c6090b3b..61d6580235 100644 --- a/apollo-router/src/plugins/telemetry/config.rs +++ b/apollo-router/src/plugins/telemetry/config.rs @@ -727,7 +727,7 @@ impl Conf { match configuration.apollo_plugins.plugins.get("telemetry") { Some(telemetry_config) => { match serde_json::from_value::(telemetry_config.clone()) { - Ok(conf) => conf.apollo.experimental_apollo_metrics_reference_mode, + Ok(conf) => conf.apollo.metrics_reference_mode, _ => ApolloMetricsReferenceMode::default(), } } @@ -741,10 +741,7 @@ impl Conf { match configuration.apollo_plugins.plugins.get("telemetry") { Some(telemetry_config) => { match serde_json::from_value::(telemetry_config.clone()) { - Ok(conf) => { - conf.apollo - .experimental_apollo_signature_normalization_algorithm - } + Ok(conf) => conf.apollo.signature_normalization_algorithm, _ => ApolloSignatureNormalizationAlgorithm::default(), } } diff --git a/apollo-router/src/plugins/telemetry/metrics/apollo/mod.rs b/apollo-router/src/plugins/telemetry/metrics/apollo/mod.rs index 00d1e01eeb..47a13762d0 100644 --- a/apollo-router/src/plugins/telemetry/metrics/apollo/mod.rs +++ b/apollo-router/src/plugins/telemetry/metrics/apollo/mod.rs @@ -55,7 +55,7 @@ impl MetricsConfigurator for Config { apollo_graph_ref: Some(reference), schema_id, batch_processor, - experimental_apollo_metrics_reference_mode, + metrics_reference_mode, .. } => { if !ENABLED.swap(true, Ordering::Relaxed) { @@ -69,7 +69,7 @@ impl MetricsConfigurator for Config { reference, schema_id, batch_processor, - *experimental_apollo_metrics_reference_mode, + *metrics_reference_mode, )?; // env variable EXPERIMENTAL_APOLLO_OTLP_METRICS_ENABLED will disappear without warning in future if std::env::var("EXPERIMENTAL_APOLLO_OTLP_METRICS_ENABLED") diff --git a/apollo-router/src/plugins/telemetry/tracing/apollo.rs b/apollo-router/src/plugins/telemetry/tracing/apollo.rs index a3cafdd5cc..b4f6589e37 100644 --- a/apollo-router/src/plugins/telemetry/tracing/apollo.rs +++ b/apollo-router/src/plugins/telemetry/tracing/apollo.rs @@ -45,7 +45,7 @@ impl TracingConfigurator for Config { .batch_config(&self.batch_processor) .errors_configuration(&self.errors) .use_legacy_request_span(matches!(spans_config.mode, SpanMode::Deprecated)) - .metrics_reference_mode(self.experimental_apollo_metrics_reference_mode) + .metrics_reference_mode(self.metrics_reference_mode) .build()?; Ok(builder.with_span_processor( BatchSpanProcessor::builder(exporter, opentelemetry::runtime::Tokio) diff --git a/apollo-router/src/query_planner/bridge_query_planner.rs b/apollo-router/src/query_planner/bridge_query_planner.rs index 1afbb60888..e215dd1503 100644 --- a/apollo-router/src/query_planner/bridge_query_planner.rs +++ b/apollo-router/src/query_planner/bridge_query_planner.rs @@ -506,8 +506,7 @@ impl BridgeQueryPlanner { // 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 - .experimental_apollo_metrics_generation_mode, + self.configuration.apollo_metrics_generation_mode, ApolloMetricsGenerationMode::Legacy | ApolloMetricsGenerationMode::Both ) && original_query != filtered_query { @@ -536,8 +535,7 @@ impl BridgeQueryPlanner { } if matches!( - self.configuration - .experimental_apollo_metrics_generation_mode, + 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 @@ -567,8 +565,7 @@ impl BridgeQueryPlanner { // 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 - .experimental_apollo_metrics_generation_mode, + self.configuration.apollo_metrics_generation_mode, ApolloMetricsGenerationMode::Both ); if !is_empty_operation_name && is_in_both_metrics_mode { @@ -626,8 +623,7 @@ impl BridgeQueryPlanner { ); } } else if matches!( - self.configuration - .experimental_apollo_metrics_generation_mode, + self.configuration.apollo_metrics_generation_mode, ApolloMetricsGenerationMode::New ) { usage_reporting.stats_report_key = diff --git a/apollo-router/src/services/execution/service.rs b/apollo-router/src/services/execution/service.rs index 9486e50ec1..1abcb0c5d3 100644 --- a/apollo-router/src/services/execution/service.rs +++ b/apollo-router/src/services/execution/service.rs @@ -190,7 +190,7 @@ impl ExecutionService { let mut nullified_paths: Vec = vec![]; let metrics_ref_mode = match &self.apollo_telemetry_config { - Some(conf) => conf.experimental_apollo_metrics_reference_mode, + Some(conf) => conf.metrics_reference_mode, _ => ApolloMetricsReferenceMode::default(), }; diff --git a/apollo-router/src/uplink/license_enforcement.rs b/apollo-router/src/uplink/license_enforcement.rs index 2d77fb3683..b9be3577ef 100644 --- a/apollo-router/src/uplink/license_enforcement.rs +++ b/apollo-router/src/uplink/license_enforcement.rs @@ -388,7 +388,7 @@ impl LicenseEnforcementReport { .name("Demand control plugin") .build(), ConfigurationRestriction::builder() - .path("$.telemetry.apollo.experimental_apollo_metrics_reference_mode") + .path("$.telemetry.apollo.metrics_reference_mode") .value("extended") .name("Apollo metrics extended references") .build(), diff --git a/apollo-router/src/uplink/snapshots/apollo_router__uplink__license_enforcement__test__restricted_features_via_config.snap b/apollo-router/src/uplink/snapshots/apollo_router__uplink__license_enforcement__test__restricted_features_via_config.snap index 70f682b0ca..f73e3c36b9 100644 --- a/apollo-router/src/uplink/snapshots/apollo_router__uplink__license_enforcement__test__restricted_features_via_config.snap +++ b/apollo-router/src/uplink/snapshots/apollo_router__uplink__license_enforcement__test__restricted_features_via_config.snap @@ -58,4 +58,4 @@ Configuration yaml: .preview_demand_control * Apollo metrics extended references - .telemetry.apollo.experimental_apollo_metrics_reference_mode + .telemetry.apollo.metrics_reference_mode diff --git a/apollo-router/src/uplink/testdata/restricted.router.yaml b/apollo-router/src/uplink/testdata/restricted.router.yaml index 67c50cac7d..dda17e6721 100644 --- a/apollo-router/src/uplink/testdata/restricted.router.yaml +++ b/apollo-router/src/uplink/testdata/restricted.router.yaml @@ -69,7 +69,7 @@ preview_entity_cache: telemetry: apollo: - experimental_apollo_metrics_reference_mode: extended + metrics_reference_mode: extended instrumentation: spans: router: 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 704cf8fb60..b2ba0ded85 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 @@ -9,4 +9,4 @@ supergraph: ttl: 10s experimental_query_planner_mode: new -experimental_apollo_metrics_generation_mode: new \ No newline at end of file +apollo_metrics_generation_mode: new \ No newline at end of file diff --git a/docs/source/configuration/overview.mdx b/docs/source/configuration/overview.mdx index d4bd85955b..ca67d1e5c5 100644 --- a/docs/source/configuration/overview.mdx +++ b/docs/source/configuration/overview.mdx @@ -607,12 +607,12 @@ This removal means some operations may have the same normalized signature though Enhanced normalization incorporates [input types](#input-types) and [aliases](#aliases) in signature generation. It also includes other improvements that make it more likely that two operations that only vary slightly have the same signature. -Configure enhanced operation signature normalization in `router.yaml` with the `telemetry.apollo.experimental_apollo_signature_normalization_algorithm` option: +Configure enhanced operation signature normalization in `router.yaml` with the `telemetry.apollo.signature_normalization_algorithm` option: ```yaml title="router.yaml" telemetry: apollo: - experimental_apollo_signature_normalization_algorithm: enhanced # Default is legacy + signature_normalization_algorithm: enhanced # Default is legacy ``` Once you enable this configuration, operations with enhanced signatures might appear with different operation IDs than they did previously in GraphOS Studio. @@ -769,12 +769,12 @@ Beginning in v1.50.0, you can configure the router to report enum and input obje Apollo's legacy reference reporting doesn't include data about enum values and input object fields, meaning you can't view enum and input object field usage in GraphOS Studio. Legacy reporting can also cause [inaccurate operation checks](#enhanced-operation-checks). -Configure extended reference reporting in `router.yaml` with the `telemetry.apollo.experimental_apollo_metrics_reference_mode` option like so: +Configure extended reference reporting in `router.yaml` with the `telemetry.apollo.metrics_reference_mode` option like so: ```yaml title="router.yaml" telemetry: apollo: - experimental_apollo_metrics_reference_mode: extended # Default is legacy + metrics_reference_mode: extended # Default is legacy ``` #### Configuration effect timing From 47fbe90ab7b28c1f622bbd93fd44de3cc19b0cba Mon Sep 17 00:00:00 2001 From: Nick Marsh Date: Tue, 13 Aug 2024 16:29:35 +1000 Subject: [PATCH 2/7] Changeset and rename --- .changesets/config_apollo_telemetry_config_rename.md | 10 ++++++++++ ...al.yaml => 0027-apollo_telemetry_experimental.yaml} | 0 2 files changed, 10 insertions(+) create mode 100644 .changesets/config_apollo_telemetry_config_rename.md rename apollo-router/src/configuration/migrations/{0028-apollo_telemetry_experimental.yaml => 0027-apollo_telemetry_experimental.yaml} (100%) diff --git a/.changesets/config_apollo_telemetry_config_rename.md b/.changesets/config_apollo_telemetry_config_rename.md new file mode 100644 index 0000000000..577bc62df3 --- /dev/null +++ b/.changesets/config_apollo_telemetry_config_rename.md @@ -0,0 +1,10 @@ +### ([#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 + +Previous configuration will warn but still work. + +By [@bonnici](https://github.com/bonnici) in https://github.com/apollographql/router/pull/5807 \ No newline at end of file diff --git a/apollo-router/src/configuration/migrations/0028-apollo_telemetry_experimental.yaml b/apollo-router/src/configuration/migrations/0027-apollo_telemetry_experimental.yaml similarity index 100% rename from apollo-router/src/configuration/migrations/0028-apollo_telemetry_experimental.yaml rename to apollo-router/src/configuration/migrations/0027-apollo_telemetry_experimental.yaml From ae67c7998b7f3618062a7ce4a8c1430c493875e1 Mon Sep 17 00:00:00 2001 From: Nick Marsh Date: Tue, 27 Aug 2024 15:53:24 +1000 Subject: [PATCH 3/7] Remove experimental_apollo_metrics_generation_mode --- .../config_apollo_telemetry_config_rename.md | 6 +- .../src/apollo_studio_interop/mod.rs | 59 ----- .../src/apollo_studio_interop/tests.rs | 210 ------------------ .../0027-apollo_telemetry_experimental.yaml | 5 +- apollo-router/src/configuration/mod.rs | 72 ------ ...nfiguration__tests__schema_generation.snap | 30 --- ...lo_telemetry_experimental.router.yaml.snap | 1 - apollo-router/src/configuration/tests.rs | 48 ---- .../src/query_planner/bridge_query_planner.rs | 151 +++---------- .../query_planner/caching_query_planner.rs | 2 +- ...nfig_update_query_planner_mode.router.yaml | 3 +- 11 files changed, 32 insertions(+), 555 deletions(-) diff --git a/.changesets/config_apollo_telemetry_config_rename.md b/.changesets/config_apollo_telemetry_config_rename.md index 577bc62df3..c766a1f7f7 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-router/src/apollo_studio_interop/mod.rs b/apollo-router/src/apollo_studio_interop/mod.rs index 16b1a2f9ee..9aeaee61f6 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 34f457a196..466ca301b0 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 3d0326e70d..02ee640c4a 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 e285698ee1..c9055eca1a 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 456799deb5..d1bbc8e256 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 7b6d107fa4..f9c70e95aa 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 721f80c26b..d1288df215 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 5923cd8769..6f0ab174cf 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 20b2f75342..7715868d52 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 b2ba0ded85..195306ed62 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 From ec5286048d74c1b9268d51814fe8d1582e29ee58 Mon Sep 17 00:00:00 2001 From: Nick Marsh Date: Wed, 28 Aug 2024 13:17:28 +1000 Subject: [PATCH 4/7] Fix changelog and a yaml config --- .changesets/config_apollo_telemetry_config_rename.md | 4 ++-- .../migrations/apollo_telemetry_experimental.router.yaml | 6 ++---- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/.changesets/config_apollo_telemetry_config_rename.md b/.changesets/config_apollo_telemetry_config_rename.md index c766a1f7f7..23f8bddfff 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_metrics_reference_mode` is now ``telemetry.apollo.metrics_reference_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 +* `experimental_apollo_metrics_generation_mode` has been removed since the Rust implementation has been the default since v1.49.0 and it is generating reports identical to the router-bridge implementation Previous configuration will warn but still work. diff --git a/apollo-router/src/configuration/testdata/migrations/apollo_telemetry_experimental.router.yaml b/apollo-router/src/configuration/testdata/migrations/apollo_telemetry_experimental.router.yaml index 1949337902..70cd0fcaad 100644 --- a/apollo-router/src/configuration/testdata/migrations/apollo_telemetry_experimental.router.yaml +++ b/apollo-router/src/configuration/testdata/migrations/apollo_telemetry_experimental.router.yaml @@ -1,6 +1,4 @@ -experimental_apollo_metrics_generation_mode: new - telemetry: apollo: - experimental_apollo_signature_normalization_algorithm: enhanced - experimental_apollo_metrics_reference_mode: extended \ No newline at end of file + signature_normalization_algorithm: enhanced + metrics_reference_mode: extended \ No newline at end of file From 1db48f7cecacd9903804b6dd97066ea530aaaad8 Mon Sep 17 00:00:00 2001 From: Nick Marsh Date: Wed, 28 Aug 2024 13:18:49 +1000 Subject: [PATCH 5/7] Restore test YAML file --- .../migrations/apollo_telemetry_experimental.router.yaml | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/apollo-router/src/configuration/testdata/migrations/apollo_telemetry_experimental.router.yaml b/apollo-router/src/configuration/testdata/migrations/apollo_telemetry_experimental.router.yaml index 70cd0fcaad..1949337902 100644 --- a/apollo-router/src/configuration/testdata/migrations/apollo_telemetry_experimental.router.yaml +++ b/apollo-router/src/configuration/testdata/migrations/apollo_telemetry_experimental.router.yaml @@ -1,4 +1,6 @@ +experimental_apollo_metrics_generation_mode: new + telemetry: apollo: - signature_normalization_algorithm: enhanced - metrics_reference_mode: extended \ No newline at end of file + experimental_apollo_signature_normalization_algorithm: enhanced + experimental_apollo_metrics_reference_mode: extended \ No newline at end of file From 2fb7a371a235ced55aeb24af87d2c2e8dd776245 Mon Sep 17 00:00:00 2001 From: Nick Marsh Date: Wed, 28 Aug 2024 13:20:26 +1000 Subject: [PATCH 6/7] Fix typo --- .changesets/config_apollo_telemetry_config_rename.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changesets/config_apollo_telemetry_config_rename.md b/.changesets/config_apollo_telemetry_config_rename.md index 23f8bddfff..a424c9a402 100644 --- a/.changesets/config_apollo_telemetry_config_rename.md +++ b/.changesets/config_apollo_telemetry_config_rename.md @@ -1,7 +1,7 @@ ### ([#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. -* `telemetry.apollo.experimental_apollo_metrics_reference_mode` is now ``telemetry.apollo.metrics_reference_mode` +* `telemetry.apollo.experimental_apollo_metrics_reference_mode` is now `telemetry.apollo.metrics_reference_mode` * `telemetry.apollo.experimental_apollo_signature_normalization_algorithm` is now `telemetry.apollo.signature_normalization_algorithm` * `experimental_apollo_metrics_generation_mode` has been removed since the Rust implementation has been the default since v1.49.0 and it is generating reports identical to the router-bridge implementation From 5bdb81c48f09654d175ced65f2d664bf13b04ebc Mon Sep 17 00:00:00 2001 From: Nick Marsh Date: Tue, 3 Sep 2024 10:02:24 +1000 Subject: [PATCH 7/7] Update .changesets/config_apollo_telemetry_config_rename.md Co-authored-by: Coenen Benjamin --- .changesets/config_apollo_telemetry_config_rename.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changesets/config_apollo_telemetry_config_rename.md b/.changesets/config_apollo_telemetry_config_rename.md index a424c9a402..569a1fb81b 100644 --- a/.changesets/config_apollo_telemetry_config_rename.md +++ b/.changesets/config_apollo_telemetry_config_rename.md @@ -1,4 +1,4 @@ -### ([#5807](https://github.com/apollographql/router/pull/5807)) +### Remove experimental label from apollo usage reporting configs ([#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. * `telemetry.apollo.experimental_apollo_metrics_reference_mode` is now `telemetry.apollo.metrics_reference_mode`