hmon: add C++ interface to heartbeat monitor#72
hmon: add C++ interface to heartbeat monitor#72arkjedrz wants to merge 2 commits intoeclipse-score:mainfrom
Conversation
License Check Results🚀 The license check job ran with the Bazel command: bazel run //:license-checkStatus: Click to expand output |
|
The created documentation from the pull request is available at: docu-html |
04a90a3 to
93f7fc7
Compare
93f7fc7 to
b3753a6
Compare
b3753a6 to
89ad2f1
Compare
578964b to
8c150e1
Compare
8c150e1 to
0104ab2
Compare
0104ab2 to
c6e4454
Compare
c6e4454 to
602b860
Compare
602b860 to
eff29f5
Compare
eff29f5 to
9d1ba1c
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a new Heartbeat monitor to the health monitoring library and exposes it through the existing C++ API surface (via Rust FFI), integrating it into the HealthMonitor build/start workflow.
Changes:
- Introduces a Rust
HeartbeatMonitor(state, evaluation logic, builder) plus FFI exports for C++. - Extends
HealthMonitorBuilder/HealthMonitorto store, retrieve, and start heartbeat monitors alongside deadline monitors. - Adds C++ wrapper types, wiring in
HealthMonitorBuilder::add_heartbeat_monitor/HealthMonitor::get_heartbeat_monitor, and updates C++ tests/build.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/health_monitoring_lib/rust/worker.rs | Passes a shared HMON starting Instant into monitor evaluation and logs heartbeat evaluation errors. |
| src/health_monitoring_lib/rust/lib.rs | Adds heartbeat monitor storage/build/retrieval and includes heartbeat monitors when starting the worker. |
| src/health_monitoring_lib/rust/heartbeat/mod.rs | Introduces the heartbeat module exports and FFI submodule. |
| src/health_monitoring_lib/rust/heartbeat/heartbeat_state.rs | Adds packed atomic heartbeat state representation plus unit tests. |
| src/health_monitoring_lib/rust/heartbeat/heartbeat_monitor.rs | Implements heartbeat monitor builder, evaluation logic, and tests (incl. loom tests). |
| src/health_monitoring_lib/rust/heartbeat/ffi.rs | Adds FFI functions for heartbeat builder creation/destruction and sending heartbeats from C++. |
| src/health_monitoring_lib/rust/ffi.rs | Adds FFI to register heartbeat monitors with the HMON builder and to retrieve heartbeat monitors. |
| src/health_monitoring_lib/rust/deadline/deadline_monitor.rs | Updates evaluator signature and reuses shared time conversion helper. |
| src/health_monitoring_lib/rust/common.rs | Extends MonitorEvaluator to include an HMON starting point and adds time helpers used by heartbeat. |
| src/health_monitoring_lib/cpp/tests/health_monitor_test.cpp | Extends the C++ test to build/get/use a heartbeat monitor. |
| src/health_monitoring_lib/cpp/include/score/hm/heartbeat/heartbeat_monitor.h | Adds the C++ heartbeat monitor and builder API declarations. |
| src/health_monitoring_lib/cpp/include/score/hm/health_monitor.h | Adds heartbeat monitor inclusion and retrieval APIs to C++ HMON. |
| src/health_monitoring_lib/cpp/heartbeat_monitor.cpp | Implements the C++ heartbeat wrapper over Rust FFI calls. |
| src/health_monitoring_lib/cpp/health_monitor.cpp | Wires heartbeat monitor builder/getter calls through to the Rust FFI. |
| src/health_monitoring_lib/BUILD | Adds the new C++ heartbeat source/header to the build. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Get current timestamp, with offset to HMON time. | ||
| let offset = hmon_time_offset(hmon_starting_point, self.monitor_starting_point); | ||
| let now = offset + duration_to_u32(hmon_starting_point.elapsed()); | ||
|
|
||
| // Load current monitor state. | ||
| let snapshot = self.heartbeat_state.snapshot(); | ||
|
|
||
| // Get and recalculate snapshot timestamps. | ||
| // IMPORTANT: first heartbeat is obtained when HMON time is unknown. | ||
| // It is necessary to: | ||
| // - use offset as cycle starting point. | ||
| // - get heartbeat snapshot in relation to zero point. | ||
| let (start_timestamp, heartbeat_timestamp) = if snapshot.post_init() { | ||
| let start_timestamp = snapshot.start_timestamp(); | ||
| let heartbeat_timestamp = start_timestamp + snapshot.heartbeat_timestamp_offset(); | ||
| (start_timestamp, heartbeat_timestamp) | ||
| } else { | ||
| let start_timestamp = offset; | ||
| let heartbeat_timestamp = snapshot.heartbeat_timestamp_offset(); | ||
| (start_timestamp, heartbeat_timestamp) | ||
| }; | ||
|
|
||
| // Get allowed time range as absolute values. | ||
| let range = self.range.offset(start_timestamp); |
There was a problem hiding this comment.
The timestamp math here uses u32 addition (now = offset + duration_to_u32(...), and later start_timestamp + snapshot.heartbeat_timestamp_offset() / self.range.offset(start_timestamp)). These additions can overflow and silently wrap in release builds, leading to incorrect evaluations after long uptimes or large start offsets. Consider using checked/saturating arithmetic (and reporting an error) or keeping timestamps in u64 until the final packing step.
| // Check range is valid. | ||
| let range_min_ms = self.range.min.as_millis() as u64; | ||
| let internal_processing_cycle_ms = internal_processing_cycle.as_millis() as u64; | ||
| if range_min_ms * 2 <= internal_processing_cycle_ms { | ||
| error!( | ||
| "Internal processing cycle duration ({} ms) must be longer than two shortest allowed ranges ({} ms).", | ||
| internal_processing_cycle_ms, range_min_ms | ||
| ); | ||
| return Err(HealthMonitorError::InvalidArgument); | ||
| } |
There was a problem hiding this comment.
The validation for internal_processing_cycle vs the heartbeat range looks incorrect: with the current condition (range_min_ms * 2 <= internal_processing_cycle_ms) you can still end up with two valid heartbeats occurring between evaluations when internal_processing_cycle_ms > range_min_ms, which will be reported as MultipleHeartbeats even though the beats are in different cycles. This check should enforce that the internal processing/evaluation cycle is strictly shorter than the minimum allowed heartbeat interval (and update the error message accordingly).
| pub extern "C" fn heartbeat_monitor_builder_create( | ||
| range_min_ms: u32, | ||
| range_max_ms: u32, | ||
| heartbeat_monitor_builder_handle_out: *mut FFIHandle, | ||
| ) -> FFICode { | ||
| if heartbeat_monitor_builder_handle_out.is_null() { | ||
| return FFICode::NullParameter; | ||
| } | ||
|
|
||
| let range_min = Duration::from_millis(range_min_ms as u64); | ||
| let range_max = Duration::from_millis(range_max_ms as u64); | ||
| let range = TimeRange::new(range_min, range_max); | ||
|
|
There was a problem hiding this comment.
heartbeat_monitor_builder_create constructs TimeRange::new(range_min, range_max) without validating range_min_ms <= range_max_ms. Since TimeRange::new asserts on invalid ranges, passing a bad range from C/C++ would panic across the FFI boundary. Please add an explicit check (mirroring deadline_monitor_builder_add_deadline) and return FFICode::InvalidArgument instead of panicking.
Add heartbeat monitor HMON.
Add C++ interface to heartbeat monitor.
9d1ba1c to
046bcf4
Compare
Add C++ interface to heartbeat monitor.
Resolves #69