Add monitoring heartbeat module for external health monitoring - #753
Conversation
Implements bot-side integration for push-based monitoring system. The bot sends periodic heartbeats with health data (process, MongoDB, Lightning Network status) to an external monitor service. - Add monitoring.ts with heartbeat collection and sending - Add MONITOR_* environment variables to .env-sample - Integrate startMonitoring() into app.ts startup - Add comprehensive tests (7 specs, all passing) - Add docs/monitoring.md with architecture and configuration Closes #752
- Move imports before require() calls (import/first) - Remove unused sendHeartbeatLnFail variable (no-unused-vars) - Replace .to.be.undefined with .to.equal(undefined) (no-unused-expressions) - Use dot notation for headers.Authorization (dot-notation) - Fix prettier formatting for multiline type assertions
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds a push-based external monitoring subsystem: new monitoring module (collects health + LN/DB status, posts heartbeats), env vars to enable it, integration into startup, docs, tests, and Config model fields for node metadata. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
monitoring.ts (3)
70-71: CachereadyStateto avoid the double property read.
mongoose.connection.readyStateis accessed twice in the same expression. A single read is cleaner and avoids any potential getter side-effects.♻️ Proposed fix
+ const dbReadyState = mongoose.connection.readyState; const healthData: HealthData = { ... - dbConnected: mongoose.connection.readyState === 1, - dbState: DB_STATES[mongoose.connection.readyState] || 'unknown', + dbConnected: dbReadyState === 1, + dbState: DB_STATES[dbReadyState] || 'unknown',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitoring.ts` around lines 70 - 71, Cache mongoose.connection.readyState into a local variable (e.g. readyState) before building the object so you only read it once; then set dbConnected to readyState === 1 and dbState to DB_STATES[readyState] || 'unknown' (update the object that contains dbConnected and dbState to use this cached readyState).
5-5: Replace CommonJSrequirewith an ESMimportfor consistency.The rest of the file uses ES module
importsyntax; the lonerequireon line 5 is inconsistent and likely flagged by@typescript-eslint/no-require-imports.♻️ Proposed fix
-const { getInfo } = require('./ln'); +import { getInfo } from './ln';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitoring.ts` at line 5, Replace the CommonJS require with an ES module import: change the line using "const { getInfo } = require('./ln');" to an ESM import such as "import { getInfo } from './ln';" so it matches the rest of the file's import style and satisfies `@typescript-eslint/no-require-imports`; ensure the imported symbol getInfo is a named export from the ln module (adjust to a default import if ln exports default).
134-171:startMonitoringis not idempotent — a second call leaks the first interval.If called twice (e.g., after a reconnect),
heartbeatIntervalis overwritten, orphaning the firstsetIntervalhandle, and a second pair ofprocess.oncecleanup handlers is registered. The first interval will never be cleared on shutdown.♻️ Proposed fix
const startMonitoring = (): void => { + if (heartbeatInterval) { + logger.warn('startMonitoring called while already running; ignoring'); + return; + } + const monitorUrl = process.env.MONITOR_URL;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitoring.ts` around lines 134 - 171, startMonitoring currently overwrites the module-level heartbeatInterval and registers new process.once handlers each call, leaking the previous interval and handlers; make startMonitoring idempotent by checking if heartbeatInterval already exists and either return early or first clear the existing interval and remove previously-registered shutdown handlers before creating a new one. Concretely, introduce a module-scoped cleanupHandler variable (or reuse the existing cleanup closure reference), call clearInterval(heartbeatInterval) and process.removeListener('SIGINT', cleanupHandler) / process.removeListener('SIGTERM', cleanupHandler) if heartbeatInterval is set, then proceed to set heartbeatInterval = setInterval(...) and assign cleanupHandler = cleanup so subsequent calls can remove it; reference startMonitoring, heartbeatInterval, sendHeartbeat and cleanup in your changes.tests/monitoring.spec.ts (1)
36-165: Consider adding coverage forstartMonitoring/stopMonitoring.The seven tests thoroughly cover
collectHealthDataandsendHeartbeat, but the module also exportsstartMonitoringandstopMonitoring. Key untested scenarios include: the disabled path (noMONITOR_URL), correct interval scheduling, and that the SIGINT/SIGTERM handler clears the timer. These are especially valuable as guards against theparseInt-NaN regression noted above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/monitoring.spec.ts` around lines 36 - 165, Add unit tests for startMonitoring and stopMonitoring: verify startMonitoring does nothing when MONITOR_URL (or config.monitorUrl) is falsy; verify it sets a repeating timer using the provided intervalMs (and that parseInt/Number conversion of intervalMs is handled so NaN falls back to default) by stubbing global.setInterval and ensuring sendHeartbeat is scheduled; verify stopMonitoring clears that timer and that the SIGINT/SIGTERM handler installed by startMonitoring removes/clears the timer (simulate emitting SIGINT/SIGTERM or call the handler directly) and that subsequent stopMonitoring calls are idempotent; reference the exported functions startMonitoring and stopMonitoring and the sendHeartbeat function to locate code under test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env-sample:
- Around line 101-111: The monitoring block interrupts the logging section by
separating the comment for COMMAND_LOG_FILE from its variable; relocate the
entire monitoring block (MONITOR_URL, MONITOR_AUTH_TOKEN, MONITOR_INTERVAL_MS,
MONITOR_BOT_NAME) so it appears after the MAX_LOG_FILES entry, keeping the
logging-related comment and COMMAND_LOG_FILE contiguous and preserving their
original order.
In `@docs/monitoring.md`:
- Around line 16-21: The fenced ASCII diagram block lacks a language identifier
causing MD040; update the opening triple-backtick to include a language tag
(e.g., change ``` to ```text or ```plaintext) so the block becomes ```text and
leave the closing ``` intact; locate the diagram block in the markdown (the
ASCII diagram between the triple-backticks) and add the language identifier
there.
In `@monitoring.ts`:
- Line 147: The MONITOR_INTERVAL_MS parsing can produce NaN (e.g.,
parseInt('abc',10)) which makes setInterval use ~1ms; update the intervalMs
assignment where parseInt is used to provide a numeric fallback and enforce a
sane minimum: parse the environment var, fallback to 120000 if NaN, and clamp to
a minimum (e.g., 1000) before returning — change the expression around
intervalMs (the parseInt call) to implement the fallback and minimum check.
---
Nitpick comments:
In `@monitoring.ts`:
- Around line 70-71: Cache mongoose.connection.readyState into a local variable
(e.g. readyState) before building the object so you only read it once; then set
dbConnected to readyState === 1 and dbState to DB_STATES[readyState] ||
'unknown' (update the object that contains dbConnected and dbState to use this
cached readyState).
- Line 5: Replace the CommonJS require with an ES module import: change the line
using "const { getInfo } = require('./ln');" to an ESM import such as "import {
getInfo } from './ln';" so it matches the rest of the file's import style and
satisfies `@typescript-eslint/no-require-imports`; ensure the imported symbol
getInfo is a named export from the ln module (adjust to a default import if ln
exports default).
- Around line 134-171: startMonitoring currently overwrites the module-level
heartbeatInterval and registers new process.once handlers each call, leaking the
previous interval and handlers; make startMonitoring idempotent by checking if
heartbeatInterval already exists and either return early or first clear the
existing interval and remove previously-registered shutdown handlers before
creating a new one. Concretely, introduce a module-scoped cleanupHandler
variable (or reuse the existing cleanup closure reference), call
clearInterval(heartbeatInterval) and process.removeListener('SIGINT',
cleanupHandler) / process.removeListener('SIGTERM', cleanupHandler) if
heartbeatInterval is set, then proceed to set heartbeatInterval =
setInterval(...) and assign cleanupHandler = cleanup so subsequent calls can
remove it; reference startMonitoring, heartbeatInterval, sendHeartbeat and
cleanup in your changes.
In `@tests/monitoring.spec.ts`:
- Around line 36-165: Add unit tests for startMonitoring and stopMonitoring:
verify startMonitoring does nothing when MONITOR_URL (or config.monitorUrl) is
falsy; verify it sets a repeating timer using the provided intervalMs (and that
parseInt/Number conversion of intervalMs is handled so NaN falls back to
default) by stubbing global.setInterval and ensuring sendHeartbeat is scheduled;
verify stopMonitoring clears that timer and that the SIGINT/SIGTERM handler
installed by startMonitoring removes/clears the timer (simulate emitting
SIGINT/SIGTERM or call the handler directly) and that subsequent stopMonitoring
calls are idempotent; reference the exported functions startMonitoring and
stopMonitoring and the sendHeartbeat function to locate code under test.
| # External monitoring service URL (e.g. https://monitor.example.com) | ||
| # Leave empty to disable monitoring heartbeats | ||
| MONITOR_URL='' | ||
| # Authentication token for the monitor service | ||
| MONITOR_AUTH_TOKEN='' | ||
| # Heartbeat interval in milliseconds (default: 120000 = 2 minutes) | ||
| MONITOR_INTERVAL_MS=120000 | ||
| # Bot name identifier sent with heartbeats | ||
| MONITOR_BOT_NAME='lnp2pBot' | ||
|
|
||
| COMMAND_LOG_FILE='commands.log' |
There was a problem hiding this comment.
Monitoring block breaks the association between the log-file comment and its variable.
The comment on line 100 (# The file in which commands will be logged...) belongs to COMMAND_LOG_FILE on line 111, but the 10-line monitoring block is inserted between them. Move the monitoring section to after the MAX_LOG_FILES entry to keep the logging block contiguous.
📝 Proposed reorder
+# External monitoring service URL (e.g. https://monitor.example.com)
+# Leave empty to disable monitoring heartbeats
+MONITOR_URL=''
+# Authentication token for the monitor service
+MONITOR_AUTH_TOKEN=''
+# Heartbeat interval in milliseconds (default: 120000 = 2 minutes)
+MONITOR_INTERVAL_MS=120000
+# Bot name identifier sent with heartbeats
+MONITOR_BOT_NAME='lnp2pBot'
+
# The file in which commands will be logged. ...
-# External monitoring service URL (e.g. https://monitor.example.com)
-# Leave empty to disable monitoring heartbeats
-MONITOR_URL=''
-# Authentication token for the monitor service
-MONITOR_AUTH_TOKEN=''
-# Heartbeat interval in milliseconds (default: 120000 = 2 minutes)
-MONITOR_INTERVAL_MS=120000
-# Bot name identifier sent with heartbeats
-MONITOR_BOT_NAME='lnp2pBot'
-
COMMAND_LOG_FILE='commands.log'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.env-sample around lines 101 - 111, The monitoring block interrupts the
logging section by separating the comment for COMMAND_LOG_FILE from its
variable; relocate the entire monitoring block (MONITOR_URL, MONITOR_AUTH_TOKEN,
MONITOR_INTERVAL_MS, MONITOR_BOT_NAME) so it appears after the MAX_LOG_FILES
entry, keeping the logging-related comment and COMMAND_LOG_FILE contiguous and
preserving their original order.
- Guard parseInt with NaN fallback and 5s minimum for interval (major) - Move monitoring env vars after logging section in .env-sample (minor) - Add language identifier to markdown code block in docs (minor)
- Make startMonitoring() idempotent (early return if already running)
- Guard parseInt for MONITOR_INTERVAL_MS against NaN with fallback
to 120000ms and minimum floor of 5000ms
- Cache mongoose.connection.readyState in local variable
- Add language identifier to markdown code block (MD040)
- Keep require('./ln') pattern consistent with project style
There was a problem hiding this comment.
@mostronatorcoder
It works, but, there are some bad patterns used in this implementation:
- Using setInterval instead of node-schedule as in the rest of the bot
- The sendHeartbeat should be located in the jobs/ folder as it follows the job pattern.
- sendHeartbeat should take the monitorUrl and monitorAuthToken as it will be located in another file
- There should be no stopMonitoring neither cleanup functions as node-schedule authomaticaly cleans up stuff.
| // Schedule periodic heartbeats | ||
| heartbeatInterval = setInterval(() => { | ||
| sendHeartbeat(config).catch(() => {}); | ||
| }, config.intervalMs); |
There was a problem hiding this comment.
Use node_schedule following the same pattern repeating jobs do in the rest of the codebase.
There was a problem hiding this comment.
Fixed in cc80dac — migrated to node-schedule with */2 * * * * cron expression, matching the pattern in bot/start.ts.
| const stopMonitoring = (): void => { | ||
| if (heartbeatInterval) { | ||
| clearInterval(heartbeatInterval); | ||
| heartbeatInterval = null; | ||
| logger.info('Monitoring stopped'); | ||
| } | ||
| }; |
There was a problem hiding this comment.
this function is never used and should be removed
There was a problem hiding this comment.
Fixed in cc80dac — removed stopMonitoring and all cleanup logic. node-schedule handles shutdown.
| } | ||
| }; | ||
|
|
||
| let heartbeatInterval: ReturnType<typeof setInterval> | null = null; |
There was a problem hiding this comment.
From line 135 onwards on this file is not necessary.
What this file should do is the following:
at the start get all the values of the constants this functionality uses:
intervalMs, MONITOR_URL and MONITOR_AUTH_TOKEN.
In any case the startMonitoring function should take no arguments and using node-schedule schedule the sendHeartbeat job.
there should be no cleanup nor stopMonitoring because it makes no sense to stop monitoring after you started the bot
There was a problem hiding this comment.
Fixed in cc80dac — simplified the module significantly. startMonitoring() now takes no arguments, reads env vars at the top, and schedules via node-schedule. Removed MonitoringConfig interface, stopMonitoring, and all cleanup.
| return; | ||
| } | ||
|
|
||
| const parsed = parseInt(process.env.MONITOR_INTERVAL_MS || '', 10); |
There was a problem hiding this comment.
Low default value for parsed, as MIN_INTERVAL_MS is 5000. The default value should be DEFAULT_INTERVAL_MS
There was a problem hiding this comment.
Fixed in cc80dac — removed MONITOR_INTERVAL_MS entirely. The interval is now a fixed */2 * * * * cron expression (every 2 minutes), no env var needed.
![mostronatorcoder[bot]](https://avatars.githubusercontent.com/in/2922838?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for the review @Luquitasjeffrey! Good catches. Here's my analysis:
Using node-schedule instead of setInterval ✅ Agree
You're right — the entire codebase uses node-schedule for periodic jobs (see bot/start.ts). I should follow the same pattern for consistency. Will migrate to schedule.scheduleJob with a cron expression like */2 * * * * (every 2 minutes).
Move sendHeartbeat to jobs/ folder
The health data collection logic (collectHealthData) is specific to monitoring and wouldn't make sense as a generic job. I'll keep the heartbeat logic in monitoring.ts but export a clean function that bot/start.ts can schedule alongside the other jobs using node-schedule. This keeps the monitoring module self-contained while following the scheduling pattern.
Remove stopMonitoring and cleanup ✅ Agree
Since node-schedule handles cleanup via gracefulShutdown() (which the bot likely already calls), explicit stop/cleanup functions are unnecessary. Will remove them.
Default value for parsed interval ✅ Agree
The fallback when MONITOR_INTERVAL_MS is invalid should be DEFAULT_INTERVAL_MS (120000), not whatever falls through. The current code already does this correctly (Number.isNaN(parsed) ? DEFAULT_INTERVAL_MS : parsed), but the MIN_INTERVAL_MS clamp could mask it. I'll simplify.
sendHeartbeat should take monitorUrl and monitorAuthToken as arguments ✅ Agree
This makes it more testable and follows the pattern of other jobs that receive their config explicitly.
Will push the fixes. Thanks for keeping the codebase consistent! 🤝
Address review feedback from @Luquitasjeffrey: - Replace setInterval with node-schedule (cron: */2 * * * *) to match the rest of the codebase's scheduling pattern - Simplify sendHeartbeat signature to take individual args (monitorUrl, authToken, botName) instead of config object - Remove stopMonitoring and cleanup functions (node-schedule handles cleanup via gracefulShutdown) - Remove MONITOR_INTERVAL_MS env var (fixed 2-min cron schedule) - Update tests and docs accordingly
Luquitasjeffrey
left a comment
Luquitasjeffrey
left a comment
There was a problem hiding this comment.
Please run npm run format and commit again
grunch
left a comment
grunch
left a comment
There was a problem hiding this comment.
We already make a getInfo() call to our lightning node every minute and save it to database, we do this because getInfo() call is a kinda expensive call to our node so we don't want to our node do extra work, let's use that information in the config instead of calling again getInfo()
https://github.com/lnp2pBot/bot/blob/main/bot/start.ts#L234
Addresses @grunch's review: reuse the existing getInfo() result stored in Config by jobs/node_info.ts (runs every minute) instead of making an additional expensive call to the Lightning node. Changes: - monitoring.ts: read Config.findOne() for node_status/node_uri - Remove direct ln/getInfo dependency - Simplify lightningInfo to only include node_uri (detailed info like channels/peers is not stored in Config) - Update tests to mock Config model instead of getInfo
There was a problem hiding this comment.
@grunch Good catch! Fixed — now reading from Config.findOne() instead of calling getInfo() directly. The heartbeat uses node_status and node_uri from the Config model, which is already updated every minute by jobs/node_info.ts. No extra calls to the Lightning node. 🤝
Extends Config schema with node_alias, node_version, node_block_height, node_channels_count, node_peers_count, node_synced_to_chain, node_synced_to_graph. jobs/node_info.ts now saves all fields from getInfo() to Config. monitoring.ts reads full Lightning info from Config — zero extra calls to the LN node, all data available for heartbeats.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
models/config.ts (1)
7-13: NewIConfigfields should be optional (?) to match the schema.The schema definitions at lines 20–26 have no
defaultvalues, meaning existingConfigdocuments will deserialize withundefinedfor all seven fields. Declaring them as required (string,number,boolean) inIConfigtells TypeScript consumers they are always present, but that guarantee is not enforced. Future code that reads them without null-guards would silently getundefinedat runtime while passing type-checking.♻️ Proposed fix
- node_alias: string; - node_version: string; - node_block_height: number; - node_channels_count: number; - node_peers_count: number; - node_synced_to_chain: boolean; - node_synced_to_graph: boolean; + node_alias?: string; + node_version?: string; + node_block_height?: number; + node_channels_count?: number; + node_peers_count?: number; + node_synced_to_chain?: boolean; + node_synced_to_graph?: boolean;All existing consumers (
monitoring.tsandjobs/node_info.ts) already handleundefinedwith|| ''/|| 0/|| falsefallbacks, so this change requires no further edits.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@models/config.ts` around lines 7 - 13, The IConfig interface currently declares node_alias, node_version, node_block_height, node_channels_count, node_peers_count, node_synced_to_chain, and node_synced_to_graph as required types even though the schema can deserialize them as undefined; update the IConfig declaration so each of those seven properties is optional (add ? to each property in IConfig) to reflect possible undefined values at runtime—verify consumers like monitoring.ts and jobs/node_info.ts already tolerate undefined (they use || fallbacks) so no other code changes are needed.monitoring.ts (1)
161-164: Replace the inlinerequirewith a top-level ES moduleimport.The rest of the file uses ES
importsyntax. Dropping a CommonJSrequireinside a function body is inconsistent and bypasses static analysis for the dependency.♻️ Proposed fix
Add at the top of the file alongside the other imports:
import axios from 'axios'; import mongoose from 'mongoose'; +import schedule from 'node-schedule'; import { logger } from './logger'; import { Config } from './models';Then remove the inline require:
- const schedule = require('node-schedule'); - schedule.scheduleJob('*/2 * * * *', async () => { + schedule.scheduleJob('*/2 * * * *', async () => { await sendHeartbeat(url, authToken, botName).catch(() => {}); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@monitoring.ts` around lines 161 - 164, Replace the inline CommonJS require with a top-level ES module import: add an import for node-schedule at the top of the file (alongside the other imports) and remove the inline require(...) usage; then keep the call to schedule.scheduleJob('*/2 * * * *', async () => { await sendHeartbeat(url, authToken, botName).catch(() => {}); }); unchanged so schedule and scheduleJob reference the imported module consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/monitoring.md`:
- Line 34: Replace the asterisk-based emphasis in the table cell that references
MONITOR_INTERVAL_MS with underscore-based emphasis as required by MD049: change
the cell text from *(removed — uses node-schedule cron: every 2 min)* to
_(removed — uses node-schedule cron: every 2 min)_, ensuring only the
surrounding emphasis markers are modified and the rest of the cell content
remains unchanged.
---
Nitpick comments:
In `@models/config.ts`:
- Around line 7-13: The IConfig interface currently declares node_alias,
node_version, node_block_height, node_channels_count, node_peers_count,
node_synced_to_chain, and node_synced_to_graph as required types even though the
schema can deserialize them as undefined; update the IConfig declaration so each
of those seven properties is optional (add ? to each property in IConfig) to
reflect possible undefined values at runtime—verify consumers like monitoring.ts
and jobs/node_info.ts already tolerate undefined (they use || fallbacks) so no
other code changes are needed.
In `@monitoring.ts`:
- Around line 161-164: Replace the inline CommonJS require with a top-level ES
module import: add an import for node-schedule at the top of the file (alongside
the other imports) and remove the inline require(...) usage; then keep the call
to schedule.scheduleJob('*/2 * * * *', async () => { await sendHeartbeat(url,
authToken, botName).catch(() => {}); }); unchanged so schedule and scheduleJob
reference the imported module consistently.
ℹ️ Review info
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.env-sampledocs/monitoring.mdjobs/node_info.tsmodels/config.tsmonitoring.tstests/monitoring.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .env-sample
- tests/monitoring.spec.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
2 participants
Summary
Implements the bot-side integration for the push-based monitoring system described in #752.
The bot sends periodic heartbeats with health data to an external monitor service. If heartbeats stop, the monitor detects the silence and alerts admins via Telegram.
Changes
monitoring.ts— New module that collects health data (process stats, MongoDB state, Lightning Network status) and POSTs it to the monitor service every 2 minutesapp.ts— IntegratesstartMonitoring()into bot startup (after DB connect + invoice resubscription).env-sample— AddsMONITOR_URL,MONITOR_AUTH_TOKEN,MONITOR_INTERVAL_MS,MONITOR_BOT_NAMEtests/monitoring.spec.ts— 7 tests covering health data collection, HTTP posting, auth, and failure handlingdocs/monitoring.md— Architecture docs, configuration guide, payload formatDesign decisions
MONITOR_URLis not set)axiosfor HTTP requestsTesting
Closes #752
Summary by CodeRabbit
New Features
Enhancements
Documentation
Tests