Skip to content

feat: configure Node UI metrics collection cadence - #1891

Open
lupuszr wants to merge 1 commit into
codex/node-ui-metrics-togglefrom
codex/configurable-node-ui-metrics
Open

feat: configure Node UI metrics collection cadence#1891
lupuszr wants to merge 1 commit into
codex/node-ui-metrics-togglefrom
codex/configurable-node-ui-metrics

Conversation

@lupuszr

@lupuszr lupuszr commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

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

  • add telemetry.metrics.collectionIntervalMs for cheap CPU, memory, process, peer, RPC, and relay snapshots
  • add telemetry.metrics.storeCollectionIntervalMs for expensive full-store SPARQL cardinality scans
  • add environment overrides with environment-over-config precedence
  • retain a 30-second default for cheap snapshots and use a conservative 12-hour default for store scans
  • replace interval scheduling with completion-relative setTimeout loops so a collection never overlaps with itself
  • keep cheap snapshots running while a slow store scan is active
  • preserve the immediate gated startup attempt, metrics-presence gate, and DKG_METRICS_ALWAYS_COLLECT=1
  • report the resolved system and store intervals in startup logs

Configuration

Setting Environment override Default
telemetry.metrics.collectionIntervalMs DKG_METRICS_COLLECTION_INTERVAL_MS 30000
telemetry.metrics.storeCollectionIntervalMs DKG_STORE_METRICS_COLLECTION_INTERVAL_MS 43200000

Intervals 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 COUNT scans. 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.storeCollectionIntervalMs to 30000.

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

  • MetricsCollector scheduling and presence tests: 25 passed
  • focused CLI configuration, doctor, presence, and query tests: 90 passed
  • Node UI package build passed
  • CLI TypeScript tsc --noEmit passed
  • git diff --check passed

@lupuszr
lupuszr force-pushed the codex/configurable-node-ui-metrics branch from 34ba762 to eab94a6 Compare July 21, 2026 14:15
@lupuszr lupuszr changed the title fix: make Node UI metrics collection configurable feat: configure Node UI metrics collection cadence Jul 21, 2026
@lupuszr
lupuszr changed the base branch from testnet-canary to codex/node-ui-metrics-toggle July 21, 2026 14:17
@Jurij89
Jurij89 marked this pull request as ready for review July 21, 2026 14:54
? this.latestStoreMetrics
: null;
const snap = await this.collectAndStoreInternal(false, cachedStoreMetrics);
if (cachedStoreMetrics && !this.initialSnapshotHandled) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 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;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 = {},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants