[health] improve health reporting #186
Conversation
WalkthroughReworks health into separate startup/readiness/liveness probes with new endpoints and handlers; moves DB health provider into the db module; converts package-level fx.Module variables to Module() constructors; updates Dockerfile healthcheck, Helm probes/values, API requests, go.mod, and related wiring. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgb(245,250,255)
participant Client
participant Handler as HealthHandler
participant Service as HealthService
participant ProvSys as SystemProvider
participant ProvDB as DBProvider
end
Client->>Handler: GET /health (mapped → readiness)
Client->>Handler: GET /health/live
Client->>Handler: GET /health/ready
Client->>Handler: GET /health/startup
Handler->>Service: CheckLiveness/CheckReadiness/CheckStartup(ctx)
par probe providers
Service->>ProvSys: Live/Ready/StartedProbe(ctx)
Service->>ProvDB: Live/Ready/StartedProbe(ctx)
end
ProvSys-->>Service: Checks (goroutines, memory)
ProvDB-->>Service: Checks (ping, failed-pings)
Service-->>Handler: CheckResult (aggregated)
Handler-->>Client: 200 (pass/warn) or 503 (fail) + JSON body
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
🧹 Nitpick comments (10)
build/package/Dockerfile (1)
51-51: Healthcheck path updated — add curl timeouts to honor 5s probe.To avoid the curl call exceeding Docker’s 5s timeout, cap total/connect time.
- CMD curl -fs http://localhost:3000/health/live + CMD curl -fsS --connect-timeout 2 -m 4 http://localhost:3000/health/liveConfirm /health/live is a pure liveness check (no DB/cache calls) so restarts don’t flap on dependencies.
internal/sms-gateway/handlers/health.go (1)
96-113: Minor: deduplicate probe response writing.Liveness/readiness/startup share the same status selection + JSON response. Extract a helper to reduce repetition.
+func writeProbe(c *fiber.Ctx, r health.CheckResult) error { + status := fiber.StatusOK + if r.Status() == health.StatusFail { + status = fiber.StatusServiceUnavailable + } + return c.Status(status).JSON(makeResponse(r)) +} @@ - result := h.healthSvc.CheckLiveness(c.UserContext()) - statusCode := fiber.StatusOK - if result.Status() == health.StatusFail { - statusCode = fiber.StatusServiceUnavailable - } - return c.Status(statusCode).JSON(makeResponse(result)) + return writeProbe(c, h.healthSvc.CheckLiveness(c.UserContext())) @@ - result := h.healthSvc.CheckReadiness(c.UserContext()) - statusCode := fiber.StatusOK - if result.Status() == health.StatusFail { - statusCode = fiber.StatusServiceUnavailable - } - return c.Status(statusCode).JSON(makeResponse(result)) + return writeProbe(c, h.healthSvc.CheckReadiness(c.UserContext())) @@ - result := h.healthSvc.CheckStartup(c.UserContext()) - statusCode := fiber.StatusOK - if result.Status() == health.StatusFail { - statusCode = fiber.StatusServiceUnavailable - } - return c.Status(statusCode).JSON(makeResponse(result)) + return writeProbe(c, h.healthSvc.CheckStartup(c.UserContext()))deployments/helm-chart/templates/deployment.yaml (1)
95-101: Service port "http" is correctly exposed — probe port references are valid.The Service in
deployments/helm-chart/templates/service.yamlproperly exposes a port named "http" (line 17), matching the port references in all probe blocks. The container also defines the "http" port (deployment.yaml:47), and the service correctly mapstargetPort: httpto reference it by name.Templating probe values via
.Values.probes.liveness,.Values.probes.readiness, and.Values.probes.startupremains a valid optimization for per-environment configuration tuning, though the hard-coded defaults are functionally sound.internal/sms-gateway/modules/health/types.go (3)
27-30: Consider JSON tags on CheckResult.Checks to lock response casing and avoid accidental field renames in API output later (e.g.,json:"checks").
50-61: ObservedValue is int; for metrics like memory or counters, int64 (or uint64) is safer and avoids narrowing on 32‑bit. ConsiderObservedValue int64.
69-71: Interface looks good. Minor naming nit: Kubernetes uses “startupProbe”; you’re using StartedProbe. If you plan to map 1:1 to HTTP routes, consider StartupProbe for clarity.internal/sms-gateway/modules/health/health.go (3)
21-51: LiveProbe works. Minor cleanups:
- Store NumGoroutine() once.
- Use a named const for 1 GiB to avoid magic numbers and match MiB unit in output.
- Return the alias type
Checks{...}for clarity.func (h *health) LiveProbe(ctx context.Context) (Checks, error) { var m runtime.MemStats runtime.ReadMemStats(&m) // Basic runtime health checks - goroutineCheck := CheckDetail{ + g := runtime.NumGoroutine() + goroutineCheck := CheckDetail{ Description: "Number of goroutines", - ObservedValue: int(runtime.NumGoroutine()), + ObservedValue: int(g), ObservedUnit: "goroutines", Status: StatusPass, } - memoryCheck := CheckDetail{ + const oneGiB = 1024 * 1024 * 1024 + memoryCheck := CheckDetail{ Description: "Memory usage", ObservedValue: int(m.Alloc / 1024 / 1024), // MiB ObservedUnit: "MiB", Status: StatusPass, } // Check for potential memory issues - if m.Alloc > 1024*1024*1024 { // 1GB + if m.Alloc > oneGiB { // 1 GiB memoryCheck.Status = StatusWarn } // Check for excessive goroutines - if runtime.NumGoroutine() > 1000 { + if g > 1000 { goroutineCheck.Status = StatusWarn } - return map[string]CheckDetail{"goroutines": goroutineCheck, "memory": memoryCheck}, nil + return Checks{"goroutines": goroutineCheck, "memory": memoryCheck}, nil }
54-56: Return an empty map instead of nil to avoid JSON “checks: null” and keep merge logic simple:return Checks{}, nil.
59-61: Same here: preferreturn Checks{}, nilto normalize responses.internal/sms-gateway/modules/db/health.go (1)
34-53: Redundant initialization on line 38.The
ObservedValueis initialized to0on line 38 but unconditionally overwritten on line 50. While harmless, you can remove the redundant initialization.Apply this diff:
func (h *health) ReadyProbe(ctx context.Context) (healthmod.Checks, error) { pingCheck := healthmod.CheckDetail{ Description: "Database ping", ObservedUnit: "failed pings", - ObservedValue: 0, Status: healthmod.StatusPass, }Note: The logic correctly tracks consecutive failed pings across health checks. The counter persists between probe calls, which is appropriate for monitoring database connection stability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
api/requests.http(2 hunks)build/package/Dockerfile(1 hunks)deployments/helm-chart/templates/deployment.yaml(1 hunks)go.mod(1 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/health.go(1 hunks)internal/sms-gateway/modules/db/health.go(1 hunks)internal/sms-gateway/modules/db/module.go(1 hunks)internal/sms-gateway/modules/health/cli.go(0 hunks)internal/sms-gateway/modules/health/db.go(0 hunks)internal/sms-gateway/modules/health/health.go(1 hunks)internal/sms-gateway/modules/health/module.go(1 hunks)internal/sms-gateway/modules/health/service.go(1 hunks)internal/sms-gateway/modules/health/types.go(3 hunks)internal/sms-gateway/modules/messages/metrics.go(1 hunks)
💤 Files with no reviewable changes (2)
- internal/sms-gateway/modules/health/db.go
- internal/sms-gateway/modules/health/cli.go
🧰 Additional context used
🧬 Code graph analysis (7)
internal/sms-gateway/modules/db/module.go (2)
internal/sms-gateway/app.go (1)
Module(35-58)internal/sms-gateway/modules/health/module.go (2)
Module(8-22)AsHealthProvider(24-30)
internal/sms-gateway/modules/health/module.go (5)
internal/sms-gateway/app.go (1)
Module(35-58)internal/sms-gateway/modules/db/module.go (1)
Module(12-22)internal/sms-gateway/modules/health/health.go (1)
NewHealth(11-13)internal/sms-gateway/modules/health/service.go (1)
NewService(24-30)internal/sms-gateway/modules/health/types.go (1)
HealthProvider(66-72)
internal/sms-gateway/app.go (2)
internal/sms-gateway/modules/db/module.go (1)
Module(12-22)internal/sms-gateway/modules/health/module.go (1)
Module(8-22)
internal/sms-gateway/handlers/health.go (3)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/modules/health/types.go (5)
Status(7-7)StatusFail(13-13)CheckResult(27-30)Checks(64-64)CheckDetail(51-61)internal/version/version.go (2)
AppVersion(9-9)AppReleaseID(13-17)
internal/sms-gateway/modules/health/health.go (1)
internal/sms-gateway/modules/health/types.go (6)
Checks(64-64)CheckDetail(51-61)Status(7-7)StatusPass(11-11)StatusWarn(12-12)HealthProvider(66-72)
internal/sms-gateway/modules/db/health.go (1)
internal/sms-gateway/modules/health/types.go (6)
Checks(64-64)CheckDetail(51-61)Status(7-7)StatusPass(11-11)StatusFail(13-13)HealthProvider(66-72)
internal/sms-gateway/modules/health/service.go (1)
internal/sms-gateway/modules/health/types.go (6)
HealthProvider(66-72)Checks(64-64)CheckResult(27-30)CheckDetail(51-61)Status(7-7)StatusFail(13-13)
⏰ 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). (5)
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Benchmark
- GitHub Check: E2E
- GitHub Check: Analyze (go)
🔇 Additional comments (15)
api/requests.http (1)
7-16: All health endpoints are properly implemented and correctly registered.The verification confirms that:
- Local endpoints (
/health,/health/startup,/health/ready,/health/live) are registered viahandlers/root.go- 3rdparty endpoints (
/3rdparty/v1/health/*) are registered viahandlers/3rdparty.gousing the samehealthHandler.Register()method- The
/3rdparty/v1/healthgeneral endpoint exists and maps to the readiness probe- All four endpoints match the requests in
api/requests.httpinternal/sms-gateway/modules/messages/metrics.go (1)
13-14: No-op formatting change.Alignment only; no behavioral impact. LGTM.
go.mod (1)
22-22: Verified: dependency correctly added and integrated.github.com/samber/lo is under MIT License. The single usage at
internal/sms-gateway/handlers/health.go:101withlo.MapValues()is confirmed,go mod tidysucceeded, andgo mod verifyreports all modules verified. No issues found.internal/sms-gateway/modules/health/types.go (2)
3-5: Imports: looks good. Nothing to change.
34-48: The review comment is incorrect for this codebase.The repository declares Go 1.24.1 in go.mod (line 3), which is well above Go 1.21. The
max()predeclared function is fully available and supported. The original code usingmax(level, levelFail)andmax(level, levelWarn)will compile without any issues. The suggested refactoring is unnecessary.Likely an incorrect or invalid review comment.
internal/sms-gateway/modules/health/module.go (1)
24-29: AsHealthProvider helper is clean and idiomatic for grouping providers.internal/sms-gateway/app.go (1)
39-39: Switching to Module() constructors aligns with new module APIs. LGTM.Also applies to: 50-50
internal/sms-gateway/modules/db/module.go (2)
7-7: Import alias for health module is correct and avoids collisions.
12-22: DB module wiring looks good. Ensure newHealth returns a type implementing HealthProvider (compile‑time assert recommended:var _ healthmod.HealthProvider = (*yourType)(nil)).internal/sms-gateway/modules/db/health.go (4)
11-15: LGTM! Clean struct design with appropriate concurrency safety.The use of
atomic.Int64forfailedPingsensures thread-safe access when multiple probes run concurrently, and the persistent counter effectively tracks consecutive ping failures across health checks.
17-21: LGTM! Constructor is straightforward and correct.The zero-initialization of the atomic counter is appropriate for tracking failed pings from a clean state.
24-31: LGTM! No-op probes align with Kubernetes semantics.The database provider appropriately implements health checks only for
ReadyProbe, while liveness and startup probes are no-ops. This follows the pattern where database availability affects readiness but not liveness.Also applies to: 56-58
60-60: LGTM! Standard compile-time interface verification.internal/sms-gateway/modules/health/service.go (2)
32-66: LGTM! Solid probe orchestration with appropriate error handling.The implementation correctly:
- Performs non-blocking context cancellation checks between provider iterations (lines 38-42)
- Isolates provider failures—one provider's error doesn't prevent checking others (line 53)
- Namespaces checks with provider names to ensure uniqueness (line 61)
- Returns partial results if the context is cancelled mid-iteration
68-84: LGTM! Clean delegation to probe-specific methods.The three public methods correctly delegate to the generic
checkProviderhelper, each invoking the appropriate probe function from theHealthProviderinterface.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
internal/sms-gateway/handlers/health.go (2)
44-45: Pass context.Context via Fiber’s UserContext, not fasthttp.RequestCtx.c.Context() returns *fasthttp.RequestCtx; health.Service expects context.Context. Use c.UserContext().
- return writeProbe(c, h.healthSvc.CheckLiveness(c.Context())) + return writeProbe(c, h.healthSvc.CheckLiveness(c.UserContext())) @@ - return writeProbe(c, h.healthSvc.CheckReadiness(c.Context())) + return writeProbe(c, h.healthSvc.CheckReadiness(c.UserContext())) @@ - return writeProbe(c, h.healthSvc.CheckStartup(c.Context())) + return writeProbe(c, h.healthSvc.CheckStartup(c.UserContext()))#!/bin/bash # Verify all probe handlers use UserContext() rg -nP --type go -C1 'Check(Liveness|Readiness|Startup)\(\s*c\.(Context|UserContext)\(\)\s*\)' internal/sms-gateway/handlersAlso applies to: 58-59, 71-72
101-106: Swagger documents /3rdparty/v1/health but route isn’t wired.Register the alias to avoid docs–implementation drift.
func (h *healthHandler) Register(router fiber.Router) { router.Get("/health", h.getReadiness) router.Get("/health/live", h.getLiveness) router.Get("/health/ready", h.getReadiness) router.Get("/health/startup", h.getStartup) + router.Get("/3rdparty/v1/health", h.getReadiness) }#!/bin/bash # Confirm swagger alias exists and router wiring matches rg -n '@Router\s+/3rdparty/v1/health\s+\[get\]' internal/sms-gateway/handlers/health.go rg -n 'router\.Get\("/3rdparty/v1/health"' internal/sms-gateway/handlers/health.go
🧹 Nitpick comments (3)
internal/sms-gateway/modules/health/health.go (2)
21-53: Type safety for 1GiB and probe efficiency/configurability.
- Use a typed uint64 for byte comparisons; avoid implicit constant conversions.
- Consider making thresholds configurable; hardcoded 1 GiB / 1000 goroutines will be env‑dependent.
- ReadMemStats is relatively heavy for tight probe intervals; runtime/metrics could be a lighter alternative.
Apply:
-func (h *health) LiveProbe(ctx context.Context) (Checks, error) { - const oneGiB = 1024 * 1024 * 1024 +func (h *health) LiveProbe(ctx context.Context) (Checks, error) { + const oneGiB uint64 = 1 << 30
56-63: Return empty map instead of nil for stable JSON shape.Prevents “checks: null” in responses downstream.
-func (h *health) ReadyProbe(ctx context.Context) (Checks, error) { - return nil, nil +func (h *health) ReadyProbe(ctx context.Context) (Checks, error) { + return Checks{}, nil } @@ -func (h *health) StartedProbe(ctx context.Context) (Checks, error) { - return nil, nil +func (h *health) StartedProbe(ctx context.Context) (Checks, error) { + return Checks{}, nil }Please confirm health.Service treats empty Checks as pass (or as intended) when no providers report readiness/startup signals.
internal/sms-gateway/handlers/health.go (1)
82-99: Ensure Checks isn’t null in JSON.MapValues returns nil when input map is nil; emit {} for stability.
-func makeResponse(result health.CheckResult) smsgateway.HealthResponse { - return smsgateway.HealthResponse{ +func makeResponse(result health.CheckResult) smsgateway.HealthResponse { + checks := lo.MapValues( + result.Checks, + func(value health.CheckDetail, key string) smsgateway.HealthCheck { + return smsgateway.HealthCheck{ + Description: value.Description, + ObservedUnit: value.ObservedUnit, + ObservedValue: value.ObservedValue, + Status: smsgateway.HealthStatus(value.Status), + } + }, + ) + if checks == nil { + checks = map[string]smsgateway.HealthCheck{} + } + return smsgateway.HealthResponse{ Status: smsgateway.HealthStatus(result.Status()), Version: version.AppVersion, ReleaseID: version.AppReleaseID(), - Checks: lo.MapValues( - result.Checks, - func(value health.CheckDetail, key string) smsgateway.HealthCheck { - return smsgateway.HealthCheck{ - Description: value.Description, - ObservedUnit: value.ObservedUnit, - ObservedValue: value.ObservedValue, - Status: smsgateway.HealthStatus(value.Status), - } - }, - ), + Checks: checks, } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/sms-gateway/handlers/health.go(1 hunks)internal/sms-gateway/modules/health/health.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/sms-gateway/modules/health/health.go (1)
internal/sms-gateway/modules/health/types.go (6)
Checks(64-64)CheckDetail(51-61)Status(7-7)StatusPass(11-11)StatusWarn(12-12)HealthProvider(66-72)
internal/sms-gateway/handlers/health.go (3)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/modules/health/types.go (5)
CheckResult(27-30)Status(7-7)StatusFail(13-13)Checks(64-64)CheckDetail(51-61)internal/version/version.go (1)
AppReleaseID(13-17)
⏰ 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). (4)
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: E2E
- GitHub Check: Analyze (go)
capcom6
force-pushed
the
handlers/improve-health-handler
branch
from
940625b to
b5eedc7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
api/requests.http (1)
198-206: Add local general /health for parity with 3rd‑party section.Include the readiness alias locally so tests cover both forms. This mirrors /3rdparty/v1/health and prior feedback.
### -GET http://localhost:3000/health/startup HTTP/1.1 +GET http://localhost:3000/health HTTP/1.1 + +### +GET http://localhost:3000/health/startup HTTP/1.1internal/sms-gateway/modules/health/module.go (1)
8-22: Fix fx.Private misuse and export Service from a private module.
- fx.Private is not a constructor; don’t pass it to fx.Provide.
- If the module is private, explicitly export *Service for external consumers (handlers).
func Module() fx.Option { return fx.Module( "health", fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("health") }), - fx.Provide( - AsHealthProvider(NewHealth), - fx.Private, - ), + fx.Private, + fx.Export((*Service)(nil)), + fx.Provide( + AsHealthProvider(NewHealth), + ), fx.Provide( NewService, ), ) }Run to confirm external usages of *health.Service (should show handlers) and avoid accidental privacy breakage:
#!/bin/bash # Show external references to *health.Service outside the health module rg -nP --type=go -C2 '\*?health\.Service\b' | grep -v 'internal/sms-gateway/modules/health/'internal/sms-gateway/handlers/health.go (2)
44-45: Use Fiber’s request context (UserContext) for service calls.
c.Context()returns *fasthttp.RequestCtx, not context.Context. Passc.UserContext()to the health service.- return writeProbe(c, h.healthSvc.CheckLiveness(c.Context())) + return writeProbe(c, h.healthSvc.CheckLiveness(c.UserContext())) @@ - return writeProbe(c, h.healthSvc.CheckReadiness(c.Context())) + return writeProbe(c, h.healthSvc.CheckReadiness(c.UserContext())) @@ - return writeProbe(c, h.healthSvc.CheckStartup(c.Context())) + return writeProbe(c, h.healthSvc.CheckStartup(c.UserContext()))Also applies to: 58-59, 71-72
101-106: Register the documented alias/3rdparty/v1/health.Swagger documents this path, but it’s not wired, causing a 404. Add the alias mapped to readiness.
func (h *healthHandler) Register(router fiber.Router) { router.Get("/health", h.getReadiness) router.Get("/health/live", h.getLiveness) router.Get("/health/ready", h.getReadiness) router.Get("/health/startup", h.getStartup) + router.Get("/3rdparty/v1/health", h.getReadiness) }
🧹 Nitpick comments (1)
internal/sms-gateway/modules/health/health.go (1)
55-63: Return empty Checks map instead of nil.Avoid nil maps to keep responses consistent and reduce nil-handling across callers.
func (h *health) ReadyProbe(ctx context.Context) (Checks, error) { - return nil, nil + return Checks{}, nil } func (h *health) StartedProbe(ctx context.Context) (Checks, error) { - return nil, nil + return Checks{}, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
api/requests.http(2 hunks)build/package/Dockerfile(1 hunks)deployments/helm-chart/templates/deployment.yaml(1 hunks)go.mod(2 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/health.go(1 hunks)internal/sms-gateway/modules/db/health.go(1 hunks)internal/sms-gateway/modules/db/module.go(1 hunks)internal/sms-gateway/modules/health/cli.go(0 hunks)internal/sms-gateway/modules/health/db.go(0 hunks)internal/sms-gateway/modules/health/health.go(1 hunks)internal/sms-gateway/modules/health/module.go(1 hunks)internal/sms-gateway/modules/health/service.go(1 hunks)internal/sms-gateway/modules/health/types.go(3 hunks)internal/sms-gateway/modules/messages/metrics.go(1 hunks)
💤 Files with no reviewable changes (2)
- internal/sms-gateway/modules/health/cli.go
- internal/sms-gateway/modules/health/db.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/sms-gateway/modules/db/health.go
- internal/sms-gateway/modules/db/module.go
- build/package/Dockerfile
- go.mod
🧰 Additional context used
🧬 Code graph analysis (5)
internal/sms-gateway/app.go (2)
internal/sms-gateway/modules/db/module.go (1)
Module(12-22)internal/sms-gateway/modules/health/module.go (1)
Module(8-22)
internal/sms-gateway/modules/health/health.go (1)
internal/sms-gateway/modules/health/types.go (6)
Checks(64-64)CheckDetail(51-61)Status(7-7)StatusPass(11-11)StatusWarn(12-12)HealthProvider(66-72)
internal/sms-gateway/modules/health/service.go (1)
internal/sms-gateway/modules/health/types.go (6)
HealthProvider(66-72)Checks(64-64)CheckResult(27-30)CheckDetail(51-61)Status(7-7)StatusFail(13-13)
internal/sms-gateway/modules/health/module.go (5)
internal/sms-gateway/app.go (1)
Module(35-58)internal/sms-gateway/modules/db/module.go (1)
Module(12-22)internal/sms-gateway/modules/health/health.go (1)
NewHealth(11-13)internal/sms-gateway/modules/health/service.go (1)
NewService(24-30)internal/sms-gateway/modules/health/types.go (1)
HealthProvider(66-72)
internal/sms-gateway/handlers/health.go (3)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/modules/health/types.go (5)
CheckResult(27-30)Status(7-7)StatusFail(13-13)Checks(64-64)CheckDetail(51-61)internal/version/version.go (2)
AppVersion(9-9)AppReleaseID(13-17)
🔇 Additional comments (6)
internal/sms-gateway/modules/messages/metrics.go (1)
13-14: LGTM!The alignment of these constant declarations improves code readability with no functional impact.
deployments/helm-chart/templates/deployment.yaml (1)
95-119: LGTM on probes; aligned with new health surface.Paths and timings are reasonable; startupProbe gates liveness/readiness correctly.
internal/sms-gateway/app.go (1)
39-39: LGTM on Module() wiring.Switching to constructor-based modules matches the new module APIs.
Also applies to: 50-50
internal/sms-gateway/modules/health/service.go (3)
38-42: Good practice: context cancellation check.The non-blocking context check between provider iterations allows graceful early exit and is appropriate here. Note that callers should use a timeout context if concerned about individual probes blocking indefinitely.
44-63: Well-designed error handling and check accumulation.The error handling correctly logs failures and records them as failed checks while allowing other providers to continue. The check naming pattern (provider:check) is clear and consistent.
32-84: Excellent refactoring: eliminates duplication while maintaining clarity.The
checkProviderhelper method and three probe-specific methods (CheckReadiness,CheckLiveness,CheckStartup) eliminate code duplication through a clean higher-order function pattern. The implementation is type-safe, handles errors gracefully, and supports cooperative cancellation.
capcom6
force-pushed
the
handlers/improve-health-handler
branch
from
d85ad8a to
f9dec80
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
internal/sms-gateway/modules/health/types.go (1)
34-48: Avoid Go 1.21 builtinmax; implement explicit level selection.Using
maxrequires Go 1.21+. The code should use explicit comparisons to ensure compatibility with the project's minimum Go version.Apply this diff to fix:
func (c CheckResult) Status() Status { - // Determine overall status level := levelPass for _, detail := range c.Checks { switch detail.Status { case StatusPass: + // no-op case StatusFail: - level = max(level, levelFail) + level = levelFail case StatusWarn: - level = max(level, levelWarn) + if level == levelPass { + level = levelWarn + } } } - return statusLevels[level] }internal/sms-gateway/modules/health/module.go (2)
14-17: Critical: fx.Private incorrectly passed to fx.Provide.
fx.Provideexpects constructor functions, not options. Passingfx.Privateas an argument tofx.Providewill cause the application to fail at startup.Apply this diff to fix:
func Module() fx.Option { return fx.Module( "health", fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("health") }), + fx.Private, fx.Provide( AsHealthProvider(NewHealth), - fx.Private, ), fx.Provide( NewService, ), ) }
8-22: Inconsistent module visibility: Service consumed externally but module is private.After fixing the
fx.Privateplacement, the module will be fully private, but*health.Serviceis consumed by handlers outside this module. This will cause dependency resolution failure.Choose one of these solutions:
Option 1 (Recommended): Export Service explicitly
func Module() fx.Option { return fx.Module( "health", + fx.Export((*Service)(nil)), fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("health") }), fx.Private, fx.Provide( AsHealthProvider(NewHealth), ), fx.Provide( NewService, ), ) }Option 2: Remove fx.Private entirely
func Module() fx.Option { return fx.Module( "health", fx.Decorate(func(log *zap.Logger) *zap.Logger { return log.Named("health") }), - fx.Private, fx.Provide( AsHealthProvider(NewHealth), ), fx.Provide( NewService, ), ) }internal/sms-gateway/handlers/health.go (4)
44-44: Usec.UserContext()instead ofc.Context()for type correctness.The method
c.Context()returns*fasthttp.RequestCtx, butCheckLivenessexpectscontext.Context. Usec.UserContext()to pass the correct type.Apply this diff:
- return writeProbe(c, h.healthSvc.CheckLiveness(c.Context())) + return writeProbe(c, h.healthSvc.CheckLiveness(c.UserContext()))
58-58: Usec.UserContext()instead ofc.Context()for type correctness.The method
c.Context()returns*fasthttp.RequestCtx, butCheckReadinessexpectscontext.Context. Usec.UserContext()to pass the correct type.Apply this diff:
- return writeProbe(c, h.healthSvc.CheckReadiness(c.Context())) + return writeProbe(c, h.healthSvc.CheckReadiness(c.UserContext()))
71-71: Usec.UserContext()instead ofc.Context()for type correctness.The method
c.Context()returns*fasthttp.RequestCtx, butCheckStartupexpectscontext.Context. Usec.UserContext()to pass the correct type.Apply this diff:
- return writeProbe(c, h.healthSvc.CheckStartup(c.Context())) + return writeProbe(c, h.healthSvc.CheckStartup(c.UserContext()))
101-106: Register the/3rdparty/v1/healthroute documented in swagger.The swagger annotation at line 54 documents
/3rdparty/v1/healthas an alias for the readiness check, but this route is not registered in theRegisterfunction, causing documentation-implementation drift.Apply this diff to add the missing route:
func (h *healthHandler) Register(router fiber.Router) { router.Get("/health", h.getReadiness) router.Get("/health/live", h.getLiveness) router.Get("/health/ready", h.getReadiness) router.Get("/health/startup", h.getStartup) + router.Get("/3rdparty/v1/health", h.getReadiness) }
🧹 Nitpick comments (1)
internal/sms-gateway/modules/db/health.go (1)
34-53: Consider removing redundant ObservedValue initialization.The logic is correct, but
ObservedValue: 0on line 38 is overwritten on line 50 regardless of the ping result.Apply this diff to simplify:
func (h *health) ReadyProbe(ctx context.Context) (healthmod.Checks, error) { pingCheck := healthmod.CheckDetail{ Description: "Database ping", ObservedUnit: "failed pings", - ObservedValue: 0, Status: healthmod.StatusPass, } if err := h.db.PingContext(ctx); err != nil { h.failedPings.Add(1) pingCheck.Status = healthmod.StatusFail } else { h.failedPings.Store(0) } pingCheck.ObservedValue = int(h.failedPings.Load()) return healthmod.Checks{"ping": pingCheck}, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (16)
api/requests.http(2 hunks)build/package/Dockerfile(1 hunks)deployments/helm-chart/templates/deployment.yaml(1 hunks)deployments/helm-chart/values.yaml(1 hunks)go.mod(1 hunks)internal/sms-gateway/app.go(2 hunks)internal/sms-gateway/handlers/health.go(1 hunks)internal/sms-gateway/modules/db/health.go(1 hunks)internal/sms-gateway/modules/db/module.go(1 hunks)internal/sms-gateway/modules/health/cli.go(0 hunks)internal/sms-gateway/modules/health/db.go(0 hunks)internal/sms-gateway/modules/health/health.go(1 hunks)internal/sms-gateway/modules/health/module.go(1 hunks)internal/sms-gateway/modules/health/service.go(1 hunks)internal/sms-gateway/modules/health/types.go(3 hunks)internal/sms-gateway/modules/messages/metrics.go(1 hunks)
💤 Files with no reviewable changes (2)
- internal/sms-gateway/modules/health/cli.go
- internal/sms-gateway/modules/health/db.go
🚧 Files skipped from review as they are similar to previous changes (5)
- deployments/helm-chart/values.yaml
- internal/sms-gateway/modules/messages/metrics.go
- go.mod
- internal/sms-gateway/modules/health/service.go
- api/requests.http
🧰 Additional context used
🧬 Code graph analysis (6)
internal/sms-gateway/app.go (2)
internal/sms-gateway/modules/db/module.go (1)
Module(12-22)internal/sms-gateway/modules/health/module.go (1)
Module(8-22)
internal/sms-gateway/modules/health/module.go (5)
internal/sms-gateway/app.go (1)
Module(36-60)internal/sms-gateway/modules/db/module.go (1)
Module(12-22)internal/sms-gateway/modules/health/health.go (1)
NewHealth(11-13)internal/sms-gateway/modules/health/service.go (1)
NewService(24-30)internal/sms-gateway/modules/health/types.go (1)
HealthProvider(66-72)
internal/sms-gateway/modules/health/health.go (1)
internal/sms-gateway/modules/health/types.go (6)
Checks(64-64)CheckDetail(51-61)Status(7-7)StatusPass(11-11)StatusWarn(12-12)HealthProvider(66-72)
internal/sms-gateway/handlers/health.go (4)
internal/sms-gateway/handlers/base/handler.go (1)
Handler(15-18)internal/sms-gateway/modules/health/service.go (1)
Service(18-22)internal/sms-gateway/modules/health/types.go (5)
CheckResult(27-30)Status(7-7)StatusFail(13-13)Checks(64-64)CheckDetail(51-61)internal/version/version.go (2)
AppVersion(9-9)AppReleaseID(13-17)
internal/sms-gateway/modules/db/health.go (1)
internal/sms-gateway/modules/health/types.go (6)
Checks(64-64)CheckDetail(51-61)Status(7-7)StatusPass(11-11)StatusFail(13-13)HealthProvider(66-72)
internal/sms-gateway/modules/db/module.go (2)
internal/sms-gateway/app.go (1)
Module(36-60)internal/sms-gateway/modules/health/module.go (2)
Module(8-22)AsHealthProvider(24-30)
⏰ 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). (5)
- GitHub Check: E2E
- GitHub Check: Benchmark
- GitHub Check: Test
- GitHub Check: Lint
- GitHub Check: Analyze (go)
🔇 Additional comments (13)
build/package/Dockerfile (1)
50-51: LGTM! Docker healthcheck correctly updated to liveness probe.The HEALTHCHECK correctly targets the new
/health/liveendpoint, which is appropriate for Docker's container health monitoring.deployments/helm-chart/templates/deployment.yaml (1)
94-119: LGTM! Kubernetes probes properly configured for all three health checks.The probe configuration correctly implements the three-probe pattern:
- Liveness probe (
/health/live): Checks if the app is alive and should be restarted if failing- Readiness probe (
/health/ready): Checks if the app can accept traffic (e.g., DB connected)- Startup probe (
/health/startup): Allows up to 120 seconds for initial startup before other probes beginThe timing values are appropriate for a backend service.
internal/sms-gateway/modules/db/health.go (4)
17-21: LGTM! Clean constructor with proper initialization.
24-26: LGTM! Provider name is clear and appropriate.
29-31: LGTM! DB health correctly excluded from liveness probe.The application should remain "alive" even if the database is temporarily unavailable. DB health affects readiness, not liveness.
56-58: LGTM! DB health correctly excluded from startup probe.internal/sms-gateway/modules/health/types.go (1)
66-72: LGTM! HealthProvider interface properly separated into three probe semantics.The interface correctly distinguishes between startup, readiness, and liveness concerns, aligning with Kubernetes probe patterns.
internal/sms-gateway/app.go (1)
36-60: LGTM! Module wiring correctly updated to use constructor pattern.The refactoring from package-level module variables (
appdb.Module,health.Module) to constructor functions (appdb.Module(),health.Module()) enables more flexible dependency injection and module initialization.internal/sms-gateway/modules/db/module.go (1)
12-22: LGTM! DB module correctly refactored with health provider integration.The module now properly exposes both the health provider and ID generator, with correct use of
AsHealthProviderfor health provider annotation.internal/sms-gateway/modules/health/health.go (3)
11-13: LGTM! Clean constructor.
21-53: LGTM! Runtime health checks are appropriate for liveness monitoring.The memory and goroutine checks correctly trigger warnings (not failures) at reasonable thresholds:
- Memory > 1 GiB: allows detecting memory issues without killing healthy pods
- Goroutines > 1000: helps identify goroutine leaks
The implementation correctly uses
StatusWarnto enable alerting while keeping the pod running.
56-63: LGTM! System health correctly limited to liveness probe only.Runtime metrics (memory, goroutines) are appropriate for liveness checks but don't affect startup or readiness, which is correct.
internal/sms-gateway/modules/health/module.go (1)
24-30: LGTM! AsHealthProvider helper is well-designed for provider registration.The function correctly uses
fx.Annotateto implement theHealthProviderinterface and group providers for collection by the health service.
Summary by CodeRabbit
New Features
Chores