feat(gos): reattach #518 sampled-insert paths onto GOS (phase 2) - #530
Merged
Merged
Conversation
Wires the GOS-aware sampled primitives (sketchlib-go #77: UpdateStringSampledPerRowGOS/UpdateStringAtRowsGOS for CountSketch, InsertWithHashSampledPerRowGOS/InsertWithHashAtRowsGOS for CMS) into the ASAPCollector wrapper layer, closing the gap #518's sampling stack would otherwise reopen: before this, a sampled insert returned before ever reaching the GOS threshold check, so a CountSketch/CMS with both sample_p<1 (or SDK row-admission) AND gos_delta_epsilon>0 silently lost insert-time detection for every sampled sample — no crash, estimates stayed unbiased, but the GOS mechanism itself never fired for that sketch. Scope: only the collector-side wrapper composition (this PR). The SDK-side machinery that DECIDES admittedRows/sampleP and gets them onto the wire (aggregationrouter.go, policyfingerprint.go, monitor engine changes, OTel Go SDK, asap-precompute-rs) is #518's own, larger, separate concern — out of scope here, reattaches independently once it needs these primitives. - observation.go: RowSampled/AdmittedRows/SampleP fields on ObservationValue (lifted from #518, unmodified) — the wire the SDK's row-admission decision travels on into the collector. - countsketch.go: UpdateString's sampler branch now calls UpdateStringSampledPerRowGOS when GOS is active (upgrading from the old whole-item Admit()+divide model to per-row NitroSketch admission, per #518's intent, while preserving GOS composition); heap-msgpack mode is still excluded (its DELTA-HEAP wire form isn't GOS-converted) via the same gosActive condition ComputeDeltaAgainst already checks. New ApplyAdmittedOccurrence method + CountSketchObserver wiring for the SDK-pre-decided row-admission path. - cms.go: CMS's whole-item WithSampleP/admit() sampling already composed correctly with GOS (InsertWithHashGOS calls admit() internally) — no change needed there. New ApplyAdmittedOccurrence method (CMS had none before) + CMSObserver wiring for the SDK-pre-decided path, mirroring CountSketch's. Test plan: - TestCountSketch_RowSampledObserve_ComposesWithGOS / TestCMS_RowSampledObserve_ComposesWithGOS: drive Observe with RowSampled=true through both wrappers, assert the wake signal fires and the drained delta is non-empty — proving the SDK-row-admission path participates in GOS instead of silently bypassing it. - TestCountSketch_PerRowSampler_ComposesWithGOS: a live WithSampleP geometric sampler still triggers GOS detection through UpdateString directly (the other reattached path). - go build/go vet/go test clean in asap-precompute-go (full suite, no regressions) and the processor package (post restore_all.sh). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
force-pushed
the
split/pr-518-reattach-sampled-gos
branch
from
July 17, 2026 02:49
05a7a6f to
884280d
Compare
3 tasks
This was referenced Jul 17, 2026
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.
Phase 2 of reattaching PR #518 (NitroSketch-style SDK-side row-admission sampling) onto the new GOS stack. Phase 1 (sketchlib-go #77) added the GOS-aware sampled primitives; this PR wires them into the ASAPCollector wrapper layer. Base: #525 (GOS CMS, itself on #524 GOS CountSketch).
The gap this closes
Before this PR, a sampled insert (either a live per-row geometric sampler, or an SDK-pre-decided row-admission bitmask via
ApplyAdmittedOccurrence) returned before ever reaching the GOS threshold check. A CountSketch/CMS configured with bothsample_p<1(or SDK row-admission) andgos_delta_epsilon>0would silently lose insert-time detection for every sampled sample — no crash, estimates stayed unbiased (the 1/p correction is intact), but the GOS mechanism itself never fired for that sketch. The design intends these to compose (the water-filling threshold even carries a sampling-noise floor term), so this was a real integration gap, not an intentional exclusion.Scope
Only the collector-side wrapper composition. The SDK-side machinery that decides
admittedRows/samplePand gets them onto the wire (aggregationrouter.go,policyfingerprint.go, monitor engine changes, the OTel Go SDK,asap-precompute-rs) is #518's own, much larger, separate concern — out of scope here. That machinery can reattach independently once it needs to call these primitives; this PR just makes sure the collector side is ready for it.Changes
observation.go:RowSampled/AdmittedRows/SamplePfields onObservationValue(lifted from feat(sdk): NitroSketch-style SDK-side row-admission sampling #518 unmodified) — the wire the SDK's row-admission decision travels on into the collector.countsketch.go:UpdateString's sampler branch now callsUpdateStringSampledPerRowGOSwhen GOS is active — this also upgrades CountSketch's sampling from the old whole-itemAdmit()+dividemodel to per-row NitroSketch admission (per feat(sdk): NitroSketch-style SDK-side row-admission sampling #518's intent: decorrelates row noise), while preserving GOS composition. Heap-msgpack mode stays excluded (DELTA-HEAP isn't GOS-converted), samegosActiveconditionComputeDeltaAgainstalready checks. NewApplyAdmittedOccurrence+ observer wiring for the SDK-row-admission path.cms.go: CMS's existing whole-itemWithSampleP/admit()sampling already composed correctly with GOS (InsertWithHashGOScallsadmit()internally) — no change needed there. NewApplyAdmittedOccurrence(CMS had none before) + observer wiring, mirroring CountSketch's.Test plan
TestCountSketch_RowSampledObserve_ComposesWithGOS/TestCMS_RowSampledObserve_ComposesWithGOS: driveObservewithRowSampled=truethrough both wrappers, assert the wake signal fires and the drained delta is non-empty.TestCountSketch_PerRowSampler_ComposesWithGOS: a liveWithSamplePgeometric sampler still triggers GOS detection throughUpdateStringdirectly.go build/go vet/go testclean inasap-precompute-go(full suite, no regressions) and the processor package.🤖 Generated with Claude Code