hmon: approach to monitors rework by arkjedrz · Pull Request #81 · eclipse-score/lifecycle

arkjedrz · 2026-02-17T11:51:37Z

Change approach to monitors abstraction.
Improve docs.
Small esthetics fixes.

github-actions · 2026-02-17T11:52:15Z

License Check Results

🚀 The license check job ran with the Bazel command:

bazel run //:license-check

Status: ⚠️ Needs Review

Click to expand output

[License Check Output]
Extracting Bazel installation...
Starting local Bazel server (8.4.2) and connecting to it...
INFO: Invocation ID: d647c520-f6d8-431c-9ef2-a8b48b690d11
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
Computing main repo mapping: 
WARNING: For repository 'score_rust_policies', the root module requires module version score_rust_policies@0.0.3, but got score_rust_policies@0.0.5 in the resolved dependency graph. Please update the version in your MODULE.bazel or set --check_direct_dependencies=off
Loading: 
Loading: 0 packages loaded
Loading: 0 packages loaded
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Loading: 0 packages loaded
    currently loading: 
Analyzing: target //:license-check (1 packages loaded)
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)
Analyzing: target //:license-check (1 packages loaded, 0 targets configured)

Analyzing: target //:license-check (31 packages loaded, 9 targets configured)

Analyzing: target //:license-check (65 packages loaded, 9 targets configured)

Analyzing: target //:license-check (85 packages loaded, 9 targets configured)

Analyzing: target //:license-check (131 packages loaded, 510 targets configured)

Analyzing: target //:license-check (148 packages loaded, 3372 targets configured)

Analyzing: target //:license-check (149 packages loaded, 7212 targets configured)

Analyzing: target //:license-check (160 packages loaded, 7817 targets configured)

Analyzing: target //:license-check (160 packages loaded, 7817 targets configured)

Analyzing: target //:license-check (160 packages loaded, 7817 targets configured)

INFO: Analyzed target //:license-check (165 packages loaded, 9955 targets configured).
[6 / 15] [Prepa] Writing repo mapping manifest for //:license.check.license_check
[14 / 16] [Prepa] JavaToolchainCompileBootClasspath external/rules_java+/toolchains/platformclasspath.jar
[15 / 16] Building license.check.license_check.jar (); 0s disk-cache, multiplex-worker
INFO: Found 1 target...
Target //:license.check.license_check up-to-date:
  bazel-bin/license.check.license_check
  bazel-bin/license.check.license_check.jar
INFO: Elapsed time: 28.540s, Critical Path: 2.52s
INFO: 16 processes: 12 internal, 3 processwrapper-sandbox, 1 worker.
INFO: Build completed successfully, 16 total actions
INFO: Running command line: bazel-bin/license.check.license_check ./formatted.txt <args omitted>
usage: org.eclipse.dash.licenses.cli.Main [-batch <int>] [-cd <url>]
       [-confidence <int>] [-ef <url>] [-excludeSources <sources>] [-help] [-lic
       <url>] [-project <shortname>] [-repo <url>] [-review] [-summary <file>]
       [-timeout <seconds>] [-token <token>]

github-actions · 2026-02-17T11:55:23Z

The created documentation from the pull request is available at: docu-html

Copilot

Pull request overview

This pull request refactors the health monitoring library to remove unnecessary abstractions, improve type safety, and better organize the codebase. The changes introduce separate MonitorTag and DeadlineTag types to replace the generic IdentTag, eliminate the MonitorEvaluator trait in favor of direct usage of Arc<DeadlineMonitorInner>, and reorganize the supervisor API client code into a dedicated module with proper feature flags.

Changes:

Introduced MonitorTag and DeadlineTag types for better type safety and clarity
Removed MonitorEvaluator trait abstraction and MonitorEvalHandle wrapper
Moved supervisor API client implementations to dedicated module with mutually exclusive feature flags
Improved documentation throughout the codebase
Fixed Cargo build configuration with proper feature flag management

Reviewed changes

