Update dependencies and performance optimizations - #41
Merged
Merged
Conversation
A queue that is neither durable nor exclusive is RabbitMQ's deprecated
transient_nonexcl_queues feature. That is no longer only a deprecation:
RabbitMQ 4 reports it as denied_by_default and refuses every
queue.declare with a connection exception. Because Connect() returns
false as soon as a declare fails, RabbitMQ does not merely lose those
queues - it fails to connect at all, and publishes nothing.
Verified against RabbitMQ 4.3.5:
rabbitmqctl list_deprecated_features
transient_nonexcl_queues | denied_by_default | denied
Durable queues have worked since AMQP 0-9-1, so this is the only setting
that works on every RabbitMQ version, from 3.x through 4.x.
This does not put monitoring events on disk. Queue durability and message
persistence are separate AMQP properties: durable stores the queue
*definition*, while a message is only written durably when its publisher
marks it persistent. SendMessage passes properties=nullptr, so every
message this module publishes is transient and stays that way. The
RabbitMQ documentation is explicit that transient messages "will be
discarded during recovery, even if they were stored in durable queues".
So the behaviour Statusengine wants is unchanged: the queues buffer in
RAM while no worker is connected, and a RabbitMQ restart empties them.
Measured rather than assumed - 5 messages in a durable queue, RabbitMQ
restart, queue present with 0 messages. Publishing 20,000 events took
0.22-0.23s with durable queues and 0.22-0.23s without, three runs each.
The exchange follows the queues. A transient exchange loses its bindings
on a RabbitMQ restart while the durable queues survive, so the pair is kept
consistent; both are metadata only and neither costs per-message I/O.
Both values remain configurable, so an installation that needs the old
behaviour can still set DurableQueues = false - on a RabbitMQ old enough to
accept it.
Tested with Naemon 1.4.1 against RabbitMQ 3.9.27, publishing to all
configured queues, alongside the Go worker consuming them. Because both
sides declare the same queues and AMQP answers a mismatched redeclare
with a 406 PRECONDITION_FAILED rather than reconciling it, this change
belongs with the matching one in Statusengine Go Worker; both start orders
were checked, module first and worker first, with no 406 either way.
Note for existing installations: the queues already exist as non-durable
and cannot be redeclared. They have to be deleted once, with the
monitoring core and the worker stopped. Messages waiting in them are
lost, which is acceptable for the same reason the design is - they are
transient and would not have survived a broker restart either.
The module runs inside the monitoring core's address space, so each of these
takes naemon down with it or grows unbounded over a long uptime.
EncodeString() had four independent defects and ran on every check output:
* the charset comparison was inverted, so the function did the opposite of
its job - non UTF-8 was passed through untouched while UTF-8 was sent
through a pointless conversion
* uchardet owns the string returned by uchardet_get_charset(), deleting it
corrupted the heap
* iconv() advances the output pointer, so the buffer was released from an
address in its middle
* iconv() returns the number of irreversible conversions, which was used as
if it were the output length
It now keeps separate cursors, derives the written length from how far the
output cursor moved, and falls back to the unchanged input when the charset
cannot be detected or iconv_open() fails. It moved to Encoding.{h,cpp}, which
depends on neither naemon nor IStatusengine, so it can be tested on its own;
conversion problems are reported through a callback that Nebmodule wires to
the log.
Strings stored in a check_result were allocated with new[] but are released by
naemon's free_check_result() with free(). get_json_string_c() now allocates
those with strndup, while get_json_string() keeps new[] for the strings that
stay on the C++ side. Assembling the plugin output moved into BuildCheckOutput()
and hands ownership over explicitly instead of aliasing, which also fixes the
leak of perf_data when only long_output was present.
RabbitmqClient::Connect() allocated a connection state and returned from nine
failure paths without destroying it. Since SendMessage() retries the connection
for every single message while the broker is unreachable, this leaked about 9 kB
per attempt - measured at 17.5 MB over 2000 attempts. CloseConnection() now also
tolerates a connection that was never opened, which is reachable when an earlier
handler fails during startup, and Worker() no longer touches a cleared handle.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
The move from cmake to meson in df3837f deleted CMakeLists.txt but left the infrastructure that used it behind, so both the CI and the documented developer workflow have been broken ever since: * .gitlab-ci.yml ran cmake, and was GitLab CI in a repository hosted on GitHub. All of its targets (ubuntu trusty/xenial/bionic, debian jessie/stretch) are end of life, and it never installed libuchardet-dev, which meson requires - so it would not have built even with CMakeLists.txt still in place. Replaced by a GitHub Actions workflow on ubuntu 22.04/24.04 and debian 12 that also covers -Dnagios=true, -Dgearman=false and -Drabbitmq=false; none of the build options were exercised before. * naemon.Dockerfile ran cmake too, so docker-compose up --build failed. It now uses meson, is based on ubuntu 24.04 instead of the EOL bionic, and builds naemon 1.5.2 instead of 1.0.10 from 2019. Meson installs into <prefix>/lib/<multiarch> by default, while statusengine.cfg and the README both expect <prefix>/lib. The module therefore ended up somewhere naemon would not load it from. Pinned with libdir=lib. The version existed four times over with three different values: meson.build said 4.2.0, VERSION said 4.0.2, Statusengine.cpp hardcoded 4.0.0 and the last release tag is v4.0.4. meson.build is now the single source and is generated into version.h; 4.0.4 was chosen because it is the version that was actually released - 4.2.0 was set once during the meson migration and never shipped. VERSION is gone, its only consumer was the CI that this commit replaces. Also switches to C++17 and replaces the deprecated get_pkgconfig_variable, and drops cmake from the README dependency lists, where it was no longer needed. The first line of .dockerignore was "/builddevtools/**", a merge of "/build" and "devtools/**" that ignored neither. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
LogStream's level check was not monotonic and got it wrong in both directions: at Level = Error warnings were still written, and at Level = Info - the most verbose setting - warnings were dropped entirely. LogLevel now has explicit ordinals and a single "message level >= configured level" comparison. The thirteen hand written operator<< overloads collapse into one template. The stream still starts at Info, because Configuration::Load() reports before SetLogLevel() can apply the configured level and that output is what explains a broker that fails to start. The vendored toml11 dates from 2018. Replaced with v4.4.0 as a single header, keeping it vendored rather than pulling in a wrap subproject so the build needs no network. Its API changed completely (toml::Table -> toml::value, cast<> and get_or gone, its own exception hierarchy that no longer derives from std::runtime_error), so Configuration::Load() was rewritten around explicit contains() checks and two helpers instead of control flow through out_of_range. The two identical private GetTomlDefault templates became one free function. Ownership moves to unique_ptr in Statusengine, MessageQueueHandler::bulkMessages and GearmanClient::workerContexts, which makes Utility.h and its manual clearContainer helpers unnecessary. Statusengine's members are ordered so that ls outlives everything that logs while being torn down, and the destructor resets them explicitly rather than relying on declaration order. Queue.h listed all 23 queue identifiers three times, so adding a queue meant three edits that could disagree. Both lookup directions are now derived from one table. Smaller corrections: NEBMODULE_MODINFO_TITLE was set twice, so the copyright string overwrote the title; toml::syntax_error was caught after std::runtime_error, which it derived from, making the handler dead code; a doubled null check and a stray empty statement are gone. The long_output duplication on the five events whose nebstruct has no such member is deliberately left as it is, to avoid breaking the message format for existing consumers. It is now documented in README.md and marked at each site so it does not get "fixed" as a typo later. .clang-format is added - it was referenced in .gitignore but never committed. The existing sources are not reformatted here, that would be about 1300 lines and would bury everything else; the CI job is advisory for now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
There were no tests at all, so nothing in this repository could be changed with any confidence. The interfaces to hang them on already existed and were simply unused: IStatusengine, IMessageHandler and IMessageQueueHandler are pure interfaces, and Configuration only depends on the first one. 33 test cases covering the encoder, the configuration parser, the log level thresholds and the queue identifier tables. They need neither an installed naemon nor a running broker: the checked in headers under devtools/ci are enough to compile against, and naemon_stubs.cpp provides the handful of symbols that are actually referenced, capturing log output so it can be asserted on. The suite is opt in via -Dtests=true so that packagers and offline builds are unaffected; doctest comes from a wrap. CI runs it twice, once plain and once under address and undefined behaviour sanitizers, which is what would have caught the allocator mismatch and the connection leak fixed earlier in this branch. Two of the tests are there specifically to pin down decisions rather than behaviour: that rabbitmq queues stay durable by default, since RabbitMQ 4 refuses to declare a queue that is neither durable nor exclusive, and that both shipped configuration files still parse, which is the regression guard for the toml11 migration. The suite earned its keep immediately - it caught an unsequenced argument evaluation introduced while deduplicating the queue name tables. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
The JSON the broker emits is its contract with the worker, and nothing guarded it: any renamed or dropped key would have gone unnoticed until the worker choked on it in production. Adds the recorded messages from the worker repository (.claude/specs) as fixtures and builds each message type from hand made nebstruct values, asserting that the produced JSON carries exactly the same keys as the recording. Values are not compared, the fixtures come from a different host - it is the shape that is the contract. All 13 recorded shapes match what the code produces today, so this commit pins the current behaviour rather than changing it. That includes the long_output duplication on state changes, which the recordings confirm has been shipping for a long time and which is documented in the README. Two of the cases cover the notification filters, which are easy to break by accident because they work by producing an empty object, and one covers a latin1 plugin output end to end: json-c cannot build a string from invalid utf8, so a successful parse of the rendered message is what proves the encoder ran. Linking Nebmodule.cpp would have pulled the whole naemon scheduling and downtime surface into the test binary, so the stubs define Nebmodule::EncodeString directly - the encoder is the only part of it that NagiosObject uses. Verified against mutation: dropping a field and renaming one each make the corresponding case fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
The RestartData message only carried object_type, so the worker had no way to
know when the restart actually happened and fell back to its own wall clock as
the cutoff for stale rows. The field was already part of the documented format -
statusngin_core_restart.json records it as 0 - and the worker already treats a
missing value or a 0 as "not set", so populating it is backwards compatible in
both directions.
The value is naemon's own event timestamp from the nebstruct rather than a fresh
time() call, which is both more accurate and consistent with every other message
type. Verified against a running naemon 1.5.2: the emitted message is
{"object_type": 102, "timestamp": 1787678804}, matching container startup.
Building the message moves into a NagiosRestartData class, like every other
message type in NagiosObject.h. It was the one message assembled inline in the
callback, which is also why it was the only recorded shape without a golden
test - the produced shape and the recording disagreed. Both now match and it is
covered like the rest.
Note for the worker: its copy of the spec and the comment in
newCoreRestartHandler still say naemon does not send this field. The fallback
stays correct and is still needed for older brokers, but the comment is now out
of date.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
MessageHandlerList::Worker() could not be bounded by its own message limit. A handler may report that it has more work without having processed anything - the gearman worker does exactly that on GEARMAN_IO_WAIT - and only a processed message advances the counter that the loop checks. A handler in that state therefore span the loop forever, inside naemon's event loop, which stops the monitoring core from scheduling anything at all. The loop now also ends when a full round over all handlers made no progress, so no handler can wedge it, and the loop body moved into a static RunWorkers() that tests can drive with fakes. The gearman side is bounded rather than removed. gearman_worker_wait() polls with gearman_universal_st's timeout, which defaults to -1 - an unbounded poll() in the event loop - and the timeout was never set. It is now capped at 10ms. Removing the wait entirely, which is what this looked like it needed, breaks the worker completely: gearman_wait() is not a sleep, it runs the poll() that refreshes the connection's readiness. Without it libgearman never learns the socket became writable and gearman_worker_work() returns GEARMAN_IO_WAIT on every tick forever, so the worker never completes its PRE_SLEEP handshake, never registers as available and never picks up a job. Verified against a running naemon: with the wait removed, gearadmin reported 0 available workers for statusngin_cmd and a submitted job sat in the queue unprocessed; with the bounded wait it reports 1 and the job is applied. 10ms was chosen by measurement: against a healthy local job server the wait completes in 3 to 119 microseconds, so the cap is only ever reached when something is wrong, and a timed out poll just retries on the next tick. The timeout only affects that poll - connection.cc does not read it, and the other user is the blocking grab path that GEARMAN_WORKER_NON_BLOCKING never reaches. The regression test is the important part: a fake handler that always asks for more without progressing hangs the loop without this change, confirmed by running it against the old code under a timeout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
Optimising the broker without numbers would be guesswork, so this measures first.
naemon's own process_check_result() is stubbed, which is the point: what is
measured is the broker's own share, the only part we can change.
Covers the send path (charset handling and building a whole service check
message) and the receive path (a single command and a 100 entry OCSP bulk,
built from the recorded fixture so the shape is realistic). Sub measurements
split the cost into its parts, which is what makes the numbers actionable
rather than just a total.
Baseline on this machine, release build:
Encoder ascii 268 ns/op
Encoder utf8 3985 ns/op
Encoder latin1 4232 ns/op
uchardet detect only ascii 59 ns/op
uchardet detect only utf8 3937 ns/op
iconv ASCII->UTF-8 only 188 ns/op
NagiosServiceCheckData build 3441 ns/op
ProcessMessage command (1 result) 2460 ns/op
ProcessMessage ocsp bulk (100) 419530 ns/op
json_tokener_parse only (bulk) 326304 ns/op
payload copy only (bulk) 815 ns/op
Two things fall straight out of that. Charset detection, not conversion,
dominates the encoder: 3937 of 3985 ns on utf8 input. And 78% of the bulk
receive path is json-c parsing, which we cannot do anything about - the
broker's own share is the remaining 93 us per 100 results.
Needs --buildtype=release, the numbers mean nothing at -O0. It is built with
-Dtests=true but not run by meson test; a benchmark that fails a build is
useful, one that fails CI on a noisy runner is not.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
Encoder::ToUtf8 ran uchardet's statistical detection over every plugin output, long output and perf data - three times per service check event. The benchmark puts that at 3937 ns of the 3985 ns an utf8 string cost, so detection, not conversion, was where the time went. Pure ASCII took a different but equally pointless route: uchardet reports the charset as "ASCII", which is neither empty nor "UTF-8", so the string went through a full iconv ASCII->UTF-8 conversion plus a buffer of four times the input length, to produce the very same bytes back. That conversion was 188 of its 268 ns. A UTF-8 validity scan up front skips both. ASCII is a subset of UTF-8, so one check covers both cases, and anything that really needs converting still falls through to uchardet exactly as before: Encoder ascii 268 -> 48 ns/op (5.6x) Encoder utf8 3985 -> 51 ns/op (78x) Encoder latin1 4232 -> 4335 ns/op (2% slower, the scan now runs first and fails) whole message 3441 -> 2750 ns/op (-20%) The validator rejects overlong encodings, surrogate halves, truncated sequences and out of range code points, so nothing it passes through unconverted is anything but well formed UTF-8. Tests cover each of those, and the malformed cases assert that they still reach the detection path. One behavioural nuance worth recording: a latin1 string whose bytes happen to also be valid UTF-8 is now passed through instead of being converted. That conversion produced mojibake, so the new result is the better one, but it is a difference. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
ParseCheckResult iterated the JSON object and ran each key down a chain of up to twelve string comparisons, copying it into a std::string first - around 78 comparisons per check result, plus a heap allocation for service_description, which at 19 characters does not fit libstdc++'s small string buffer. json-c keeps object members in a hash table, so asking for the twelve known fields directly is one lookup each and the nesting disappears with it: ProcessMessage command (1 result) 2460 -> 2151 ns/op (-13%) ProcessMessage ocsp bulk (100) 419530 -> 352246 ns/op (-16%) The headline numbers understate it, because most of the bulk path is json-c parsing that neither version can avoid. Subtracting it, the broker's own share per check result drops from 932 to 312 ns, so the part we control got 67% cheaper. Behaviour is unchanged: same fields, same types, a missing field is still left at the value init_check_result gave it, and a JSON null still yields a null pointer. Verified against a running naemon by submitting a check_result over gearman and confirming both plugin output and perf data arrive. The same pattern remains in the three other parse functions, which handle far lower volumes and were left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
Numbers before deciding anything, and in this case they mostly argue against acting: QueueIds() copied (auto) 357 ns/op QueueIds() by reference 1 ns/op Log line, level discards it 46 ns/op Log line, level emits it 106 ns/op ToString SPACED (current) 1192 ns/op 571 B ToString PLAIN 1130 ns/op 519 B (9.1% smaller) bulk of 100 results 63318 B -> 58014 B (8.4%, 5304 B saved) The map copy and the discarded log line looked like waste, and they are, but both sit in FlushBulkQueue, which runs once per bulk flush - by default every ten seconds or every 200 messages, not per message. 357 ns and 46 ns at that rate are nothing. Neither is worth changing for performance; the map copy is still worth a const reference on its own merits, as plain tidiness. Serialisation is the only one with a real effect, and it is more about size than time: 5 percent less CPU, but 8 to 9 percent fewer bytes on every message the broker sends. That one changes the wire format, so it is left alone here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
json_object_to_json_string() defaults to JSON_C_TO_STRING_SPACED, which puts a space after every colon and comma. Switching to PLAIN produces the same JSON with 8 to 9 percent fewer bytes: a single service check message goes from 571 to 519 bytes, a bulk of 100 check results from 63318 to 58014. Size is the point rather than CPU here - serialising is only 5 percent cheaper - but those bytes are network, queue memory, and parsing work on the consumer side, where 78 percent of the receive path is json-c parsing. Safe for the existing consumers: this is a whitespace only difference and both the Go worker (encoding/json) and the PHP worker (json_decode) parse rather than match on the raw text, and no tests depend on the exact bytes. Confirmed with the project maintainer before making the change. Verified against a running naemon: messages on the queue are compact, still parse as JSON, and a check_result submitted over gearman is still applied. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
QueueNameHandler::QueueIds() and QueueNames() return a const reference, but four call sites captured the result with plain auto and copied the whole 23 entry map of strings - 357 ns against 1 ns for a reference. This is tidiness, not performance. The measurement in the previous commit shows why: the only one on a repeating path sits in FlushBulkQueue, which runs once per bulk flush, by default every ten seconds or every 200 messages. Two of the others are error paths that never run in normal operation, and the fourth runs at startup. None of it is worth a change on its own, but leaving a needless copy in place once it is known is worse than the one word it takes to fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
The measurements only existed in commit messages, which is no use to somebody deciding whether to upgrade. This puts the before and after numbers where they can be found, together with what they do and do not mean. The before/after pair was measured again for this, directly back to back on one machine, rather than assembled from the individual commits. That corrected one figure along the way: Latin-1 input is unchanged at 4298 -> 4248 ns, not the two percent slower an earlier commit message claimed - that was measurement noise. The caveats are part of the point, not a disclaimer: single runs that vary by a few percent, one machine and compiler, an encoder gain that only applies when the output already is UTF-8, and roughly 320 of the remaining 352 us in the bulk receive path being json-c parsing that none of this touched. A section claiming 83x without saying when it applies would not survive contact with a sceptical reader. The benchmark ships with the sources, so the numbers can be checked rather than believed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
There was none, so the only way to find out what changed between releases was reading the commit log. This covers everything since v4.0.4, grouped by what it means for someone deciding whether to upgrade rather than by which commit it came from. Kept under [Unreleased]: which version number this becomes is a release decision, and renaming the heading is a one line change when that is made. The correctness fixes lead, because they are the stronger argument than the performance work - RabbitMQ 4 support in particular, where the broker previously could not connect at all, and a set of memory errors in a module that runs inside the monitoring core's address space. Every figure was checked against its measurement rather than copied from memory, which corrected one date along the way: cmake was dropped in 2020, not 2019. The claim that the old CI had not run since is gone too - the configuration is demonstrably stale, but its run history is not something this repository knows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
An unreachable job server made SendMessage log one line per message. On the demo installation that was 254 lines in 60 seconds; the repeat interval added later measured 352 failed sends in five minutes. It scales with the number of checks, so a real installation would bury its own log. An outage is now reported when it starts, again at most every five minutes while it lasts, and once when it recovers, each with the number of failed attempts. Worker side failures get the same treatment and are named through gearman_strerror() rather than printed as a bare number - the old message read "Unknown gearman worker error: 12" for what is GEARMAN_GETADDRINFO, and the GEARMAN_NO_ACTIVE_FDS case it did name is only one of several connection errors. The interval matters more than it looks. Logging only the first failure and then staying silent until recovery was the first attempt, and it was wrong: it would have hidden an outage that never ends. Which is not hypothetical - see below. The recovery line counts "failed send(s)", not "messages lost", deliberately. What is counted is the attempts libgearman reported an error for, which is not necessarily everything that failed to reach the job server, and a number that looks authoritative should be one. Found while testing this, not caused by it: the gearman client never recovers from a job server restart. gearman_client_add_servers() runs once at startup and SendMessage() has no reconnect path, unlike RabbitmqClient::SendMessage which calls Connect(). After the server came back the worker reconnected but the client kept failing every send, with no messages delivered. Left alone here because it is a separate change; the five minute repeat at least keeps it visible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
The client never recovered from one. gearman_client_add_servers() ran once
during Connect() and SendMessage() had no recovery path at all, so after the job
server had been restarted every send kept failing and the broker delivered
nothing to gearman until naemon itself was restarted.
The worker side was fine, which is what made this easy to miss and why it was
only noticed while testing the logging change: libgearman resets the worker's
connection internally on a connect error, but nothing does that for the client
and there is no public reset for it either. Removing and re-adding the server
list is what forces a fresh connection.
Rate limited to once every five seconds, so a job server that stays down does
not get a reconnect attempt per message.
Measured on a running stack, stopping and restarting the gearman container:
before no recovery at all, still zero messages delivered after three minutes
after recovered 11 seconds after the server returned, "writable again after
12 failed send(s)", and the queues started filling again
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
The headers that let the module build without an installed monitoring core were six years old: naemon 1.0.6 from 2019 and nagios 4.4.5. They are what CI builds against and what the test suite compiles with, so they were quietly defining which API the project was verified against. Now naemon 1.5.2 and nagios 4.5.14, taken from a real build of each rather than copied by hand - the nagios set in particular needs configure to have run, since locations.h, iobroker.h and snprintf.h are generated. naemon.pc is updated to match. Nothing in the broker broke, which is the interesting part: six years of API drift and every build variant still compiles warning free. The module only touches the NEB structures and those stayed compatible. One rename shows up in the file list, nagios lib/pqueue.h became prqueue.h, and nagios gained the configure generated config.h, config_pwd.h and ignored_config.h. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
The suite only ever compiled in the naemon configuration. Its stubs assumed naemon's API, so -Dnagios=true failed to build long before it could fail a test, and the nagios code paths - every #ifdef BUILD_NAGIOS branch - had no coverage at all. Three differences had to be handled: nagios calls the comment struct nagios_comment, its get_program_version() returns a non const char*, and it routes nm_log() through an inline in Nebmodule.h to write_to_all_logs(), so that is where the log has to be captured for the assertions to see it. The check_result stub no longer touches output_file, which the broker never sets and which nagios declares const while naemon does not. The nagios CI job now runs the tests instead of only building, so this does not quietly rot again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
An acknowledgement can be set to expire, and until now the message did not say when. naemon carries it in nebstruct_acknowledgement_data as end_time; nagios has no such member. Reported as null under nagios rather than 0, which is what makes this worth a paragraph. Under naemon 0 is not a missing value, it is a real and common one meaning "does not expire" - confirmed against a running naemon, where an acknowledgement set with ACKNOWLEDGE_SVC_PROBLEM_EXPIRE produced end_time=1787692952 and a plain ACKNOWLEDGE_SVC_PROBLEM produced end_time=0. A 0 for nagios would have been indistinguishable from that, and a consumer reading it as an expiry time would be wrong in both directions. null says "this core cannot tell you", which is the truth. The key is always present either way, so consumers do not have to special case its absence. NagiosObject gains an explicit SetNull() for this - writing a null through the const char* overload works but reads like an accident. The recorded fixture is updated along with it, since the message shape is deliberately changing, and the golden test would otherwise be right to fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
The section already said null was not the same as 0, but only in prose. Someone writing a consumer has to decide what to do with each of the three cases, so they are now a table with the actual values, followed by what each one means for the reader. The concrete values are the ones observed against a running naemon: ACKNOWLEDGE_SVC_PROBLEM_EXPIRE gives a timestamp, ACKNOWLEDGE_SVC_PROBLEM gives 0, nagios gives null. Also records that this is the only field where the two cores produce different output. That is checked, not assumed: NagiosObject.h, which builds every message, contains exactly one BUILD_NAGIOS guard and it is this one. The remaining guards in the project are in Nebmodule and cover scheduling, downtime deletion and logging, none of which reach the message format. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
Nagios has no expiring acknowledgements - the feature does not exist, which is why nebstruct_acknowledgement_data has no end_time member there. Reporting null treated that as missing information, but it is not missing: every nagios acknowledgement genuinely never expires, and 0 already means exactly that under naemon. So the field is now always an integer with one meaning across both cores, and neither a consumer nor a database schema needs to tell them apart. SetNull() went with it. It was added for this case and nothing else used it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
The bound added in 48b9f3c ended the loop as soon as a full round over all handlers processed no message. That is wrong by one: draining a gearman queue produces exactly such a round per message. libgearman answers the first read after GRAB_JOB with GEARMAN_IO_WAIT because the response is still in flight, and only the following round returns the job. Ending on the first one therefore ends almost every tick before any work happens, and since the worker tick is one second, the module settled at roughly one message per second. Measured against a real gearmand 1.1.19 with the broker's exact call sequence, 50000 queued jobs: bounded on the first round 48 messages in 50 ticks unbounded (master) 49952 messages in one tick, 5.1s And end to end, naemon in docker applying 100000 check results from statusngin_cmd, queue depth sampled while draining: bounded on the first round 99872 left after 120s (128 processed, 1/s) no progress budget of 16 0 left after 120s (~830/s) The loop now counts consecutive rounds without progress and gives up after 16, so a handler that has genuinely stopped progressing still cannot spin inside naemon's event loop - the reason the bound exists. Any completed message resets the count, which is what restores throughput. 16 is headroom, not a tuned value: draining 110000 jobs with concurrent submitters and a competing worker never produced a run longer than two rounds. The 10ms poll cap is unaffected and stays. Measured with and without it, the same 50000 jobs drain in 5.14s and 5.34s, so it costs nothing while still keeping an unresponsive job server from stalling the core. The regression test is a fake handler shaped like the real worker: one round that asks for more without progressing per message, then one that delivers it. It drains under this change and stalls at zero messages without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
The worker runs inside naemon's event loop, and nothing bounded how long one run could take: the loop ended only when the queues were empty. Instrumenting the callback in docker showed what that means - applying 100000 queued check results kept naemon in a single callback for 128425ms. For over two minutes the core scheduled no checks, reaped no results and read no external commands. MaxWorkerMessagesPerInterval could not prevent it and never could. It counts messages as they arrive from gearman or rabbitmq, not the check results inside them: GearmanClient::Worker() increments once per job, and the bulk is unpacked a layer below in MessageHandler::ProcessMessage(), which iterates messages[] and recurses without ever seeing the counter. At its default of 1000000 and 200 check results per bulk message the real ceiling was 200 million items in one run. So RunWorkers() now takes a wall clock budget, [Worker] MaxRuntimeMilliseconds, defaulting to 100ms, and reports whether it stopped with work left over. When it did, the callback reschedules itself with schedule_event(0, ...) instead of its one second interval. That zero delay is what makes the budget free. naemon's event_poll_full() clamps the wait to zero for an event that is already due, polls its own descriptors with that timeout, and skips running the timed event entirely if any of them had input, so the core always gets a pass of its own between two slices and its I/O keeps priority over ours. Measured on identical fresh stacks, 100000 check results: budget 100ms drained in 127s, longest run 102ms budget 0 drained in 135s, longest run 132776ms The throughput difference is run to run noise. The blocking difference is a factor of 1300. naemon kept executing its own host checks throughout the bounded run. The budget is checked between messages and a message cannot be interrupted - a bulk job is already acknowledged, abandoning it half way would discard check results. The guarantee is therefore "budget plus the message in flight": with bulks of 200 the longest run was 302ms, because one such message costs ~220ms on its own. Documented rather than papered over. Hitting the budget is logged, because a broker that has to stop before the queues are empty is being handed messages faster than the core can apply them. A backlog hits it on every run, so the reporting follows the shape already used for gearman outages: once when it starts, every 60s while it lasts, once when it is over. Not applied under nagios. Its event is recurring and there is no way to ask for an earlier run, so a budget there would buy latency with throughput; the key is ignored with a warning. [Worker] is also documented for the first time - both keys existed but neither appeared in statusengine.toml or the README. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
Found by running the new budget against a naemon whose job server had been stopped mid backlog: Worker stopped with messages still queued, having reached its time budget (227 run(s), 0 message(s) so far). The queues are filling up faster than they can be applied. 227 runs, zero messages. With the job server gone libgearman answers GEARMAN_IO_WAIT round after round, each costing a 10ms poll, so a 100ms budget is spent in ten rounds - before the sixteen round no-progress guard can end the run. The budget then reported work left over, which is wrong twice over: it made the callback reschedule itself immediately, retrying a dead connection ten times a second, and it blamed the queues in the log for what was a connection problem. The handlers report a lost connection themselves, and that is the message an operator needs to see. Both the budget and the message limit now only count as work left over if the run actually completed a message. A run that spends its whole budget without finishing one is not behind, it is stuck, and stuck is worth waiting a second over. Verified the same way it was found: with the job server stopped for 70 seconds under a 26558 job backlog, naemon used 20ms of CPU and logged no overload at all, only the connection errors. MaxWorkerMessagesPerInterval = 0 is left as it is - it means one message per run, not unbounded, which is now written down next to the setting. It is not a useful value, but changing what it does would be a silent behaviour change for anyone who set it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RzCPmi3qofthqD8WiHkjno
Profiling naemon with this broker loaded, 5000 hosts / 100000 services on
a 60s interval with the production bulk queues, put Statusengine::Callback
at 52.9% of the event loop. Nearly all of it was json-c: the allocator
alone accounted for ~29% self time, driven by a json_object tree being
built and torn down for every single event, plus serialising the batch.
yyjson allocates its nodes from a per-document arena, so a message is
built in one block and released in one go rather than node by node.
NagiosObject now holds a yyjson_mut_doc. The SetData API is unchanged, so
all event classes move over with it. Nested objects are copied into the
parent document rather than reference counted, since a yyjson value
belongs to the document it was created in.
Bulk queues now keep the serialised text of each message instead of the
object. Holding the object would mean copying the whole tree into a batch
document at flush time; keeping the text also removes the single large
serialisation of the batch and the array_list that went with it. The
envelope around them is fixed and has no dynamic content.
Parsing (worker commands) still uses json-c and is untouched.
The wire format is unchanged with one exception: json-c escaped forward
slashes as "\/", yyjson does not. Both decode to the same string in any
conforming parser - verified with the two consumers, PHP 8.1 (=== is true)
and Go 1.26 encoding/json (DeepEqual is true). Messages get slightly
smaller as a result. Everything else was compared field by field and is
identical, including doubles keeping their .0 form so the JSON type stays
float, integers beyond 2^53, null, UTF-8, escaping and key order. Real
messages captured from both builds have the same 51 fields in the same
order with the same types.
vendor/ carries yyjson 0.12.0 (MIT) as two files, the same way toml11 is
already vendored, so no distribution package is required. yyjson has its
own fallbacks for compilers without C99, so RHEL 8 builds as well.
loop CPU per check 80.9us -> 51.5us -36.4%
loop CPU per 60s 8.09s -> 5.15s
Four interleaved pairs, naemon main thread only, identical check rate and
100% coverage in every run, negative in all four (-33/-27/-30/-29 us).
Verified over a 14 minute run under full load: RSS flat at 198MB, no
errors logged, 99.7% of services checked within their interval, mean check
latency 0.05s. Unit tests: 75 cases, 432 assertions, all passing.
Signed-off-by: nook24 <info@nook24.eu>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WY8mbGLNkt5eQfc57cTnZ5
The previous commit moved message building to yyjson but left json-c in
place for the worker command path, which meant carrying two JSON
libraries. Measuring the parse side says there is no reason to.
Parse plus extraction of every field ParseCheckResult() reads, both
libraries doing the same work and returning the same values:
single command 371 B json-c 2.24us yyjson 0.18us -92%
bulk of 50 18631 B json-c 121.12us yyjson 7.51us -94%
bulk of 100 37231 B json-c 239.24us yyjson 15.02us -94%
The sizes are the ones that actually arrive: single messages, and bulk
messages of 50 to 100.
So the whole path moves over. json_tokener_parse() becomes yyjson_read(),
the tree walk uses yyjson_obj_get() and yyjson_obj_foreach(), and
IMessageHandler::ProcessMessage takes a yyjson_val. json-c is gone from
the sources, from meson.build and from the test build; the module no
longer links against it.
The tests move with it. tests/bench.cpp loses its SPACED vs PLAIN
comparison, which measured a json-c output option that no longer exists
in the code path - yyjson always writes minified.
One test deserves a note: the perf data case asserted that invalid utf8
had been converted, and used to lean on json-c refusing to build a string
from invalid utf8. It now proves the same thing by round tripping the
message through the parser and comparing the string, which does not
depend on a particular library's behaviour. It passes.
Verified against naemon with 5000 hosts and 100000 services: messages
keep their shape (6 fields in the envelope, 45 in servicestatus), no
errors logged, RSS flat at 198MB over 12 minutes, 100% of services
checked within their interval at 1667 checks/s, mean check latency
0.05s. Unit tests: 75 cases, 432 assertions, all passing.
Signed-off-by: nook24 <info@nook24.eu>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WY8mbGLNkt5eQfc57cTnZ5
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
1 participant
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.
ibjson-cwithyyjsongo gain massive performance improvements