fix(precompute): value-weighted top-k on the raw-input warm path - #372
Merged
Merged
Conversation
The raw-input top-k path returned recall 0 for value-weighted topk:
`CountMinSketchWithHeap` / `CountSketchWithHeap` policies routed to the
heap-LESS `CmsAccumulatorUpdater`, so no top-k heap was ever built and
`topk(...)` could not enumerate top-k keys at all.
Add a real `CmsHeapAccumulatorUpdater` (wraps `CountMinSketchWithHeap`,
which carries a size-k heap) and route both `*WithHeap` raw-input policies
to it. It keys by the configured group-by `aggregated_labels` (e.g. `host`,
not the metric label `item` — keying already flowed through
`extract_aggregated_key_from_series`) and accumulates a quantity selected
by an explicit `TopkWeight`:
* Value (DEFAULT): Σ datapoint value per key — answers
"top-k <host> by total <metric>".
* Count: +1 per event per key — genuine frequency / heavy-hitter top-k.
Mode is config-driven via `parameters.weight_mode` ("value"/"sum" vs
"count"/"frequency"/"freq"), defaulting to value-weighted. The previous
heap-less routing could not answer top-k, so there is no count-weighted
heap caller to regress; frequency-top-k callers opt in explicitly.
Tests (all green): value-weighted top-k returns the correct hosts by Σ
value (recall 1.0 on a toy multi-host stream where value-rank and
count-rank deliberately disagree); count mode still ranks by occurrence
frequency; CountSketchWithHeap shares the value-weighted heap path;
weight_mode parsing.
Scope: covers the raw-input precompute ingest path. The OTLP
modified-sketch path (agent pre-builds the heap in sketchlib-go) is
unchanged — making that path value-weighted needs an agent/SDK-side change
and is out of scope here.
Co-Authored-By: Claude Opus 4.8 (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.
Fixes the value-weighted top-k defect surfaced by the multi-sketch eval (CountSketch/CMS top-k returned recall 0 for value-ranked top-k).
Root cause
On the raw-input precompute warm path (
data_plane/src/precompute_engine/accumulator_factory.rs), theCountMinSketchWithHeap/CountSketchWithHeappolicies routed to a heap-less plain-CMS updater. So:sketch_reducer.rs,FrequencyTopkarm) had nothing to enumerate → recall 0.worker.rs::extract_aggregated_key_from_seriesuses the policy'saggregated_labels, e.g.host); the "keys byitem" symptom was a cosmetic hardcoded output label + the wrong upstream group-by, not a structural limit.Fix
Added
CmsHeapAccumulatorUpdater(wraps the real size-k heap accumulator) and routed both*WithHeapraw-input policies to it. It:aggregated_labels(flows through unchanged),TopkWeightenum: Value (Σ datapoint value per key — the default) or Count (frequency top-k),parameters.weight_mode:value/sumvscount/frequency). The old heap-less route could not answer top-k at all, so there is no count-weighted caller to regress; frequency-top-k callers opt in explicitly.Tests (real
cargo test)Toy stream where value-rank and count-rank disagree (a: 1×100, b: 2×30, c: 4×5):
value_weighted_topk_ranks_hosts_by_sum_of_value→ a(100)>b(60)>c(20), top-2 recall 1.0count_weighted_topk_still_ranks_by_occurrence_frequency→ c(4)>b(2)>a(1)countsketch_with_heap_also_routes_to_value_weighted_heap,topk_weight_param_parses_modesBroader:
precompute_engine::237 passed,sketch_db::query74 passed.Scope (honest)
Fully fixed: the raw-input precompute ingest path (Mode 2, backend builds the sketch). Deferred: the OTLP modified-sketch path (Mode 1, agent pre-builds the heap in sketchlib-go) — value-weighting that needs an agent/SDK-side change, out of scope here. Orthogonal to #371 (bind-rule selection; this is the ingest/accumulator + reducer path).
🤖 Generated with Claude Code