fix(edge): P0/P1/P2 code-review fixes across gorilla-go, precompute-go, asapedgeprocessor (+Rust parity, docs) - #451
Merged
Merged
Conversation
…apedgeprocessor Agent-implemented batch (pre-reconciliation): - precompute-go: HLL/CountSketch full-state ApplyDelta, CMS dim validation, regex matchers, sliding windows, quantile clamp, telemetry counters - gorilla-go: OOODrop per-fragment delta, INT decode cursor, README+archive-tier docs, dead code, low-sev guards - asapedgeprocessor: unsupported-type passthrough, MaxSeries/overflow caps, shutdown durable drain, control-plane wiring, delta transmission (default OFF), AllowedLateness, sum ts fix, observe-error counter, B6 CountSketch subject, cold seal dedup, -race ConsumeMetrics test Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- precompute-go: document Sliding window must not feed the additive-merge backend (overlapping emissions double-count); correct stale 'tumbling-only' comment now that sliding is implemented. - precompute-rs: mirror Go's CMS/CountSketch dim normalization (cols->next_pow2, clamp rows to the 64-bit row-hash budget) so Go and Rust emit byte-identical envelopes (#243); cols=2000->2048 on both sides. - docs: add delta-baseline-contract.md — why edge delta transmission must stay OFF until the edge per-window-reset model is reconciled with the backend's additive-onto-running-base merge; recommends Option A, DDSketch first. Backend (ASAPQuery-backend) needs no code changes; delta transmission ships default-OFF behind the per-family flag. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Companion PRs (both open):
End-to-end verified: OCB |
…etch/CMS) Change SketchObserver::observe(sketch, v) -> observe(sketch, obs) so the family-specific observer keys itself from obs.labels (the window.rs call site already has obs in scope). No Go change, no plugin/lifecycle plumbing. - CMSObserver: drop the hard 'expected Bytes' error; key = utf8(bytes) if present else attributes_key(labels) -> fixes CMS-records-nothing on the OTAP Float-kind path. - CountSketchObserver: key = bytes else attributes_key(labels) else default_key; weight 1 on the labels path -> counts per-attribute-set frequency like the Go edge, not the degenerate metric-name key. - DDSketch/KLL/HLL observers + 2 test mocks: mechanical (read obs.value.*). - Fix stale otap_lifecycle CMS test (now emits) + add attribute-set tests. - cargo test + --features otap green. docs/delta-baseline-contract.md: correct the now-invalid 'DDSketch carries explicit scalars' rationale — per the design rule, sketches carry NO count/sum/min/max (full or delta); exact aggregates are separate controller-provisioned edge aggregations; delta is uniformly sketch-state-only. 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
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.
Implements the P0/P1/P2 fixes from a code review of the edge stack (
asap-gorilla-go,asap-precompute-go,asapedgeprocessor), plus Rust byte-parity reconciliation, the B6 fix in both runtimes, and a delta-baseline design doc.ASAPQuery-backendneeds no code changes (verified against its currentmain).P0 — correctness / silent data loss
ApplyDelta(asap-precompute-go/sketches/{hll,countsketch}.go): tried delta-decode first, so a full-state envelope silently merged to 0. Now full-state-first (mirrors CMS). Real-wrapper regression test added.asapedgeprocessor/ingest.go): Histogram/Summary/ExponentialHistogram were silently dropped from passthrough and never archived. Now always forwarded raw + counted.warm_sketch.go/warm_sum.go): wiredMaxSeries/OnOverflow(+ sum-group cap), default 100000.processor.go): no longer skips cold-part/spool drain when the flush-wait ctx expires.sketches/cms.go): non-pow2colspanicked on query; nowcols→next pow2 +rowsclamped to the 64-bit row-hash budget. Mirrored into Rust for asap_sketchlib ↔ sketchlib-go byte-format harmonization (cross-language parity) #243 parity.asap-gorilla-go/fragment.go): cumulative counter was summed per fragment (~N·D over-count); now a per-flush delta.P1 — designed but missing
asapedgeprocessor/control_plane.go): optionalcontrol_channelblock;Start()polls and applies config viaPrecompute.UpdateConfigin-place. Disabled by default.AllowedLatenessset to match coldReorderGrace.precompute-go/matchers.go):=~/!~, Prometheus-anchored.precompute-go/window.go) implemented, with a guardrail doc that they must not feed the additive-merge backend.P2 — hygiene
gorilla-go: INT decode cursor advance, README rewrite (removed non-existent
GORILLA1API),ASAPCC1/intchunkarchive-tier doc, dead code, low-sev guards. precompute-go: quantile clamp, telemetry counters. edge: sumstart>end+maxObservedreset, observe-error counter, cold seal/ship dedup,-raceconcurrentConsumeMetricstest.B6 — CountSketch/CMS observation subject (fixed in BOTH runtimes)
warm_sketch.go): CountSketch now counts the attribute-set (key=AttributesKey, weight 1), and CMS counts the attribute-set viaKindBytes— not the degenerate metric-name key.asap-precompute-rs):SketchObserver::observe(sketch, v)→observe(sketch, obs)so the family-specific observer keys itself fromobs.labels. This fixes CountSketch (was counting the metric name) and the CMS-records-nothing bug (CMS errored on the OTAP Float-kind path). No Go change, no plugin/lifecycle plumbing;cargo test+--features otapgreen; stale lifecycle test fixed + attribute-set tests added.Design decision — sketches carry no metric scalars
Per project direction: no sketch (DDSketch included) carries metric
count/sum/min/maxin the full proto or the delta proto. Exact aggregates are separate, controller-provisioned exact edge aggregations (e.g. the existingsumfamily → delta-Sum metric; aMinMaxfamily), never fields on the sketch. This makes delta transmission uniformly sketch-state-only across all families.docs/delta-baseline-contract.mdis updated to this model. The actual proto field removal is a queued follow-up effort (see below).Backend compatibility (
ASAPQuery-backend) — no changes requireddocs/delta-baseline-contract.md(Option A; uniform sketch-state-only delta; e2e equality-test gate).cols→pow2 in both Go and Rust so envelopes stay byte-identical (asap_sketchlib ↔ sketchlib-go byte-format harmonization (cross-language parity) #243).Rust parity (#243)
Mirrored CMS/CountSketch dim normalization into
asap-precompute-rs; B6 OTAP observer keying fixed there too. (Full-stateApplyDeltaneeded no Rust change — already correct.)Verification
-racetest).cargo build+cargo test(incl.--features otap) green.build_asap_otel.shproduced the fullasap-otelcollector;asap-otel componentslistsasap_edge. ✅Related PRs
queryFrequencyFastcolumn-fold.Follow-ups (not in this PR)
count/sum/min/maxfrom all 5*SketchDataPoints + delta protos; route exact aggregates to controller-provisionedsum/MinMaxedge aggregations. Spans proto →sketchlib-go/asap_sketchlib→ edge → backend → regenerated asap_sketchlib ↔ sketchlib-go byte-format harmonization (cross-language parity) #243 goldens. Lands before/with delta enablement.docs/delta-baseline-contract.md(edge per-window deltas + backend per-window base rotation; DDSketch first), gated by an e2e delta-ON==delta-OFF equality test; re-establish the asap_sketchlib ↔ sketchlib-go byte-format harmonization (cross-language parity) #243 golden parity harness removed in chore: remove unused integration/ test suite #450.🤖 Generated with Claude Code