fix(emit): B3-pop + B4-window bundle (B2-Sum deferred) - #282
Merged
Merged
Conversation
…-Sum deferred) Two fixes that all live in control_plane/src/emit/ and adjacent files, bundled to avoid sequential merge churn. The third intended fix (B2 — Sum aggregation alongside sketch) was deferred because the existing workload_store keys by metric name and stores ONE QueryWorkload per metric — the multi-aggregation shape needed for `sum by (zone)` alongside `quantile_over_time` requires either a store-level restructure or a second PhysicalExpr per metric, both substantially bigger than #1+#2. Left as a follow-up; the YAML's existing sum-shaped entries (MVP entries 2/3/4 targeting http_requests_total) still collapse to the last write on a per-metric basis. == Fix #1: B3 population gap == Symptom (smoke test): every emitted `transform/keep_for_<metric>` block had `keep_keys(datapoint.attributes, [])` — strips ALL attrs instead of keeping `grouping_labels`. Sid catalog landed with one sid per metric instead of one per (metric, zone). Root cause: the OpAMP-push path's `metric_to_grouping_labels` WAS being populated from the WorkloadStore (in `main:: emit_bootstrap_typed`, `main::handle_plan`'s typed-stage-split branch, and `replan::Replanner::try_emit_typed_edge_yaml`). But the source `QueryWorkload.group_by_labels` was empty for the canonical MVP query `quantile_over_time(0.99, http_requests_total_latency_ms[30s])` — the PromQL parser only surfaces grouping labels from `by (...)` clauses, and a bare `quantile_over_time` has none. The pre-pop loop in `main.rs` also passed `group_by_labels: vec![]` in the QuerySpec, leaving nothing to merge with the empty parsed value. Fix: add a declarative `grouping_labels: Vec<String>` field to `WorkloadEntry` so the YAML can state the streaming-config grouping contract directly. Thread `entry.grouping_labels` into `QuerySpec.group_by_labels` in the pre-pop loop (main.rs ~276) so `analyzer.analyze()` merges the YAML-declared labels with any PromQL `by` keys into `QueryWorkload.group_by_labels`, which `collect_metric_to_grouping_labels` then drops into `EdgeStageConfig.metric_to_grouping_labels` → the emitter's `keep_keys(datapoint.attributes, [...])` list. Same plumbing in `emit::runtime_tests::populate_store_from_registry` so the existing 5-sketch round-trip test keeps tracking main.rs. Regression coverage (2 new tests in emit/mod.rs): * workload_entry_grouping_labels_round_trip_through_emit_to_keep_keys * workload_entry_grouping_labels_surface_in_emit_keep_keys_list == Fix #2: B4 — controller picks window_duration from query == Symptom: agent's sketch processor's `window_duration` was hardcoded at 300s (5m) for KLL/CMS/etc., 60s for others. The backend's streaming-config `windowSize` likewise drifted from whatever the user wrote in their PromQL `[range]`. Queries with `[30s]` ranges always landed inside an open sketch window and returned NoData. Root cause: the pre-pop QuerySpec hardcoded `time_window: "5m".into()` (main.rs ~278), which the analyzer prefers over the PromQL-parsed value at pipeline.rs:203. The parsed `[30s]` was thrown away. No clamp existed downstream to catch this either, so a `[5m]` workload landed a 300s sketch window that fell outside every sensible replay range. Fix: 1. Pass `time_window: ""` from main.rs and emit/runtime_tests when the entry HAS a `query_string` — lets the analyzer extract the matrix-selector range itself. Falls back to "5m" only when query_string is None (so the analyzer doesn't error at Step 4). 2. Add `clamp_window_secs(Option<u64>) -> Option<u64>` in emit/stage_config.rs with bounds [5, 60]: * Lower 5s — below this the sketch processor mints new windows before it has enough samples for the family's quality bound, and per-flush sid-catalog cardinality explodes. * Upper 60s (= MAX_WINDOW_SECS, the historical default). Above this the user's replay range no longer contains a closed sketch window. 3. Apply the clamp at every emission site so the agent's `window_duration` and the backend's `windowSize` agree exactly (drift de-syncs warm-tier replay): * `build_edge_processor_block` callers in the legacy single-pipeline and 5-sketch routing emit paths (stage_config.rs ~169 and ~1003). * `build_backend_aggregation_json` (stage_config.rs ~1636 — the streaming-config JSON path). * `generate_streaming_config_yaml` (asapquery_backend.rs — the legacy YAML emit path; same clamp so legacy / typed paths agree). * `build_processor_block` in the legacy agent emitter (agent.rs ~138). Regression coverage: * 4 unit tests for `clamp_window_secs` itself (in-range, above-max, below-min, None). * 4 emit-level tests: legacy edge-yaml clamps 300 → 60, legacy edge-yaml preserves 30, 5-sketch routing clamps across all 5 family processors, streaming-config JSON clamps `windowSize`. * 3 legacy `agent.rs` tests covering the same clamp contract (clamps_oversize, clamps_undersize, preserves_inrange). * 1 pre-existing test (`contains_window_duration`) updated to assert the post-clamp 60s value instead of the fixture's pre-clamp 5m. Test plan: * `cargo test -p control_plane --lib`: 719 pass (was 706 pre-change baseline; +13 new tests covering both fixes) * `cargo test -p data_plane --lib`: 712 pass (no data_plane changes — verification only) * `cargo check -p control_plane`: clean Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced May 18, 2026
zzylol
added a commit
that referenced
this pull request
May 18, 2026
…ti-role metrics (B2 full restructure) (#283) Closes the B2 thread (Sum-as-separate-aggregation alongside sketch). PR #282's bundle deferred this with note "needs (metric, role)-keyed restructure"; the B2-planning-agent's analysis concluded the restructure is the architecturally clean solution despite its size (vs the side-table alternative). What changed: * New AggRole enum (Quantile / Sum / Count / Topk / Other) in control_plane/src/workload.rs + derive_agg_role() classifier that resolves from (a) sketch_family_override (DDSketch/KLL → Quantile, HLL → Count, CountSketch → Topk, CMS → Other) then (b) outermost PromQL token of query_string. * WorkloadStore keyed by (metric: String, role: AggRole) — append-not-collapse on duplicate metric inserts. New `get_all_for_metric` + `keys()` accessors for emit-path walks. * PlanStore keyed by (metric, role) — one CollectionPlan per pair; rollback/diff/expired now per-pair. `metrics()` dedups across roles to preserve the metrics-exposer wire shape. * agent_to_metric → agent_to_metrics: Vec<(metric, role)> in replan.rs so a single agent can serve multiple (metric, role) plans. register_agent / unregister_agent updated; push_config_to_agent picks the first pair (the 5-sketch routing-connector edge YAML carries every metric's pipeline anyway, so one push covers them all). * Replanner::replan_metric stays as a wrapper that loops over every role registered for the metric; new replan_metric_role(metric, role) for targeted single-role replans. replan_expired loops over expired (metric, role) pairs; handle_violation re-plans every pair the agent serves. * main.rs pre-pop loop now derives the role per WorkloadEntry via derive_agg_role() before set — the three http_requests_total entries in mvp-workload.yaml (sum / sum+ rate / count) now persist as TWO distinct keys (http_requests_total, Sum) + (http_requests_total, Count) instead of collapsing onto one with only the last entry's plan surviving. * handle_plan derives the role from the QuerySpec's query_string + optional sketch_type override and threads it into store.set / workload_store.set / register_agent. * HTTP endpoints (/api/v1/plan/:metric, /diff, /rollback, /config) preserved at metric granularity; responses extended with a per-role `roles: [...]` array (additive, no breaking URL change). Rollback returns BAD_REQUEST when no role has a previous plan (matches pre-B2 single-role behaviour). * Emit helpers (collect_metric_to_family, collect_metric_to_grouping_labels) walk the new (metric, role) keyed store and take the FIRST sketch-binding role per metric for the routing-connector OTTL conditions — Sum-shaped roles correctly decline bind_workload_typed and flow through the metrics/raw_passthrough pipeline. * Bootstrap typed path (emit_bootstrap_typed) walks every role's workload until one binds, instead of the prior single-role lookup. * metrics_exposer's refresh_plan_ids iterates per-pair so each (metric, role) emits its own asap_active_plan_id row under the metric label. Role folded into the plan_id hash so distinct-role plans produce distinct ids. Test plan: * cargo test -p control_plane --lib: 740 pass (was 719; +21 new tests covering AggRole classification, WorkloadStore multi-role coexistence, PlanStore role-keyed rollback/diff/expired, Replanner replan_metric_role + multi-role agent registration + violation handling, and the synthetic http_requests_total 3-entry regression test). * cargo test -p control_plane --bins: 27 pass (unchanged). * cargo test -p data_plane --lib: 712 pass (no data_plane changes — backend's ingest_precompute_for_agg_config already handles AggKind::ExactAgg(Sum)). Smoke test (NOT run by this commit): The mvp-smoke-test/run_smoke.sh end-to-end check requires docker compose + the full ASAP stack + a backend-streaming.yaml that includes a Sum-shaped aggregationType entry for http_requests_total. The static backend-streaming.yaml in deploy/configs/ today only carries DDSketch entries — that's a deployment-config follow-up, NOT a controller change. Without the static YAML update the data_plane has no Sum capability for http_requests_total at startup and the `sum by (zone)` PromQL still surfaces capability-miss until the dynamic streaming-config POST path (Phase 5 controller → backend hot-reload) plumbs through to the live backend. Ambiguous-case decisions (documented inline): * count_over_time(...) classifies to Count (cardinality semantics). count(over_time(sum_over_time(...))) shape would be Sum via the outer aggregation. * rate / increase classify to Sum (both bind to ExactAgg(Sum) on the data plane). * CountMinSketch override classifies to Other (frequency estimator without an inherent topk shape — keep Topk pure for sketch-with-heap families). Closes B2. 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
Two control-plane follow-ups bundled into one PR (third intended fix deferred — see below).
transform/keep_for_<metric>blocks had empty keep_keys lists, stripping ALL wire attrs instead of preserving the streaming-config's grouping labels. Root cause: the canonical MVP queryquantile_over_time(0.99, http_requests_total_latency_ms[30s])carries no PromQLby (...)clause, and the pre-pop loop in main.rs passedgroup_by_labels: vec![]in the QuerySpec, soQueryWorkload.group_by_labelsended up empty. Fix: add a declarativegrouping_labels: Vec<String>field toWorkloadEntryand thread it through the analyzer into the typed emit path.window_durationwas hardcoded at 300s / 60s independent of the workload's[range]. Root cause: the pre-pop QuerySpec hardcodedtime_window: \"5m\", overriding the parser. Fix: drivetime_windowfrom the parsed PromQL whenquery_stringis present, and add aclamp_window_secshelper in emit/stage_config.rs with[5, 60]bounds applied at every emission site (sketch processor'swindow_durationAND streaming-config'swindowSize) so the agent and backend keep the same window.Deferred
WorkloadStorekeys by metric name and stores ONEQueryWorkloadper metric — the multi-aggregation shape needed forsum by (zone)alongsidequantile_over_timeon the same metric requires either a store-level restructure or a second PhysicalExpr per metric, both substantially larger than feat: PrecomputeJob execution endpoint for DataCollector integration #1+feat: DataCollector controller client — dynamic query config #2. Left as a follow-up.Test plan
cargo test -p control_plane --lib: 719 pass (706 baseline + 13 new: 2 Fix-feat: PrecomputeJob execution endpoint for DataCollector integration #1 regression tests + 8 Fix-feat: DataCollector controller client — dynamic query config #2 emit/clamp tests + 3 legacy-agent clamp tests)cargo test -p data_plane --lib: 712 pass (no data_plane changes, verification only)cargo check -p control_plane: clean