fix(analyzer): align PromQL count(metric) semantics with sketch dispatch - #255
Merged
Merged
Conversation
PromQL `count(metric)` is the spec's distinct-counting idiom — counts
the number of label sets in the result vector — and SHOULD route to
HLL via `Capability::CardinalityApprox`. Three engine-side gaps
prevented this; this PR closes them.
## Gap 1: analyzer over-collects from bare-metric inner
`walk_qe::Expr::VectorSelector` defaulted to wrapping every bare
selector in `Aggregate(Sum)`. Under an outer `count(metric)` (with
`ctx.outer_count: true`), this synthesizes a redundant
`AggIntent::Sum` candidate alongside the intended Cardinality one.
The engine's "all candidates must succeed" loop then bails with
`CapabilityMiss` whenever no Sum policy is registered for the metric
(every HLL-only deploy).
Fix: gate the implicit `Aggregate(Sum)` wrapper on `!ctx.outer_count`.
Bare selector under `count(...)` is just a label-set selector, not a
value to sum — per the PromQL operators spec.
## Gap 2: reducer doesn't recognize `count` as a cardinality function
`SketchReducer::function_to_family` accepted `distinct_over_time` /
`count_distinct_over_time` / `cardinality_estimate` /
`count_distinct` for the Cardinality family but rejected plain
`count`. The candidate's `function` field is the raw PromQL function
name string ("count"), so the reducer returned
`UnsupportedFunction("count")` even when everything else matched.
Fix: add `"count"` to the cardinality alias list in
`function_to_family`.
## Diagnostic test added
`asap_tier_analysis::tests::analyze_count_bare_metric_yields_only_cardinality_candidate`
pins the fixed semantic — `count(metric)` produces exactly one
`CardinalityApprox` candidate with no spurious `ExactAgg(Sum)`
sibling. Future regressions in either gap fail this test loudly.
## Test 5 (HLL e2e roundtrip): analyzer + reducer now work; serialization gap remains
With the fixes above, Test 5 now goes the full distance through the
ASAP-tier engine: streaming-config registers, OTLP DP lands in
`SketchStore`, both sids share the right `policy_fp`, reducer
`evaluate` returns `Ok(...)`. **But the HTTP response body comes back
empty** (`reqwest::Error: EOF while parsing a value`). There's a
separate response-serialization bug in the instant-vector cardinality
response path. Test 5 stays `#[ignore]`'d with the precise gap noted
in its doc-comment so the next investigator knows exactly where to
pick it up.
Test 5 also caught two test-side issues fixed here:
- OTLP DP precision must match what the controller plans
(`HLLDefaults`); a mismatch makes the OTLP-side `policy_fp`
resolve to UNSET and the sid stays orphaned from the policy.
- DP `start_time_unix_nano` must be near `time_unix_nano` (not
Unix epoch 0) so the stored window falls within the query's
PromQL lookback range.
## Tests
- `cargo test --lib -p control_plane`: **691 passed; 0 failed** (+1 new diagnostic).
- `cargo test --test e2e_controller_plans_and_backend_serves`:
**4 passed; 0 failed; 1 ignored** (Test 5).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
zzylol
added a commit
that referenced
this pull request
May 16, 2026
…sult (#256) Closes the response-serialization gap that #255's PR-body flagged as the last blocker for Test 5 (HLL roundtrip). With #255's analyzer + reducer fixes in place, the ASAP-tier engine path went the full distance — `idx.sids_for_policy(fp)` resolved correctly, `SketchReducer::evaluate` returned `Ok(...)` — but the HTTP response came back empty (`reqwest::Error: EOF while parsing a value`). Root cause: `asap_tier_result_to_query_result` unconditionally wrapped the result in `QueryResult::matrix(...)`. The Prometheus adapter's `format_success_response` requires `resultType: vector` for instant queries (those the analyzer marked `range_seconds == 0`, i.e. no `[range]` selector — `count(metric)`, `quantile(...)`, etc.) and `resultType: matrix` for range queries (`*_over_time(...)[range]`). A Matrix response for an instant request gets rejected with HTTP 500 and an empty body. Fix: thread the analyzer's `range_seconds` through to the result builder. If any candidate is range-shaped, build a Matrix; otherwise project the latest sample per series into an `InstantVectorElement` and wrap as `Vector` (with `now_ms` as the wire timestamp). `InstantVectorElement` doesn't carry a per-element `label_keys_override` today (only `RangeVectorElement` does, for the topk-`item`-key case) — labels render against the query-scoped `KeyByLabelNames` the serializer holds. Correct for cardinality (the only instant-vector consumer today); a future per-element override on `InstantVectorElement` is plumbable when needed. ## Test 5 (HLL e2e roundtrip) now passes strict success The full chain — controller plan → POST /api/v1/streaming-config → OTLP HLL DPs → window close → GET /api/v1/query?query=count(metric) → status: success — works end-to-end. All 5 e2e tests now pass: * Test 1: streaming-config round-trip (DDSketch) * Test 2: grouping plumb (zone label) * Test 3: full roundtrip DDSketch quantile → strict success * Test 4: full roundtrip KLL quantile → strict success * Test 5: full roundtrip HLL cardinality → strict success ## Tests - `cargo test --test e2e_controller_plans_and_backend_serves`: **5 passed; 0 failed; 0 ignored**. - Full sweep: lib 691 + bins 27 + integration tests all green. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
zzylol
added a commit
that referenced
this pull request
May 16, 2026
Closes the FrequencyTopk strict-success path end-to-end through the gateway-less OTLP wire format. Tests 8+9 ingest msgpack-encoded heap envelopes and answer `topk(...)` queries with the heap-bearing variants (CmsWithHeap / CountSketchWithHeap). To make the strict path work, several gaps along the analyzer ↔ ingest ↔ policy match chain are closed: - `sketch_kind_handle_for` (otel.rs): mirror the existing CMS-with-heap auto-detection on the CountSketch branch. A msgpack-encoded CountSketch DP with a non-empty heap promotes the sid to `SketchKindHandle::CountSketchWithHeap`. The wire envelope is shared with CmsWithHeap (the reducer dispatches both through `decode_cms_with_heap_from_msgpack`). - `sketch_config_to_params` (otel.rs): emit canonical `w`/`d` keys to match the controller's `sketch_params_to_json`. Previously emitted `rows`/`cols`, which made `find_policy_by_content`'s content match silently miss every CMS / CountSketch sid (registered with `policy_fp = UNSET`, unreachable through `sids_for_policy`). - `Expr::VectorSelector` (promql.rs): also suppress the implicit `Aggregate(Sum)` wrapper when `ctx.topk.is_some()`. PR #255 had already done this for `outer_count`; topk has the same semantics (the inner is the population to rank, not a value to sum), and without this gate the analyzer emits a spurious ExactAgg(Sum) candidate that bails the engine's "all candidates must succeed" loop before the FrequencyTopk candidate is reached. - `capability_for(&AggIntent::TopK)` (capability.rs): return `FrequencyTopk(Any)` instead of pinning `CmsWithHeap`. The analyzer doesn't know which heap-bearing variant the ingest tier registered, and `handles_compatible_for_topk` already wildcards on `Any` — so this lets either `CmsWithHeap` or `CountSketchWithHeap` answer a `topk(...)` query. - New `AggregationType::CountSketchWithHeap` variant (promql_utilities, control_plane, data_plane) so the streaming- config can register a CountSketch-with-heap policy whose `policy_capability` returns `FrequencyTopk(CountSketchWithHeap)`. Wires through enums.rs (variants + as_str + from_str), the policy_capability bridge, the ingest-side `aggregation_type_for_sketch_handle` mapping, and the accuracy module (CountSketch ε plus heap-retention ε bound). The streaming-config emit gap (controller still writes `aggregationType: "CountMinSketch"` / `"CountSketch"` regardless of the planner's `with_heap` flag) is patched in-test for now — in-place JSON rewrite to the heap-bearing variant. The corresponding controller fix (consult `with_heap` when emitting `aggregationType`) is the natural follow-up. Two adjacent golden tests refreshed for the new analyzer output: `capability_for_topk_returns_frequency_topk_any` (was `_cms_with_heap`) and the engine-side `analyzer_parity_18_query_corpus` GOLDEN string (Q06/Q07 lose the spurious Sum candidate, FrequencyTopk now reports `Any`). 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
PromQL
count(metric)is the spec's distinct-counting idiom — counts the number of label sets in the result vector — and SHOULD route to HLL viaCapability::CardinalityApprox. Three engine-side gaps prevented this. This PR closes the analyzer + reducer ones; the residual response-serialization gap is documented in Test 5's#[ignore]doc.Gap 1: analyzer over-collects from bare-metric inner
walk_qe::Expr::VectorSelectordefaulted to wrapping every bare selector inAggregate(Sum). Undercount(metric)(withctx.outer_count: true), this synthesizes a redundantAggIntent::Sumcandidate alongside the intended Cardinality one. The engine's "all candidates must succeed" loop then bails withCapabilityMisswhenever no Sum policy is registered (every HLL-only deploy).Fix: gate the implicit
Aggregate(Sum)wrapper on!ctx.outer_count. Bare selector undercount(...)is a label-set selector, not a value to sum — per the PromQL operators spec.Gap 2: reducer rejects
countas a function nameSketchReducer::function_to_familyaccepteddistinct_over_time/count_distinct_over_time/cardinality_estimate/count_distinctfor the Cardinality family but rejected plaincount. The candidate'sfunctionfield is the raw PromQL function name, so the reducer returnedUnsupportedFunction("count")even when everything else matched.Fix: add
"count"to the cardinality alias list.Test 5 (HLL e2e roundtrip): analyzer + reducer now work; serialization gap remains
With the fixes above, Test 5 now goes the full distance through the ASAP-tier engine: streaming-config registers, OTLP DP lands in
SketchStore, both sids share the rightpolicy_fp, reducerevaluatereturnsOk(...). But the HTTP response body comes back empty (reqwest::Error: EOF while parsing a value). There's a separate response-serialization bug in the instant-vector cardinality response path. Test 5 stays#[ignore]'d with the precise gap noted in its doc-comment.Test 5 also caught two test-side alignment issues fixed here:
HLLDefaults).start_time_unix_nanomust be neartime_unix_nanoso the stored window falls within the query's PromQL lookback.Diagnostic test
asap_tier_analysis::tests::analyze_count_bare_metric_yields_only_cardinality_candidatepins the fixed semantic. Future regressions fail it loudly.Test plan
cargo test --lib -p control_plane: 691 passed; 0 failed (+1 new diagnostic)cargo test --test e2e_controller_plans_and_backend_serves: 4 passed; 0 failed; 1 ignored (Test 5)🤖 Generated with Claude Code