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 5 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 optimizing plan option exploration", | ||
| "{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( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -195,6 +195,7 @@ | |
| - `apollo.router.query_planning.total.duration` - Histogram of plan durations including queue time. | ||
| - `apollo.router.query_planning.plan.evaluated_plans` - Histogram of the number of evaluated query plans. | ||
| - `apollo.router.query_planning.plan.evaluated_paths` - Histogram of the number of paths (including intermediate ones) the planner considers before generating a plan. High values often correlate with long planning times on complex schemas or queries. Tune the limits as described in [Tuning query planner limits](/graphos/routing/query-planning/query-planning-best-practices#tuning-query-planner-limits). | ||
| - `apollo.router.query_planning.plan.non_local_selections` - Histogram of the number non-local selections estimated during query planning traversal. This is used to optimize option exploration when planning an operation. | ||
|
Check notice on line 198 in docs/source/routing/observability/router-telemetry-otel/enabling-telemetry/standard-instruments.mdx
|
||
|
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. “Non-local selections” and “option exploration” don't tell an operator what this metric is useful for. Explain its relationship to the planning safety limit instead:
Update the instrument description to match. This also makes the limitation clear: the histogram cannot show how far a rejected query exceeded the limit.
Member
Author
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. Your suggestion makes it less clear what this metric does. But let me see if I can word what I already have slightly better. |
||
| - `apollo.router.query_planner.memory` - Histogram of memory allocated during query planning, in bytes. Tracks memory allocation patterns specifically for query planning operations executed in the compute job thread pool. Attributes: | ||
| - `allocation.type`: The type of memory operation (`allocated`, `deallocated`, `zeroed`, `reallocated`) | ||
| - `context`: The context name where the allocation occurred (e.g., `query_planning`) | ||
|
|
||
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.