Add HTTP health check handler for server health monitoring #952
Conversation
|
|
WalkthroughThe recent changes introduce a straightforward HTTP health check endpoint to the server, enhancing service observability. A new handler responds to GET requests, providing an accessible way for uptime checkers to monitor service health. This feature complements existing gRPC health checks, ensuring a versatile and user-friendly monitoring solution. 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: 3
Outside diff range, codebase verification and nitpick comments (1)
server/rpc/health/health.go (1)
1-15: Add a package-level comment.The file lacks a package-level comment describing the purpose and usage of the
healthpackage. Adding this would improve code readability and maintainability.// Package health provides an HTTP handler for health checks.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- server/rpc/health/health.go (1 hunks)
- server/rpc/server.go (2 hunks)
- test/integration/health_test.go (2 hunks)
Additional context used
golangci-lint
server/rpc/health/health.go
55-55: Error return value of
w.Writeis not checked(errcheck)
GitHub Check: build
server/rpc/health/health.go
[failure] 55-55:
Error return value ofw.Writeis not checked (errcheck)
[failure] 27-27:
exported: exported type HealthCheckResponse should have comment or be unexported (revive)
[failure] 31-31:
exported: exported function NewHTTPHealthCheckHandler should have comment or be unexported (revive)
Additional comments not posted (6)
server/rpc/health/health.go (1)
35-36: Usehttp.StatusMethodNotAllowedfor unsupported methods.The handler should return
http.StatusMethodNotAllowedfor unsupported methods like POST, PUT, etc. This ensures that only GET requests are allowed.test/integration/health_test.go (2)
Line range hint
35-50:
LGTM! The gRPC health check test is correctly implemented.The
TestRPCHealthCheckfunction is correctly implemented and verifies the gRPC health check functionality.
52-65: LGTM! The HTTP health check test is correctly implemented.The
TestHTTPHealthCheckfunction is correctly implemented and verifies the HTTP health check functionality.server/rpc/server.go (3)
39-39: LGTM! The new health check import is correctly added.The import statement for the
healthpackage is correctly added.
66-70: LGTM! The health checker initialization is correctly implemented.The
healthCheckervariable is correctly initialized with the necessary services.
77-77: LGTM! The HTTP health check handler is correctly integrated.The
NewHTTPHealthCheckHandleris correctly integrated into the server's HTTP multiplexer.
|
@taeng0204 Could you please sign the CLA? |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
server/rpc/health/health.go (1)
32-33: Add a comment for the exported functionNewHTTPHealthCheckHandler.The exported function
NewHTTPHealthCheckHandlershould have a comment or be unexported, as per Go's best practices.// NewHTTPHealthCheckHandler creates a new HTTP handler for health checks.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- server/rpc/health/health.go (1 hunks)
- server/rpc/server.go (2 hunks)
- test/integration/health_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/rpc/server.go
Additional context used
GitHub Check: build
server/rpc/health/health.go
[failure] 28-28:
exported: type name will be used as health.HealthCheckResponse by other packages, and that stutters; consider calling this CheckResponse (revive)
Additional comments not posted (3)
test/integration/health_test.go (2)
Line range hint
35-50:
LGTM! The renaming enhances clarity.The renaming of the function from
TestHealthChecktoTestRPCHealthCheckclarifies its focus on gRPC health check functionality. The implementation is correct.
52-65: LGTM! The new test function is well implemented.The new function
TestHTTPHealthCheckcorrectly implements the HTTP health check test. It verifies the response status code and decodes the response body intohealth.HealthCheckResponse.server/rpc/health/health.go (1)
55-59: Check the error return value ofw.Write.The error return value of
w.Writeshould be checked to ensure that the response is successfully written to the client.- w.Write(resp) + if _, err := w.Write(resp); err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + }Likely invalid or redundant comment.
|
@krapie |
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/health/health.go (1 hunks)
- test/integration/health_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- server/rpc/health/health.go
- test/integration/health_test.go
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
|
My real email address and the email info in the commit didn't match, which is why the CLA wasn't working... |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- server/rpc/health/health.go (1 hunks)
- server/rpc/server.go (2 hunks)
- test/integration/health_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- server/rpc/health/health.go
- test/integration/health_test.go
Additional comments not posted (3)
server/rpc/server.go (3)
39-39: Import statement for health package added.The import statement for the
healthpackage has been correctly added to support the new HTTP health check functionality.
66-70: Initialization of healthChecker variable.The
healthCheckervariable has been correctly initialized with the services' names, enhancing code clarity and maintainability.
77-77: Addition of HTTP health check handler.The HTTP health check handler has been correctly added to the server multiplexer, improving service reliability and monitoring capabilities.
The package name includes health, so we remove health from the handler function
taeng0204
changed the title
Write a health check for each service and a check failure test for the unknown service.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- server/rpc/health/health.go (1 hunks)
- server/rpc/server.go (2 hunks)
- test/integration/health_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- server/rpc/health/health.go
Additional comments not posted (11)
server/rpc/server.go (2)
39-39: Import statement for health package looks good.The import statement for the
healthpackage is necessary for the new HTTP health check handler.
66-77: Initialization of healthChecker and addition of HTTP health check handler looks good.The initialization of
healthCheckerand the addition of the HTTP health check handler to the multiplexer are implemented correctly.However, ensure that the new HTTP health check handler is properly integrated and tested.
test/integration/health_test.go (9)
23-30: Import statements for health checks and testing look good.The import statements for the
healthpackage,json,http, andtestingare necessary for the new test functions.
53-70: New test function for gRPC health check of Yorkie service looks good.The test function performs a gRPC health check for the Yorkie service and asserts the expected status.
72-89: New test function for gRPC health check of Admin service looks good.The test function performs a gRPC health check for the Admin service and asserts the expected status.
91-108: New test function for gRPC health check of Health service looks good.The test function performs a gRPC health check for the Health service and asserts the expected status.
110-126: New test function for gRPC health check of unknown service looks good.The test function performs a gRPC health check for an unknown service and asserts the expected error.
143-156: New test function for HTTP health check of Yorkie service looks good.The test function performs an HTTP health check for the Yorkie service and asserts the expected status and response.
158-171: New test function for HTTP health check of Admin service looks good.The test function performs an HTTP health check for the Admin service and asserts the expected status and response.
173-186: New test function for HTTP health check of Health service looks good.The test function performs an HTTP health check for the Health service and asserts the expected status and response.
188-196: New test function for HTTP health check of unknown service looks good.The test function performs an HTTP health check for an unknown service and asserts the expected status and response.
|
@taeng0204 If you finish your work, please re-request review. |
Simplify too many health check functions
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (1)
test/integration/health_test.go (1)
Line range hint
36-57:
Consider using subtests for better structure.Using subtests can help organize the checks for known and unknown services, making the test output more readable.
func TestRPCHealthCheck(t *testing.T) { // use gRPC health check t.Run("KnownService", func(t *testing.T) { conn, err := grpc.Dial( defaultServer.RPCAddr(), grpc.WithTransportCredentials(insecure.NewCredentials()), ) assert.NoError(t, err) defer func() { assert.NoError(t, conn.Close()) }() cli := healthpb.NewHealthClient(conn) resp, err := cli.Check(context.Background(), &healthpb.HealthCheckRequest{}) assert.NoError(t, err) assert.Equal(t, resp.Status, healthpb.HealthCheckResponse_SERVING) }) // use gRPC health check for unknown service t.Run("UnknownService", func(t *testing.T) { conn, err := grpc.Dial( defaultServer.RPCAddr(), grpc.WithTransportCredentials(insecure.NewCredentials()), ) assert.NoError(t, err) defer func() { assert.NoError(t, conn.Close()) }() cli := healthpb.NewHealthClient(conn) _, err = cli.Check(context.Background(), &healthpb.HealthCheckRequest{ Service: "unknown", }) assert.Error(t, err) }) }
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- test/integration/health_test.go (2 hunks)
Additional comments not posted (7)
test/integration/health_test.go (7)
59-76: LGTM!The function correctly performs a gRPC health check for the Yorkie service.
78-95: LGTM!The function correctly performs a gRPC health check for the Admin service.
97-114: LGTM!The function correctly performs a gRPC health check for the health service.
139-152: LGTM!The function correctly performs an HTTP health check for the Yorkie service.
154-167: LGTM!The function correctly performs an HTTP health check for the Admin service.
169-181: LGTM!The function correctly performs an HTTP health check for the health service.
23-30: LGTM!The imports are necessary and correctly added to support the new tests.
Modified test functions for each service to simplify them
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 (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- test/integration/health_test.go
|
@krapie I've completed it to requirements. |
There was a problem hiding this comment.
Thanks! Test code looks good to me.
One last suggestion before merging the PR.
Does Yorkie have any documentation or guidelines for writing test code?
We only have overall guideline in CONTRIBUTING.MD file.
Rename for consistency
There was a problem hiding this comment.
Actionable comments posted: 0
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 (10)
server/rpc/httphealth/httphealth.go (3)
1-18: File header and package declaration look good.The file header and package declaration are standard and correctly formatted.
20-30: Imports and type definition look good.The imported packages are relevant, and the
CheckResponsetype is correctly defined.
32-62:NewHandlerfunction looks good.The function correctly handles HTTP GET requests, checks service health, and returns a JSON response. Error handling is appropriately implemented.
test/integration/health_test.go (4)
Line range hint
1-22:
File header and package declaration look good.The file header and package declaration are standard and correctly formatted.
23-40: Imports and global variables look good.The imported packages are relevant, and the
servicesvariable is correctly defined.
Line range hint
42-79:
TestRPCHealthCheckfunction looks good.The function correctly tests the gRPC health checks for default, known, and unknown services, including proper error handling.
81-124:TestHTTPHealthCheckfunction looks good.The function correctly tests the HTTP health checks for default, known, and unknown services, including proper error handling.
server/rpc/server.go (3)
Line range hint
1-18:
File header and package declaration look good.The file header and package declaration are standard and correctly formatted.
Line range hint
19-39:
Imports look good.The imported packages are relevant and correctly organized.
Line range hint
41-79:
NewServerfunction looks good.The function correctly initializes the server, sets up health check handlers, and includes proper error handling.
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.
|
@taeng0204 There are two things that I want to share:
|
|
@krapie I've processed it in this branch and created a new PR as we talked about today. |
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.
Added a handler to allow health checks to be performed with plain HTTP GET requests, rather than health checks using rpc.
What this PR does / why we need it:
In issue #832 , we needed a health check via HTTP GET or HEAD method because of NCP's load balancer.
The existing grpchealth could not satisfy this, so we implemented a separate handler.
This allows us to check the health of a specific service via the path
/healthz/and the health of a specific service via the queryservice=<serviceName>.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