Closed taeng0204 closed 4 weeks ago
The recent updates introduce a plain HTTP health check endpoint to the server, enhancing monitoring capabilities by allowing both HTTP GET and HEAD requests. This functionality complements the existing gRPC health checks, providing a user-friendly method for uptime verification. The changes include new handlers and tests, ensuring robust validation for service health across different protocols.
Files | Change Summary |
---|---|
server/rpc/httphealth/httphealth.go |
Introduced HTTP health check handler with NewHandler for service health checks using GET/HEAD. |
server/rpc/server.go |
Modified NewServer to include integration of the new HTTP health check handler alongside gRPC. |
test/integration/health_test.go |
Expanded health check tests to cover both gRPC and HTTP methods with new test functions. |
Objective | Addressed | Explanation |
---|---|---|
Add Plain HTTP Health Check Endpoint (#832) | ✅ |
🐰 In the meadow, where bunnies hop,
A health check blooms, no need to stop!
With GET and HEAD, we watch and play,
Uptime’s a joy, come what may!
Hooray for health, let’s cheer and sing,
Monitoring made easy, oh what joy it brings! 🌼
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?
@taeng0204 Could you please resolve the conflicts?
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 51.06%. Comparing base (
80c6ea0
) to head (505a40b
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@taeng0204 I think we should include that the health check is within Yorkie service to state that this health check is related to Yorkie, so /yorkie.v1.YorkieService/health
or other names that includes /yorkie.v1.YorkieService
will be better.
@krapie In the existing grpc health check, there is a grpc.health.v1.Health
that supports health checks for the yorkie service, admin service, and health check service, so why not http.health.v1.Health
instead?
@taeng0204 That is also a good approach. When we proceeding with http.health.v1.Health
, we need reconfigure yorkie cluster's routing conditions.
Note that we are using /yorkie.v1
for yorkie service route, /grafana
for Grafana dashbaord, and /argocd
for ArgoCD dashboard.
@krapie thanks for the answer:)
I've been thinking a lot about it, and I think it's better to integrate it into the existing /yorkie.v1
. This code was written for a special situation for ncp lb, and I don't think it changes the existing system too much.
I'll fix the code:)
@krapie I tried modifying it like grpchealth
's code, but I'm not sure if it's okay to use the variable names and comments as is.
// HealthV1ServiceName is the fully-qualified name of the v1 version of the health service.
const HealthV1ServiceName = "/yorkie.v1.YorkieService/health"
What this PR does / why we need it: In PR #952 , There were issues with existing health checks needing to support HEAD methods for UptimeRobot and overlapping with envoy's health check path. So I added the HEAD method and test code, and modified the health check endpoint.
Which issue(s) this PR fixes:
Fixes #832
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes
Tests