Copilot reviewed 26 out of 28 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/health_monitoring_lib/rust/tag.rs	New module implementing `MonitorTag` and `DeadlineTag` with FFI-safe memory management
src/health_monitoring_lib/rust/worker.rs	Updated to use `DeadlineMonitorInner` directly and import `SupervisorAPIClient` from new module
src/health_monitoring_lib/rust/supervisor_api_client/*.rs	New module structure with feature-gated implementations
src/health_monitoring_lib/rust/lib.rs	Refactored monitor state management using `MonitorState` enum
src/health_monitoring_lib/rust/ffi.rs	Updated FFI functions to use new tag types
src/health_monitoring_lib/rust/deadline/*.rs	Updated to use new tag types and expose `DeadlineMonitorInner`
src/health_monitoring_lib/rust/common.rs	Removed `IdentTag`, `MonitorEvaluator`, and related types
src/health_monitoring_lib/cpp/include/score/hm/tag.h	New C++ tag types for FFI compatibility
src/health_monitoring_lib/cpp/include/score/hm/*.h	Updated to use new tag types
src/health_monitoring_lib/cpp/*.cpp	Updated implementations to use new tag types
src/health_monitoring_lib/Cargo.toml	Added feature flags for supervisor API client selection
Cargo.toml	Set default-members and removed global monitor_rs dependency
src/health_monitoring_lib/BUILD	Added crate_features configuration
.github/workflows/lint_clippy.yml	Updated to test both feature flag configurations
examples/*	Updated to use new tag types

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/health_monitoring_lib/rust/tag.rs

arkjedrz · 2026-02-23T19:14:02Z

src/health_monitoring_lib/rust/tag.rs

This change was meant to be placed in other PR.

- Fix Cargo for `health_monitoring_lib`. - Fix leftover test changes from previous PR. - Small code changes.

- Less restrictive feature flags. - Additional small code changes.

- Remove unnecessary abstraction from monitors. - Improve docs. - Move `FFIBorrowed` and `FFIHandle` to main `ffi` module. - Small esthetics fixes.

- Restore some parts. - Separate errors into groups.

- Add `HealthMonitorError`. - Not fully utilized in this change, required for future monitors. - Simplify implementation of `start` and `build`. - No longer must be reimplemented for FFI. - New unit tests for `lib.rs`.

Copilot

Pull request overview

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-26T12:49:52Z

src/health_monitoring_lib/rust/lib.rs

        let monitoring_logic = worker::MonitoringLogic::new(
-            monitors,
+            collected_monitors,
            self.supervisor_api_cycle,
-            // Currently only `ScoreSupervisorAPIClient` and `StubSupervisorAPIClient` are supported.
-            // The later is meant to be used for testing purposes.
-            #[cfg(not(any(test, feature = "stub_supervisor_api_client")))]
-            worker::ScoreSupervisorAPIClient::new(),
-            #[cfg(any(test, feature = "stub_supervisor_api_client"))]
-            worker::StubSupervisorAPIClient {},
+            #[cfg(all(not(test), feature = "score_supervisor_api_client"))]
+            supervisor_api_client::score_supervisor_api_client::ScoreSupervisorAPIClient::new(),
+            #[cfg(any(
+                test,
+                all(feature = "stub_supervisor_api_client", not(feature = "score_supervisor_api_client"))
+            ))]
+            supervisor_api_client::stub_supervisor_api_client::StubSupervisorAPIClient::new(),
        );


The conditional compilation logic has several edge cases that would cause compilation failures. When only score_supervisor_api_client feature is enabled (without stub), test builds would fail because neither cfg condition would match (line 259 requires not(test), line 262-263 requires test OR stub without score). Similarly, when neither feature is enabled, all builds would fail. Consider adding a compile_error! to explicitly handle unsupported feature combinations, or ensure the cfg conditions cover all valid scenarios, particularly test builds when only score_supervisor_api_client is enabled.

Copilot · 2026-02-26T12:49:52Z

src/health_monitoring_lib/rust/lib.rs

+        // Should never occur if created by `HealthMonitorBuilder`!
+        let num_monitors = self.deadline_monitors.len();
+        if num_monitors == 0 {
+            error!("No monitors have been added. HealthMonitor cannot be created.");


The error message says "HealthMonitor cannot be created" but this error occurs in the start() method, not during creation. The HealthMonitor was already created by the builder. Consider changing the message to "HealthMonitor cannot be started without monitors" or similar to accurately reflect the operation that is failing.

Suggested change

error!("No monitors have been added. HealthMonitor cannot be created.");

error!("No monitors have been added. HealthMonitor cannot be started without monitors.");

Copilot · 2026-02-26T12:49:53Z

src/health_monitoring_lib/rust/lib.rs

+    /// This method shall be called before `Lifecycle.running()`.
+    /// Otherwise the supervisor might consider the process not alive.
+    ///
+    /// Health monitoring logic stop when the [`HealthMonitor`] is dropped.


Documentation says "Health monitoring logic stop when the [HealthMonitor] is dropped" but this is grammatically incorrect. Consider changing to "Health monitoring logic stops when the [HealthMonitor] is dropped" to fix the verb conjugation.

Suggested change

/// Health monitoring logic stop when the [`HealthMonitor`] is dropped.

/// Health monitoring logic stops when the [`HealthMonitor`] is dropped.

Copilot · 2026-02-26T12:49:53Z

src/health_monitoring_lib/rust/worker.rs

+                    MonitorEvaluationError::Heartbeat => unimplemented!(),
+                    MonitorEvaluationError::Logic => unimplemented!(),


The MonitorEvaluationError::Heartbeat and MonitorEvaluationError::Logic variants call unimplemented!() which will panic if ever reached. While these appear to be placeholders for future monitor types, consider using compile-time feature flags or removing these variants until they are actually implemented to avoid unexpected runtime panics. Alternatively, add clear documentation indicating these are not yet supported and should not be used.

Suggested change

MonitorEvaluationError::Heartbeat => unimplemented!(),

MonitorEvaluationError::Logic => unimplemented!(),

MonitorEvaluationError::Heartbeat => {

warn!(

"Heartbeat monitor with tag {:?} reported an error.",

monitor_tag

)

},

MonitorEvaluationError::Logic => {

warn!(

"Logic monitor with tag {:?} reported an error.",

monitor_tag

)

},

Copilot · 2026-02-26T12:49:53Z

src/health_monitoring_lib/rust/supervisor_api_client/mod.rs

+    fn notify_alive(&self);
+}
+
+// NOTE: various implementations are not mutually exclusive.


The comment states that various implementations are not mutually exclusive, but the feature flag configuration in lib.rs lines 261-265 suggests they should be. When both features are enabled, the code uses score_supervisor_api_client in production and stub_supervisor_api_client in tests, which means they are used in different contexts. Consider clarifying this comment to explain that while both features can be enabled, only one implementation is used at runtime based on conditional compilation.

Suggested change

// NOTE: various implementations are not mutually exclusive.

// NOTE: multiple implementations can be enabled via features at the same time,

// but conditional compilation ensures that only one is used in a given build

// context (e.g., production vs tests), as configured in lib.rs.

arkjedrz had a problem deploying to workflow-approval February 17, 2026 11:51 — with GitHub Actions Error

arkjedrz had a problem deploying to workflow-approval February 17, 2026 11:51 — with GitHub Actions Failure

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from fda115c to 9e958f1 Compare February 17, 2026 12:22

arkjedrz had a problem deploying to workflow-approval February 17, 2026 12:22 — with GitHub Actions Failure

arkjedrz had a problem deploying to workflow-approval February 17, 2026 12:22 — with GitHub Actions Error

arkjedrz self-assigned this Feb 17, 2026

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from 9e958f1 to 88e93ee Compare February 23, 2026 13:09

arkjedrz temporarily deployed to workflow-approval February 23, 2026 13:09 — with GitHub Actions Inactive

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from 88e93ee to f08a948 Compare February 23, 2026 13:16

arkjedrz temporarily deployed to workflow-approval February 23, 2026 13:16 — with GitHub Actions Inactive

arkjedrz marked this pull request as ready for review February 23, 2026 13:44

arkjedrz requested review from Copilot and pawelrutkaq February 23, 2026 13:44

Copilot started reviewing on behalf of arkjedrz February 23, 2026 13:45 View session

Copilot AI reviewed Feb 23, 2026

View reviewed changes

src/health_monitoring_lib/rust/tag.rs Outdated Show resolved Hide resolved

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from f08a948 to df8135d Compare February 23, 2026 14:02

arkjedrz temporarily deployed to workflow-approval February 23, 2026 14:02 — with GitHub Actions Inactive

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from df8135d to 08e6e6d Compare February 23, 2026 15:52

arkjedrz temporarily deployed to workflow-approval February 23, 2026 15:52 — with GitHub Actions Inactive

arkjedrz commented Feb 23, 2026

View reviewed changes

src/health_monitoring_lib/rust/tag.rs

Copy link

Contributor Author

arkjedrz Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was meant to be placed in other PR.

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from 08e6e6d to 6afc701 Compare February 24, 2026 15:23

arkjedrz temporarily deployed to workflow-approval February 24, 2026 15:23 — with GitHub Actions Inactive

hmon: fix Cargo build, small code changes

3cb438a

- Fix Cargo for `health_monitoring_lib`. - Fix leftover test changes from previous PR. - Small code changes.

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from 6afc701 to 94920a7 Compare February 25, 2026 07:56

arkjedrz had a problem deploying to workflow-approval February 25, 2026 07:56 — with GitHub Actions Failure

arkjedrz had a problem deploying to workflow-approval February 25, 2026 07:56 — with GitHub Actions Error

arkjedrz temporarily deployed to workflow-approval February 25, 2026 08:31 — with GitHub Actions Inactive

arkjedrz temporarily deployed to workflow-approval February 25, 2026 08:37 — with GitHub Actions Inactive

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from 94920a7 to a41b786 Compare February 25, 2026 12:16

arkjedrz temporarily deployed to workflow-approval February 25, 2026 12:16 — with GitHub Actions Inactive

arkjedrz added 3 commits February 25, 2026 13:59

hmon: Cargo fix post-review fixes

35c7c0d

- Less restrictive feature flags. - Additional small code changes.

hmon: approach to monitors rework

168d203

- Remove unnecessary abstraction from monitors. - Improve docs. - Move `FFIBorrowed` and `FFIHandle` to main `ffi` module. - Small esthetics fixes.

hmon: approach to monitors rework

c85301c

- Restore some parts. - Separate errors into groups.

arkjedrz force-pushed the arkjedrz_remove-monitor-abstraction branch from a41b786 to c85301c Compare February 25, 2026 13:01

arkjedrz temporarily deployed to workflow-approval February 25, 2026 13:01 — with GitHub Actions Inactive

hmon: rework behavior on panics

aa819a8

- Add `HealthMonitorError`. - Not fully utilized in this change, required for future monitors. - Simplify implementation of `start` and `build`. - No longer must be reimplemented for FFI. - New unit tests for `lib.rs`.

arkjedrz temporarily deployed to workflow-approval February 26, 2026 12:25 — with GitHub Actions Inactive

arkjedrz requested a review from Copilot February 26, 2026 12:45

Copilot started reviewing on behalf of arkjedrz February 26, 2026 12:45 View session

Copilot AI reviewed Feb 26, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hmon: approach to monitors rework#81

hmon: approach to monitors rework#81
arkjedrz wants to merge 5 commits intoeclipse-score:mainfrom
qorix-group:arkjedrz_remove-monitor-abstraction

arkjedrz commented Feb 17, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Feb 17, 2026 •

edited

Loading

Uh oh!

github-actions bot commented Feb 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

arkjedrz Feb 23, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 26, 2026

Uh oh!

Copilot AI Feb 26, 2026

Uh oh!

Copilot AI Feb 26, 2026

Uh oh!

Copilot AI Feb 26, 2026

Uh oh!

Copilot AI Feb 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	error!("No monitors have been added. HealthMonitor cannot be created.");
	error!("No monitors have been added. HealthMonitor cannot be started without monitors.");

	/// Health monitoring logic stop when the [`HealthMonitor`] is dropped.
	/// Health monitoring logic stops when the [`HealthMonitor`] is dropped.

		MonitorEvaluationError::Heartbeat => unimplemented!(),
		MonitorEvaluationError::Logic => unimplemented!(),

-                    MonitorEvaluationError::Heartbeat => unimplemented!(),
-                    MonitorEvaluationError::Logic => unimplemented!(),
+                    MonitorEvaluationError::Heartbeat => {
+                        warn!(
+                            "Heartbeat monitor with tag {:?} reported an error.",
+                            monitor_tag
+                        )
+                    },
+                    MonitorEvaluationError::Logic => {
+                        warn!(
+                            "Logic monitor with tag {:?} reported an error.",
+                            monitor_tag
+                        )
+                    },

-// NOTE: various implementations are not mutually exclusive.
+// NOTE: multiple implementations can be enabled via features at the same time,
+// but conditional compilation ensures that only one is used in a given build
+// context (e.g., production vs tests), as configured in lib.rs.

Conversation

arkjedrz commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

License Check Results

Uh oh!

github-actions bot commented Feb 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

arkjedrz Feb 23, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

arkjedrz commented Feb 17, 2026 •

edited

Loading

github-actions bot commented Feb 17, 2026 •

edited

Loading