Repository navigation
Conversation
✅ Docs preview readyThe preview is ready to be viewed. View the preview File Changes 0 new, 2 changed, 0 removedBuild ID: 4f260efb2223832b6935873a URL: https://www.apollographql.com/docs/deploy-preview/4f260efb2223832b6935873a
|
Co-authored-by: Iryna Shestak <shestak.irina@gmail.com>
BrynCooke
left a comment
There was a problem hiding this comment.
Mostly it's the redis key thing that needs fixing.
| } | ||
|
|
||
| pub(crate) fn metric_query_planning_non_local_selections(count: u64) { | ||
| u64_histogram_with_unit!( |
There was a problem hiding this comment.
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.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. |
There was a problem hiding this comment.
“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:
Shows how close successfully planned queries come to a query-planning safety limit. The router estimates the work needed to plan each query and rejects queries whose estimate exceeds the limit (100,000 by default).
Update the instrument description to match. This also makes the limitation clear: the histogram cannot show how far a rejected query exceeded the limit.
There was a problem hiding this comment.
Your suggestion makes it less clear what this metric does. But let me see if I can word what I already have slightly better.
| @@ -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 | |||
There was a problem hiding this comment.
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.
Hmmmm I don't know about your wording 😅 . I feel that mine is clearer.
There was a problem hiding this comment.
I've removed the mention of the env var and just kept the changelog around the metric.
…che is refreshed when env var changes
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.
It is read during query planning config initialisation to make sure that the cache is refreshed when the env var changes. To make that happen,
QueryPlannerConfignow hasmax_non_local_selectionas an additional field.This additionally 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. Given that it is such an internal query planning detail, I can be persuaded to not add this metric also, so let me know what you think!I also considered adding a counter metric for when the limit is exceeded. The issue is that that error (
QueryPlanComplexityExceeded) fires for a bunch of other very internal to query planning limits, so that felt like out of scope for this work.This is still kept as 500 when surfaced to the end user, again due to the internal nature of this functionality.
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
A lot of (if not most) features benefit from built-in observability and
debug-level logs. Please read this guidance on metrics best-practices. ↩Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