feat(go.d/metrix): transactional, bounded descriptor lifecycle - #23053
Conversation
…tor lifecycle metrix kept one instrument descriptor per metric name for the life of the process: the registry was registration-only and never pruned, and a one-name-one-kind conflict panicked immediately. That is fine for fixed-schema collectors but unbounded for push/client-driven metric names (statsd), and the permanent-descriptor assumption forced consumers to work around it. This makes the descriptor lifecycle a transactional, name-authoritative, bounded-growth subsystem - the framework prerequisite for a push-based statsd collector. Descriptor resolution is now transactional and name-authoritative. Registration is staged during a cycle and resolved atomically at commit: a truly-live conflict fails the whole commit (loud, no partial publish), an idle/in-grace kind is superseded, and an ambiguous new name is dropped for the cycle while other names still commit. Each accepted name publishes exactly one canonical descriptor (compatible declarations merged), stamped onto every surviving series so readers, the registry, and series always agree. An aborted cycle leaves the registry exactly as before. Descriptor growth is now bounded. After a name's last series is evicted by retention, its descriptor is kept for a grace window and then removed by a commit-time sweep over the descriptor universe (O(names), within the commit envelope). NewCollectorStore becomes variadic with WithExpireAfterSuccessCycles, WithMaxSeries, and WithDescriptorGraceCycles (grace defaults to expire, so grace >= expire by construction). Eviction and ambiguous-drop activity is surfaced as cumulative CollectMeta counters (metrix never logs). An optional DescriptorRetention interface exposes the retention window (expire+grace, or an unbounded sentinel when expiry is disabled) and the successful-commit clock, for consumers that cache per-name state. The prometheus writer consumes that contract. It previously kept a per-name family handle forever because metrix descriptors were permanent; now handles are two-phase (staged on a compatible write, promoted on a successful commit, never-accepted handles from an aborted commit dropped) and aged on the successful-commit clock to the descriptor window, in lockstep with metrix's own eviction. A name that goes idle or drifts to a changed contract past the window is evicted and re-registers cleanly instead of being drift-skipped forever; drift-skip is preserved while the descriptor is live. API compatibility is preserved: NewCollectorStore stays additive/variadic, the CollectorStore interface is unchanged (DescriptorRetention is an optional type-assertion), and existing callers behave identically. The behavior change is that descriptors are no longer permanent - a fully idle name evicts after expire+grace and a changed-contract name becomes re-registerable. The metrix README is rewritten around the descriptor lifecycle and retention, and the framework-V2 collector skill note is corrected for the now non-permanent, bounded descriptor lifecycle. Closes netdata#22648.
There was a problem hiding this comment.
Pull request overview
This PR updates the Go metrix storage layer to make per-metric-name descriptor registration transactional and bounded, so descriptor memory does not grow without limit under push/client-driven metric-name streams (e.g., future statsd/push integrations). It also aligns the Prometheus go.d writer’s per-name caches with the new descriptor retention window to allow clean re-registration after idle eviction.
Changes:
- Implement staged (cycle-scoped) descriptor registration with commit-time conflict resolution, plus descriptor eviction after an idle grace window.
- Add
CollectorStoreconstructor options to tune retention (expire,maxSeries,descriptorGrace) and expose optionalDescriptorRetentionaccessors for consumers that cache per-name state. - Update the Prometheus writer to reconcile/evict cached family handles based on the store’s successful-commit clock and descriptor retention window; add lifecycle tests and expand metrix conflict/retention tests/benchmarks.
Reviewed changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/go/plugin/go.d/collector/prometheus/writer.go | Couple per-name writer handle lifetime to metrix descriptor retention; add commit-clock reconciliation. |
| src/go/plugin/go.d/collector/prometheus/writer_test.go | Adjust drift-skip tests to reflect “within descriptor window” semantics and keepalive reconciliation. |
| src/go/plugin/go.d/collector/prometheus/writer_lifecycle_test.go | New tests pin writer handle aging/eviction behavior vs descriptor window and abort semantics. |
| src/go/pkg/metrix/types.go | Add CollectMeta counters for descriptor eviction and dropped ambiguous names. |
| src/go/pkg/metrix/summary.go | Reconcile same-key summary descriptor conflicts and use baseline authority guard for cumulative seeding. |
| src/go/pkg/metrix/stateset.go | Reconcile same-key stateset descriptor conflicts. |
| src/go/pkg/metrix/register_txn_store_test.go | New tests for transactional (commit/abort) registration behavior. |
| src/go/pkg/metrix/README.md | Major documentation expansion: cycle model, descriptor lifecycle/retention, conflict resolution, new options/accessor. |
| src/go/pkg/metrix/options.go | Introduce CollectorStoreOption and retention-related constructor options. |
| src/go/pkg/metrix/measureset.go | Reconcile same-key measureset descriptor conflicts and use baseline authority guard for additive seeding. |
| src/go/pkg/metrix/interfaces.go | Add optional DescriptorRetention interface for descriptor-window/commit-clock exposure. |
| src/go/pkg/metrix/hotpath_bench_test.go | Expand benchmark docs and add new benchmarks for supersede/drop/schema/declaration-conflict paths. |
| src/go/pkg/metrix/histogram.go | Make nil-bounds snapshot histograms adopt observed bounds; reconcile same-key bounds differences without panics. |
| src/go/pkg/metrix/histogram_store_test.go | Update histogram tests to validate non-panicking nil-bounds behavior and commit-time resolution. |
| src/go/pkg/metrix/gauge.go | Reconcile same-key gauge descriptor conflicts and use baseline authority guard for additive seeding. |
| src/go/pkg/metrix/descriptor_universe_sweep_store_test.go | New tests pin descriptor-universe sweep/eviction behavior and counters. |
| src/go/pkg/metrix/descriptor_retention_accessor_test.go | New tests pin DescriptorRetention window/clock semantics and exposure via managed store. |
| src/go/pkg/metrix/descriptor_conflict_store_test.go | New tests pin commit-time authority conflict resolution (fail/supersede/drop) and determinism. |
| src/go/pkg/metrix/descriptor_compat_test.go | New tests for authority/declaration compatibility rules (including histogram wildcard semantics). |
| src/go/pkg/metrix/descriptor_authority_store_test.go | Extensive new tests for authority fingerprints, same-key conflicts, canonical metadata, and commit-cost guards. |
| src/go/pkg/metrix/counter.go | Reconcile same-key counter descriptor conflicts and use baseline authority guard for additive seeding. |
| src/go/pkg/metrix/collector_store.go | Core implementation: transactional registration, commit-time resolution, canonicalization after retention, descriptor sweep, retention window/clock accessors. |
| src/go/pkg/metrix/collector_store_options_test.go | New tests validating constructor options and grace-default coupling. |
| .agents/skills/project-writing-go-modules-framework-v2/SKILL.md | Update guidance to reflect bounded/transactional descriptor lifecycle and required cache-window coupling via DescriptorRetention. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
1 issue found across 24 files
Confidence score: 5/5
- In
src/go/plugin/go.d/collector/prometheus/writer_lifecycle_test.go, the fallback test mutates state post-initialization instead of covering the constructor path innewMetricFactory, so a real fallback break for stores withoutDescriptorRetentioncould ship unnoticed—add a constructor-level test case that initializes such a store before merging.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/go/plugin/go.d/collector/prometheus/writer_lifecycle_test.go">
<violation number="1" location="src/go/plugin/go.d/collector/prometheus/writer_lifecycle_test.go:223">
P3: This fallback check only simulates the missing accessor by mutating the writer after initialization, so it does not exercise the actual constructor path for a store without `DescriptorRetention`. A broken `newMetricFamilyWriter` fallback could still pass here; constructing a real stub store would make the regression test reliable.</violation>
</file>
Architecture diagram
sequenceDiagram
participant Collector as Collector (ModuleV2)
participant Metrix as metrix CollectorStore
participant Cycle as CycleController
participant Registry as Descriptor Registry
participant Series as Series Retention
participant Sweep as Descriptor Sweep
participant Snapshot as Read Snapshot
participant Writer as Prometheus Writer (metricFamilyWriter)
Note over Collector,Writer: NEW: Bounded, transactional descriptor lifecycle
alt Normal collect cycle (name observed, then idle, then evicted)
Collector->>Cycle: BeginCycle()
Cycle->>Metrix: Open staged frame
Metrix-->>Cycle: OK
Collector->>Metrix: Write().SnapshotMeter("svc").Gauge("m").Observe(1)
Metrix->>Metrix: Stage entry in gauges map (no commit yet)
Collector->>Cycle: CommitCycleSuccess()
Cycle->>Metrix: Resolve descriptors
Metrix->>Registry: Check "svc.m" – not in instruments
alt Name is new
Registry-->>Metrix: No existing descriptor
Metrix->>Registry: Install gauge descriptor for "svc.m"
end
Cycle->>Series: Apply series retention (expireAfterSuccessCycles)
Series->>Series: No series to evict (still present)
Cycle->>Sweep: Run descriptor sweep
Sweep->>Sweep: Check idle descriptors (none)
Cycle->>Snapshot: Publish atomically (swap pointer)
Snapshot-->>Cycle: done
Cycle-->>Collector: success
Note over Collector,Writer: Next cycles: "m" not observed
loop for expireAfterSuccessCycles successful commits
Collector->>Cycle: BeginCycle / CommitCycleSuccess (no write for "m")
Cycle->>Series: Evict series for "svc.m" when idle limit reached
Note over Series: Series gone, but descriptor still in Registry
Series->>Registry: Mark "svc.m" as zero-live (set instrumentZeroSince seq)
end
loop for descriptorGraceCycles successful commits
Collector->>Cycle: CommitCycleSuccess (still not observed)
Cycle->>Sweep: Check grace status
Sweep->>Sweep: "svc.m" grace not yet elapsed
end
Cycle->>Sweep: On final grace cycle, sweep descriptor
Sweep->>Registry: Remove "svc.m" from instruments
Sweep-->>Cycle: Increment EvictedDescriptors counter
Cycle->>Snapshot: Publish updated CollectMeta
Cycle-->>Collector: success
Writer->>Writer: reconcileHandles() at start of next scrape
Writer->>Metrix: Check DescriptorRetention (optional interface)
Metrix-->>Writer: retention window (expire+grace), successful commits count
Writer->>Writer: Compute that "svc.m" = gauge handle is idle past window
Writer->>Writer: Delete metricFamilyHandle for "svc.m"
Note over Writer: Handle evicted in lockstep with descriptor eviction
else Conflict: live name observed with incompatible kind
Collector->>Cycle: BeginCycle()
Collector->>Metrix: Write gauge "svc.m".Observe(2)
Collector->>Metrix: Write counter "svc.m".ObserveTotal(3) (same key, conflicting kind)
Metrix->>Metrix: Reconcile same-key: record conflict evidence
Collector->>Cycle: CommitCycleSuccess()
Cycle->>Metrix: Resolve conflicts
alt Live conflict: established gauge also observed
Metrix->>Metrix: Fail commit – conflicting instrument kinds
Cycle-->>Collector: error (commit aborted)
Note over Snapshot: Snapshot unchanged, staged writes discarded
else Idle supersede: gauge not observed this cycle
Metrix->>Registry: Check "svc.m" – gauge descriptor present but series dead (idle)
Metrix->>Registry: Supersede: remove gauge descriptor and all stale series
Metrix->>Registry: Install counter descriptor for "svc.m"
Cycle->>Series: Evict old series by name
Cycle->>Sweep: Ensure no orphan idle stamp
Cycle->>Snapshot: Publish new snapshot with counter value
Cycle-->>Collector: success
else Ambiguous new name: no prior registry
Metrix->>Metrix: No committed authority – drop both kinds for the name
Cycle->>Snapshot: Publish snapshot without "svc.m" (increment DroppedNames)
Cycle-->>Collector: success
end
else Abort cycle discards staged registrations
Collector->>Cycle: BeginCycle()
Collector->>Metrix: Write gauge "svc.m".Observe(1) (staged)
Collector->>Cycle: AbortCycle()
Cycle->>Metrix: Discard staged frame including pendingInstruments
Cycle-->>Collector: nothing committed
Note over Registry: "svc.m" never entered committed instruments
else Prometheus writer: handle lifecycle coupled to descriptor window
Writer->>Writer: WriteMetricFamilies (scrape)
Writer->>Writer: reconcileHandles() – check successful commits
Writer->>Metrix: DescriptorRetention.SuccessfulCommits()
Metrix-->>Writer: current commit count
alt Previous cycle committed and handle was staged
Writer->>Writer: Promote handle to accepted, record commit count
else Previous cycle aborted and handle never accepted
Writer->>Writer: Delete bogus handle
end
Writer->>Writer: For each handle, if accepted and idle ≥ window (and not unbounded), delete
Note over Writer: Handle eviction matches descriptor eviction timing
end
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
…ter fallback test The fallback test simulated a store without DescriptorRetention by nulling w.retention after construction, so it never exercised newMetricFamilyWriter's actual type-assertion fallback - a broken constructor branch would still pass. Use a stub CollectorStore that embeds the interface (so the optional DescriptorRetention methods are not promoted) and assert the constructor leaves retention nil and the window unbounded, then that handles are kept. Part of netdata#22648.
|


Summary
Closes #22648.
metrixstores one descriptor per metric name. Until now, those descriptors have been kept for the entire life of the process. Fine for fixed collectors, but unbounded for push/client-driven metric names (statsd). This makes the descriptor lifecycle bounded, the prerequisite for a push-based statsd collector.After:
Test Plan
Additional Information
For users: How does this change affect me?
Summary by cubic
Bounded, transactional descriptor lifecycle in
metrixwith commit-time resolution, plus Prometheus writer handles coupled to the descriptor window. Prevents unbounded growth for push/client-driven names and allows clean re-register when a name’s contract changes after going idle.New Features
NewCollectorStore(...)addsWithExpireAfterSuccessCycles,WithMaxSeries,WithDescriptorGraceCycles(grace defaults to expire).DescriptorRetentionexposes the retention window and successful-commit clock; the Prometheus writer now ages its family handles to that window, only promotes handles on successful commits, and falls back to keep handles if the accessor isn’t available.CollectMetacounters:EvictedDescriptors,DroppedNames.Migration
DescriptorRetentionand align their cache window toexpire+gracefor fewer drift-skips and clean re-registers.Written for commit e1948d7. Summary will update on new commits.