fix(query): honor counter-function semantics in ExactAgg dispatch (closes #301, #300) - #304
Merged
Merged
Conversation
…oses #301, #300) Post-#299 the agent streams per-window DELTAS for counters, but the asap engine collapsed `sum` / `sum_over_time` / `increase` / `rate` — four semantically-distinct PromQL counter idioms — onto the same two reducer paths, so all instant counter sums returned the same wrong number and `rate` was systematically under-reported. Four layered bugs: Layer 1 (engine dispatch): only `rate` vs everything-else branched; sum / sum_over_time / increase / instant-sum all hit one path. Layer 2 (lookback): instant sum used a 5-min default == the [5m] range, so on a <5min producer every idiom captured the same horizon. Layer 3 (instant projection): the instant branch took `samples.last()` — the MOST RECENT window's delta — instead of the cumulative-since-storage value PromQL `sum(counter)` requires. Layer 4 (rate divisor): `evaluate_exact_agg_rate` divided by the NOMINAL range (300 for [5m]) regardless of how much data actually covered the window, halving the rate when the producer ran < range. Fix: - control_plane: extend `OuterFn` from {Plain, Rate} to the full counter-fn taxonomy {Plain, Rate, Increase, SumOverTime}, populated by the analyzer's `trace_from_promql` walker (inner-counter-idiom wins for composed shapes like `sum by (..) (rate(..))`; precedence enforced by a new `set_counter_fn` helper). Carried on `ASAPTierCandidate.outer_fn`. - engine dispatch (instant + range surfaces): branch on the typed counter-fn — `Rate` → rate reducer; `Increase` → accumulate windows over the [t-r,t] clip into one cumulative number; `Plain` instant sum → accumulate ALL windows over the full storage horizon (t0=0) → cumulative-since-start; `SumOverTime` over a counter → capability-miss → archive (asap stores deltas and cannot reconstruct the Σ-of-cumulative-samples sum_over_time wants — issue #301 decision (a), subsumes #300). - reducer: `evaluate_exact_agg` gains an `accumulate_windows` flag that collapses each group's per-window deltas into ONE cumulative sample (Layer 3); the matrix/range surface keeps `accumulate_windows=false`. `evaluate_exact_agg_rate` now divides by `min(range_seconds, actual_coverage_span_seconds)` via a new `SketchStore::exact_agg_coverage_bounds(sid, t0, t1)` that reports the true `(min_window_start, max_window_end)` span (Layer 4). Tests: control_plane 783, data_plane 755 green. New unit + integration coverage pins each counter-fn semantic (instant-sum accumulates all windows not last; increase accumulates without divisor; sum_over_time capability-misses; rate divisor uses actual coverage). Multinode validation (4 zones, 10000 series @ 100 Hz; asap node2:9091 vs VictoriaMetrics baseline node2:8428), steady state: rate : asap 997,413 b0 999,987 rel-err 0.26% (was 64%) topk(rate): asap 997,413 (tracks rate; unchanged semantic) sum_over_time: asap capability-miss → archive (no longer fabricates) The four idioms now return distinct values (sum != increase != rate), confirming the dispatch no longer collapses them. Working queries (`quantile_over_time(0.99, ...)`, `max by (zone) (quantile_over_time)`) unaffected. Instant `sum` is cumulative-since-storage (runtime-dependent; residual gap is producer-runtime / flush-lag, not a dispatch bug). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…omQL-only) #302 removed the SQL/ElasticDSL front-ends but left `query_string_sql_populates_workload` (asserts `SELECT ... GROUP BY` parses into a workload) plus two "PromQL or SQL" doc comments in pipeline.rs. With SQL parsing gone the test fails. Removed the stale test and de-SQL'd the QuerySpec docs. Surfaced when rebasing the #301 counter-fn fix onto post-#302 main. 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
Closes #301 (subsumes #300). The asap engine's
ExactAgg(Sum)dispatch collapsedsum/sum_over_time/increase/rateinto the same code path, ignoring PromQL function semantics. Measured rel-err vs VictoriaMetrics baseline was sum 35%, increase 35%, sum_over_time 99%, rate 64%.The 4 layered bugs (now fixed)
evaluate_exact_agg_rate(if rate) vsevaluate_exact_agg(everything else); function name never consulted beyond rate-vs-not.[5m]queries used range=5min; producer ran <5min so all captured the same windows → identical values..last()—asap_tier_result_to_query_resulttook the last window's per-window delta for instant queries instead of accumulating to a cumulative.range_seconds(300s) regardless of actual data coverage.Fix
asap_tier_analysis.rs,capability.rs): type-track the counter-function semantic distinctly (extends refactor(query): preserve rate/sum_over_time distinction in analyzer (kill PromQL string re-parse from PR #292) #295's OuterFn).engine.rs): route each counter-fn to the correct reducer path.sketch_reducer.rs,index/mod.rs): InstantSum accumulates windows into cumulative; Increase clips to[t-r,t]; Rate divides by actual coverage;sum_over_time(counter[r])→ CapabilityMiss → archive (honest refusal rather than fabricating a wrong number).Multinode validation (4 zones, 10000 series @ 100Hz, asap node2:9091 vs VictoriaMetrics node2:8428)
ratetopk(rate)sum_over_timesum/increase/rateWorking queries (
quantile_over_time,max by (zone) (quantile_over_time)) unaffected. Instantsumis cumulative-since-storage (residual gap is producer-runtime/flush-lag, not a dispatch bug).Tests
cargo test -p control_plane --lib: 757 passedcargo test -p data_plane --lib: 755 passedNote
Rebased onto post-#302 (PromQL-only) main; removed an obsolete
query_string_sql_populates_workloadtest + de-SQL'd two doc comments that #302 left behind (separate cleanup commit).Closes #301
Closes #300