Phase2: Implementation of metrics collectors for Dashboard - #464
Phase2: Implementation of metrics collectors for Dashboard#464kushagra0902 wants to merge 14 commits into
Conversation
…and ws connection management
…dex based incremental scanning)
…ted polling services
There was a problem hiding this comment.
Pull request overview
Implements Phase 2 of the dashboard service by introducing a dedicated HTTP/WebSocket app that serves KPI snapshots and periodically refreshes them from PostgreSQL-backed collectors, alongside DB migrations intended to support efficient KPI querying and change detection.
Changes:
- Added dashboard-service runtime (Express + WebSocket) with polling scheduler and snapshot caching.
- Implemented PostgreSQL KPI collector queries (events-by-kind, admitted users, sats paid, top talkers) plus snapshot/update-version plumbing.
- Added migrations for KPI query indexes and new triggers/state to detect changes, plus unit tests for scheduler/snapshot/app wiring.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/dashboard-service/snapshot-service.spec.ts | Adds unit coverage for snapshot refresh/sequence behavior and error propagation. |
| test/unit/dashboard-service/polling-scheduler.spec.ts | Adds unit coverage for non-overlapping polling behavior and failure recovery. |
| test/unit/dashboard-service/app.spec.ts | Adds an integration-style unit test asserting health/snapshot/ws endpoints are served. |
| src/dashboard-service/app.ts | Wires Express routes, WebSocket hub, snapshot service, and polling scheduler into a runnable service. |
| src/dashboard-service/config.ts | Adds dashboard-service CLI/env configuration parsing. |
| src/dashboard-service/index.ts | Adds dashboard-service entrypoint with lifecycle/shutdown handling. |
| src/dashboard-service/types.ts | Introduces dashboard KPI and websocket message types. |
| src/dashboard-service/services/snapshot-service.ts | Adds in-memory snapshot caching, fingerprinting, and optional revision-based skipping. |
| src/dashboard-service/services/kpi-collector-service.ts | Implements PostgreSQL-backed KPI aggregation queries. |
| src/dashboard-service/services/dashboard-update-version-service.ts | Reads dashboard revision from DB with fallback-on-error behavior. |
| src/dashboard-service/polling/polling-scheduler.ts | Adds recursive setTimeout scheduler to avoid overlapping polls. |
| src/dashboard-service/ws/dashboard-ws-hub.ts | Adds websocket connection handling and snapshot/tick broadcasts. |
| src/dashboard-service/api/dashboard-router.ts | Adds /api/v1/kpis/snapshot route wiring. |
| src/dashboard-service/controllers/get-health-controller.ts | Adds /healthz controller. |
| src/dashboard-service/controllers/get-kpi-snapshot-controller.ts | Adds KPI snapshot controller. |
| src/dashboard-service/handlers/request-handlers/get-health-request-handler.ts | Adds request handler wrapper for health route. |
| src/dashboard-service/handlers/request-handlers/get-kpi-snapshot-request-handler.ts | Adds request handler wrapper for KPI snapshot route. |
| package.json | Adds dashboard dev/start scripts and dashboard-focused unit test script. |
| migrations/20260412_020000_add_dashboard_kpi_indexes.js | Adds concurrent indexes intended to support KPI queries. |
| migrations/20260411_210100_add_dashboard_live_notify_triggers.js | Adds NOTIFY triggers for dashboard change events. |
| migrations/20260419_120000_add_dashboard_updated_revision.js | Adds dashboard_state + revision trigger mechanism for change detection. |
| migrations/20260415_010000_add_nip29_group_control_tables.js | Adds NIP-29 group control tables (unrelated to dashboard KPIs). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await knex.raw(` | ||
| CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_events_pubkey | ||
| ON events (event_pubkey); | ||
| `) |
There was a problem hiding this comment.
This migration creates idx_events_pubkey on events(event_pubkey), but the events table already defines an index on event_pubkey (created in the initial events table migration). Adding a second btree index on the same column increases write amplification and disk usage without improving plans. Consider dropping idx_events_pubkey and relying on the existing index (or change it to a composite/covering index if you need different ordering).
| await knex.raw(` | ||
| CREATE INDEX CONCURRENTLY IF NOT EXISTS idx_events_pubkey | ||
| ON events (event_pubkey); | ||
| `) |
There was a problem hiding this comment.
This migration creates idx_events_pubkey on events(event_pubkey), but the events table already has a btree index on event_pubkey from its original migration. A duplicate index increases insert/update cost and disk usage without improving query plans. Consider removing idx_events_pubkey (or changing it to a genuinely different composite/covering index if needed).
| UPDATE dashboard_state | ||
| SET | ||
| revision = revision + 1, | ||
| updated_at = NOW() | ||
| WHERE id = 1; |
There was a problem hiding this comment.
dashboard_updated() performs UPDATE dashboard_state ... WHERE id = 1 on every INSERT/UPDATE/DELETE statement for both events and users. Because event writes are typically single-row statements, this introduces a hot-row contention point and extra WAL writes on high-throughput relays. Consider using a sequence-based revision (nextval) or relying on LISTEN/NOTIFY (added separately) instead of updating a single shared row for every write.
| export class KPICollectorService { | ||
| public constructor( | ||
| private readonly dbClient: DatabaseClient, |
There was a problem hiding this comment.
KPICollectorService adds non-trivial SQL aggregation logic, but there are currently no unit tests referencing it anywhere under test/. Adding tests that validate the query result mapping (counts, sums, top talkers ordering) would help catch regressions in the dashboard KPIs.
| private snapshot: KPISnapshot = { | ||
| sequence: this.sequence, | ||
| generatedAt: new Date(0).toISOString(), | ||
| status: 'live', |
There was a problem hiding this comment.
SnapshotService initializes generatedAt to the Unix epoch and status to 'live'. If the first successful collection returns the default metrics, refresh() treats it as unchanged and will not update generatedAt/sequence, leaving a "live" snapshot with an epoch timestamp indefinitely. Consider initializing status as 'stale' (or forcing the first refresh to update the snapshot) so the first successful refresh always sets a real generatedAt.
| status: 'live', | |
| status: 'stale', |
| const toNumber = (value: unknown): number => { | ||
| if (typeof value === 'number') { | ||
| return value | ||
| } | ||
| if (typeof value === 'string' && value !== '') { | ||
| return Number(value) | ||
| } | ||
There was a problem hiding this comment.
toNumber() converts Postgres COUNT/BIGINT/NUMERIC string results via Number(value). For large relays this can exceed Number.MAX_SAFE_INTEGER and silently lose precision, producing incorrect KPI counts. Consider returning BigInt/string from the collector (and updating API types), or detect unsafe conversions and handle them explicitly.
| consttoNumber=(value: unknown): number=>{ | |
| if(typeofvalue==='number'){ | |
| returnvalue | |
| } | |
| if(typeofvalue==='string'&&value!==''){ | |
| returnNumber(value) | |
| } | |
| constINTEGER_STRING_PATTERN=/^-?\d+$/ | |
| consttoNumber=(value: unknown): number=>{ | |
| if(typeofvalue==='number'){ | |
| if(!Number.isFinite(value)){ | |
| thrownewRangeError(`Cannot convert non-finite number to KPI metric: ${value}`) | |
| } | |
| if(Number.isInteger(value)&&!Number.isSafeInteger(value)){ | |
| thrownewRangeError(`Cannot convert unsafe integer to KPI metric: ${value}`) | |
| } | |
| returnvalue | |
| } | |
| if(typeofvalue==='bigint'){ | |
| if(value>BigInt(Number.MAX_SAFE_INTEGER)||value<BigInt(Number.MIN_SAFE_INTEGER)){ | |
| thrownewRangeError(`Cannot convert bigint outside safe integer range to KPI metric: ${value.toString()}`) | |
| } | |
| returnNumber(value) | |
| } | |
| if(typeofvalue==='string'&&value!==''){ | |
| if(INTEGER_STRING_PATTERN.test(value)){ | |
| constbigintValue=BigInt(value) | |
| if(bigintValue>BigInt(Number.MAX_SAFE_INTEGER)||bigintValue<BigInt(Number.MIN_SAFE_INTEGER)){ | |
| thrownewRangeError(`Cannot convert integer string outside safe integer range to KPI metric: ${value}`) | |
| } | |
| returnNumber(bigintValue) | |
| } | |
| constnumericValue=Number(value) | |
| if(!Number.isFinite(numericValue)){ | |
| thrownewRangeError(`Cannot convert non-finite numeric string to KPI metric: ${value}`) | |
| } | |
| returnnumericValue | |
| } |
| .sum<{ total: string | null }>('balance as total') | ||
| .first() | ||
| const millisats = toNumber(result?.total) | ||
| return millisats / 1000 |
There was a problem hiding this comment.
SUM(users.balance) is over a BIGINT column, but getSatsPaidCount converts the result through Number(...) and divides by 1000. If the total exceeds JS safe integer range this will silently produce the wrong satsPaid. Consider fetching/handling the aggregate as BigInt/string and formatting for the API response.
| wsPath: '/api/v1/kpis/stream', | ||
| pollIntervalMs: 1000, | ||
| useDummyData: true, | ||
| collectorMode: 'full', |
There was a problem hiding this comment.
The config literal passed to createDashboardService includes collectorMode: 'full', but DashboardServiceConfig/createDashboardService don't define that option. With ts-node/register this can fail TS compilation due to excess property checks. Remove collectorMode from the test config (or add it to DashboardServiceConfig and implement it).
| collectorMode: 'full', |
Description
This PR delivers Phase 2 of the dashboard service by replacing placeholder KPI generation with real PostgreSQL-backed metrics. Appropriate indexs are also added in the database for efficient querying.
Related Issue
Solves issue #144 partially by implementing phase 2 of the proposed plan
Motivation and Context
The main motivation is discussed in the issue discussion. TLDR: It helps in solving the problem of monitoring Relay performance by adding a dashboard showing the KPIs discussed.
How Has This Been Tested?
Unit tests have been added for the appropriate files. Other than this manual testing involves spinnig up relay server, and dashboard together and monitoring the dashboard endpoints
Screenshots (if appropriate):
N/A
Types of changes
Checklist: