feat: configure Node UI metrics collection cadence - #1891
Conversation
34ba762 to
eab94a6
Compare
| ? this.latestStoreMetrics | ||
| : null; | ||
| const snap = await this.collectAndStoreInternal(false, cachedStoreMetrics); | ||
| if (cachedStoreMetrics && !this.initialSnapshotHandled) { |
There was a problem hiding this comment.
🔴 Bug: Delayed backfill rewrites idle history with a later store count
What's wrong
Because initialSnapshotHandled stays false until cached store metrics exist, a node that runs idle can accumulate null cardinality snapshots and later rewrite them all with the first count observed after a consumer appears. That corrupts metric history and contradicts the documented gate behavior that expensive columns are null when the gate is closed.
Example
Scenario: the daemon starts with no metrics consumer, so the presence gate is closed and several cheap snapshots are stored with total_triples = NULL. Hours later a dashboard opens, the store lane collects totalTriples = 1000, and the next system tick enters this block. backfillNulls then rewrites all earlier NULL total_triples rows to 1000 even though those expensive counts were intentionally skipped at those timestamps.
Suggested direction
Do not let the first later store scan backfill rows that were written while the presence gate was closed; keep those skipped-scan columns null or limit backfill to the original startup window.
For Agents
In packages/node-ui/src/metrics-collector.ts, preserve the presence-gate contract that skipped store scans leave expensive columns null. Either mark the initial snapshot as handled on the first system collection regardless of cachedStoreMetrics, remove this delayed backfill, or constrain it only to true cold-start rows that should be backfilled. Add a scheduling test: start with shouldCollectStoreMetrics false for multiple ticks, switch it true, let store and system ticks run, and assert the earlier rows still have null total_triples.
| } | ||
|
|
||
| try { | ||
| this.latestStoreMetrics = await this.collectStoreMetricsSerialized(); |
There was a problem hiding this comment.
🔴 Bug: Scheduled store scan results are not verified at the snapshot boundary
What's wrong
This change separates expensive store scans from the snapshot writer. The tests verify that the store getter runs on the slower cadence, but they do not verify the user-facing result: that those values are actually written into the SQLite snapshots served by the Node UI. That leaves a meaningful gap where the cadence tests can stay green while the dashboard loses or preserves stale cardinality values.
Example
A regression that changes line 215 to just await this.collectStoreMetricsSerialized(); would still make storeCalls equal 1/2 in the scheduling tests, but later /api/metrics snapshots would keep total_triples null because the scanned values were never persisted.
Suggested direction
Extend the scheduling tests to assert persisted snapshot contents, not only getter call counts, for the decoupled store scan/cache path.
For Agents
In packages/node-ui/test/metrics-collector.test.ts, add scheduler-level assertions around db.getLatestSnapshot() after a store scan completes and after the next cheap tick. Prove scanned cardinalities appear without another store getter call, and consider a follow-up case where a later failed store scan replaces previously cached cardinalities with nulls.
| // Kept in sync with @origintrail-official/dkg-node-ui's constructor guard. | ||
| // This resolver lives in the CLI package so config errors are reported before | ||
| // daemon lifecycle constructs the collector. | ||
| export const DEFAULT_METRICS_COLLECTION_INTERVAL_MS = 30_000; |
There was a problem hiding this comment.
🟡 Issue: Avoid shadowing the collector's interval contract in CLI
What's wrong
This PR introduces the same defaults and validation bounds in both CLI and node-ui. That makes the package boundary harder to trust: daemon config validation, doctor diagnostics, runtime constructor validation, docs, and tests now depend on duplicated constants staying manually synchronized.
Example
If @origintrail-official/dkg-node-ui later changes the max timer guard or default store cadence, CLI startup/doctor can keep accepting or logging the old values while the collector enforces the new ones.
Suggested direction
Make one module own the local metrics interval contract. The CLI resolver can still own env/config parsing, but the accepted range and defaults should come from the collector package or a shared config module.
For Agents
In packages/cli/src/metrics-collector-config.ts, import the interval defaults and guard from @origintrail-official/dkg-node-ui or move the resolver into a canonical shared module. Preserve env precedence and error subjects; update tests to assert against the shared constants rather than repeated magic numbers.
| */ | ||
| private readonly shouldCollectStoreMetrics: () => boolean = () => true, | ||
| ) {} | ||
| options: MetricsCollectorOptions = {}, |
There was a problem hiding this comment.
🟡 Issue: Do not extend the collector with a fifth positional constructor argument
What's wrong
The new MetricsCollectorOptions only owns intervals, while dataDir and shouldCollectStoreMetrics remain separate positional parameters. This splits the construction contract across two styles and makes callsites brittle and cryptic.
Example
new MetricsCollector(db, source, undefined, () => true, { collectionIntervalMs: 1_000, storeCollectionIntervalMs: 5_000 }) requires readers to remember that arg 3 is dataDir, arg 4 is the presence gate, and arg 5 is scheduler config.
Suggested direction
Fold the optional constructor concerns into one named options object so future collector settings do not keep growing the positional API.
For Agents
Refactor the constructor toward a single options object such as { dataDir, shouldCollectStoreMetrics, collectionIntervalMs, storeCollectionIntervalMs }. If external compatibility matters, keep a backwards-compatible overload or static factory while moving new callsites to the object form. Cover default construction and configured intervals in tests.
| return this.collectInternal(this.shouldCollectStoreMetricsSafely()); | ||
| } | ||
|
|
||
| private async collectInternal( |
There was a problem hiding this comment.
🟡 Issue: Replace boolean/null collection modes with an explicit snapshot plan
What's wrong
The two-cadence scheduler adds a hidden mode system via a boolean plus nullable cached value. That makes the collector harder to reason about because scheduling policy, presence-gate policy, cache reuse, and row construction are all interleaved in the same call path.
Example
collectAndStoreInternal(false, cachedStoreMetrics) means "write cheap metrics using cached cardinalities", while collectAndStoreInternal(true) means "write cheap metrics and run fresh store scans". The type signature also permits unclear combinations like includeStoreMetrics=true with cached metrics present.
Suggested direction
Make store metrics a first-class input to snapshot assembly, and isolate the policy that decides whether that input is fresh, cached, or empty.
For Agents
Refactor collectInternal to accept a concrete StoreMetricsSnapshot only, using an EMPTY_STORE_METRICS constant for null columns. Centralize the gate/cache/fresh-scan decision in a small helper used by collect() and runSystemCollection; keep scheduled store scans serialized and preserve the current snapshot output.
Depends on
This PR is stacked on #1892. Its review diff contains only interval configuration and the cheap-system versus expensive-store scheduling split.
Summary
telemetry.metrics.collectionIntervalMsfor cheap CPU, memory, process, peer, RPC, and relay snapshotstelemetry.metrics.storeCollectionIntervalMsfor expensive full-store SPARQL cardinality scanssetTimeoutloops so a collection never overlaps with itselfDKG_METRICS_ALWAYS_COLLECT=1Configuration
telemetry.metrics.collectionIntervalMsDKG_METRICS_COLLECTION_INTERVAL_MS30000telemetry.metrics.storeCollectionIntervalMsDKG_STORE_METRICS_COLLECTION_INTERVAL_MS43200000Intervals must be finite positive integers between 1,000ms and Node's maximum safe timer delay of 2,147,483,647ms. These local collector settings remain independent of OpenTelemetry
exportIntervalMs.Why
The existing 30-second collector tick includes several global SPARQL
COUNTscans. On large Blazegraph stores, those scans can consume most available CPU, outlive the interval, congest the query queue, and cause store-scheduler timeouts.The independent store cadence preserves useful 30-second operational dashboards without repeatedly scanning the full RDF store.
Compatibility
Cheap metrics retain their 30-second default cadence. The intentional behavior change is that expensive store scans default to 12 hours. Operators that require the legacy store cadence can set
telemetry.metrics.storeCollectionIntervalMsto30000.With
DKG_STORE_METRICS_COLLECTION_INTERVAL_MS=43200000, the collector makes one gated startup attempt and schedules no subsequent expensive scan until at least 12 hours after that scan finishes.Validation
tsc --noEmitpassedgit diff --checkpassed