[OpAMP client] Added health reporting support#2867
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for reporting agent health (ComponentHealth) to the OpAMP server, exposing a new setHealth API and a corresponding builder configuration.
Changes:
- Introduces
State.Healthobservable state,HealthAppender, andField.HEALTHplumbed throughOpampClientStateandAgentToServerAppenders. - Adds
OpampClient.setHealth(...)andOpampClientBuilder.enableHealthReporting()/setHealthState(...)APIs. - Adds unit tests for the new health appender and client behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| opamp-client/src/main/java/io/opentelemetry/opamp/client/OpampClient.java | New setHealth method on the public client interface. |
| opamp-client/src/main/java/io/opentelemetry/opamp/client/OpampClientBuilder.java | Builder support for enabling health reporting and supplying a State.Health. |
| opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/impl/OpampClientImpl.java | Wires HealthAppender, registers HEALTH as compressable, implements setHealth. |
| opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/impl/OpampClientState.java | Adds State.Health field to client state. |
| opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/impl/recipe/AgentToServerAppenders.java | Registers the new HealthAppender. |
| opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/impl/recipe/appenders/HealthAppender.java | New appender writing ComponentHealth into AgentToServer. |
| opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/request/Field.java | New HEALTH field enum value. |
| opamp-client/src/main/java/io/opentelemetry/opamp/client/internal/state/State.java | New Health observable state class. |
| opamp-client/src/test/java/.../OpampClientImplTest.java | Tests for health initialization, sending, and setter. |
| opamp-client/src/test/java/.../OpampClientStateTest.java | Adds health mock field. |
| opamp-client/src/test/java/.../AgentToServerAppendersTest.java | Verifies HEALTH-to-appender mapping. |
| opamp-client/src/test/java/.../HealthAppenderTest.java | Unit tests for HealthAppender. |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
LikeTheSalad
left a comment
LikeTheSalad
left a comment
There was a problem hiding this comment.
Thank you, @robsunday 🙏
It looks good overall, and it follows the same patterns as other fields. Thank you for keeping that consistency. I only have some comments regarding the default value for this new field.
|
|
||
| @Override | ||
| public void setHealth(@Nonnull ComponentHealth health) { | ||
| if (!Objects.equals(state.health.get(), health)) { |
There was a problem hiding this comment.
[for reviewer] I think this needs to be refactored because it is not really thread safe. Other thread may manipulate the state between state.health.get() and state.health.set() calls.
I'm thinking about adding boolean update(@Nonnull T value) method to State interface. It would do atomic get and set, and return true if value was updated, so OpampClientImpl can call addFieldAndSend if value was changed.
There was a problem hiding this comment.
Sounds good. I just think it'd be a bit strange to have 2 methods that set the state's value. Maybe the atomic update could be done in the existing set, and change its signature to return a boolean.
There was a problem hiding this comment.
Cheers! I already approved your changes, so we just have to wait for one of the repo maintainers to have a look.
LikeTheSalad
left a comment
LikeTheSalad
left a comment
There was a problem hiding this comment.
Thanks again, @robsunday 🙏
|
Thanks for quick reviews @LikeTheSalad ! |
Add support for reporting component health to the OpAMP server.
The following actions have to be taken to report agent's health:
OpampClientBuilder#enableHealthReporting()OpampClientBuilder#setHealth(ComponentHealth health)or OpampClient#setHealth(ComponentHealth health)OpampClient#setHealth(ComponentHealth health). Original agent start time should be preserved.