fix(asapedge): value-weighted top-k on the agent-side modified-sketch path (Mode 1) - #499
Merged
Merged
Conversation
… path (Mode 1) The OTLP modified-sketch path (Mode 1) — where the edge agent pre-builds the heap-bearing CountSketch (emit_heap) and ships it via modified OTLP — built its top-k heap by FREQUENCY (+1 per datapoint), not by summed value. PRs #375/#372 fixed value-weighted top-k on the read-side reducer and the raw-input precompute path, but left this agent-side build by frequency: a `topk(metric)` answered by a Mode 1 heap ranked the most FREQUENT item, not the item with the largest total value. The heap-bearing CountSketch observe path (obsKindKeyedItem in warm_sketch.go) fed `Float: 1` per sample. sketchlib-go's CountSketch.UpdateString(key, w) feeds that weight into BOTH the count matrix and the Space-Saving candidate tracker, and the emitted wire heap derives each item's value from the CS matrix estimate (buildWireHeap → EstimateStringCount). So the +1 made the matrix — and thus the heap — a frequency count. Make the heap weight config-driven, mirroring the backend's TopkWeight::Value (default) / Count enum (PR #372): * weight_mode: "" / "value" / "sum" (DEFAULT) — add the datapoint VALUE per item, so the heap ranks by Σ value (top-k <item_label> by total <metric>). * weight_mode: "count" / "frequency" / "freq" — +1 per event (heavy-hitter frequency top-k), explicit opt-in. No sketchlib-go / wire / proto / backend-decode change is needed: UpdateString and the Space-Saving tracker already honour an arbitrary weight, and the backend decodes the heap value verbatim (CountMinSketchWithHeap::from_msgpack) and the read-side reducer already sorts the heap descending by that value (PR #375), so a value-built heap round-trips and ranks by value-sum end to end. weight_mode is validated/normalised at boot (unknown value rejected; rejected on any non-emit_heap family). Tests: an adversarial stream where value-rank and count-rank deliberately disagree asserts the default heap is top-k-by-value (recall 1.0) and `count` mode still ranks by frequency; plus alias/validation coverage. Existing emit_heap tests stay green. 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.
Summary
Closes the remaining gap from PR ProjectASAP/ASAPQuery-backend#372: the OTLP modified-sketch path (Mode 1) — where the edge agent pre-builds the heap-bearing CountSketch (
emit_heap) in sketchlib-go and ships it via modified OTLP — built its top-k heap by frequency (+1 per datapoint), not by summed value.PRs ProjectASAP/ASAPQuery-backend#375 and #372 fixed value-weighted top-k on (a) the read-side reducer and (b) the raw-input precompute ingest path, but left the agent-side build by frequency. So a
topk(metric)answered by a Mode 1 heap ranked the most frequent item, not the item with the largest total value.Root cause
The heap-bearing CountSketch observe path (
obsKindKeyedIteminwarm_sketch.go) fedFloat: 1per sample. sketchlib-go'sCountSketch.UpdateString(key, w)feeds that weight into BOTH the count matrix and the Space-Saving candidate tracker, and the emitted wire heap derives each item's value from the CS matrix estimate (buildWireHeap→EstimateStringCount). The+1therefore made the matrix — and thus the heap — a frequency count.Fix
Make the heap weight config-driven, mirroring the backend's
TopkWeight::Value(default) /Countenum (PR #372):weight_mode: "" | "value" | "sum"(DEFAULT) — add the datapoint VALUE per item, so the heap ranks by Σ value (top-k<item_label>by total<metric>).weight_mode: "count" | "frequency" | "freq"—+1per event (heavy-hitter frequency top-k), explicit opt-in.weight_modeis validated/normalised at boot: unknown value rejected; rejected on any non-emit_heapfamily.Scope: no proto / backend change needed
UpdateStringand the Space-Saving tracker already honour an arbitrary weight, and the backend decodes the heap value verbatim (CountMinSketchWithHeap::from_msgpack) and the read-side reducer already sorts the heap descending by that value (PR #375). So a value-built heap round-trips through the same MSGPACK frame and ranks by value-sum end to end — no sketchlib-go, wire, proto, or backend-decode change required.Tests (all green)
go test ./...inprocessor/asapedgeprocessor:TestWeightMode_DefaultValueWeightedTopK— adversarial stream where value-rank ≠ count-rank (/big: 10×50=500,/mid: 30×5=150,/small: 100×1=100). Default heap ranks/big > /mid > /small(recall@k = 1.0 for k=1..3); the highest-COUNT item (/small) is NOT first.TestWeightMode_CountStillRanksByFrequency—countmode ranks/small > /mid > /big(by occurrence).TestWeightMode_AliasesAndValidation— alias normalisation + unknown-value and misplaced-on-wrong-family rejection.TestEmitHeap_*stay green.🤖 Generated with Claude Code