feat: add Node UI metrics collection toggle

[lupuszr]

Summary

• add telemetry.metrics.collectionEnabled for the local Node UI SQLite metrics collector
• add the DKG_METRICS_COLLECTION_ENABLED environment override with config precedence
• keep local collection enabled by default for backward compatibility
• avoid constructing or starting MetricsCollector when collection is disabled
• report the disabled state in daemon startup logs
• validate configuration in daemon startup and dkg doctor
• document the local collector as independent from OpenTelemetry metric export

Scope

This is intentionally the minimal enable/disable change. It does not change the existing 30-second collector interval, collection scheduling, presence gating, or DKG_METRICS_ALWAYS_COLLECT=1 behavior.

The follow-up cadence PR is #1891.

Configuration

{
  "telemetry": {
    "metrics": {
      "collectionEnabled": false
    }
  }
}

The environment override accepts 1, 0, true, or false:

export DKG_METRICS_COLLECTION_ENABLED=0

Validation

• focused configuration and doctor tests: 59 passed
• CLI TypeScript tsc --noEmit passed
• Node UI package build passed
• git diff --check passed

[otReviewAgent]

🟡 Issue: Validate the raw toggle without fabricating a typed runtime config

What's wrong
The doctor check operates on untrusted parsed config, but this change forces it through the daemon runtime type by constructing a fake config object and casting it. That muddies the boundary between raw config validation and resolved runtime config, and it makes future toggle validation likely to grow more nested shape traversal and more casts instead of using one clear parser.

Example
resolveMetricsCollectorConfig({ telemetry: { metrics: { collectionEnabled } } } as unknown as Pick<DkgConfig, 'telemetry'>, {}) is a sign that the reusable boundary is one level too high for raw config validation.

Suggested direction
Move the boolean parsing/validation into a small canonical helper, for example parseMetricsCollectionEnabledConfigValue(value: unknown): boolean, and keep resolveMetricsCollectorConfig as the daemon-facing env-precedence adapter.

For Agents
In packages/cli/src/metrics-collector-config.ts, extract/export a raw-value parser or validator for telemetry.metrics.collectionEnabled; have resolveMetricsCollectorConfig call it, and have config-sanity.ts call it directly against parsed JSON. Preserve env precedence in daemon startup and prove invalid raw config still produces the same doctor finding.

[otReviewAgent]

🟡 Issue: Keep the collector toggle inside a focused lifecycle helper

What's wrong
The PR adds the new feature flag by threading nullable collector state through a 3.8k-line daemon function. That preserves the existing sprawl and adds another conditional lifecycle concern at the top level instead of making the metrics collector a self-contained subsystem.

Example
A reader now has to connect the early metricsCollectorConfig, the conditional constructor, route-time optional collector use, and optional cleanup to understand the disabled state. That is more daemon-level state for a feature that could own its own start/stop policy.

Suggested direction
Replace the top-level nullable resource pattern with a small startMetricsCollector(...) or createLocalMetricsCollectorLifecycle(...) abstraction that resolves the toggle, starts when enabled, formats/logs startup, and exposes a uniform stop() cleanup handle.

Confidence note
This is a structural concern in an already very large function; the added lines are small, but they add another piece of cross-cutting lifecycle state instead of shrinking the collector ownership boundary.

For Agents
In packages/cli/src/daemon/lifecycle.ts, extract the local Node UI metrics collector lifecycle into a focused helper near the metrics section, returning the collector passed to node-ui routes plus a stop no-op when disabled. Preserve default enabled behavior, disabled no-start behavior, the startup log text, and cleanup behavior.

[otReviewAgent]

🟡 Issue: Daemon collector toggle is not covered at the wiring point

What's wrong
The operator-facing promise is that disabling local collection stops new SQLite snapshots and store scans. The current tests verify that the config helper returns { enabled: false }, but they do not verify that runDaemonInner uses that value to suppress the MetricsCollector side effect. That leaves the changed behavior unverified at the point where the store scans are actually started.

Example
A regression that left metricsCollector.start() unconditional in runDaemonInner, or accidentally ignored telemetry.metrics.collectionEnabled: false before this block, would still pass the new resolver and doctor tests. A focused test could mock @origintrail-official/dkg-node-ui's MetricsCollector, start runDaemonInner with { telemetry: { metrics: { collectionEnabled: false } } }, abort after startup like the existing daemon wiring tests, and assert the constructor/start were not called and the disabled startup log was emitted.

Suggested direction
Add a small daemon-level wiring regression test instead of relying only on the pure config resolver tests.

