-
Notifications
You must be signed in to change notification settings - Fork 10
feat: add Node UI metrics collection toggle #1892
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| # Node UI metrics collection | ||
|
|
||
| The daemon writes local Node UI metric snapshots to `node-ui.db`. This local | ||
| collector is separate from OpenTelemetry metric export: | ||
|
|
||
| - `telemetry.metrics.collectionEnabled` controls local SQLite snapshots and | ||
| the store queries used to populate them. | ||
| - `telemetry.metrics.enabled`, `endpoint`, and `exportIntervalMs` control OTLP | ||
| export. Disabling local collection does not disable OTLP export. | ||
|
|
||
| Local collection remains enabled by default for backward compatibility. To | ||
| disable it, add: | ||
|
|
||
| ```json | ||
| { | ||
| "telemetry": { | ||
| "metrics": { | ||
| "collectionEnabled": false | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| The environment override takes precedence over configuration: | ||
|
|
||
| ```bash | ||
| export DKG_METRICS_COLLECTION_ENABLED=0 | ||
| ``` | ||
|
|
||
| The environment value accepts `1`, `0`, `true`, or `false`. Invalid config or | ||
| environment values fail daemon startup instead of silently enabling the | ||
| collector. Restart the daemon after changing the setting. | ||
|
|
||
| Disabling the collector stops new local snapshots and store scans. Existing | ||
| history remains in SQLite. When collection is enabled, the existing metrics | ||
| presence gate and `DKG_METRICS_ALWAYS_COLLECT=1` behavior are unchanged. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -145,6 +145,10 @@ import { | |
| } from '../config.js'; | ||
| import { projectRuntimeEvmChainConfig } from '../runtime-chain-config.js'; | ||
| import { resolveOtelSignals, resolveLogExporterMode, isUnknownLogExporter } from '../telemetry-config.js'; | ||
| import { | ||
| formatMetricsCollectorStartupLog, | ||
| resolveMetricsCollectorConfig, | ||
| } from '../metrics-collector-config.js'; | ||
| import { createDaemonLogSink } from './log-sink.js'; | ||
| import { startRpcUsageTelemetry } from './rpc-usage-log.js'; | ||
| import { startDashboardLogVolumePruner } from './dashboard-log-volume-pruner.js'; | ||
|
|
@@ -1133,6 +1137,9 @@ export async function runDaemonInner( | |
| startedAt: number, | ||
| ): Promise<void> { | ||
| configureKaPublishLifecycleDebugLogging(config); | ||
| // Resolve the local collector toggle before constructing daemon resources. | ||
| // This is independent from OTLP metrics export configuration. | ||
| const metricsCollectorConfig = resolveMetricsCollectorConfig(config); | ||
| const logFile = logPath(); | ||
| // Rotate before installing the in-process stdout/stderr tee so startup does | ||
| // not race the truncation with fresh log appends. Existing logs survive | ||
|
|
@@ -2642,14 +2649,17 @@ export async function runDaemonInner( | |
| alwaysCollect: process.env.DKG_METRICS_ALWAYS_COLLECT === "1", | ||
| }); | ||
|
|
||
| const metricsCollector = new MetricsCollector( | ||
| dashDb, | ||
| metricsSource, | ||
| dkgDir(), | ||
| () => metricsPresence.hasRecentConsumer(), | ||
| ); | ||
| metricsCollector.start(); | ||
| log("Metrics collector started (30s interval)"); | ||
| let metricsCollector: MetricsCollector | undefined; | ||
| if (metricsCollectorConfig.enabled) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Issue: Daemon collector toggle is not covered at the wiring point What's wrong Example Suggested direction For Agents |
||
| metricsCollector = new MetricsCollector( | ||
| dashDb, | ||
| metricsSource, | ||
| dkgDir(), | ||
| () => metricsPresence.hasRecentConsumer(), | ||
| ); | ||
| metricsCollector.start(); | ||
| } | ||
| log(formatMetricsCollectorStartupLog(metricsCollectorConfig)); | ||
|
|
||
| // --- Telemetry: syslog log streaming (opt-in) --- | ||
| const networkKey = network?.networkName?.toLowerCase().includes("testnet") | ||
|
|
@@ -3729,7 +3739,7 @@ export async function runDaemonInner( | |
| // log-derived request totals exact across process lifecycles. | ||
| rpcUsageTelemetry.stop(); | ||
| rateLimiter.destroy(); | ||
| metricsCollector.stop(); | ||
| metricsCollector?.stop(); | ||
| // Stops log exporters AND flushes + shuts down the OTel SDK. | ||
| await stopTelemetry(); | ||
| natStatusWatcherStop?.(); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,7 +18,9 @@ import { join } from 'node:path'; | |
| import { | ||
| AUTO_UPDATE_GIT_ONLY_FIELDS, | ||
| parseAutoUpdateVerifyTagSignature, | ||
| type DkgConfig, | ||
| } from '../../config.js'; | ||
| import { resolveMetricsCollectorConfig } from '../../metrics-collector-config.js'; | ||
| import { | ||
| formatAutoUpdateTagVerificationWarning, | ||
| resolveAutoUpdateGitRefPlan, | ||
|
|
@@ -97,6 +99,29 @@ export async function runConfigSanityCheck(deps: DoctorDeps): Promise<Finding[]> | |
| } | ||
| } | ||
|
|
||
| const telemetry = parsed.telemetry; | ||
| if (telemetry && typeof telemetry === 'object' && !Array.isArray(telemetry)) { | ||
| const metrics = (telemetry as Record<string, unknown>).metrics; | ||
| if (metrics && typeof metrics === 'object' && !Array.isArray(metrics)) { | ||
| const collectionEnabled = (metrics as Record<string, unknown>).collectionEnabled; | ||
| if (collectionEnabled !== undefined) { | ||
| try { | ||
| resolveMetricsCollectorConfig({ | ||
| telemetry: { metrics: { collectionEnabled } }, | ||
| } as unknown as Pick<DkgConfig, 'telemetry'>, {}); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Issue: Validate the raw toggle without fabricating a typed runtime config What's wrong Example Suggested direction For Agents |
||
| } catch (err) { | ||
| findings.push({ | ||
| check: 'config-sanity', | ||
| severity: 'error', | ||
| message: err instanceof Error ? err.message : String(err), | ||
| advisory: 'Set telemetry.metrics.collectionEnabled to true or false.', | ||
| subject: 'telemetry.metrics.collectionEnabled', | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // autoUpdate sub-config | ||
| const autoUpdate = parsed.autoUpdate; | ||
| if (autoUpdate && typeof autoUpdate === 'object' && !Array.isArray(autoUpdate)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| import type { DkgConfig } from './config.js'; | ||
|
|
||
| export interface ResolvedMetricsCollectorConfig { | ||
| enabled: boolean; | ||
| } | ||
|
|
||
| function resolveEnabled(configValue: unknown, envValue: string | undefined): boolean { | ||
| if (envValue !== undefined) { | ||
| const normalized = envValue.trim().toLowerCase(); | ||
| if (normalized === '1' || normalized === 'true') return true; | ||
| if (normalized === '0' || normalized === 'false') return false; | ||
| throw new Error( | ||
| 'DKG_METRICS_COLLECTION_ENABLED must be one of 1, 0, true, or false ' + | ||
| `(received ${JSON.stringify(envValue)})`, | ||
| ); | ||
| } | ||
| if (configValue === undefined) return true; | ||
| if (typeof configValue !== 'boolean') { | ||
| throw new Error( | ||
| 'telemetry.metrics.collectionEnabled must be a boolean ' + | ||
| `(received ${JSON.stringify(configValue)})`, | ||
| ); | ||
| } | ||
| return configValue; | ||
| } | ||
|
|
||
| /** | ||
| * Resolve local Node UI snapshot collection independently from OTLP export. | ||
| * The dedicated environment variable wins over config; invalid values fail | ||
| * startup rather than silently enabling collection. | ||
| */ | ||
| export function resolveMetricsCollectorConfig( | ||
| config: Pick<DkgConfig, 'telemetry'> | null | undefined, | ||
| env: Record<string, string | undefined> = process.env, | ||
| ): ResolvedMetricsCollectorConfig { | ||
| return { | ||
| enabled: resolveEnabled( | ||
| config?.telemetry?.metrics?.collectionEnabled, | ||
| env.DKG_METRICS_COLLECTION_ENABLED, | ||
| ), | ||
| }; | ||
| } | ||
|
|
||
| export function formatMetricsCollectorStartupLog( | ||
| resolved: ResolvedMetricsCollectorConfig, | ||
| ): string { | ||
| return resolved.enabled | ||
| ? 'Metrics collector started (30s interval)' | ||
| : 'Metrics collector disabled'; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| import { describe, expect, it } from 'vitest'; | ||
| import { | ||
| formatMetricsCollectorStartupLog, | ||
| resolveMetricsCollectorConfig, | ||
| } from '../src/metrics-collector-config.js'; | ||
|
|
||
| describe('resolveMetricsCollectorConfig', () => { | ||
| it('keeps local metric collection enabled by default', () => { | ||
| expect(resolveMetricsCollectorConfig(undefined, {})).toEqual({ enabled: true }); | ||
| }); | ||
|
|
||
| it('disables local collection without changing OTLP metrics.enabled', () => { | ||
| expect(resolveMetricsCollectorConfig({ | ||
| telemetry: { metrics: { enabled: true, collectionEnabled: false } }, | ||
| }, {})).toEqual({ enabled: false }); | ||
| }); | ||
|
|
||
| it.each([ | ||
| ['1', true], | ||
| ['true', true], | ||
| ['0', false], | ||
| ['false', false], | ||
| ])('accepts environment toggle %j', (value, enabled) => { | ||
| expect(resolveMetricsCollectorConfig(undefined, { | ||
| DKG_METRICS_COLLECTION_ENABLED: value, | ||
| })).toEqual({ enabled }); | ||
| }); | ||
|
|
||
| it('gives the environment override precedence over config', () => { | ||
| expect(resolveMetricsCollectorConfig({ | ||
| telemetry: { metrics: { collectionEnabled: false } }, | ||
| }, { DKG_METRICS_COLLECTION_ENABLED: '1' })).toEqual({ enabled: true }); | ||
| }); | ||
|
|
||
| it('rejects invalid environment values', () => { | ||
| expect(() => resolveMetricsCollectorConfig(undefined, { | ||
| DKG_METRICS_COLLECTION_ENABLED: 'yes', | ||
| })).toThrow(/DKG_METRICS_COLLECTION_ENABLED/); | ||
| }); | ||
|
|
||
| it('rejects non-boolean config values', () => { | ||
| expect(() => resolveMetricsCollectorConfig({ | ||
| telemetry: { metrics: { collectionEnabled: 'yes' } }, | ||
| } as never, {})).toThrow(/telemetry\.metrics\.collectionEnabled/); | ||
| }); | ||
| }); | ||
|
|
||
| describe('formatMetricsCollectorStartupLog', () => { | ||
| it('preserves the enabled startup log', () => { | ||
| expect(formatMetricsCollectorStartupLog({ enabled: true })) | ||
| .toBe('Metrics collector started (30s interval)'); | ||
| }); | ||
|
|
||
| it('reports when local collection is disabled', () => { | ||
| expect(formatMetricsCollectorStartupLog({ enabled: false })) | ||
| .toBe('Metrics collector disabled'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 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(...)orcreateLocalMetricsCollectorLifecycle(...)abstraction that resolves the toggle, starts when enabled, formats/logs startup, and exposes a uniformstop()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 astopno-op when disabled. Preserve default enabled behavior, disabled no-start behavior, the startup log text, and cleanup behavior.