feat(toolkit): add REST healthcheck infrastructure#4167
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
💤 Files with no reviewable changes (7)
📝 WalkthroughWalkthroughAdds a shared readiness healthcheck registry in ChangesREST Readiness Healthcheck
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🧹 Nitpick comments (4)
libs/toolkit/src/contracts.rs (1)
72-75: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit: TODO is doubly-commented inside a doc comment.
Lines 74-75 use
///+//, so the rendered rustdoc shows literal// TODO: .... Drop the inner//for clean rendering. The gRPC hook is also tracked here as a follow-up — I can open an issue to track it if helpful.Proposed doc tweak
/// # gRPC healthcheck /// - /// // TODO: Add gRPC healthcheck hook in a follow-up PR. - /// // This PR intentionally implements REST readiness healthchecks only. + /// TODO: Add gRPC healthcheck hook in a follow-up PR. + /// This PR intentionally implements REST readiness healthchecks only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/toolkit/src/contracts.rs` around lines 72 - 75, The rustdoc on the gRPC healthcheck section is doubly commented, so the rendered docs show literal comment markers instead of clean text. Update the doc comments in the contracts.rs healthcheck block to remove the inner “//” while keeping the note about the gRPC healthcheck hook and the REST-only scope; use the existing healthcheck-related doc section and surrounding contract comments to locate it.libs/toolkit/src/healthcheck.rs (2)
77-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffOptional:
HealthcheckStatusandHealthcheckAggregateStatusare structurally identical.Two enums with the same three variants and serde config add maintenance overhead (any new variant must be mirrored, plus the manual mapping in
compute_aggregate). If the aggregate truly can never diverge from the per-check status set, consider a single type with an alias; if divergence is anticipated (e.g., a future aggregate-onlyInitializing), keeping them separate is justified — a short comment noting the rationale would help.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/toolkit/src/healthcheck.rs` around lines 77 - 93, The two status enums, HealthcheckStatus and HealthcheckAggregateStatus, are identical and currently require duplicated serde settings plus manual mirroring in compute_aggregate. Either consolidate them behind a single shared type/alias if the aggregate will always use the same variants, or keep both enums only if future divergence is intended; in that case, add a short rationale comment near the enum definitions and ensure compute_aggregate clearly uses the intended type mapping.
320-353: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: broad substring blocklist can mask useful diagnostics.
Entries like
"tenant","select ","update ", and"delete "will redact otherwise-safe operator messages (e.g.,"tenant pool exhausted"or"update queue backlog") to a generic"health check failed", reducing debuggability on/health. This is reasonable defense-in-depth for a public endpoint, but worth a brief comment documenting the intentional trade-off so future maintainers don't treat redacted messages as a bug.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/toolkit/src/healthcheck.rs` around lines 320 - 353, The broad blocklist in sanitize_message is intentionally redacting some otherwise-safe operator diagnostics, so add a brief comment near SUSPICIOUS_SUBSTRINGS or sanitize_message explaining that this is a deliberate defense-in-depth tradeoff for the public health endpoint. Keep the note focused on the intent behind entries like tenant, select , update , and delete , so future maintainers understand why messages may collapse to "health check failed" and do not treat it as an accidental bug.gears/system/api-gateway/src/web.rs (1)
40-57: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftConsider caching/throttling readiness results.
/healthand/readyzare public routes that run every registered check (each spawning tasks, potentially pinging DB/upstreams) on every request with no caching. Under frequent probing or hostile traffic this can amplify load on downstream dependencies. Consider a short-lived cached report (e.g., a few hundred ms TTL) so bursts of probes reuse one result. Worth verifying whether these root-mounted health routes sit behind any rate limiting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gears/system/api-gateway/src/web.rs` around lines 40 - 57, The health endpoints in health_detail and readyz_check currently call registry.report(HEALTHCHECK_TIMEOUT) on every request, which can repeatedly fan out expensive checks. Add a short-lived cached/throttled report layer inside the RestHealthcheckRegistry or a helper used by both handlers so bursty /health and /readyz probes reuse a recent result instead of spawning new checks each time. Keep the existing status_for_report and response shaping logic, but have both health_detail and readyz_check read from the shared cached report path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@gears/system/api-gateway/src/gear.rs`:
- Around line 105-138: The root-mounted health routes created in
apply_prefix_nesting/health_routes are being merged in a way that bypasses the
middleware stack applied to the prefixed router. Update apply_prefix_nesting so
the merged Router that serves /health, /healthz, and /readyz also goes through
the same apply_middleware_stack path as the rest of the API, using the existing
health_routes and apply_middleware_stack symbols to keep request IDs, tracing,
metrics, and CatchPanicLayer consistent.
In `@gears/system/api-gateway/src/web.rs`:
- Around line 51-54: Update the documentation comment for the `readyz` endpoint
in `web.rs` so it no longer calls `/readyz` “Shallow” or “all registered REST
healthchecks”; the implementation uses `registry.report(...)`, which is the deep
readiness probe. Align the wording with the module-level terminology in
`healthcheck.rs` by describing `/readyz` as readiness/deep and reserving
“shallow” or liveness language for `/healthz`, so operators are not misled when
wiring probes.
---
Nitpick comments:
In `@gears/system/api-gateway/src/web.rs`:
- Around line 40-57: The health endpoints in health_detail and readyz_check
currently call registry.report(HEALTHCHECK_TIMEOUT) on every request, which can
repeatedly fan out expensive checks. Add a short-lived cached/throttled report
layer inside the RestHealthcheckRegistry or a helper used by both handlers so
bursty /health and /readyz probes reuse a recent result instead of spawning new
checks each time. Keep the existing status_for_report and response shaping
logic, but have both health_detail and readyz_check read from the shared cached
report path.
In `@libs/toolkit/src/contracts.rs`:
- Around line 72-75: The rustdoc on the gRPC healthcheck section is doubly
commented, so the rendered docs show literal comment markers instead of clean
text. Update the doc comments in the contracts.rs healthcheck block to remove
the inner “//” while keeping the note about the gRPC healthcheck hook and the
REST-only scope; use the existing healthcheck-related doc section and
surrounding contract comments to locate it.
In `@libs/toolkit/src/healthcheck.rs`:
- Around line 77-93: The two status enums, HealthcheckStatus and
HealthcheckAggregateStatus, are identical and currently require duplicated serde
settings plus manual mirroring in compute_aggregate. Either consolidate them
behind a single shared type/alias if the aggregate will always use the same
variants, or keep both enums only if future divergence is intended; in that
case, add a short rationale comment near the enum definitions and ensure
compute_aggregate clearly uses the intended type mapping.
- Around line 320-353: The broad blocklist in sanitize_message is intentionally
redacting some otherwise-safe operator diagnostics, so add a brief comment near
SUSPICIOUS_SUBSTRINGS or sanitize_message explaining that this is a deliberate
defense-in-depth tradeoff for the public health endpoint. Keep the note focused
on the intent behind entries like tenant, select , update , and delete , so
future maintainers understand why messages may collapse to "health check failed"
and do not treat it as an accidental bug.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6107aa71-8c94-4412-a51c-37e23dd44ec2
📒 Files selected for processing (7)
gears/system/api-gateway/src/gear.rsgears/system/api-gateway/src/web.rsgears/system/api-gateway/tests/health_endpoints.rslibs/toolkit/src/contracts.rslibs/toolkit/src/healthcheck.rslibs/toolkit/src/lib.rslibs/toolkit/src/runtime/host_runtime.rs
refur-nfn
changed the title
refur-nfn
force-pushed
the
add-rest-healthcheck
branch
3 times, most recently
from
838d355 to
36b1e50
Compare
- Concurrent healthcheck execution with timeout protection - HealthcheckRegistry for registering and running per-gear probes - Message sanitization to prevent secret leaks in responses - API Gateway integration for /health and /readyz endpoints - Comprehensive test coverage with panic/timeout/degradation scenarios Signed-off-by: Raudur Refur <261796072+refur-nfn@users.noreply.github.com>
refur-nfn
force-pushed
the
add-rest-healthcheck
branch
from
36b1e50 to
5254ada
Compare
1 participant
Summary by CodeRabbit
/health,/healthz, and/readyzendpoints with JSON status, RFC3339 timestamp, and per-component details.200vs503.