[health] add internal healthcheck command #202
Conversation
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as cmd/sms-gateway/main
participant HealthRun as health.Run()
participant Fx as Fx App
participant Checker as Health Checker
participant Endpoint as /health/live
User->>CLI: run "app health"
CLI->>HealthRun: call health.Run()
HealthRun->>Fx: compose & start (logger, config, health module)
Fx->>Checker: construct & register OnStart -> Execute
Fx->>Checker: invoke OnStart -> Checker.Execute(ctx)
Checker->>Endpoint: HTTP GET /health/live (1s timeout)
Endpoint-->>Checker: response (status, body)
Checker->>Checker: read & log body
alt status >= 400
Checker->>Fx: return ErrNotHealthy (startup fails)
else status < 400
Checker->>Fx: call Shutdown (graceful stop)
end
Fx-->>HealthRun: shutdown complete / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
Dockerfile.goreleaser (1)
7-7: Consider removing unused curl.Since the healthcheck now uses the internal binary command instead of curl, the
curlpackage is no longer needed and can be removed to reduce the image size.🔎 Proposed change
-RUN apk add --no-cache ca-certificates tzdata curl +RUN apk add --no-cache ca-certificates tzdatainternal/health/app.go (1)
31-33: OnStop hook is a no-op.While this is functionally correct for a health check command that exits after execution, consider adding debug logging to track the shutdown lifecycle for observability.
🔎 Optional enhancement
Add logger to the invoke function parameters and log the shutdown:
- fx.Invoke(func(lc fx.Lifecycle, checker *Checker) { + fx.Invoke(func(lc fx.Lifecycle, checker *Checker, logger *zap.Logger) { lc.Append(fx.Hook{ OnStart: func(ctx context.Context) error { return checker.Execute(ctx) }, OnStop: func(_ context.Context) error { + logger.Debug("health check lifecycle stopped") return nil }, }) }),internal/health/checker.go (1)
58-62: Consider returning shutdown errors.The shutdown error is logged but not returned, which means
Executereturnsnileven if shutdown fails. For better error propagation and observability, consider returning the shutdown error.🔎 Suggested change
if err := c.shutdowner.Shutdown(); err != nil { c.logger.Error("failed to shutdown", zap.Error(err)) + return fmt.Errorf("failed to shutdown after health check: %w", err) } return nilcmd/sms-gateway/main.go (1)
62-71: Consider adding an explicit default case.The switch statement currently falls through to
smsgateway.Run()for any unrecognized command. While this works, adding an explicit default case could improve clarity and allow for validation of unknown commands.🔎 Optional enhancement
switch cmd { case cmdHealth: health.Run() return case cmdWorker: worker.Run() return + default: + // Fall through to main application } smsgateway.Run()Or to validate unknown commands:
switch cmd { case cmdHealth: health.Run() return case cmdWorker: worker.Run() return + case "start", "": // Explicitly handle default commands + // Fall through to main application + default: + fmt.Fprintf(os.Stderr, "Unknown command: %s\n", cmd) + os.Exit(1) } smsgateway.Run()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Dockerfile.goreleaserbuild/package/Dockerfilecmd/sms-gateway/main.gointernal/health/app.gointernal/health/checker.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/health/app.go (1)
internal/health/checker.go (2)
NewChecker(22-28)Checker(15-20)
cmd/sms-gateway/main.go (3)
internal/health/app.go (1)
Run(12-20)internal/sms-gateway/app.go (1)
Run(69-76)internal/worker/app.go (1)
Run(19-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: E2E
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (5)
build/package/Dockerfile (1)
36-36: Good dependency cleanup.Removing
curlreduces the final image size and attack surface since the healthcheck now uses the internal binary command.internal/health/app.go (1)
12-20: Verify the 1-second start timeout is sufficient.The
fx.StartTimeout(time.Second)is quite aggressive. If module initialization (logger, config, health checker) takes longer than 1 second, the application will fail to start. For a health check command, this might be intentional to fail fast, but ensure all required modules can initialize within this constraint.internal/health/checker.go (1)
22-28: LGTM!The constructor is straightforward and properly initializes the Checker with injected dependencies.
cmd/sms-gateway/main.go (2)
6-6: LGTM!The new health command constant and import follow the existing pattern established by the worker command.
Also applies to: 13-13
56-60: LGTM!The refactored argument parsing is more flexible and cleaner than the previous implementation.
🤖 Pull request artifacts
|
capcom6
force-pushed
the
health/add-internal-health-checker
branch
from
024be30 to
eefd48e
Compare
capcom6
force-pushed
the
health/add-internal-health-checker
branch
from
3be283f to
6ddb34c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/health/checker.go (1)
55-68: Good: Response handling improved.The error handling for
io.ReadAllnow correctly returns immediately on failure, preventing logging of partial content. However, logging the full response body at Info level (line 60) may be verbose for routine health checks.🔎 Optional: Log body only at Debug level or when status indicates failure
- c.logger.Info(string(body)) - if res.StatusCode >= httpclient.StatusBadRequest { c.logger.Error("health check failed", zap.Int("status", res.StatusCode), zap.String("body", string(body))) return fmt.Errorf("%w: health check failed: %s", ErrNotHealthy, string(body)) } - c.logger.Info("health check passed", zap.Int("status", res.StatusCode)) + c.logger.Info("health check passed", zap.Int("status", res.StatusCode), zap.String("body", string(body)))internal/health/app.go (1)
12-20: Consider increasing StartTimeout.The 1-second
StartTimeoutmatches the timeout inchecker.Execute(line 34 in checker.go), but doesn't account for Fx initialization overhead. If startup takes longer, the health check may timeout prematurely.🔎 Optional: Increase StartTimeout to allow for initialization overhead
- fx.StartTimeout(time.Second), + fx.StartTimeout(3*time.Second),
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Dockerfile.goreleaserbuild/package/Dockerfilecmd/sms-gateway/main.gointernal/health/app.gointernal/health/checker.go
🧰 Additional context used
🧬 Code graph analysis (2)
internal/health/app.go (1)
internal/health/checker.go (2)
NewChecker(25-31)Checker(18-23)
cmd/sms-gateway/main.go (3)
internal/health/app.go (1)
Run(12-20)internal/sms-gateway/app.go (1)
Run(69-76)internal/worker/app.go (1)
Run(19-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
Dockerfile.goreleaser (2)
7-7: LGTM: Removed external curl dependency.The removal of
curlaligns with the shift to an internal health check mechanism, reducing the image size and external dependencies.
23-24: LGTM: HEALTHCHECK syntax is now correct.The JSON exec form is properly formatted with a comma between arguments. This resolves the critical syntax issue from the previous review.
build/package/Dockerfile (2)
36-36: LGTM: Removed external curl dependency.The removal of
curlis consistent with the internal health check approach and reduces the final image footprint.
49-50: LGTM: HEALTHCHECK syntax is now correct.The JSON exec form is properly formatted with a comma between arguments, resolving the critical syntax issue flagged in the previous review.
internal/health/checker.go (1)
69-71: Verify: Shutdown on successful health check.The code initiates a graceful shutdown after a successful health check. This is expected for a one-shot health verification command that should exit after checking, but confirm this is the intended behavior.
internal/health/app.go (1)
22-37: LGTM: Lifecycle integration is correct.The health module properly wires
NewCheckerand invokeschecker.Executein theOnStarthook. The error fromExecutewill correctly prevent the application from starting if the health check fails.cmd/sms-gateway/main.go (3)
6-6: LGTM: Health package imported.The import statement correctly adds the internal health package for the new health command.
11-14: LGTM: Health command constant added.The
cmdHealthconstant is properly defined alongsidecmdWorker, maintaining consistency in command definitions.
56-72: LGTM: Command routing logic is correct.The switch statement properly routes commands:
health→health.Run()with early returnworker→worker.Run()with early return- default →
smsgateway.Run()The early returns prevent fallthrough, and the default case correctly handles the main application flow.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.