Modify health check endpoint and add HEAD method #958
Conversation
Added a handler to allow health checks to be performed with plain HTTP GET requests, rather than health checks using rpc.
Rename existing names used by other packages for lint
The package name includes health, so we remove health from the handler function
Write a health check for each service and a check failure test for the unknown service.
Simplify too many health check functions
Modified test functions for each service to simplify them
Rename for consistency
Added the handler to allow health checks to be performed with plain HTTP GET requests needed for traditional uptime checker or load balancer, along with existing gRPC health check.
Need HEAD method for UptimeRobot and fix path overlap with Envoy
In the existing test code, the order of expected and actual values is different, which makes it inconvenient to check the log when the test fails.
WalkthroughThe 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. Changes
Assessment against linked issues
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? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
server/rpc/httphealth/httphealth.go (1)
33-64: Recommendation: Validate theserviceparameter inNewHandler.The
NewHandlerfunction inserver/rpc/httphealth/httphealth.godoes not validate theserviceparameter. This can lead to potential security vulnerabilities. Consider adding validation logic to ensure theserviceparameter is safe and expected before using it.
- File:
server/rpc/httphealth/httphealth.go- Function:
NewHandlerAnalysis chain
Ensure proper error handling and response construction.
The
NewHandlerfunction is well-implemented, supporting both GET and HEAD methods. However, consider the following improvements:
- Error Handling: Ensure that all potential errors are logged for better debugging.
- Security: Validate and sanitize the
serviceparameter to prevent injection attacks.- Performance: Consider using a
sync.PoolforCheckResponseto reduce allocations in high-load scenarios.Overall, the function is correctly handling HTTP methods and constructing responses.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the `NewHandler` function and ensure the service parameter is validated. # Test: Search for the `NewHandler` function usage. Expect: Proper validation of the service parameter. rg --type go 'NewHandler'Length of output: 440
Script:
#!/bin/bash # Extract lines around the usage of NewHandler in server/rpc/server.go to check for service parameter validation. rg --type go -A 10 -B 10 'NewHandler' server/rpc/server.goLength of output: 1011
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- server/rpc/httphealth/httphealth.go (1 hunks)
- server/rpc/server.go (2 hunks)
- test/integration/health_test.go (2 hunks)
Additional comments not posted (4)
server/rpc/server.go (1)
Line range hint
39-77:
Verify server configuration and integration of new health check handler.The integration of the HTTP health check handler is correctly done. However, consider the following:
- Configuration: Ensure that the server configuration, especially for the new health check route, is thoroughly tested.
- Security: Verify that the health check endpoint is secured appropriately, especially if exposed to the public.
Overall, the integration appears sound, but verification is recommended to ensure robustness.
test/integration/health_test.go (3)
Line range hint
42-79:
Comprehensive gRPC health check tests.The
TestRPCHealthCheckfunction is well-implemented, covering default, specific, and unknown services. Ensure that any additional services are added to theserviceslist for comprehensive coverage.
81-125: Thorough HTTP GET health check tests.The
TestHTTPGETHealthCheckfunction is comprehensive, testing default, specific, and unknown services. The use of sub-tests enhances clarity and reporting.
127-160: Comprehensive HTTP HEAD health check tests.The
TestHTTPHEADHealthCheckfunction is well-implemented, covering default, specific, and unknown services. The tests are consistent with the GET tests, ensuring robust validation.
|
@taeng0204 Could you please resolve the conflicts? |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/integration/health_test.go (4 hunks)
Additional comments not posted (3)
test/integration/health_test.go (3)
Line range hint
48-79:
LGTM!The restructuring of the gRPC health check tests into sub-tests is well-organized and enhances readability.
81-125: LGTM!The implementation of HTTP GET health checks with sub-tests and response decoding is clear and effective.
127-161: LGTM!The implementation of HTTP HEAD health checks with sub-tests is well-structured and effective.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #958 +/- ##
=======================================
Coverage 51.06% 51.06%
=======================================
Files 73 73
Lines 10782 10782
=======================================
Hits 5506 5506
Misses 4725 4725
Partials 551 551 ☔ View full report in Codecov by Sentry. |
server/rpc/httphealth/httphealth.go
Outdated
| // CheckResponse represents the response structure for health checks. | ||
| type CheckResponse struct { | ||
| Status string `json:"status"` | ||
| } | ||
|
|
||
| // NewHandler creates a new HTTP handler for health checks. | ||
| func NewHandler(checker grpchealth.Checker) (string, http.Handler) { | ||
| const serviceName = "/yorkie.v1/healthz/" |
There was a problem hiding this comment.
How about changing the service name to /yorkie.v1.Health/Check to correspond with /grpc.health.v1.Health/Check and /yorkie.v1.YorkieService/XXX to keep consistency with other RPCs?
Also, I think we don't need the ending / slash, since we are using ? query parameter after.
|
@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 |
|
@krapie In the existing grpc health check, there is a |
|
@taeng0204 That is also a good approach. When we proceeding with Note that we are using |
|
@krapie thanks for the answer:) |
Modified the endpoint for consistency with gRPC
|
@krapie I tried modifying it like // HealthV1ServiceName is the fully-qualified name of the v1 version of the health service.
const HealthV1ServiceName = "/yorkie.v1.YorkieService/health" |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- server/rpc/httphealth/httphealth.go (2 hunks)
- test/integration/health_test.go (4 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/rpc/httphealth/httphealth.go
Additional comments not posted (3)
test/integration/health_test.go (3)
Line range hint
41-79:
LGTM! Enhanced test coverage for gRPC health checks.The restructuring of the health check tests into sub-tests for each service improves clarity and coverage.
81-125: LGTM! Comprehensive HTTP GET health check tests.The addition of HTTP GET health checks for multiple services, including error handling for unknown services, is well-implemented.
127-161: LGTM! Well-structured HTTP HEAD health check tests.The addition of HTTP HEAD health checks for multiple services, including error handling for unknown services, is well-implemented.
server/rpc/httphealth/httphealth.go
Outdated
| @@ -34,36 +34,34 @@ type CheckResponse struct { | |||
|
|
|||
| // NewHandler creates a new HTTP handler for health checks. | |||
| func NewHandler(checker grpchealth.Checker) (string, http.Handler) { | |||
| const serviceName = "/yorkie.v1.YorkieService/health" | |||
There was a problem hiding this comment.
I think now we have introduced HealthV1ServiceName constant, we can remove this variable.
There was a problem hiding this comment.
It preserves the structure of the existing grpchealth, which we don't need! Thanks:)
We only left one duplicate const service name.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- server/rpc/httphealth/httphealth.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/rpc/httphealth/httphealth.go
Change health check endpoint to include yorkie service endpoint along with HEAD method to support various health checkers.
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