fix(kll,cms): use proto decoder on ingest to match emit wire format - #222
Merged
Merged
Conversation
KLL and CMS processors emit proto-encoded sketches but were calling the gob decoder on inbound. Production traffic is proto, so inbound merges failed silently. Match emit format on both ends; round-trip test added. 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
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
kllprocessorandcountminsketchprocessoremit proto-encoded sketches (SerializePortable()+proto.Marshalfor KLL,SerializeProtoBytesFO()for CMS) but their inbound paths were calling the gob-format decoders (DeserializeKLLSketchFromBytes,DeserializeCountMinSketchFromBytes).KLLSketch/CountMinSketchdata points uses the proto envelope. The wire-format mismatch made every inbound pre-aggregated sketch fail to deserialize and silently drop on merge.kll.DeserializeKLLSketchFromProtoBytesandcms.DeserializeCountMinSketchFromProtoBytes— symbols that already exist insketchlib-go(verified at/home/.../sketchlib-go/sketches/KLL/portable.go:58and.../CountMinSketch/portable.go:111).Why this surfaced now
PR #220 (3b05c93) audited the processor sink/wire-format expectations end-to-end as part of unbreaking the post-DataDog→sketchlib-go fixtures. While reviewing the existing
TestBatchModeTransmitSketch(which already correctly decodes the emit payload viaDeserializeKLLSketchFromProtoBytes), the asymmetry on the ingest path stood out: the emit and ingest sides of the same processor disagreed on the wire format.TestProcessor_TumblingWindow_Correctnessin CMS shows the same pattern — emit decoded withDeserializeCountMinSketchFromProtoBytes, butdeserializeCMS(the function used on ingest) calls the gob path.What the round-trip test proves
Added
TestRoundTripIngestProtoSketchin both packages. Each test:serializeKLLSketch/serializeCMS) — i.e. exactly the bytes a peer collector would put on the wire.KLLSketch/CountMinSketchinput data point.Count()(KLL) / per-hash frequency estimates (CMS) as the source.Pre-fix, both new tests fail (KLL: no output emitted because deserialize errored before merge; CMS:
gob: duplicate type receivedwhile trying to gob-decode proto bytes). Post-fix both pass, exercising the full ingest → merge → emit → decode round-trip on the production wire format.No emit-side behavior change
The diff is one-line per processor on the ingest path only:
kllprocessor/processor.golines 208 and 462:DeserializeKLLSketchFromBytes→DeserializeKLLSketchFromProtoBytes.countminsketchprocessor/processor.goline 693 (deserializeCMS): same swap.serializeKLLSketch,serializeCMS,appendTypedKLLSketchDataPoint,buildWindowMetricsAndReset,cloneCMS, the delta-encoding ingest branch, and every other emit-related callsite are untouched. Wire format on the emit side is unchanged; downstream consumers see byte-identical output.Test plan
cd opentelemetry-collector-contrib-patch/processor/kllprocessor && go test ./...— passes (includes newTestRoundTripIngestProtoSketch).cd opentelemetry-collector-contrib-patch/processor/countminsketchprocessor && go test ./...— passes (includes newTestRoundTripIngestProtoSketch).🤖 Generated with Claude Code