Only report new log lines with check_logfile bookmarks - #1406
Merged
Conversation
check_logfile read the entire file on every run, so a single ERROR line kept failing the check for as long as it stayed in the file (#561). The only way to get each line reported once was real-time monitoring, which has to be configured on the agent; the polled path had no equivalent of the bookmarks check_eventlog gained. Add a `bookmark` option which remembers, per file and per bookmark name, how far the previous check read and resumes from there. As in check_eventlog the value is optional: `bookmark` (or `bookmark=auto`) derives the name from the file plus the filter, warning and critical expressions so two checks over one file do not consume each other's lines. Positions live in the module and are persisted to the core storage on unload, so a restart does not re-report everything. Rotation is detected by size *and* by a fingerprint (FNV-1a over the first bytes) of the file: a size comparison alone - what the real-time path does - misses a file replaced under the same name which is already larger than the stored offset. The fingerprint length is stored next to the hash so both sides always hash the identical byte range, which would otherwise look like a rotation on every check while a file shorter than the cap is still growing. A trailing line which is not terminated yet is held back and the offset parks in front of it, so a half-written line is reported once, in full, by the check which sees its terminator rather than twice. Checks without a bookmark neither read nor advance any position and behave exactly as before. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Michael Medin <michael@medin.name>
check_logfile had no way to look at part of a file: it read every line, or - with a bookmark - every line added since the last check (#583). A check which only cares about recent entries had to pull a whole log through the filter, and a file written with the newest entry at the top, as hand-maintained changelogs often are, could not be limited at all. Add `max-lines=N`, which examines only the newest N lines of each file, and `newest=last|first`, which says which end of the file those lines are at. The selected lines are always matched in file order, so %(list) reads the way the file does. The limit bounds the reading, not just the matching: without a bookmark the check seeks to the start of the last N records instead of loading the file, so tailing a multi-gigabyte log costs a few kilobytes. A delimiter which can overlap itself (`aaa`, `--`) cannot be located from the end - records are split by a left-to-right scan which consumes each delimiter whole, so `aaaa` split on `aaa` is one delimiter at offset 0, not one at offset 1 - and those files fall back to being read in full, with the surplus records dropped afterwards. Same result, more I/O. With a bookmark the limit caps how many of the new lines are reported when a burst arrives at once, and the lines it drops are still consumed: the stored position moves past everything which was read, so they do not come back as new on the next check. `newest=first` is rejected together with a bookmark, since a file rewritten from the top changes its fingerprint on every write and the bookmark would re-read it in full every single time. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Michael Medin <michael@medin.name>
Four things the review of #1406 turned up, all in the new bookmark path. An explicitly empty value (`bookmark=`) silently disabled bookmarking even though the documentation says an empty value means `auto`. The REST API renders a valueless parameter as a bare token so it never hit this, but the client-query path passes `k=v` through verbatim and a check written that way quietly went back to reporting the whole file on every run. The option now uses a notifier instead of binding a variable, which is what tells "given without a value" apart from "not given at all", and maps the empty value to `auto`. Positions moved as each file was read, so a check which failed on a later file - a second `file=` which cannot be opened - returned an error having already consumed the lines of the files before it, and nothing ever reported them. Positions are now collected and applied only once every file has been read. Nothing bounded the stored state. A bookmark name comes from the caller and an automatic one changes with the filter, so a host which generates names grew nsclient.db without end. Positions are now held in a bounded LRU map (1000 file/bookmark pairs); a position which ages out is dropped from the live set and blanked in the store on the next shutdown, and its file is simply read in full the next time that name appears. The automatic name also carries a hash of the filter, warning and critical expressions rather than the expressions themselves, so a filter of arbitrary length and content no longer ends up in the persisted key. Finally the documentation now covers what a bookmark costs: a line is consumed when the check runs rather than when its result is submitted, positions are saved on a clean shutdown so a crash re-reports the backlog, a file whose last line has no terminator never reports that line until something appends to it, and `${total}` counts the lines examined rather than the lines in the file. Assisted-by: Claude Code:claude-opus-5 Signed-off-by: Michael Medin <michael@medin.name>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #561.
check_logfilereads the entire file on every run, so a singleERRORline keeps failing the check for as long as it stays in the file. The only way to get each line reported once was real-time monitoring, which has to be configured on the agent; the polled path had no equivalent of the bookmarkscheck_eventloggained.The change
A
bookmarkoption which remembers, per file and per bookmark name, how far the previous check read:As in
check_eventlogthe value is optional:bookmark(orbookmark=auto) derives the name from the file plus thefilter/warning/criticalexpressions, so two checks over one file do not consume each other's lines. Positions live in the module and are persisted to the core storage (logfile.bookmarks) on unload, so a restart does not re-report everything.Design notes
file_reader::compute_seek— misses a file replaced under the same name which is already larger than the stored offset. The fingerprint length is stored next to the hash so both sides always hash the identical byte range; hashingmin(size, 256)on both sides would look like a rotation on every check while a file shorter than the cap is still growing.check_eventlogbehaves on a fresh bookmark.bookmarkneither read nor advance any position and behave exactly as before, so an ad-hoc full scan can be run at any time.Tests
check_logfile_test— 25 new unit tests over the pure state logic (hash stability, serialize/parse incl. rejecting a negative offset that would wrap into a huge one, and every resume/restart/skip decision). 35/35 pass.tests/checklogfile-commands.test.ts— 10 integration tests over REST (which also pins that a barebookmarktoken is accepted), covering incremental reporting, the held-back partial line, truncation, replacement by a larger file, separate/auto names, and mixing bookmarked with unbookmarked checks. One case drives twonscp client --bootprocesses against a shareddata-pathto prove the position survives a restart. 10/10 pass.Docs:
docs/samples/CheckLogFile_check_logfile_{desc,samples}.md, with output captured from real runs.Caveat
An
autoname embeds the filter text, so editing a filter starts a new position and leaves the old entry behind innsclient.db. That is noted in the docs;check_eventloghas the same property.🤖 Generated with Claude Code