feat(precompute): migrate precompute mint to resolver; delete compute_sid (PR-4) - #192
Merged
Merged
Conversation
zzylol
added a commit
that referenced
this pull request
May 13, 2026
…ce (#190) Three-commit chain implementing Option B (backend-allocated registry) for sid mint on the OTel ingest path. - PR-1 (84abac7): SeriesIdResolver authoritative; populate series_assignments. - PR-2 (3ac90cf): WAL-backed persistence (header b"ASAPSRP\x01" initially, bumped to v2 in PR-3). - PR-3 (f72b533): resolver key = (metric, fp, agg_kind); WAL v2 with agg_kind_canonical; delete compute_sketch_sid. Sid identity contract: `(metric, attrs_fingerprint, agg_kind_canonical)` — registry-allocated u64, unique by construction, durable via fsync per mint. Sender caches via `series_assignments`, recovers via `unknown_series_ids` eviction primitive on cache divergence. PR-4 (#192) follows, migrating the precompute path off compute_sid onto the same resolver. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…_sid (PR-4) Closes the registry-allocated-sid design across the system. After PR-1/2/3 the OTel sketch ingest path was on the resolver; the precompute path was still content-addressing via `compute_sid`. This commit makes the two paths share a single mint authority, matching the design goal of "single sid across asapcollector and asapquery-backend; precompute is just an extension of the OTel processors at the edge." API change: - `SketchStore::ingest_precompute_for_agg_config` now takes a closure parameter `mint_sid: impl FnOnce(&str, &str, &str) -> u64` instead of calling `compute_sid` internally. Closure receives (metric, fp, agg_kind_canonical); callers typically pass `|m, fp, ak| series_resolver.resolve(m, fp, ak)`. Keeps SketchStore free of any layer-inverted dependency on the resolver (which lives in `drivers::ingest`). Production wiring: - `SketchStoreSink` gains `series_resolver: Arc<SeriesIdResolver>` field; constructor takes it. main.rs passes the shared resolver. - `BackfillWindowProcessor` gains an optional `series_resolver` field via `with_series_resolver(...)` builder. When absent + sketch_index is set, processor logs a warn and skips precompute writes for the window (registry provenance still recorded). - `BackfillService` plumbs the resolver through `with_series_resolver` to its per-job processors. main.rs wires it alongside `with_sketch_index`. Test sites updated: - `capability_matching_tests.rs` (2 sites): local resolver per fixture. - `schema_timeline_dispatch_tests.rs::seed_sum_at`: thread-local resolver so multiple seeds share `next_sid`. - `eviction.rs::tests::write_one`: thread-local resolver. - `test_utilities/engine_factories.rs` (8 sites across 6 factory functions): `ingest_with_fresh_resolver` helper threads a per-factory resolver. - `output_sink.rs` (2 inline tests): pass `SeriesIdResolver::new()`. Deletions: - `pub fn compute_sid` from `sketch_db/data/mod.rs` (53 LOC). - `fn sketch_kind_tag` + `fn encode_sketch_config` helpers (the only callers were inside `compute_sid`). - `xxhash_rust::xxh64` import — no consumer in this module after `compute_sid` is gone. - 4 unit tests at `sketch_db/index/mod.rs:1130+` that exercised `compute_sid` directly. Identity properties (metric/attrs/agg_kind discriminate sids, sketch vs precompute never collide, agg_type and parameters each contribute) are now covered structurally by `AggKind::canonical_string` exhaustiveness + the resolver's `distinct_agg_kinds_same_series_distinct_sids` test. After this PR: - Every sid in the system is allocated by `SeriesIdResolver`. - Sketch sids and precompute sids share one namespace, one `AtomicU64::fetch_add` counter, one WAL. - A series with both a DDSketch and a Sum aggregation gets TWO distinct sids (different `agg_kind_canonical`) — same identity model the system had under `compute_sketch_sid`/`compute_sid`. - No content-addressed sid hashing anywhere in the data plane. cargo build -p data_plane (lib + bin): clean (5 pre-existing warnings) cargo test -p data_plane --lib: 759/759 passing (was 763; -4 deleted hash tests) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
zzylol
force-pushed
the
feat/sid-registry-precompute
branch
from
May 13, 2026 18:32
5d3fa7d to
9c8c614
Compare
3 tasks
zzylol
added a commit
that referenced
this pull request
May 13, 2026
…sketch (#193) Adds docs/design-sid-lifecycle.md (559 lines) — design doc for the registry-allocated sid model that landed in #190 + #192. §1-§4 capture shipped behavior: identity contract (sid = registry-allocated u64 for (metric, fp, agg_kind_canonical)), end-to-end architecture diagram, per-DP wire-case table, failure-recovery sequence diagrams (cold start, stale sender sid, restart with/without WAL replay), durability semantics (fsync-per-mint, torn-write detection, WAL v2 format). §5 is a forward-looking sketch for distributed asapquery-backend (sharding on hash(tenant, metric), 8-bit shard_id in top of u64, query coordinator fan-out, HA options, single→sharded migration path). Not implemented; doc says so explicitly. §6 lists 5 open questions for follow-up: ResolveSeriesIDs RPC fate, WAL compaction threshold, per-tenant sid subspace, collector-side routing colocation, cross-shard PromQL semantics. 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.
Summary
PR-4 in the registry-allocated sid chain. After PR-1+2+3 (PR #190) the OTel sketch ingest path was on
SeriesIdResolver; the precompute path was still content-addressing viacompute_sid. This PR migrates the precompute path to the same resolver, making sid identity uniform across the system:sid = registry-allocated u64 for (metric, attrs_fingerprint, agg_kind_canonical)Matches the design goal: "single sid across asapcollector and asapquery-backend; precompute is just an extension of the OTel processors at the edge." Sketch sids and precompute sids now share one namespace, one
AtomicU64::fetch_addcounter, one WAL.API change
SketchStore::ingest_precompute_for_agg_configtakes a closure mint parameter:Closure receives
(metric, attrs_fingerprint, agg_kind_canonical); callers pass|m, fp, ak| series_resolver.resolve(m, fp, ak). Keeps SketchStore free of any layer-inverted dependency onSeriesIdResolver(which lives indrivers::ingest).Production wiring
SketchStoreSink: gainsseries_resolver: Arc<SeriesIdResolver>field; constructor takes it; main.rs passes the shared resolver.BackfillWindowProcessor: gains optionalseries_resolverfield viawith_series_resolver(...)builder. When absent butsketch_indexis set, processor logs a WARN and skips precompute writes (registry provenance still recorded — safe degradation, not a crash).BackfillService: plumbs the resolver through to its per-job processors via the same builder pattern.main.rs: wires the resolver alongsidewith_sketch_indexfor both sites (SketchStoreSink::newandBackfillService).Tests touched
capability_matching_tests.rs(2 sites): local resolver per fixture.schema_timeline_dispatch_tests.rs::seed_sum_at: thread-local resolver so multiple seeds sharenext_sid.lifecycle/eviction.rs::tests::write_one: thread-local resolver.test_utilities/engine_factories.rs(8 sites across 6 factory functions):ingest_with_fresh_resolverhelper threads a per-factory resolver.output_sink.rs(2 inline tests): passArc::new(SeriesIdResolver::new()).Deletions
pub fn compute_sid(53 LOC,sketch_db/data/mod.rs).fn sketch_kind_tag+fn encode_sketch_confighelpers.xxhash_rust::xxh64import (no consumer left in this module).sketch_db/index/mod.rs:1130+that exercisedcompute_siddirectly.Identity properties the deleted tests covered (metric/attrs/agg_kind discriminate sids, sketch vs precompute never collide, agg_type and parameters each contribute) are now covered structurally by:
AggKind::canonical_stringexhaustiveness over the enum.series_resolver::tests::distinct_agg_kinds_same_series_distinct_sids(the resolver's identity contract under Interpretation B).What this PR does NOT change
agg_kind_canonicalper record).Cargo.toml(no dep changes —xxhash_rustis still used byxxh3in other code).Stacking
Branched off
feat/sid-registry-canonical(PR #190 head). PR #190 must merge first; this PR will then merge cleanly onto the new main. Reviewing the two PRs back-to-back makes the chain easier to follow.Test plan
cargo build -p data_plane(lib + bin) — cleancargo test -p data_plane --lib— 759/759 passing (was 763; -4 deleted hash tests)grep -rn "compute_sid\|compute_sketch_sid" data_plane/src— only historical comments remainBackfillWindowProcessorskips writes when resolver is missing (logs WARN). Is this the right degradation, or should it fail loudly? Production wires both via main.rs so the path doesn't fire there; only tests can hit it.🤖 Generated with Claude Code