fix(asap-precompute-go): SnapshotCache always-refresh + extract common sketch wrappers - #232
Merged
zzylol merged 1 commit intoMay 4, 2026
Conversation
…n sketch wrappers Correct SnapshotCache semantics — every ComputeDelta updates the cached snapshot, matching all 5 legacy OTel processors. The previous "refresh-only-on-full" path was a design error; no configurable policy knob is needed because there's only one correct behavior. Extract platform-independent sketch wrappers (DDSketch, KLL, HLL, CountSketch, CountMinSketch) into asap-precompute-go/sketches/ so Telegraf / Vector / OTAP adapters can reuse them. Document delta-snapshot semantics in design doc. Note: PR #229 (CountSketch) and PR #230 (CMS) workaround in shim_helpers.go::applyDeltaTransmission becomes redundant after this lands; those PRs can rebase to drop the workaround. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
zzylol
added a commit
that referenced
this pull request
May 4, 2026
…wrappers (#235) Two cleanups, both enabled by PR #232: 1. CountSketch and CMS shims drop the applyDeltaTransmission workaround (shim-side snapshot cache that mirrored legacy delta-refresh semantics). Runtime's SnapshotCache::ComputeDelta now always-refreshes correctly, so the workaround is redundant. 2. All 5 shims (DDSketch / KLL / HLL / CountSketch / CMS) drop their per-processor sketch_wrapper.go files and import the canonical wrappers from asap-precompute-go/sketches/. Net: each shim shrinks by ~150-300 LoC. Parity harness still all-PASS byte-identical. 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
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
This PR fixes a design bug in
SnapshotCache::ComputeDeltaand extracts the five sketch wrappers into a commonasap-precompute-go/sketches/package so every adapter (OTel, Telegraf, Vector, OTAP) reuses the same production-grade adapters.(a) The design bug
SnapshotCache::ComputeDeltapreviously updated the cached "previous snapshot" only when emitting a full payload (isFull=true). Successive sub-threshold deltas were therefore all computed against the same stale baseline — forcing downstream consumers to merge a chain of overlapping deltas back to the original snapshot rather than apply each delta to the previous window's reconstructed state.The five legacy OTel sketch processors (DDSketch / KLL / HLL / CountSketch / CountMinSketch) all use always-refresh: after every emit, the cache advances to the latest full snapshot, so each window's delta is computed against the immediately previous window. This PR makes
SnapshotCache::ComputeDeltamatch.refresh-only-on-full (REMOVED)(b) Why no config knob
There is exactly one correct behavior. The legacy processors agree on it; the parity harness's byte-equality contract requires it; and the bug only persisted because two later shims (PR #229 CountSketch, PR #230 CMS) bypassed the runtime cache to work around it. Adding a
DeltaSnapshotPolicyenum would invite future regressions; deleting the wrong path eliminates the choice.(c) Common wrappers extracted into
asap-precompute-go/sketches/Five wrappers, one per concrete sketch type, mechanically extracted from the OTel shim PRs (#226-#230). Public API (capitalized constructors and types) so any adapter can consume:
DDSketchWrapper+DDSketchObserver— implementsprecompute.QuantileSketchKLLWrapper+KLLObserver— implementsprecompute.QuantileSketchHLLWrapper+HLLObserver— implementsprecompute.CardinalitySketchCountSketchWrapper+CountSketchObserver— implementsprecompute.FrequencySketchCMSWrapper+CMSObserver— implementsprecompute.FrequencySketchEach wrapper carries the same byte-format invariant the shim version did:
Snapshot()produces the canonical sketchlib-go portable format that the backend's modified-OTLP decoder expects (see ADR-0002 §"Behavior preservation"). One small upgrade inCMSWrapper: the wrapper'sComputeDeltaAgainstnow drives realcms.ComputeDelta(the previous "always full" stub existed only to work around the SnapshotCache bug — fixed here).Light unit-test coverage on each wrapper (construct + observe + snapshot + delta-shape). Heavy correctness coverage stays in the parity harness at
integration/parity.(d) Shim PRs that become simpler
The
shim_helpers.go::applyDeltaTransmissionworkaround in PR #229 (CountSketch) and PR #230 (CMS) is now redundant. Those PRs can rebase onto this one and drop the workaround in a follow-up commit on each branch — left out of scope here per the request to keep the change isolated.Test plan
cd asap-precompute-go && go test ./...— passes (new always-refresh tests + sketches/ unit tests + existing suites)cd integration/parity && go test -v ./...— all 5 sketch sub-tests still pass byte-for-byte against the legacy emit pathdocs/design-asap-edge-framework.md§6.2 documents the always-refresh semanticdocs/adr/adr-0002-extract-precompute-runtime.md"Behavior preservation" section strengthened to state the cache contract explicitly