Di.heartbeat - #109
Conversation
…r dep example found during manual test with a real di.timer
… of their own log messages
…s. Following code guidelines
| normlog:{[logdict] | ||
| / detect kx.log instance by presence of kx.log-specific keys (getlvl, sinks, fmts) | ||
| / kx.log functions are monadic - wrap each into binary {[c;m]} and embed context in the message | ||
| / plain {[c;m]} log dicts (info`warn`error only) pass through unchanged |
There was a problem hiding this comment.
Single-slash / is used for inline comments throughout this file. In q, a lone / at the start of a line opens a multiline comment block, which will consume all following lines until a matching \ is found. Use // for inline/end-of-line comments to avoid accidentally commenting out code.
There was a problem hiding this comment.
The code is not all commented out
| }; | ||
|
|
||
| / extract a required dependency dictionary, erroring immediately if absent or null | ||
| requiredep:{[deps;name] |
There was a problem hiding this comment.
requiredep returns ()!() (an empty dict) and then checks not count d. An empty dict has count 0, so any dep that resolves to an empty dict — e.g. a timer or pubsub dict with no keys — will produce the error message "di.heartbeat: ",(string name)," dependency is required; pass it via init deps - see di.",string name where the last string name is wrong: the error message suffix says "di.",(string name) instead of "di.heartbeat", making the error message misleading for servers, handlers, etc.
There was a problem hiding this comment.
di.heartbeat: <name> dependency is required and must be a non-empty function dict; pass it via init deps - see heartbeat.md
| timerdict:requiredep[deps;`timer]; | ||
| .z.m.timeraddjob:timerdict`addjob; | ||
| .z.m.timerdeletejobs:timerdict`deletejobs; | ||
| pubsubdict:requiredep[deps;`pubsub]; |
There was a problem hiding this comment.
setdeps checks if[not log in key deps;...]and thenif[99h<>type depslog;...], but both of these come after the earlier if[99h<>type deps;...] guard. However, between those two checks the code does timerdict:requiredep[deps;timer] which calls `` depstimer — a key lookup on `deps`. If `deps` passes the `99h` check but does not contain timer ``, requiredepreturns()!()with a count of 0 and throws a confusingly-suffixed error (see prior finding). More critically,requiredep accesses `` timerdictaddjob and timerdictdeletejobs `` without guarding against those keys being absent, which will throw a type` error rather than the intended descriptive error message.
There was a problem hiding this comment.
Added function-key validation to requiredep
| registerhandlers:{ | ||
| if[subenabled; | ||
| .z.m.handlersregister[`.z.pc;`heartbeat;closeconnection]]; | ||
| }; |
There was a problem hiding this comment.
In warn, the upsert only projects sym,procname,warning:1b — it does not include the error column. If a process is already in error:1b state and transitions through warn again (e.g. after the store is partially cleared), the error flag is not cleared. The intended semantics are that a fresh heartbeat clears both flags (done in storeheartbeat), but if warn is called independently the error flag is silently left set.
There was a problem hiding this comment.
warn now projects error:0b alongside warning:1b
| .z.m.hb:hb upsert select sym,procname,warning:1b from procs; | ||
| onwarning procs; | ||
| }; | ||
|
|
There was a problem hiding this comment.
subscribeone uses a projection {[h;e] ...}[h] as the error handler in @[f;x;h]. The outer lambda captures h from the enclosing scope via projection, but the first argument of that lambda is also named h, shadowing the outer h. The projection {[h;e]...}[h] passes the outer h as the first argument correctly, so this works — but only because the outer h is the function argument to subscribeone, not a free variable. This is fragile and potentially confusing; if the function is refactored, the shadowing could silently bind the wrong value.
There was a problem hiding this comment.
Renamed the error handler's parameter h→hdl so it no longer shadows the outer h. The projection [h] still supplies the handle; identical behaviour, just no shadowing to misread on a refactor.
| closeconnection:{[h] | ||
| .z.m.subscribedhandles:subscribedhandles except h; | ||
| }; | ||
|
|
There was a problem hiding this comment.
In addprocs, the seed table is built with time:cp[] (a scalar), counter:0N, pid:0Ni, host:, port:0Ni. Then it is keyed with 2! (keying on first 2 columns). The merge seed,hb uses the , (join) operator on two keyed tables — this will retain the seed row for any sym,procname key not already in hb, but will not overwrite existing rows in hb. That matches the documented intent. However, if proctypes and procnames are atoms rather than lists, proctypes,() and procnames,() produce lists of length 1 — this is correct. But time:cp[] is a scalar while all other columns are lists of length equal to count proctypes, so the table construction will either error or atom-extend depending on q version. time should be count[proctypes]#cp[] or (count proctypes)#enlist cp[] to be safe.
There was a problem hiding this comment.
counter/pid/host/port/warning/error are all atoms too; only sym/procname are lists. q broadcasts atom columns in a table literal to the list length (([]a:\xy;b:5)→b:5 5), which is stable core semantics, not version-dependent, and the addprocstests exercise it and pass.count[proctypes]#cp[] would produce an identical result, so I've left the idiomatic form.
| / flag processes that have not heartbeated within the warning / error grace periods - timer job | ||
| checkheartbeat:{ | ||
| / status: 0 healthy, 1 warning, 2+ error | ||
| / grace periods are computed as locals first - module functions do not resolve inside qsql |
There was a problem hiding this comment.
In checkheartbeat, wp and ep are computed with warningperiod each t\symanderrorperiod each t`sym, then used as: now>time+wp. Here wpandepare lists of timespans. The columntimeintis a list of timestamps. Adding a timespan to a timestamp is valid in q, butwpandepare computed *outside* theupdatestatement and then referenced as local variables inside it. Inside aupdate … from tfunctional form, bare names resolve to columns oftfirst;wpandepare not columns oftso they resolve to the module-level variables — but those module-levelwarningperiodanderrorperiodare *functions*, not the computed lists. The local variableswpandepare not in scope inside the qSQL statement. This meanstime+wpinside theupdatewill use the module-levelwp(which does not exist as a variable at module level — onlywarningperiod/errorperiod` are defined), causing a runtime error. The computation must be done outside qSQL and joined back, or the update must use functional form with the local values passed explicitly.
There was a problem hiding this comment.
This works and is covered by the suite. Locals resolve inside of qSQL; module-level functions do not
| t:0!hb; | ||
| wp:warningperiod each t`sym; | ||
| ep:errorperiod each t`sym; | ||
| stats:update status:(`short$now>time+wp)+`short$2*now>time+ep from t; |
There was a problem hiding this comment.
newwarn selects status=1 (warning) and newerr selects status>1 (error). A process can transition from healthy directly to error (status=2) and newerr will catch it, but newwarn won't. This is correct for new escalations. However, a process already in warning:1b that escalates to status=2 will match newerr (since not error is true), but it will also still have warning:1b set from before — err only upserts error:1b, it does not clear warning. The warning flag is never cleared except by storeheartbeat. This means a process can simultaneously have warning:1b and error:1b, which may be intentional but is worth flagging as err should probably also clear warning.
There was a problem hiding this comment.
Made the fix. Made err project warning:0b alongside error:1b, symmetric with the warn fix above
| registerhandlers:{ | ||
| if[subenabled; | ||
| .z.m.handlersregister[`.z.pc;`heartbeat;closeconnection]]; | ||
| }; |
There was a problem hiding this comment.
In warn and err, hb upsert select sym,procname,warning:1b from procs relies on upsert into a keyed table by key columns sym,procname. The projected table from procs only has those 3 columns, so the upsert will only update warning (or error) and leave all other columns unchanged. This is the intended behaviour, but note that if procs contains a sym,procname pair not yet in hb, upsert will insert a new row with all other columns null/default — including time:0Np, which would immediately look stale on the next check. This can happen if warn/err are called from a path that somehow produces rows not in the store.
There was a problem hiding this comment.
Added a comment to document this
| / seed the store with expected processes so a never-seen process is flagged | ||
| addprocs:{[proctypes;procnames] | ||
| / real heartbeats arriving later override these seeded rows | ||
| seed:2!([]sym:proctypes,();procname:procnames,();time:cp[];counter:0N;pid:0Ni;host:`;port:0Ni;warning:0b;error:0b); |
There was a problem hiding this comment.
init calls setconfig before setdeps. Inside setdeps, the guard if[subenabled; setmonitordeps deps] reads the module-level subenabled. Because setconfig runs first and writes .z.m.subenabled, this is correct. However, if init is called a second time and setconfig updates subenabled to 1b, setdeps will then call setmonitordeps — but if the second init call does not include servers/handlers in deps, it will throw. This is the documented behaviour, but it means re-running init with subenabled:1b and incomplete deps will throw after setconfig has already mutated module state (e.g. subenabled is now 1b) even though the init failed. State is left partially mutated on error.
There was a problem hiding this comment.
init is now atomic: setdeps validates every dependency (type, log keys, timer, pubsub, and servers/handlers when subenabled in deps) and only then writes any .z.m state; setconfig runs afterwards and can't throw. So a missing/malformed dep throws before a single byte of module state is mutated
| /version:"0.1.0"; | ||
|
|
||
| / public api - only the functions intended to be called externally are exported | ||
| export:([init;publishheartbeat;checkheartbeat;storeheartbeat;addprocs;subscribe;gethb;setcp]) |
There was a problem hiding this comment.
export:([init;publishheartbeat;checkheartbeat;storeheartbeat;addprocs;subscribe;gethb;setcp]) — this uses a single-column keyed table syntax (([…])). In standard TorQ/kdb-x module patterns, export is typically a list or dict of symbols. A single-column keyed table with no value columns is an unusual construct; verify this is the correct export form for the use/module system in use, otherwise functions may not be accessible via heartbeat.fn[] after use\di.heartbeat`.
There was a problem hiding this comment.
Non-issue. export is the standard kdb-x module form
| / injected dependencies via .z.m at every call site | ||
|
|
||
| / ============================================================ | ||
| / module state and defaults |
There was a problem hiding this comment.
The entire file is written outside any namespace (\d .heartbeat … \d . is absent). According to TorQ conventions, all module code should be enclosed in a namespace block. Without it, all definitions are created in the root namespace and are not isolated, which risks name collisions with other modules.
There was a problem hiding this comment.
nope. do not add \d. this is not how modules are loaded in kdb-x
DIReview Summary1 critical | 11 warning(s) | 0 suggestion(s)
|
Summary
Adds
di.heartbeat, a standalone kdb-x module extracted from TorQ'scode/common/heartbeat.q. Any process can publish a periodic heartbeat overpub/sub; monitoring processes detect when a process has stopped beating — i.e.
stalled or blocked — even when the underlying connection is still valid, and
escalate from a warning to an error.
Trello ticket: https://trello.com/c/093fht6Z/95-kdb-x-heartbeat
Files created
di/heartbeat/init.qdi/heartbeat/heartbeat.qdi/heartbeat/deps.qdi/heartbeat/heartbeat.mddi/heartbeat/test.csvHow to test
Coverage includes: all 8 exported functions, the publish / store / check flows,
warning→error escalation against a stale process, subscribe and
connection-close handling, dependency validation (all failure modes: non-dict
deps, missing/non-dict log, log missing a required level, missing timer, missing
pubsub, missing monitor servers/handlers), info-only-via-kx.log acceptance and
the normlog wrapping (with extra log levels passing through untouched), init
idempotency, and a real-process integration test driving a background publisher
over a live handle.
Design Choices
No hard dependencies - everything injected
Heartbeat has no hard module dependency.
log, timer and pubsub (plus servers/handlers when monitoring) are all
injected via init as dictionaries of functions, so the module is decoupled from
the concrete di.* implementations and deps.q declares no hard deps.
Single deps dict
Config values and the injected dependencies are passed together in a single
deps dict to init, consistent with the project dependency injection guidelines.
All config keys are optional with sensible defaults; the dependencies are
required and init errors immediately if any is missing or malformed.
Required log dependency with normlog
The log dep is required. info, warn and error are all required since the
module uses all three. The normlog internal function detects a kx.log instance
by the presence of getlvl, sinks, fmts keys and wraps its monadic functions
into the binary {[c;m]} contract automatically, so callers can pass a kx.log
instance directly without manual wrapping. Identical to di.eodtime's normlog.
Setter and getter functions
Direct assignment inside a module is not possible, so gethb
exposes the heartbeat store for inspection and setcp replaces the current-time
function at runtime.
Owns its own clock
The module keeps its own current-time function (cp, default .z.p) rather than
taking one from the timer dependency, so it does not rely on the timer exporting
a clock getter. setcp overrides it for deterministic tests / simulation -
verified in the suite by advancing simulated time to trigger warning then error.
Timer mode 2 to avoid catch-up storms
Heartbeat jobs are scheduled with mode 2 (period after the previous actual
start). A heartbeat means "alive now", so missed beats must not be replayed as a
catch-up storm (which mode 1 would do). Jobs are also registered before handing
off to the timer, which sidesteps di.timer not running jobs added after its first
tick.
Idempotent init
Init deletes its jobs (hbpublish/hbcheck/hbsubscribe) before re-registering,
so it is safe to call again; re-running also resets unset config keys to their
defaults.
Checklist
86/86 k4unit tests passing
Follows consistency.md and style.md
Follows dependency injection guidelines
Structure and conventions follow di.eodtime as the reference module
heartbeat.md documents all exported functions, config, usage examples and notes
No hard-dependency prerequisites (all dependencies injected)
Documentation
See heartbeat.md for full reference including dependency table, configuration
options, exported function documentation with examples, usage example, and notes.