Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: simplify HealthCheckService #4218

Conversation

paullatzelsperger
Copy link
Member

@paullatzelsperger paullatzelsperger commented May 28, 2024

What this PR changes/adds

This PR simplifies and refactors the health check feature in the following aspects:

  • the HealthCheckService does not cache results anymore, to avoid additional latencies, and potentially wrong system health status
  • the HealthCheckResult#component field is now mandatory, because it makes no sense to have unnamed component results
  • static factory methods of the HealthCheckResult are deprecated in favor of the Builder as they don't enforce the non-nullness of the component field
  • delets the HealthCheckServiceConfiguration at it is not needed anymore

Why it does that

Caching health results may increase the responsiveness of the caller (e.g. Kubernetes probes), but will ultimately introduce an additional delay, and convey a potentially wrong system picture: components don't show up in the overall result unless either refresh() was called, or the next update cycle had gone through.
Configuring periods, delays etc. should be up to the caller, not the health check system.

Further notes

  • This new immediate health check status was applied to the DataplaneSelfRegistrationExtension to have K8S kill the pod if the self-registration fails.

Linked Issue(s)

Closes # <-- insert Issue number if one exists

Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.

@paullatzelsperger paullatzelsperger added the refactoring Cleaning up code and dependencies label May 28, 2024
@paullatzelsperger paullatzelsperger marked this pull request as draft May 28, 2024 07:10
@paullatzelsperger paullatzelsperger marked this pull request as ready for review May 28, 2024 07:13
@paullatzelsperger paullatzelsperger force-pushed the feat/simplify_health_check_service branch from a7d7158 to 3c68b72 Compare May 28, 2024 07:25
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 79.48718% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 75.35%. Comparing base (7f20ba5) to head (3c68b72).
Report is 277 commits behind head on main.

Files Patch % Lines
...gistration/DataplaneSelfRegistrationExtension.java 80.95% 4 Missing ⚠️
...lipse/edc/spi/system/health/HealthCheckResult.java 0.00% 3 Missing ⚠️
...va/org/eclipse/edc/boot/BootServicesExtension.java 0.00% 1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4218      +/-   ##
==========================================
+ Coverage   71.74%   75.35%   +3.61%     
==========================================
  Files         919     1039     +120     
  Lines       18457    20636    +2179     
  Branches     1037     1152     +115     
==========================================
+ Hits        13242    15551    +2309     
+ Misses       4756     4574     -182     
- Partials      459      511      +52     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paullatzelsperger paullatzelsperger merged commit 7aa6b80 into eclipse-edc:main May 28, 2024
17 of 18 checks passed
@paullatzelsperger paullatzelsperger deleted the feat/simplify_health_check_service branch May 28, 2024 07:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Cleaning up code and dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants