hmon: add logic monitor#95
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run --lockfile_mode=error //:license-checkStatus: Click to expand output |
There was a problem hiding this comment.
Pull request overview
This PR extends the health monitoring library with new monitor types (heartbeat + logic) and refactors the worker/supervisor client plumbing so the worker can evaluate multiple monitor kinds and report typed evaluation errors across Rust + C++/FFI.
Changes:
- Add new
HeartbeatMonitor(state + evaluator + FFI + C++ wrapper) andLogicMonitor(state-transition validator). - Refactor monitor evaluation to pass an HMON starting
Instantand to report typedMonitorEvaluationErrorvariants (deadline/heartbeat/logic). - Move supervisor API client implementations into a feature-gated
supervisor_api_client/module and adjust build config defaults/features.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/health_monitoring_lib/rust/worker.rs | Updates monitoring loop to pass a shared starting Instant and log typed monitor errors; removes in-file supervisor client impls. |
| src/health_monitoring_lib/rust/tag.rs | Adds StateTag newtype for logic monitor states; updates tag tests. |
| src/health_monitoring_lib/rust/supervisor_api_client/stub_supervisor_api_client.rs | Adds stub supervisor client implementation behind feature gate. |
| src/health_monitoring_lib/rust/supervisor_api_client/score_supervisor_api_client.rs | Adds SCORE supervisor client implementation (monitor_rs-backed). |
| src/health_monitoring_lib/rust/supervisor_api_client/mod.rs | Introduces SupervisorAPIClient trait and feature-gated implementations. |
| src/health_monitoring_lib/rust/logic/mod.rs | Adds logic module exports. |
| src/health_monitoring_lib/rust/logic/logic_monitor.rs | Implements logic monitor state machine + evaluator + tests. |
| src/health_monitoring_lib/rust/lib.rs | Integrates deadline/heartbeat/logic monitors into HealthMonitorBuilder + HealthMonitor; changes build/start to return Result. |
| src/health_monitoring_lib/rust/heartbeat/mod.rs | Adds heartbeat module wiring and FFI submodule. |
| src/health_monitoring_lib/rust/heartbeat/heartbeat_state.rs | Adds compact atomic heartbeat state representation (+ loom-aware atomics). |
| src/health_monitoring_lib/rust/heartbeat/heartbeat_monitor.rs | Adds heartbeat monitor implementation + builder validation + tests. |
| src/health_monitoring_lib/rust/heartbeat/ffi.rs | Adds FFI for heartbeat monitor builder/monitor and tests. |
| src/health_monitoring_lib/rust/ffi.rs | Extends core FFI to build/start using Result, adds heartbeat builder/get FFI, and maps HealthMonitorError to FFICode. |
| src/health_monitoring_lib/rust/deadline/mod.rs | Re-exports DeadlineEvaluationError for typed evaluation errors. |
| src/health_monitoring_lib/rust/deadline/deadline_monitor.rs | Refactors deadline monitor to use typed DeadlineEvaluationError and updated evaluator signature. |
| src/health_monitoring_lib/rust/common.rs | Introduces HasEvalHandle, typed MonitorEvaluationError, shared evaluation signature, and time conversion helpers. |
| src/health_monitoring_lib/cpp/tests/health_monitor_test.cpp | Extends C++ test to build/get/use heartbeat monitor. |
| src/health_monitoring_lib/cpp/include/score/hm/heartbeat/heartbeat_monitor.h | Adds C++ API surface for heartbeat monitor + builder. |
| src/health_monitoring_lib/cpp/include/score/hm/health_monitor.h | Adds heartbeat monitor APIs to C++ HealthMonitor(Builder). |
| src/health_monitoring_lib/cpp/heartbeat_monitor.cpp | Implements C++ heartbeat monitor wrapper calling Rust FFI. |
| src/health_monitoring_lib/cpp/health_monitor.cpp | Wires heartbeat builder/get calls through Rust FFI. |
| src/health_monitoring_lib/Cargo.toml | Adds features (default stub; optional score uses monitor_rs) and loom dependency config. |
| src/health_monitoring_lib/BUILD | Adds heartbeat C++ sources/headers and adjusts crate feature flags for Bazel targets. |
| examples/rust_supervised_app/src/main.rs | Updates example to handle build()/start() returning Result. |
| Cargo.toml | Sets workspace default-members and adds cfg(loom) lint configuration. |
| Cargo.lock | Adds new dependency entries (loom + transitive deps). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| match value { | ||
| value if value == LogicEvaluationError::InvalidState as u8 => LogicEvaluationError::InvalidState, | ||
| value if value == LogicEvaluationError::InvalidTransition as u8 => LogicEvaluationError::InvalidTransition, | ||
| _ => panic!("Invalid underlying value of logic evaluation error."), |
There was a problem hiding this comment.
impl From<u8> for LogicEvaluationError panics on unknown values. Since this conversion is used when interpreting AtomicU8 state during evaluate, an unexpected value would crash the monitoring thread/process instead of reporting an error. Please make this conversion non-panicking (e.g., map unknown values to InvalidState or return Option/Result) and handle that path in evaluate.
| _ => panic!("Invalid underlying value of logic evaluation error."), | |
| _ => LogicEvaluationError::InvalidState, |
src/health_monitoring_lib/rust/supervisor_api_client/score_supervisor_api_client.rs
Show resolved
Hide resolved
|
The created documentation from the pull request is available at: docu-html |
38a2bdf to
07352fd
Compare
07352fd to
f96e585
Compare
f96e585 to
4ab21c9
Compare
4ab21c9 to
743411c
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
pawelrutkaq
left a comment
There was a problem hiding this comment.
Stopped as my comment will probably simplify more code
743411c to
4686fb0
Compare
|
|
||
| protected: | ||
| std::optional<internal::FFIHandle> __drop_by_rust_impl() | ||
| std::optional<internal::FFIHandle> _drop_by_rust_impl() |
There was a problem hiding this comment.
_drop_by_rust_impl -> drop_by_rust_impl
Compilers like GCC, Clang, and MSVC often use leading underscores for internal macros or system-level functions. If a future update to your compiler introduces a macro or an internal header named _drop_by_rust_impl, your code will fail to compile—or worse, behave unpredictably—and it will be considered your fault, not the compiler’s, because you used a reserved naming pattern.
There was a problem hiding this comment.
__ and _ is just considered not a good style. They are reserved.
There was a problem hiding this comment.
That one's fine. It was previously double underscore, and that's actually an issue.
https://devblogs.microsoft.com/oldnewthing/20230109-00/?p=107685
|
The rust part looks like it supports monitoring from multiple threads. I could not find any hint how to set this up correctly form the cpp side. |
4686fb0 to
6247d38
Compare
6247d38 to
48bc6f2
Compare
| current_state_node.tag, target_state | ||
| ); | ||
| let error = LogicEvaluationError::InvalidTransition; | ||
| let _ = self.logic_state.update(|mut current_state| { |
There was a problem hiding this comment.
since the handle is only reading the values, you can simply use store or swap, no need to use update
|
|
||
| // Find index of target state, then change current state. | ||
| let target_state_index = self.find_index_by_tag(target_state)?; | ||
| let _ = self.logic_state.update(|mut current_state| { |
There was a problem hiding this comment.
no update needed as above
|
@paulquiring any more comments or fine for You ? |
Add logic monitor to HMON.
- Add const constructor to `StateTag`. - Atomic `LogicState` containing current state index and monitor status. - Updated logic monitor API. - Reworked internal of logic monitor.
- Move `loom` imports to `common`. - Remove `Sync` from `LogicMonitor`. - `PhantomUnsync` added to `common`. - `LogicEvaluationError` -> replace `From` with `TryFrom`. - Replace `monitor_status` and `set_monitor_status` type. - From `u8` to `LogicEvaluationError`. - Replace `update` with `swap` in `LogicState`.
fb1c854 to
9e81eb0
Compare
|
@paulquiring if some issues are still there that you forsee please leave comments and we will adress them in follow up PR. Thanks |

Add logic monitor to HMON.
Resolved #15