-
Notifications
You must be signed in to change notification settings - Fork 272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move QP compatibility checks into constructor and add metric #5811
Changes from all commits
dc408ec
8f925fb
793f91e
7e8bbc4
5cb4d8f
6a90334
009436b
64f33e1
503bc89
30f9d7e
b28cbf5
91062d4
4e9890e
8dff332
dc2504d
ba83265
b95b1c7
49c6a69
0ba82ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ use apollo_compiler::ast; | |
use apollo_compiler::validation::Valid; | ||
use apollo_compiler::Name; | ||
use apollo_federation::error::FederationError; | ||
use apollo_federation::error::SingleFederationError; | ||
use apollo_federation::query_plan::query_planner::QueryPlanner; | ||
use futures::future::BoxFuture; | ||
use opentelemetry_api::metrics::MeterProvider as _; | ||
|
@@ -64,6 +65,10 @@ use crate::Configuration; | |
|
||
pub(crate) const RUST_QP_MODE: &str = "rust"; | ||
const JS_QP_MODE: &str = "js"; | ||
const UNSUPPORTED_CONTEXT: &str = "context"; | ||
const UNSUPPORTED_OVERRIDES: &str = "overrides"; | ||
const UNSUPPORTED_FED1: &str = "fed1"; | ||
const INTERNAL_INIT_ERROR: &str = "internal"; | ||
|
||
#[derive(Clone)] | ||
/// A query planner that calls out to the nodejs router-bridge query planner. | ||
|
@@ -162,10 +167,7 @@ impl PlannerMode { | |
QueryPlannerMode::BothBestEffort => match Self::rust(schema, configuration) { | ||
Ok(planner) => Ok(Some(planner)), | ||
Err(error) => { | ||
tracing::warn!( | ||
"Failed to initialize the new query planner, \ | ||
falling back to legacy: {error}" | ||
); | ||
tracing::info!("Falling back to the legacy query planner: {error}"); | ||
Ok(None) | ||
} | ||
}, | ||
|
@@ -189,10 +191,34 @@ impl PlannerMode { | |
}, | ||
debug: Default::default(), | ||
}; | ||
Ok(Arc::new(QueryPlanner::new( | ||
schema.federation_supergraph(), | ||
config, | ||
)?)) | ||
let result = QueryPlanner::new(schema.federation_supergraph(), config); | ||
|
||
match &result { | ||
Err(FederationError::SingleFederationError { | ||
inner: error, | ||
trace: _, | ||
}) => match error { | ||
SingleFederationError::UnsupportedFederationVersion { .. } => { | ||
metric_rust_qp_init(Some(UNSUPPORTED_FED1)); | ||
} | ||
SingleFederationError::UnsupportedFeature { message: _, kind } => match kind { | ||
apollo_federation::error::UnsupportedFeatureKind::ProgressiveOverrides => { | ||
metric_rust_qp_init(Some(UNSUPPORTED_OVERRIDES)) | ||
} | ||
apollo_federation::error::UnsupportedFeatureKind::Context => { | ||
metric_rust_qp_init(Some(UNSUPPORTED_CONTEXT)) | ||
} | ||
_ => metric_rust_qp_init(Some(INTERNAL_INIT_ERROR)), | ||
}, | ||
_ => { | ||
metric_rust_qp_init(Some(INTERNAL_INIT_ERROR)); | ||
} | ||
}, | ||
Err(_) => metric_rust_qp_init(Some(INTERNAL_INIT_ERROR)), | ||
Ok(_) => metric_rust_qp_init(None), | ||
} | ||
|
||
Ok(Arc::new(result.map_err(ServiceBuildError::QpInitError)?)) | ||
} | ||
|
||
async fn js( | ||
|
@@ -975,6 +1001,25 @@ pub(crate) fn metric_query_planning_plan_duration(planner: &'static str, start: | |
); | ||
} | ||
|
||
pub(crate) fn metric_rust_qp_init(init_error_kind: Option<&'static str>) { | ||
if let Some(init_error_kind) = init_error_kind { | ||
u64_counter!( | ||
"apollo.router.lifecycle.query_planner.init", | ||
"Rust query planner initialization", | ||
1, | ||
"init.error_kind" = init_error_kind, | ||
"init.is_success" = false | ||
); | ||
} else { | ||
u64_counter!( | ||
"apollo.router.lifecycle.query_planner.init", | ||
"Rust query planner initialization", | ||
1, | ||
"init.is_success" = true | ||
); | ||
} | ||
Comment on lines
+1005
to
+1020
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it ok for a metric attribute to be sometimes missing? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think so. but it could maybe be collapsed to a single attribute ( |
||
} | ||
|
||
#[cfg(test)] | ||
mod tests { | ||
use std::fs; | ||
|
@@ -1617,4 +1662,42 @@ mod tests { | |
"planner" = "js" | ||
); | ||
} | ||
|
||
#[test] | ||
fn test_metric_rust_qp_initialization() { | ||
metric_rust_qp_init(None); | ||
assert_counter!( | ||
"apollo.router.lifecycle.query_planner.init", | ||
1, | ||
"init.is_success" = true | ||
); | ||
metric_rust_qp_init(Some(UNSUPPORTED_CONTEXT)); | ||
assert_counter!( | ||
"apollo.router.lifecycle.query_planner.init", | ||
1, | ||
"init.error_kind" = "context", | ||
"init.is_success" = false | ||
); | ||
metric_rust_qp_init(Some(UNSUPPORTED_OVERRIDES)); | ||
assert_counter!( | ||
"apollo.router.lifecycle.query_planner.init", | ||
1, | ||
"init.error_kind" = "overrides", | ||
"init.is_success" = false | ||
); | ||
metric_rust_qp_init(Some(UNSUPPORTED_FED1)); | ||
assert_counter!( | ||
"apollo.router.lifecycle.query_planner.init", | ||
1, | ||
"init.error_kind" = "fed1", | ||
"init.is_success" = false | ||
); | ||
metric_rust_qp_init(Some(INTERNAL_INIT_ERROR)); | ||
assert_counter!( | ||
"apollo.router.lifecycle.query_planner.init", | ||
1, | ||
"init.error_kind" = "internal", | ||
"init.is_success" = false | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we look up the correct name of the directive on the supergraph's
.metadata()
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On one hand, probably yes. On the other hand, we have enough occurrences of
"join__
in the code base that I doubt anything works if a supregraph uses import renames.Maybe composition supports renames when reading subgraphs be never emits them in a supergraph?