fix(emit): propagate metric_name/window_secs through L5 walk (DAG visit order) - #249
Merged
Merged
Conversation
…it order) PR #247's belt-and-braces patches (`handle_plan` patching `BackendAggregation.metric_name` / `window_secs` / `grouping` from the workload spec) were load-bearing for `metric_name` and `window_secs`, not no-ops. Root cause was a DAG-visit-order bug in `ThreeStageEmitter::emit_per_stage`: * The colored DAG's node order is depth-first parent-first (`SketchEstimate`, `SketchAgg`, `Logical`) so a SketchAgg appears BEFORE its `Logical(Window{Scan})` child in `dag.nodes`. * Pass 2's SketchAgg arm captures `edge.source_metric` / `edge.window_secs` / `edge.label_filters` *at push time* while building `BackendAggregation`. * But those fields are populated by Pass 2's Logical arm calling `extract_edge_facts(qe, &mut edge)` — which fires LATER in the same loop, after SketchAgg. * Result: every `BackendAggregation` shipped with empty `metric_name` / `window_secs: 0`, which the backend's `AggregationConfig::from_yaml_data` rejects on `Missing metric` / `Missing windowSize`. The Replanner's separate legacy emitter (`generate_streaming_config_yaml`) papered over it by reading directly from `plan.agent_config`; `handle_plan` was papering over it with the workload-spec patch from #247. Fix: add **Pass 0** that pre-walks the DAG looking only for `Logical(qe) @ Edge` nodes and calls `extract_edge_facts` to populate edge facts. Pass 2's Logical arm stays (it's idempotent — only sets on `None` and dedupes label_filters), so the change is safe even if node order changes upstream. Grouping stays empty out of the L5 walk: canonical L3 `QueryExpr::Aggregate.by` is `Vec<ColumnId>` against a synthesized schema with no label columns (Step γ TODO in `intent_algebra::column_resolution`). The workload-spec patch in `handle_plan` remains the source of truth for grouping today. ## Diagnostic tests added `physical::stage_split::l5_walk_propagation_tests` — three characterisation tests that pin the L5 walk's behaviour for `bind_workload_typed` output: 1. `l5_walk_surfaces_metric_name_for_bind_workload_typed_output` 2. `l5_walk_surfaces_window_secs_for_bind_workload_typed_output` 3. `l5_walk_leaves_grouping_empty_pending_step_gamma` The first two now pass (the fix). The third pins the current limitation so a future Step γ fix fails it loudly and signals that the handle_plan grouping patch can be retired alongside. ## Tests - `cargo test --lib -p control_plane`: 690 passed (was 687; +3 new diagnostic tests). - `cargo test --tests --bins -p control_plane`: 27 passed. - `cargo test --test e2e_controller_plans_and_backend_serves`: 3 passed (the e2e suite's metric_name / windowSize assertions now pass via the real fix, not the patch). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 task
zzylol
added a commit
that referenced
this pull request
May 15, 2026
… gap (#250) Investigation following #249 (L5-walk fix) clarified two more ingest-path requirements that were silently dropping the OTLP DPs even after the streaming-config JSON shape was correct: 1. **OTLP DP needs at least one attribute (or known sid).** With both empty, the receiver hits the "invalid wire shape" drop in `route_modified_otlp_sketches_to_precompute` (per the comment block at otel.rs:926 — `(sid=0, no attrs)` is dropped). Test 3 now attaches `service="e2e-test"` to the DP; with this, the resolver mints a sid and the sketch state lands in `SketchStore`. 2. **The DP's `group_by_keys` (from `dp.attrs.keys()`) must EXACTLY match the streaming-config policy's `grouping_labels`** for `find_policy_by_content` to bind `policy_fp` to the registered sid. The match is **strict** at ingest (asap_tier_analysis.rs:525) but **subset** at query time (asap_tier_analysis.rs:587) — that asymmetry is intentional (ingest needs uniqueness; queries can re-aggregate down). Test 3 now uses `group_by_labels: ["service"]` in the workload so the streaming-config grouping aligns with the DP's attrs. After both alignments, the runtime-info endpoint shows the sketch state DID land in `SketchStore` (`earliest_timestamp_per_sid` is populated). The query nevertheless still returns `bad_data` / "No result for query" — there's a remaining gap between `SketchStore::instances_matching` and the engine's reducer dispatch. Untangling that needs deeper engine-path tracing (likely a candidate. required_capability vs policy_capability asymmetry, or a sid-by-policy_fp reverse-index lookup failure). This PR captures the findings as comments and tightens the test's wire-shape requirements so the next investigation starts from a realistic OTLP DP shape rather than the minimal "invalid wire shape" one. The query soft-check (status field exists, value not asserted) remains in place. Tightening to `status == "success"` plus an exact-value-within-SLA check on the returned quantile is what remains for whoever closes the engine-path gap. Tests: 3/3 pass. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
zzylol
added a commit
that referenced
this pull request
May 15, 2026
…itations (#252) Engine-path debug session diagnosis (post-#247 → #248 → #249 → #250). The query layer's "No result for query" symptom for sketch-backed metrics has a precise root cause: `query_precomputes_by_agg` (called unconditionally by `execute_store_query`) filters its candidate-sid scan with `matches!(&m.agg_kind, AggKind::ExactAgg { … })` — never matching `AggKind::Sketch`. Since OTLP-arriving DDSketch / KLL / HLL / CountSketch / CountMinSketch DPs are registered with `AggKind::Sketch { kind, config, .. }`, the lookup always returns an empty map for them, the engine bubbles up "No precomputed outputs found", and `handle_query` returns `None` → HTTP responds `errorType: bad_data` / `error: "No result for query"`. Sketches DO reach `SketchStore` — they're readable via the sid-keyed `query_range(sid, ...)` path which filters on `payload.as_sketch()`. The agg-keyed precompute lookup is the gap. This PR adds doc-comment blocks at both the call site (`engine.rs::execute_store_query`) and the function definition (`mod.rs::query_precomputes_by_agg`) flagging the gap with file:line citations and pointing at the two viable fixes: * teach `query_precomputes_by_agg` to also collect sketch payloads (assemble `Box<dyn AggregateCore>` from `payload.as_sketch()`) — non-trivial; payload shapes diverge. * route the legacy `handle_query` path through the newer `ASAPQueryEngine::execute(&str)` trait dispatcher (around engine.rs:3430) which already handles sketches via `idx.sids_for_policy(fp)` + reducer dispatch. No behaviour change. 690 lib + 27 binary tests pass. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#247's belt-and-braces patches in
handle_plan(patchingBackendAggregation.metric_name/window_secs/groupingfrom the workload spec) were load-bearing for metric_name and window_secs, not no-ops. Found via the diagnostic tests added in this PR. Root cause: DAG-visit-order bug inThreeStageEmitter::emit_per_stage.Root cause
SketchEstimate,SketchAgg,Logical. So SketchAgg appears BEFORE itsLogical(Window{Scan})child indag.nodes.edge.source_metric/edge.window_secs/edge.label_filtersat push time when buildingBackendAggregation.extract_edge_facts— which fires LATER in the same loop, after SketchAgg has already pushed.BackendAggregationshipped with emptymetric_nameandwindow_secs: 0, which the backend'sAggregationConfig::from_yaml_datarejects onMissing metric/Missing windowSize. The Replanner's separate legacy emitter (generate_streaming_config_yaml) papered over it by reading directly fromplan.agent_config;handle_planwas papering over it via test(e2e): controller → backend streaming-config round-trip + grouping plumb #247's workload-spec patch.Fix
Add Pass 0 that pre-walks the DAG looking only for
Logical(qe) @ Edgenodes and callsextract_edge_factsto populate edge facts. Pass 2's Logical arm stays as a safety belt — it's idempotent (only sets onNone, dedupes label_filters), so the fix is safe even if node order changes upstream.Grouping stays empty out of the L5 walk: canonical L3
QueryExpr::Aggregate.byisVec<ColumnId>against a synthesized schema with no label columns (Step γ TODO inintent_algebra::column_resolution). The workload-spec patch inhandle_planremains the source of truth for grouping today — pinned by the third diagnostic test.Diagnostic tests added
physical::stage_split::l5_walk_propagation_tests— three characterisation tests pinning the L5 walk's behaviour forbind_workload_typedoutput. The first two now pass (the fix). The third pins the grouping-empty limitation so a future Step γ fix fails it loudly and signals that thehandle_plangrouping patch can be retired alongside.Test plan
cargo test --lib -p control_plane: 690 passed (was 687; +3 new diagnostic tests)cargo test --tests --bins -p control_plane: 27 passedcargo test --test e2e_controller_plans_and_backend_serves: 3 passed (the e2e suite's metric_name / windowSize assertions now pass via the real fix, not the workload patch)🤖 Generated with Claude Code