fix(ingest): decode envelope-wrapped CountMinState from DC processor - #14
Merged
Merged
Conversation
DataCollector's countminsketchprocessor serializes via sketchlib-go's SerializePortableFO(), which wraps the CountMinState in a SketchEnvelope before proto.Marshal. The Rust decoder in CountMinSketchAccumulator::from_sketchlib_proto_bytes tried to decode the wire bytes as a bare CountMinState, producing "invalid wire type" errors on field `cols` and silently falling through to the §5.2 fallback. Every real-world sketch data point was decode-failing. Try SketchEnvelope first, fall back to bare CountMinState for callers (existing unit tests) that encode the state directly. Add two regression tests: one for the envelope-wrapped happy path, and one asserting a clear error when the envelope carries a non-CountMin sketch type. Caught by live Stage A bring-up of backend + sketchcollector. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
zzylol
added a commit
that referenced
this pull request
Apr 17, 2026
…Sketch PR #14 fixed `CountMinSketchAccumulator::from_sketchlib_proto_bytes` to try `SketchEnvelope::decode` first and fall back to bare `CountMinState`. DataCollector's sketchlib-go processors wrap every sketch state in a `SketchEnvelope` via `SerializePortableFO` + `proto.Marshal`, so the bare-proto decoder was producing "invalid wire type" errors and silently falling through to §5.2 for every real-world sketch data point. The same bug existed for the other four sketch accumulators. This PR applies the envelope-first-then-bare-fallback pattern verbatim to: * `CountSketchAccumulator::from_sketchlib_proto_bytes` * `DatasketchesKLLAccumulator::from_sketchlib_proto_bytes` * `HllSketchAccumulator::from_sketchlib_proto_bytes` * `DDSketchAccumulator::from_sketchlib_proto_bytes` On envelope decode success, dispatch on the oneof variant and extract the expected state. On non-matching variant, return a clear `SketchEnvelope contains non-<Name> sketch` error instead of silently producing garbage. On envelope decode failure or empty oneof, fall back to bare proto decode so unit tests (which encode states directly without the envelope) keep working. Tested end-to-end via 8 new unit tests: two per accumulator (envelope-wrapped happy path + wrong-sketch-type rejection). All 10 envelope tests pass (8 new + 2 pre-existing CMS), 578 lib tests total (up from 570); clippy + fmt clean. Before the fix, wiring any of these four processors in a real deployment would fail with a decode error on every data point. The CMS fix shipped in PR #14; this PR completes the pattern across the remaining sketch types. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
zzylol
added a commit
that referenced
this pull request
Apr 17, 2026
…Sketch (#26) PR #14 fixed `CountMinSketchAccumulator::from_sketchlib_proto_bytes` to try `SketchEnvelope::decode` first and fall back to bare `CountMinState`. DataCollector's sketchlib-go processors wrap every sketch state in a `SketchEnvelope` via `SerializePortableFO` + `proto.Marshal`, so the bare-proto decoder was producing "invalid wire type" errors and silently falling through to §5.2 for every real-world sketch data point. The same bug existed for the other four sketch accumulators. This PR applies the envelope-first-then-bare-fallback pattern verbatim to: * `CountSketchAccumulator::from_sketchlib_proto_bytes` * `DatasketchesKLLAccumulator::from_sketchlib_proto_bytes` * `HllSketchAccumulator::from_sketchlib_proto_bytes` * `DDSketchAccumulator::from_sketchlib_proto_bytes` On envelope decode success, dispatch on the oneof variant and extract the expected state. On non-matching variant, return a clear `SketchEnvelope contains non-<Name> sketch` error instead of silently producing garbage. On envelope decode failure or empty oneof, fall back to bare proto decode so unit tests (which encode states directly without the envelope) keep working. Tested end-to-end via 8 new unit tests: two per accumulator (envelope-wrapped happy path + wrong-sketch-type rejection). All 10 envelope tests pass (8 new + 2 pre-existing CMS), 578 lib tests total (up from 570); clippy + fmt clean. Before the fix, wiring any of these four processors in a real deployment would fail with a decode error on every data point. The CMS fix shipped in PR #14; this PR completes the pattern across the remaining sketch types. Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
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
DataCollector's
countminsketchprocessorserializes via sketchlib-go'sSerializePortableFO(), which wraps theCountMinStatein aSketchEnvelopebeforeproto.Marshal. The Rust decoder inCountMinSketchAccumulator::from_sketchlib_proto_bytesassumed bareCountMinStateand producedinvalid wire type: LengthDelimited (expected Varint)on fieldcols, silently falling through to the §5.2 fallback. Every real-world sketch data point from DC was decode-failing.Fix
Try
SketchEnvelopefirst, fall back to bareCountMinStatefor callers (existing unit tests) that encode the state directly.Regression tests
test_from_sketchlib_proto_bytes_envelope_wrapped— the happy path matching what DC actually emits.test_from_sketchlib_proto_bytes_envelope_wrong_sketch_type— rejects envelopes with a non-CountMin sketch inside with a clear error.How it was caught
Live Stage A bring-up: patched
sketchcollectorbinary → backend OTLP ingest →route_modified_otlp_sketches_to_precomputepath, with 50 synthetictest_metricpoints. Before the fix every point showed0 routed, 1 decode-failed; after the fix every point shows1 routed, 0 decode-failedandGET /api/v1/store/metricsreports 1 populated aggregation.Follow-up
The same envelope-unwrap pattern likely needs to land in the other four sketch decoders (
KLLSketchAccumulator,HLLSketchAccumulator,CountSketchAccumulator,DDSketchAccumulator), but I'm scoping this PR to CountMin because that's the one exercised by the live stack so far. Tracking the others as a follow-up once the remaining processors go through the same bring-up.Test plan
🤖 Generated with Claude Code