Repository navigation
feat(federation): add env var for MAX_NON_LOCAL_SELECTIONS #10349
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
base: dev
Are you sure you want to change the base?
Changes from 4 commits
24f7098
ede23dd
bd5952e
507d311
7c77228
124edbc
46dc665
ca054d3
3d1bd14
9c98942
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 |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| ### Add environment variable for MAX_NON_LOCAL_SELECTIONS | ||
|
|
||
| Adds a environment variable for MAX_NON_LOCAL_SELECTIONS const used in query | ||
| planning traversal. This is an undocumented env var, as it's intended for | ||
| internal use. | ||
|
|
||
| This additional adds a metric for tracking current number of non-local | ||
| selections, which is a histogram available under | ||
| `apollo.router.query_planning.plan.non_local_selections`. | ||
|
|
||
| By [@lrlna](https://github.com/lrlna) in https://github.com/apollographql/router/pull/10349 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -208,6 +208,7 @@ impl QueryPlannerService { | |
| query_plan_root_node: root_node.map(Arc::new), | ||
| evaluated_plan_count: plan.statistics.evaluated_plan_count.clone().into_inner() as u64, | ||
| evaluated_plan_paths: plan.statistics.evaluated_plan_paths.clone().into_inner() as u64, | ||
| non_local_selections_count: plan.statistics.non_local_selections_count, | ||
| }) | ||
| } | ||
|
|
||
|
|
@@ -331,6 +332,7 @@ impl QueryPlannerService { | |
| formatted_query_plan, | ||
| evaluated_plan_count, | ||
| evaluated_plan_paths, | ||
| non_local_selections_count, | ||
| } = plan_result; | ||
|
|
||
| // If the query is filtered, we want to generate the signature using the original query and generate the | ||
|
|
@@ -366,6 +368,9 @@ impl QueryPlannerService { | |
| "Number of paths (including intermediate ones) considered to plan a query before starting to generate a plan", | ||
| evaluated_plan_paths | ||
| ); | ||
| if let Some(non_local_selections_count) = non_local_selections_count { | ||
| metric_query_planning_non_local_selections(non_local_selections_count); | ||
| } | ||
|
|
||
| Ok(QueryPlannerContent::Plan { | ||
| plan: Arc::new(super::QueryPlan { | ||
|
|
@@ -614,6 +619,7 @@ pub(crate) struct QueryPlanResult { | |
| pub(super) query_plan_root_node: Option<Arc<PlanNode>>, | ||
| pub(super) evaluated_plan_count: u64, | ||
| pub(super) evaluated_plan_paths: u64, | ||
| pub(super) non_local_selections_count: Option<u64>, | ||
| } | ||
|
|
||
| /// The outcome of a query-planning attempt. Shared across query-planning metrics (e.g. | ||
|
|
@@ -673,6 +679,15 @@ pub(crate) fn metric_query_planning_plan_duration( | |
| ); | ||
| } | ||
|
|
||
| pub(crate) fn metric_query_planning_non_local_selections(count: u64) { | ||
| u64_histogram_with_unit!( | ||
|
Contributor
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. This histogram inherits the default bucket boundaries, which stop at 10, so estimates near the 100,000 limit all fall into the +Inf bucket. Provide count-appropriate boundaries (including the region around the limit) and test bucket placement; otherwise drop it from this change. |
||
| "apollo.router.query_planning.plan.non_local_selections", | ||
| "Number of non-local selections estimated during query planning traversal, used for optimising plan option exploration", | ||
|
lrlna marked this conversation as resolved.
Outdated
|
||
| "{selection}", | ||
| count | ||
| ); | ||
| } | ||
|
|
||
| pub(crate) fn metric_rust_qp_init(init_error_kind: Option<&'static str>) { | ||
| if let Some(init_error_kind) = init_error_kind { | ||
| u64_counter!( | ||
|
|
@@ -1340,6 +1355,27 @@ mod tests { | |
| .await; | ||
| } | ||
|
|
||
| #[test(tokio::test)] | ||
| async fn test_non_local_selections_histogram() { | ||
| async { | ||
| let _ = plan( | ||
| EXAMPLE_SCHEMA, | ||
| include_str!("testdata/query.graphql"), | ||
| include_str!("testdata/query.graphql"), | ||
| None, | ||
| PlanOptions::default(), | ||
| ) | ||
| .await | ||
| .unwrap(); | ||
|
|
||
| assert_histogram_exists!("apollo.router.query_planning.plan.non_local_selections", u64); | ||
| assert_histogram_count!("apollo.router.query_planning.plan.non_local_selections", 1 as u64); | ||
| assert_histogram_sum!("apollo.router.query_planning.plan.non_local_selections", 7 as u64); | ||
| } | ||
| .with_metrics() | ||
| .await; | ||
| } | ||
|
|
||
| async fn plan_unauthorized_operation(compute_job_type: ComputeJobType) -> QueryPlannerContent { | ||
| let configuration: Arc<Configuration> = Arc::default(); | ||
| let schema = Schema::parse( | ||
|
|
||
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.
This release note exposes an internal constant and advertises an “undocumented” setting without explaining the user benefit. Replace the heading and first paragraph with “Allow support-guided adjustment of query-planning protection” and “Support can adjust a query-planning safety limit for legitimate operations that exceed the default.” If the histogram remains, replace the second paragraph with “Adds
apollo.router.query_planning.plan.non_local_selections, a histogram of estimated selections requiring query-planning exploration for successful, newly generated plans.”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.
Hmmmm I don't know about your wording 😅 . I feel that mine is clearer.
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.
I've removed the mention of the env var and just kept the changelog around the metric.