For Agents
Look at packages/cli/test/*wiring*.test.ts for the existing runDaemonInner mock harness. Add coverage around packages/cli/src/daemon/lifecycle.ts that preserves default enabled startup, proves collectionEnabled: false skips MetricsCollector construction/start, and ideally proves the env override path reaches the same daemon wiring.

[Jurij89]

Adversarial review — feat: add Node UI metrics collection toggle

Reviewed at head 2ece9f7ea against base b8dedb63d (testnet-canary). Five independent angles
(boot/failure semantics, disabled-runtime behaviour, efficacy of the premise, config surface +
doctor, tests/contract), each surviving finding then put through three adversarial skeptics
(correctness / reproducibility / is-it-a-regression), plus a completeness critic.

21 raw candidates → 16 after dedupe → 2 survived. The refutation rate is the headline: this is
a tight, well-scoped PR and most of what an aggressive reviewer would flag here does not hold up
against the base branch. I've listed the notable kills at the bottom so nobody re-raises them.

Verdict: approve with one fix worth making before merge. No blocker.

---

What's good

• The helper is genuinely pure and injectable (env parameter), which is what makes the 11 new
  tests possible at all. resolveMetricsCollectorConfig short-circuiting on env before
  type-checking config is subtle but correct, and — see below — it's also why the doctor check
  deliberately passes {}.
• metricsCollector?.stop() (lifecycle.ts:3742) and the already-optional 7th parameter of
  handleNodeUIRequest (node-ui/src/api.ts:94, guarded at :127) mean the undefined-collector
  path was safe before you got there. Nothing downstream needed changing — I checked.
• Default-true preserves behaviour, and the enabled-path startup log is byte-identical to base.

---

MEDIUM — the new fatal validation runs above the daemon.log tee, so its message reaches no log

resolveMetricsCollectorConfig(config) is the second statement of runDaemonInner
(lifecycle.ts:1142). The stdout/stderr tee into daemon.log is installed at :1150-1165, and
log() is only defined at :1167. The throw therefore predates the only thing that would record
it. Both spawn paths discard the worker's stderr — cli-supervisor.ts:108 and
commands/lifecycle.ts:199 are both stdio: ['ignore','ignore','ignore'].

Concretely, with DKG_METRICS_COLLECTION_ENABLED=yes in a systemd unit: worker throws → supervisor
respawns 5× and gives up (cli-supervisor.ts:141-143) → dkg start prints
Daemon did not start within 15s. Check logs: ~/.dkg/daemon.log (commands/lifecycle.ts:234) →
and that file has gained nothing. The operator is pointed at a log that structurally cannot contain
the answer.

Two honest corrections my own skeptics forced on me, which is why this is MEDIUM and not higher:

1. Not a regression. A throw above the tee is already invisible on base — malformed
   config.json fails identically via loadConfig() at runDaemon:947. This PR adds a new input
   to a pre-existing gap; it doesn't create the gap.
2. Not fully undiagnosable. dkg start --foreground spawns stdio: 'inherit'
   (cli-supervisor.ts:174), so that path does print the trace. And dkg doctor catches the
   config-file form. It's specifically the env-var form on the default detached path that has
   no diagnostic channel at all.

I also want to explicitly retract the "this violates the repo's degrade-don't-throw convention"
argument — I went looking for it and the opposite is true. config.ts:1035-1058
(resolveApprovalPolicy) throws with the docstring "Fails fast at startup rather than silently
falling back — config bugs are easier to find when they don't lurk for hours", and runDaemonInner
already process.exit(1)s on operator config at :1279, :1284, :1314, :1331. Fail-fast is
the convention. The problem isn't that you throw — it's only where you throw.

Fix (one line of movement). The resolved value's only consumer is at :2653. Move the call
below the tee and use the pattern already sitting at :1278-1286:

try {
  metricsCollectorConfig = resolveMetricsCollectorConfig(config);
} catch (err) {
  log(`FATAL: ${(err as Error).message}`);
  process.exit(1);
}

That makes the PR's own documented promise — "Invalid config or environment values fail daemon
startup instead of silently enabling the collector" — actually observable.

Sub-point worth a decision: DKG_METRICS_COLLECTION_ENABLED= (empty) is fatal

resolveEnabled branches on envValue !== undefined (metrics-collector-config.ts:8), so an empty
string is "supplied" and throws. That's the shape you get from
environment: - DKG_METRICS_COLLECTION_ENABLED=${FOO} in Compose with FOO unset, or a blanked
Environment= line in a unit file — a fleet-rollout templating accident, not a typo.

Counter-argument I'll grant: the variable is new, so no existing deployment carries it. Still, is
empty-means-invalid the intent, or should empty fall through to config? supervisor-liveness.ts:109
maps '' to the default; http-utils.ts:1644-1647 documents "empty string yields the documented
default". Your call — but worth being deliberate about, and worth a test either way.

---

LOW — coverage: the toggle's only load-bearing line isn't pinned by any test

All 11 new tests exercise the pure helper. Nothing asserts the thing the PR exists to do:
lifecycle.ts:2652-2662 (skip construction + start() when disabled). Revert that block to
unconditional and the entire new suite stays green while the daemon still logs
Metrics collector disabled.

Same for the env = process.env default binding (metrics-collector-config.ts:34): production is
the only one-argument caller (lifecycle.ts:1142); every test passes an explicit second argument.
Change the default to {} and nothing fails, but DKG_METRICS_COLLECTION_ENABLED becomes a silent
no-op in production.

The repo has four precedents for exactly this seam — daemon-sync-agents-meta-wiring.test.ts,
daemon-storage-ack-timing-wiring.test.ts, publisher-backfill-wiring-1828.test.ts,
publisher-maxretries-wiring-1836.test.ts. Two small tests close both gaps.

LOW — doc gap: /api/metrics keeps serving the last snapshot as a live 200

api.ts:122-123 returns db.getLatestSnapshot() whenever any row exists, and that query
(db.ts:1475-1478) has no freshness predicate. With the collector never constructed, nothing writes
new rows, so the route serves the final pre-disable row until retention prunes it
(DEFAULT_RETENTION_DAYS = 14, db.ts:33), after which it returns 200 {}.

I had this at MEDIUM with a monitoring-blindness story; the skeptics were right to cut it down, and
I'm reporting the reduced version: the handler is byte-identical to base (not your code), the
payload is SELECT * so it carries ts and is machine-detectably stale, the repo's Grafana
dashboard is Prometheus/OTLP-fed and never touches this route, and the whole state requires an
explicit opt-in. What survives is narrow but true: the new doc says "Existing history remains in
SQLite" and doesn't mention that the live route also freezes. One sentence in
docs/use-dkg/node-ui-metrics.md covers it.

Design question (not a defect): is the lever the right shape?

The expensive work is the six store-scan getters, which metrics-collector.ts:163-170 already gates
on #1066 presence. This toggle also drops the cheap ones — CPU/mem/heap/disk/peers/RPC — which are
outside that gate and are the entire content of the Hardware tab.

I tried hard to turn this into a finding ("the benefit is already zero") and it was correctly
refuted: any open node-UI tab pins sseClientCount() > 0 (metrics-presence.ts:57, fed by
/api/events), so on the node an operator is actually investigating, the gate is open and this
toggle does remove six full-store COUNTs per tick. The benefit is real. The question is only
whether an operator who wants the store scans off also wants to lose their CPU graphs — or whether
#1891's cadence change is the better default answer for that operator.

---

Notable claims that did not survive — please don't re-raise these

• "doctor should pass process.env instead of {}" — the fix is backwards. resolveEnabled
  short-circuits on env before type-checking config, so passing process.env would make doctor
  report clean on a broken config.json whenever a valid env override exists. {} is deliberate
  isolation for a check scoped to the config file. Separately, a manually-run dkg doctor never
  inherits a systemd Environment= anyway. (The residual gap — nothing validates the env value —
  is real, but it needs its own check, not this parameter changed.)
• "the env vocabulary contradicts the repo's parseBooleanEnv (on/off/yes/no)" — that parser has
  zero callers in packages/cli and lives in packages/agent/src/sync/. The CLI's actual
  convention is bare === '1', under which off/yes silently mean false — the exact silent
  failure this PR prevents.
• "Hardware tab goes blank, indistinguishable from a pegged store" — false equivalence. Every
  field that tab renders comes from the cheap getters outside the presence gate, and each store
  getter is individually try/caught, so a pegged store leaves the tab populated.
• "RUNBOOK now contradicts itself" — :117 says "leave out or set enabled: false", which
  keeps the block. No conflict.
• "this is the only telemetry setting that hard-fails boot" — see the retraction above.
• Base being testnet-canary — normal for this repo; Promote testnet-canary to main (10.0.9 line + fifa-class meta ceiling fix) #1888 is the promotion vehicle. Not a finding.

_{Method: Workflow harness, 56 agents / 1,247 tool calls. Two angles settled contested claims by
executing the real modules under node --experimental-strip-types rather than reasoning about
them — the value matrix ('', ' ', 'off', 'yes' all throw; 'TRUE', ' 1 ' pass) and the
presence-gate probe (gate closed ⇒ zero SPARQL getters, one INSERT) are transcripts, not readings.
I verified the four load-bearing citations in this comment myself before posting.}