Skip to content

fix(asapedgeprocessor): feed CountMinSketch KindBytes so it records frequencies - #449

Merged
zzylol merged 1 commit into
mainfrom
fix/asapedge-cms-bytes-observe
May 26, 2026
Merged

zzylol merged 1 commit into
mainfrom
fix/asapedge-cms-bytes-observe

Conversation

@zzylol

@zzylol zzylol commented May 26, 2026

Copy link
Copy Markdown
Contributor

What

For family: countminsketch, the fused asap_edge processor recorded nothing: it emitted a CountMinSketch envelope shell every window but never counted any frequencies. This fixes that and adds a regression test that asserts a real count.

Root cause

warm_sketch.go's observe built Value: precompute.FloatValue(val) for every family. But sketches.CMSObserver is the only observer that requires KindBytes (cms.go:263) — it hashes the encoded attribute key to count series cardinality, not the numeric value. So for CMS:

  1. ObserveKeyedCMSObserver.Observe returned "expected KindBytes, got Float" and never called InsertHash.
  2. The series was still admitted (empty sketch), so Drain emitted an envelope with data points.
  3. The observe error was discarded with _ =.

Net: an envelope was emitted while zero frequencies were recorded — and the existing all_families smoke test (asserts DataPointCount() != 0) stayed green, hiding it.

Fix

  • For the CMS family, build the observation value as BytesValue(AttributesKey(labels, nil)) — the full attribute key, matching the standalone countminsketchprocessor shim (shim_helpers.go:58). AggregateBy grouping is applied separately by SeriesKeyFor, so the inserted key stays the full attribute set (nil), preserving query-time parity.
  • Stop swallowing the ObserveKeyed error: retain it on the aggregator and log it once. This silent _ = is exactly what hid the bug.
  • CountSketch is unaffected — its observer takes KindFloat (countsketch.go:211). Only CMS needed KindBytes.

Test

TestCountMinSketchRecordsFrequency drives 40 CMS observations through the fused observe, reconstructs the emitted CMS from the envelope payload (ApplyDelta), and asserts EstimateCount(key) >= 40. It fails without the fix (CMSObserver: expected KindBytes, got Float) and passes with it. go build/vet/test ./... all green.

Based on top of the reorg (#448), so the fix lives in warm_sketch.go.

🤖 Generated with Claude Code

The fused sketch observe handed precompute.FloatValue to every family, but
sketches.CMSObserver only accepts KindBytes (it hashes the encoded attribute
key to count series cardinality, not the numeric value). So for a metric with
family=countminsketch, ObserveKeyed -> CMSObserver.Observe rejected every
sample with "expected KindBytes, got Float". The error was discarded with
`_ =`, and the series was still admitted (empty sketch), so an envelope shell
was emitted while no frequency was ever recorded.

Fix:
- For the CMS family, build the observation value as
  BytesValue(AttributesKey(labels, nil)) — the full attribute key, matching the
  standalone countminsketchprocessor shim. AggregateBy grouping is applied
  separately by SeriesKeyFor, so the inserted key stays the full attribute set.
- Stop swallowing the ObserveKeyed error: retain it on the aggregator and log
  once per aggregator. This is what hid the bug.
- Add TestCountMinSketchRecordsFrequency, which reconstructs the emitted CMS and
  asserts the inserted key's estimated frequency (~N). It fails without the fix
  ("expected KindBytes, got Float") and passes with it.

CountSketch is unaffected (its observer takes KindFloat); only CMS required
KindBytes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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