fix(precompute): restore HLL delta-vs-full invariant under sparse representation - #474
Merged
Merged
Conversation
…resentation The sketchlib-go #66 merge (in-memory sparse HLL with dense promotion) changed the full HLL wire snapshot to be representation-adaptive: a low/ medium-cardinality sketch now serializes to a small wire-sparse frame, and only a high-cardinality sketch reaches the dense register array (~16.5KB). TestHLLWrapper_DeltaNeverLargerThanFull's second sub-case asserted that a near-empty base + medium increment (base=1, +400) must clamp to a FULL frame (isFull==true). That assumption is now stale: the same-state full snapshot is itself sparse (957B) and the register delta (805B) is genuinely SMALLER, so ComputeDeltaAgainst correctly KEEPS the delta. The production min(full,delta) clamp is intact — the real invariant "payload never larger than the same-state full snapshot" holds in every regime (verified: 20000+50 -> 88<=16532; 1+400 -> 805<=957; 0+50000 -> 16532==16532, clamp fires). This is a stale-test fix (B), not a production bug. The test now asserts the size invariant directly against the ACTUAL same-state full snapshot (never a hardcoded dense constant), and exercises BOTH clamp regimes: a small/medium increment keeps a strictly-smaller delta, and a high-cardinality-from-empty delta grows until it reaches the dense full size and clamps to full. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zzylol
force-pushed
the
fix/hll-sparse-delta-invariant
branch
from
May 31, 2026 18:17
115ec23 to
28627ce
Compare
zzylol
marked this pull request as ready for review
June 1, 2026 13:21
zzylol
added a commit
that referenced
this pull request
Jun 1, 2026
…ssor Complete the processor side of the opt-in sparse HLL feature (engine side landed in the preceding commit). Lives in the contrib-patch overlay (opentelemetry-collector-contrib-patch/processor/asapedgeprocessor), the source of truth committed in this private repo; restore_otel_collector_contrib_patches.sh stages it onto the upstream-pinned submodule for build/test. - config.go: add `HLLSparse bool` (mapstructure "hll_sparse") to MetricFamily. Default false (dense). Opt-in selects the sparse in-memory HLL base for an HLL family so low-cardinality warm series avoid the dense ~16KB/series register array; serialized output is byte-identical to dense (pure in-memory footprint win, no wire change). Only consulted for family=hll. - config_validate.go: reject hll_sparse on any non-HLL family at boot (mirrors the emit_heap family guard) so a misconfiguration surfaces early. - warm_sketch.go: in the FamilyHLL branch, build sketches.NewHLLWrapperSparse() when fam.HLLSparse, else the dense NewHLLWrapper(). The constructor is the source of truth for base selection; the choice is also surfaced as the documented HLL "sparse" SketchParams key (1=sparse, absent=dense) on the emitted PrecomputeConfig for introspection. - hll_sparse_test.go: assert default => dense (SketchParams[sparse] absent), hll_sparse=true => SketchParams[sparse]=1, both factories build a usable *sketches.HLLWrapper, plus config round-trip + the non-HLL family guard. Remove the now-obsolete asapedge-hll-sparse-wiring/README.md placeholder (it captured the pending fragments; the wiring is now committed). Rebased onto merged #471 (dual-mode aggregation) and #474 (HLL delta invariant). The sketchlib-go NewSparseHyperLogLog dependency is merged to main and resolves via the existing local replace; no pin. Build/test: asap-precompute-go `go test ./... && go vet ./...` pass; the asapedgeprocessor module builds, vets, gofmts clean, and `go test ./...` passes (incl. the new sparse tests and #471's whole_stream tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Jun 1, 2026
) * feat(precompute): opt-in sparse in-memory HLL for warm aggregators Add an opt-in sparse base for the HLL precompute wrapper so low-cardinality warm series no longer pay the dense ~16KB/series register-array cost. - sketches/hll.go: add NewHLLWrapperSparse(), backed by hll.NewSparseHyperLogLog() (sketchlib-go #66). A new `sparse` field on HLLWrapper is threaded through the shared newSketch() helper so Reset / Merge / ApplyDelta rebuild a sparse base and a sparse wrapper never reverts to the dense footprint. The wrapper drives the inner sketch only through its public methods and never touches the exported Registers field. Snapshot / Merge / ApplyDelta / Reset / EstimateCardinality are unchanged and remain byte-identical / interoperable with the dense base. - config.go: document the recognized HLL "sparse" SketchParams key. - sketches/hll_sparse_test.go: estimate parity, byte-identical snapshots, dense<->sparse merge interop, Reset, and a low-cardinality heap check. Dependency: sketchlib-go #66 is MERGED to main (fff038b); resolved via the existing `replace => ../../sketchlib-go` directive, so no go.mod change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(asap_edge): wire opt-in sparse in-memory HLL into the edge processor Complete the processor side of the opt-in sparse HLL feature (engine side landed in the preceding commit). Lives in the contrib-patch overlay (opentelemetry-collector-contrib-patch/processor/asapedgeprocessor), the source of truth committed in this private repo; restore_otel_collector_contrib_patches.sh stages it onto the upstream-pinned submodule for build/test. - config.go: add `HLLSparse bool` (mapstructure "hll_sparse") to MetricFamily. Default false (dense). Opt-in selects the sparse in-memory HLL base for an HLL family so low-cardinality warm series avoid the dense ~16KB/series register array; serialized output is byte-identical to dense (pure in-memory footprint win, no wire change). Only consulted for family=hll. - config_validate.go: reject hll_sparse on any non-HLL family at boot (mirrors the emit_heap family guard) so a misconfiguration surfaces early. - warm_sketch.go: in the FamilyHLL branch, build sketches.NewHLLWrapperSparse() when fam.HLLSparse, else the dense NewHLLWrapper(). The constructor is the source of truth for base selection; the choice is also surfaced as the documented HLL "sparse" SketchParams key (1=sparse, absent=dense) on the emitted PrecomputeConfig for introspection. - hll_sparse_test.go: assert default => dense (SketchParams[sparse] absent), hll_sparse=true => SketchParams[sparse]=1, both factories build a usable *sketches.HLLWrapper, plus config round-trip + the non-HLL family guard. Remove the now-obsolete asapedge-hll-sparse-wiring/README.md placeholder (it captured the pending fragments; the wiring is now committed). Rebased onto merged #471 (dual-mode aggregation) and #474 (HLL delta invariant). The sketchlib-go NewSparseHyperLogLog dependency is merged to main and resolves via the existing local replace; no pin. Build/test: asap-precompute-go `go test ./... && go vet ./...` pass; the asapedgeprocessor module builds, vets, gofmts clean, and `go test ./...` passes (incl. the new sparse tests and #471's whole_stream tests). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- 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
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
TestHLLWrapper_DeltaNeverLargerThanFull(inasap-precompute-go/sketches/) began failing on a pristineorigin/maintree after thesketchlib-godependency was bumped to the merged in-memory sparse HLL commit (fff038b, sketchlib-go PR #66, "in-memory sparse representation with dense promotion"). Consumed via the localreplace => ../../sketchlib-godirective.Verdict: (B) — the test's assumption is stale. This is a test-only fix. The production delta/serialization path is correct: the
min(full, delta)clamp inHLLWrapper.ComputeDeltaAgainstis intact and the real bandwidth invariant holds in every regime.Evidence (byte numbers across cardinalities)
The failure is not the size invariant (
len(payload) > len(full)) — that never triggers. It is the second sub-case's stale expectation thatnear-empty-base(base=1, +400) must clamp to a FULL frame (isFull == true).Measured
HLLWrapper.ComputeDeltaAgainstpayload vs the actual same-state full snapshot:Before #66 the "full" frame for a low-cardinality sketch was effectively the dense ~16KB register array, and for the
base=1, +400case the register delta exceeded the serialized full frame, so the clamp fired (isFull=true). After #66 the full snapshot is representation-adaptive: that same state now serializes to a 957-byte wire-sparse frame, while the register delta is 805 bytes — genuinely smaller. SoComputeDeltaAgainstcorrectly keeps the delta (isFull=false) instead of clamping. The test's hardcoded expectation that this specific input forces a full is what broke; the production invariant did not.The genuine invariant — "the delta path never ships more bytes than the same-state full snapshot" — holds everywhere (
delta > full?is always no). The clamp still fires correctly at high cardinality (0, +50000: the delta reaches the dense full size and emits full).Fix
Rewrote
TestHLLWrapper_DeltaNeverLargerThanFull(test only) to:The original intent (the delta path is never a pessimization vs sending full) is preserved, not loosened.
asap-precompute-go/sketches/hll_test.go(test only; no production code).Results
go test ./sketches/ -run HLL— PASS (all 9 HLL tests)go test ./sketches/— okgo vet ./sketches/— cleangofmt -l sketches/hll_test.go— cleanProvenance / stacking
fff038b).🤖 Generated with Claude Code