Refactor server cmd #119
Refactor server cmd #119
Conversation
Signed-off-by: naveensrinivasan <172697+naveensrinivasan@users.noreply.github.com>
WalkthroughThe pull request introduces several changes across multiple files, primarily adding a health check feature to the service. A new Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
|
![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 and nitpick comments (10)
api/v1/service.proto (2)
67-69: Consider using enum for status field.Using a string type for status might lead to inconsistent status values across the system. Consider using an enum to ensure consistent status values.
+enum HealthStatus { + HEALTH_STATUS_UNSPECIFIED = 0; + HEALTH_STATUS_SERVING = 1; + HEALTH_STATUS_NOT_SERVING = 2; + HEALTH_STATUS_UNKNOWN = 3; +} message HealthCheckResponse { - string status = 1; + HealthStatus status = 1; }
91-92: Consider future extensibility of health check.While using
google.protobuf.Emptyas request type is common for health checks, it might limit future extensibility. If you anticipate needing health check parameters in the future (e.g., component-specific checks), consider defining a custom request message.+message HealthCheckRequest { + // Reserved for future use + reserved 1 to 10; +} service HealthService { - rpc Check(google.protobuf.Empty) returns (HealthCheckResponse) {} + rpc Check(HealthCheckRequest) returns (HealthCheckResponse) {} }🧰 Tools
🪛 buf
92-92: "google.protobuf.Empty" is used as the request or response type for multiple RPCs.
(RPC_REQUEST_RESPONSE_UNIQUE)
92-92: RPC request type "Empty" should be named "CheckRequest" or "HealthServiceCheckRequest".
(RPC_REQUEST_STANDARD_NAME)
92-92: RPC response type "HealthCheckResponse" should be named "CheckResponse" or "HealthServiceCheckResponse".
(RPC_RESPONSE_STANDARD_NAME)
go.mod (2)
Line range hint
3-3: Fix invalid Go version.The specified Go version
1.23.1is invalid as Go versions currently only go up to 1.22.x. Please update to a valid version.-go 1.23.1 +go 1.22.1
42-42: Consider using structured logging consistently.The addition of
github.com/go-logr/logrsuggests implementing structured logging. This aligns well with the health check feature for better observability.Consider implementing consistent logging patterns across the codebase:
- Use log levels appropriately (debug, info, error)
- Include relevant context in log fields
- Add trace IDs for request tracking
cmd/server/server_test.go (2)
11-63: Consider adding more edge cases to the test suite.The table-driven test structure is well-implemented and covers the basic scenarios. However, consider adding the following test cases to improve coverage:
- Negative concurrency values
- Invalid address formats (e.g., missing port, invalid hostname)
- Boundary testing for concurrency (e.g., very high values, zero)
{ + name: "invalid concurrency", + initialOptions: &options{}, + flagValues: map[string]string{ + "addr": "localhost:8089", + "concurrency": "-1", + }, + shouldSetFlags: true, + }, + { + name: "invalid address format", + initialOptions: &options{}, + flagValues: map[string]string{ + "addr": "localhost", + "concurrency": "10", + }, + shouldSetFlags: true, + },
69-111: Add test cases for error scenarios and command execution.While the basic command structure testing is good, consider:
- Testing the RunE function execution
- Adding error scenarios
- Verifying command persistence
tests := []struct { name string storage graph.Storage + wantErr bool want struct { use string short string } }{ + { + name: "handles storage error", + storage: &mockStorage{GetError: errors.New("storage error")}, + wantErr: true, + want: struct { + use string + short string + }{ + use: "server", + short: "Start the minefield server for graph operations and queries", + }, + },api/v1/service_test.go (1)
150-156: Consider enhancing test coverage with additional test cases.The test follows good practices by using the shared setup helper and maintaining consistency with existing test patterns. However, consider adding:
- Negative test cases:
- Test behavior with nil context
- Test behavior with nil request
- Additional assertions:
- Verify response is not nil before checking status
Example enhancement:
func TestHealthCheck(t *testing.T) { s := setupService() + t.Run("success case", func(t *testing.T) { req := connect.NewRequest(&emptypb.Empty{}) resp, err := s.Check(context.Background(), req) require.NoError(t, err) + require.NotNil(t, resp) + require.NotNil(t, resp.Msg) assert.Equal(t, "ok", resp.Msg.Status) + }) + + t.Run("nil context", func(t *testing.T) { + req := connect.NewRequest(&emptypb.Empty{}) + _, err := s.Check(nil, req) + assert.Error(t, err) + }) + + t.Run("nil request", func(t *testing.T) { + _, err := s.Check(context.Background(), nil) + assert.Error(t, err) + }) }cmd/server/server.go (3)
46-50: Remove redundant default assignment of service addressIn the
AddFlagsmethod, theaddrfield already has a default value of"localhost:8089". Therefore, checking ifserviceAddris empty and reassigning the same default value insetupServeris redundant. Consider simplifying the code by removing this conditional.Apply this diff to streamline the code:
func (o *options) setupServer() (*http.Server, error) { if o.concurrency <= 0 { return nil, fmt.Errorf("concurrency must be greater than zero") } - serviceAddr := o.addr - if serviceAddr == "" { - serviceAddr = "localhost:8089" - } + serviceAddr := o.addr
80-82: Handle server startup errors more explicitlyCurrently, startup errors after
ListenAndServeare logged but not returned. Consider returning the error to allow higher-level handling or to exit the application if needed.Apply this diff to return the error:
func (o *options) startServer(server *http.Server) error { // Handle graceful shutdown stop := make(chan os.Signal, 1) signal.Notify(stop, os.Interrupt, syscall.SIGTERM) go func() { log.Printf("Server is starting on %s\n", server.Addr) - if err := server.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) { + err := server.ListenAndServe() + if err != nil && !errors.Is(err, http.ErrServerClosed) { log.Printf("ListenAndServe(): %s\n", err) + stop <- os.Interrupt } }()
104-105: Ensure all references to the command are updatedThe command usage has changed to
"server". Please verify that all documentation, scripts, and references have been updated to reflect this change to prevent any confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (3)
gen/api/v1/apiv1connect/service.connect.gois excluded by!**/gen/**gen/api/v1/service.pb.gois excluded by!**/*.pb.go,!**/gen/**go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
api/v1/service.go(1 hunks)api/v1/service.proto(2 hunks)api/v1/service_test.go(1 hunks)cmd/root/root.go(2 hunks)cmd/server/server.go(4 hunks)cmd/server/server_test.go(1 hunks)go.mod(1 hunks)
🧰 Additional context used
🪛 buf
api/v1/service.proto
92-92: "google.protobuf.Empty" is used as the request or response type for multiple RPCs.
(RPC_REQUEST_RESPONSE_UNIQUE)
92-92: RPC request type "Empty" should be named "CheckRequest" or "HealthServiceCheckRequest".
(RPC_REQUEST_STANDARD_NAME)
92-92: RPC response type "HealthCheckResponse" should be named "CheckResponse" or "HealthServiceCheckResponse".
(RPC_RESPONSE_STANDARD_NAME)
🔇 Additional comments (4)
cmd/root/root.go (2)
8-8: LGTM: Clean import addition
The addition of the server package import is clean and properly organized with other internal imports.
38-38: LGTM: Command registration updated correctly
The command registration has been properly updated to use the new server package, maintaining consistency with other command registrations and aligning with the server command refactoring objective.
Let's verify the server package structure and its compatibility:
✅ Verification successful
Server command registration is correctly implemented
The verification confirms that:
- The server package has the correct
New(storage graph.Storage)function signature - The server implementation properly uses the new service package from
github.com/bitbomdev/minefield/api/v1 - The command setup follows the same pattern as other commands in the codebase
- No legacy
start_servicereferences remain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the server package structure and its New function signature
# Test 1: Verify the server package exists and has the expected New function
ast-grep --pattern 'func New(storage $_) *cobra.Command {
$$$
}'
# Test 2: Check for any remaining references to start_service to ensure complete migration
rg "start_service"
Length of output: 14891
Script:
#!/bin/bash
# Let's verify the server package implementation and its compatibility
# Check the server package implementation
rg -A 10 "type options struct" cmd/server/server.go
# Check if there are any references to the old service implementation
rg -l "service" cmd/server/
Length of output: 495
Script:
#!/bin/bash
# Let's check the actual server implementation and service references
# Check the server implementation details
rg -A 20 "func \(o \*options\) Run" cmd/server/server.go
# Check for any service-related imports
rg -A 5 "^import" cmd/server/server.go
Length of output: 706
Script:
#!/bin/bash
# Let's check the service package references and implementation
# Check all imports in server.go to see service package path
rg -p "^import \(" -A 15 cmd/server/server.go
# Check for the service package implementation
fd service pkg/
Length of output: 404
cmd/server/server_test.go (1)
1-9: LGTM! Package setup and imports are well-structured.
The package name matches the directory, and all necessary testing dependencies are properly imported.
api/v1/service.go (1)
310-312: 🛠️ Refactor suggestion
Enhance health check implementation with comprehensive status checks.
The current implementation is overly simplistic and doesn't provide meaningful health status information. Consider implementing these improvements:
- Verify critical dependencies (e.g., storage connectivity)
- Include additional status information (version, uptime, etc.)
- Handle different health states
- Utilize the context for timeout handling
Here's a suggested implementation:
func (s *Service) Check(ctx context.Context, req *connect.Request[emptypb.Empty]) (*connect.Response[service.HealthCheckResponse], error) {
- return connect.NewResponse(&service.HealthCheckResponse{Status: "ok"}), nil
+ // Check storage health
+ if err := s.checkStorageHealth(ctx); err != nil {
+ return connect.NewResponse(&service.HealthCheckResponse{
+ Status: "degraded",
+ Details: fmt.Sprintf("storage error: %v", err),
+ Timestamp: time.Now().Unix(),
+ }), nil
+ }
+
+ return connect.NewResponse(&service.HealthCheckResponse{
+ Status: "healthy",
+ Details: "all systems operational",
+ Timestamp: time.Now().Unix(),
+ }), nil
}
+
+func (s *Service) checkStorageHealth(ctx context.Context) error {
+ // Add timeout to context
+ ctx, cancel := context.WithTimeout(ctx, 5*time.Second)
+ defer cancel()
+
+ // Perform a simple storage operation to verify connectivity
+ _, err := s.storage.GetAllKeys()
+ return err
+}Note: You'll need to update the HealthCheckResponse proto message to include the additional fields (details and timestamp).
Let's verify the proto definition:
Signed-off-by: neilnaveen <42328488+neilnaveen@users.noreply.github.com>
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores