Skip to content

fix: IncreaseAccumulator implements Statistic::Sum (sum-instant of counters now works) - #109

Merged
zzylol merged 1 commit into
mainfrom
fix/counter-accumulator-implements-sum
May 8, 2026
Merged

zzylol merged 1 commit into
mainfrom
fix/counter-accumulator-implements-sum

Conversation

@zzylol

@zzylol zzylol commented May 8, 2026

Copy link
Copy Markdown
Contributor

Summary

Issue: ProjectASAP/ASAPCollector#46. PR #108 pinned the warm-tier matcher contract for sum by (zone) (http_requests_total) and concluded:

the warm tier ingests counters as IncreaseAccumulator, which does not support Statistic::Sum for the bare-counter spatial sum

This PR closes that data-flow gap.

A failing replay.jsonl line of the form (paraphrased):

{"type":"query","query":"sum by (zone) (http_requests_total)","instant":true,"expected_status":"success","actual_status":"error"}

would, pre-fix, hit EngineError::CapabilityMiss because:

  1. compatible_agg_types(Statistic::Sum) did not include Increase / MultipleIncrease, so capability matching rejected counter-shaped configs.
  2. Even if matching accepted, IncreaseAccumulator::query(Sum, ..) errored with Unsupported statistic.

Approach

Picked the simplest correct fix per the task description's first option: implement Statistic::Sum directly on IncreaseAccumulator.

  • IncreaseAccumulator::query(Sum, ..) returns last_seen_measurement.value — the latest cumulative counter value of that series. This is exactly what Prometheus does for sum(<counter>) instant: per-series take the latest cumulative; the engine's outer sum by aggregation groups + sums those across keys.
  • MultipleIncreaseAccumulator::query already delegates to the inner IncreaseAccumulator, so per-key Sum on the multi-population variant follows automatically.
  • compatible_agg_types(Statistic::Sum) extended with AggregationType::Increase and AggregationType::MultipleIncrease so capability matching accepts counter-shaped configs.
  • No shim needed; no ingest-pipeline change needed.

rate(<counter>[5m]) / increase(<counter>[5m]) are unaffected — those resolve to Statistic::Rate / Statistic::Increase which already worked; the changed match arm only adds a new case for Sum.

New test

#[test]
fn sum_by_zone_instant_over_increase_accumulator_does_not_error() {
    let query = "sum by (zone) (http_requests_total)";
    let east = IncreaseAccumulator::new(Measurement::new(10.0), .., Measurement::new(123.0), ..);
    let west = IncreaseAccumulator::new(Measurement::new(0.0),  .., Measurement::new(45.0),  ..);
    let engine = build_engine_with_window(
        "http_requests_total",
        AggregationType::Increase,
        vec!["zone"],
        vec![
            (Some(vec!["us-east-1".to_string()]), Box::new(east)),
            (Some(vec!["us-west-2".to_string()]), Box::new(west)),
        ],
        query,
    );
    let (_labels, qr) = engine
        .handle_query_promql(query.to_string(), QUERY_TIME_SEC)
        .expect("warm engine must answer instant `sum by (zone) (counter)` against IncreaseAccumulator");
    // assert per-zone Sum equals the series' latest cumulative
    // (us-east-1 → 123.0, us-west-2 → 45.0)
}

Two unit tests inside precompute_operators/{increase_accumulator,multiple_increase_accumulator}.rs that previously asserted Sum errored were updated to assert Sum returns the latest cumulative value, plus a new positive test for each.

Test plan

  • cargo build --release -p query_engine_rust
  • cargo test --release --lib -p query_engine_rust -- sum_by_zone_instant_over_increase_accumulator_does_not_error sum_by_zone_instant_does_not_error test_increase_accumulator_query test_increase_accumulator_sum_is_latest_cumulative_value test_multiple_increase_accumulator_query test_multiple_increase_accumulator_sum_per_key — all 6 pass
  • Full cargo test --release --lib -p query_engine_rust — 914 passed, 33 failed (same 33 pre-existing failures as origin/main; baseline comparison confirms no new regressions)
  • cargo test --release -p asap_types — 38/39 pass; the one failure is avg_finds_sum_and_count, which is a pre-existing flaky test relying on HashMap iteration order (fails sporadically on origin/main too — confirmed via 5x run on stashed clean tree, 1/5 failures)

Honest notes

  • The fix is conservative: per-series Sum returns just last_seen_measurement.value. This matches Prometheus' instant sum(<counter>) semantics. It does not also expose Statistic::Count from IncreaseAccumulator (the trait still errors there), so count(http_requests_total) against an Increase-only config will still capability-miss — out of scope for this PR.
  • The changed compatible_agg_types(Sum) ordering puts Increase / MultipleIncrease last, after the existing Sum / MultipleSum / CountMinSketch. With aggregation_priority's descending-window-size sort, this means a true Sum config wins over a co-deployed Increase config of equal window — preserving today's planner picks for explicitly-planned sum_over_time precomputes.

Refs: ProjectASAP/ASAPCollector#46
Refs: PR #108 (warm-tier replay regression diagnosis)

…unters now works)

Counters are ingested by the warm tier as `IncreaseAccumulator` (and
`MultipleIncreaseAccumulator` for keyed variants). Pre-fix neither
trait `query` answered `Statistic::Sum`, so an instant
`sum by (zone) (http_requests_total)` capability-missed even though
the matcher and OnlySpatial dispatch path were correct (PR #108
regression test pinned the matcher contract; the actual data-flow
gap is here).

Fix:
* `IncreaseAccumulator::query(Sum, ..)` returns
  `last_seen_measurement.value` — the latest cumulative counter
  value of that series, matching Prometheus' `sum(<counter>)`
  instant semantics.
* `MultipleIncreaseAccumulator::query` already delegates to the
  inner `IncreaseAccumulator`, so per-key Sum follows automatically.
* `compatible_agg_types(Statistic::Sum)` now lists `Increase` /
  `MultipleIncrease` so capability matching accepts counter-shaped
  configs.
* Updated the two unit tests that previously asserted Sum errors.
* Added a new warm-tier regression test
  (`sum_by_zone_instant_over_increase_accumulator_does_not_error`)
  that builds a `SimpleEngine` with two `IncreaseAccumulator`
  series under different zone labels and asserts
  `sum by (zone) (http_requests_total)` returns
  `QueryResult::Vector` with the per-zone latest cumulative values.

`rate(<counter>[5m])` and `increase(<counter>[5m])` are unaffected
— those statistics resolve to `Statistic::Rate` / `Statistic::Increase`
which already worked, and the changed match arm only adds a new
case for `Sum`.

Refs: ProjectASAP/ASAPCollector#46
Refs: PR #108 (warm-tier replay regression diagnosis)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@zzylol
zzylol merged commit e29c734 into main May 8, 2026
@zzylol
zzylol deleted the fix/counter-accumulator-implements-sum branch May 9, 2026 18:00
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