Conversation
This was referenced Jul 16, 2026
Moves row-admission sampling for CMS/CountSketch from the collector into the OTel-Go SDK: the SDK decides, per RAW OCCURRENCE at Record() time, which rows of the TARGET PHYSICAL SKETCH get updated (matching NitroSketch's one-update-stream-per-sketch model), ships the occurrence + admission bitmask over OTLP, and the collector applies it verbatim instead of re-deriving its own sampling decision. - asap-precompute-go: PolicyFingerprint (content-addressed, byte-for-byte match with ASAPQuery-backend's Rust implementation), AggregationIdentity (AggID + Filter — one physical sketch instance per AggregateBy group), AggregationRouter. - Fixes a real bug: this edge's AggID was fnv64(metric name) — completely different from the backend's PolicyFingerprint, silently desyncing CDM identity from the sketch-DB's materialized-view identity for the same policy. Now computed identically on both sides. - sketchlib-go primitives (UpdateStringAtRows/InsertWithHashAtRows) wired through ApplyAdmittedOccurrence on CMSWrapper/CountSketchWrapper, and a per-row (not whole-item) geometric admission swap on CountSketch's plain UpdateString path. - SDK: AggregationRowSampledSketch aggregation type (liveSampleGrant mirrors otel-app/sample_controller.go, dialing the coordinator directly), new metricdata.RowSampledSketch[N] wire type. - OTLP wire transport: both otlpmetricgrpc/otlpmetrichttp dispatch RowSampledSketch to a standard Gauge/NumberDataPoint (no new proto message — a row-sampled point carries no sketch state, just one raw occurrence + 3 scalars stamped as reserved attributes). - Collector: asapedgeprocessor decodes the reserved attributes, strips them before series identity, and routes to ApplyAdmittedOccurrence; drops (rather than misapplies) a row-sampled observation against a family with no *AtRows primitive (DDSketch/KLL/HLL aren't row-replicated matrices) — defensive against an SDK/collector AggID disagreement. - asap-precompute-rs: matching coordinated producer-side sampling for the Rust/OTAP path. Part of splitting #515 into a reviewable stack (cleanup -> GOS -> NitroSketch sampling -> eval/docs). Builds/vets/tests clean standalone: asap-precompute-go, asapedgeprocessor, sdk/metric, otlpmetricgrpc, otlpmetrichttp.
zzylol
force-pushed
the
split/pr3-sampling
branch
from
July 16, 2026 18:30
5adb178 to
636d137
Compare
This was referenced Jul 16, 2026
zzylol
added a commit
that referenced
this pull request
Jul 17, 2026
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>
3 tasks
zzylol
added a commit
that referenced
this pull request
Jul 17, 2026
…d-gos feat(gos): reattach #518 sampled-insert paths onto GOS (phase 2)
This was referenced Jul 17, 2026
Contributor
Author
|
Split into 5 fresh PRs against current `main`, since this PR's base (`split/pr2-gos` → `split/pr1-cleanup`) predates the 6-family GOS redesign and the Discipline B split (all merged via #524→#532) — its `CLEAN`/`MERGEABLE` status is computed against that stale base, not real mergeability against `main` today. Roughly a third of this PR's diff was already superseded by that later work (`ApplyAdmittedOccurrence`'s GOS-aware composition from #530, the Discipline B `monitor/engine.go` slack-crossing → periodic rate reports, the 6-family `config.go`/`precompute.go`) and would have regressed main if forward-ported as-is. The genuinely new, still-needed scope was extracted fresh:
Closing without merging. |
zzylol
added a commit
that referenced
this pull request
Jul 17, 2026
Adds RowSampledSketch as a new Aggregation kind: the SDK decides per-occurrence row admission at Record() time (NitroSketch model) instead of the collector deciding after the fact. LiveSampleGrant dials the coordinator directly per precompute.AggregationIdentity and tracks the live sample_p grant; rowSampledSketchValues routes each occurrence through the grant before feeding admitted rows into the underlying CMS/CountSketch/DDSketch aggregate. Depends on PR-A's AggregationIdentity/AggregationRouter types (split/pr-aggid-policyfingerprint) — based on that branch rather than main directly. Extracted from #518 (stale base); sourced from feat/gos-unified-monitoring where this is already wired and tested, confirmed byte-identical between the two source branches. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Jul 17, 2026
…dOccurrence ingest.go: decode the 3 reserved wire attributes an SDK running AggregationRowSampledSketch stamps on an individually-admitted raw occurrence (admitted-rows bitmask, row count, sample_p), strip them before building series identity, and skip cold-archiving a row-sampled point (it's a raw occurrence, not an aggregate sample). warm_sketch.go: observe() takes rowSampled/admittedRows/sampleP and, when set, tags the ObservationValue so CMS/CountSketch route through Sketch.ApplyAdmittedOccurrence (the SDK's admission decision applied verbatim) instead of the plain insert path. Families with no *AtRows sketchlib primitive (DDSketch/KLL/HLL) drop rather than silently misapply an unrelated observer — this can only happen if the SDK's AggregationRouter and this collector's AggID disagreed about which family a PolicyFingerprint targets. Built fresh against current main's warm_sketch.go/ingest.go (not merged wholesale from feat/gos-unified-monitoring, which independently regressed the insert-time GOS wake/anisotropic-threshold work already merged here via #524-#532) — only the row-sampled-specific decode/ route logic was ported; the isotropic GOS priming, wakeCh wiring, and sub-window trigger logic already on main are unchanged. Extracted from #518 (stale base); depends on PR-A's aggID/config plumbing (split/pr-aggid-policyfingerprint) — based on that branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zzylol
added a commit
that referenced
this pull request
Jul 17, 2026
…porters Dispatches a RowSampledSketch metric point as a standard Gauge/ NumberDataPoint with 3 reserved attributes (raw occurrence count, admission bitmask, and sample_p) — no new proto message needed, both otlpmetricgrpc and otlpmetrichttp transform paths generated from the same shared metricdata.go.tmpl source. Depends on PR-B's RowSampledSketch Aggregation (split/pr-sdk-row-sampling-types) — based on that branch. go.mod/go.sum: derived via `go mod tidy` against current main's dependency baseline rather than copied wholesale from feat/gos-unified-monitoring, which carries unrelated dependency-version drift from that branch's independent history. Only the new asap-precompute-go transitive dependency (pulled in via PR-B's sdk/metric change) and its local-module replace directives were added. Extracted from #518 (stale base); wire-format logic sourced from feat/gos-unified-monitoring, confirmed clean apply against current main. Co-Authored-By: Claude Sonnet 5 <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
Third PR in the split of #515 (cleanup → GOS → NitroSketch sampling → eval/docs). Base: #517.
Moves row-admission sampling for CMS/CountSketch from the collector into the OTel-Go SDK: the SDK decides, per RAW OCCURRENCE at
Record()time, which rows of the TARGET PHYSICAL SKETCH get updated (matching NitroSketch's one-update-stream-per-sketch model), ships the occurrence + admission bitmask over OTLP, and the collector applies it verbatim instead of re-deriving its own sampling decision.asap-precompute-go:PolicyFingerprint(content-addressed, byte-for-byte match with ASAPQuery-backend's Rust implementation),AggregationIdentity(AggID + Filter — one physical sketch instance per AggregateBy group),AggregationRouter.fnv64(metric name)— completely different from the backend's PolicyFingerprint, silently desyncing CDM identity from the sketch-DB's materialized-view identity for the same policy. Now computed identically on both sides.UpdateStringAtRows/InsertWithHashAtRows) wired throughApplyAdmittedOccurrenceonCMSWrapper/CountSketchWrapper, and a per-row (not whole-item) geometric admission swap on CountSketch's plainUpdateStringpath.AggregationRowSampledSketchaggregation type (liveSampleGrantmirrorsotel-app/sample_controller.go, dialing the coordinator directly), newmetricdata.RowSampledSketch[N]wire type.otlpmetricgrpc/otlpmetrichttpdispatchRowSampledSketchto a standard Gauge/NumberDataPoint (no new proto message — a row-sampled point carries no sketch state, just one raw occurrence + 3 scalars stamped as reserved attributes).asapedgeprocessordecodes the reserved attributes, strips them before series identity, and routes toApplyAdmittedOccurrence; drops (rather than misapplies) a row-sampled observation against a family with no*AtRowsprimitive (DDSketch/KLL/HLL aren't row-replicated matrices) — defensive against an SDK/collector AggID disagreement.asap-precompute-rs: matching coordinated producer-side sampling for the Rust/OTAP path.Test plan
asap-precompute-go,asapedgeprocessor,sdk/metric,otlpmetricgrpc,otlpmetrichttpall build/vet/test clean standalone on this branch.feat/gos-unified-monitoring(diff -rqclean, zero deltas).sketchlib-go#69should merge first (per-cell delta / row-admission primitives this branch depends on).🤖 Generated with Claude Code