Skip to content

fix(query): honor counter-function semantics in ExactAgg dispatch (closes #301, #300) - #303

Merged
zzylol merged 1 commit into
mainfrom
fix/counter-fn-semantics-301
May 20, 2026
Merged

zzylol merged 1 commit into
mainfrom
fix/counter-fn-semantics-301

Conversation

@zzylol

@zzylol zzylol commented May 20, 2026

Copy link
Copy Markdown
Contributor

Summary

The asap engine collapsed sum / sum_over_time / increase / rate — four semantically-distinct PromQL counter idioms — onto the same two ExactAgg(Sum) reducer paths, so all instant counter sums returned the same wrong number and rate was systematically under-reported (issue #301; subsumes #300). Post-#299 the agent streams per-window deltas, so the inputs are correct; only the query-side dispatch lost the function distinction.

The 4 layered bugs (all fixed)

  1. Engine dispatch — only rate vs everything-else branched; sum / sum_over_time / increase / instant-sum all hit one path.
  2. Lookback — instant sum used a 5-min default == the [5m] range, so on a <5min producer every idiom captured the same horizon → identical values.
  3. Instant projection (biggest) — the instant branch took samples.last() (the most-recent window's delta) instead of the cumulative-since-storage value PromQL sum(counter) requires.
  4. Rate divisorevaluate_exact_agg_rate divided by the nominal range (300 for [5m]) regardless of actual data coverage, halving the rate when the producer ran < range.

Fix shape

  • 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 sum by (..) (rate(..)); precedence via new set_counter_fn). 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 Σ-of-cumulative-samples — issue asap engine: ExactAgg(Sum) dispatch collapses sum/sum_over_time/increase/rate to same code path (function semantics lost) #301 decision (a)).
  • reducer: evaluate_exact_agg gains an accumulate_windows flag (collapse per-window deltas to one cumulative sample for instant-sum/increase; matrix surface keeps it false). evaluate_exact_agg_rate divides by min(range_seconds, actual_coverage_span_seconds) via new SketchStore::exact_agg_coverage_bounds.

Multinode validation (4 zones, 10000 series @ 100 Hz; asap node2:9091 vs VictoriaMetrics baseline node2:8428), steady state

Query asap baseline rel-err was (#301)
sum by (zone) (rate(...[5m])) 997,413 999,987 0.26% 64%
topk(5, sum by (zone) (rate(...))) 997,413 tracks rate
sum by (zone) (sum_over_time(...[5m])) capability-miss → archive 16.99B refuses (decision a) returned wrong 60M

The four idioms now return distinct values (sum ≠ increase ≠ rate), confirming the dispatch no longer collapses them. Instant sum(counter) is cumulative-since-storage (runtime-dependent; residual gap is producer-runtime / flush-lag, not a dispatch bug). Working queries quantile_over_time(0.99, ...) (n=10000) and max by (zone) (quantile_over_time(...)) (#297) unaffected.

Test plan

  • cargo build -p control_plane -p data_plane clean
  • cargo test -p control_plane --lib → 783 passed
  • cargo test -p data_plane --lib → 755 passed
  • New unit/integration coverage: instant-sum accumulates all windows (not last); increase accumulates without divisor; sum_over_time capability-misses; rate divisor uses actual coverage
  • Multinode rate rel-err 64% → 0.26%
  • (pre-existing, unrelated) controller_plan_to_query_full_roundtrip_hll fails on origin/main too — not touched by this PR

🤖 Generated with Claude Code

…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>
@zzylol
zzylol merged commit 704a7bc into main May 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant