Service uptime - #203
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds configurable downstream service health monitoring with asynchronous probes, Postgres persistence, lifecycle management, health-history APIs, new response schemas, shared managers, updated Postgres settings, tests, documentation, and removal of the Kong Docker configuration. ChangesService health monitoring
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant FastAPI
participant ServiceHealthMonitor
participant DownstreamServices
participant Postgres
FastAPI->>ServiceHealthMonitor: start monitoring
ServiceHealthMonitor->>DownstreamServices: probe configured health endpoints
DownstreamServices-->>ServiceHealthMonitor: return status and latency
ServiceHealthMonitor->>Postgres: persist sweep results
FastAPI->>ServiceHealthMonitor: stop monitoring
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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: 4
🧹 Nitpick comments (5)
hub_adapter/service_health.py (3)
175-180: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
create_tablesruns on every bind, including read paths.Every
summarize_range,fetch_last_checks,fetch_checksandrecord_sweepcall issuesCREATE TABLE IF NOT EXISTSplus index DDL._connectalready ensures the table exists at startup, so this is redundant round-trips on each request/sweep (and DDL requires broader grants than reads).Consider making creation explicit and idempotent-once:
♻️ Proposed refactor
`@contextmanager` -def bind_service_health(db: pw.Database): - """Bind the health check model to a database, creating the table if it does not exist yet.""" - with db.bind_ctx((ServiceHealthCheck,)): - db.create_tables((ServiceHealthCheck,)) - yield +def bind_service_health(db: pw.Database, create: bool = False): + """Bind the health check model to a database, optionally creating the table.""" + with db.bind_ctx((ServiceHealthCheck,)): + if create: + db.create_tables((ServiceHealthCheck,)) + yieldand pass
create=Trueonly from_connect(Line 363).🤖 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 `@hub_adapter/service_health.py` around lines 175 - 180, Update bind_service_health so it only binds ServiceHealthCheck and no longer calls create_tables on every context entry. Move table creation to the _connect initialization path by invoking bind_service_health with create=True there, and support the create flag so schema creation remains explicit and occurs once at startup.
428-430: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBlocking peewee calls run on the event loop.
record_sweepandprune_old_checksare synchronous Postgres round trips executed directly inside the async sweep, so they block request handling — the periodic pruneDELETEover the retention window is the risky one. Wrapping them inasyncio.to_threadkeeps the loop responsive.♻️ Proposed refactor
- record_sweep(self._db, results) - self._prune_if_due() + await asyncio.to_thread(record_sweep, self._db, results) + await asyncio.to_thread(self._prune_if_due)🤖 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 `@hub_adapter/service_health.py` around lines 428 - 430, Update the async sweep around record_sweep and _prune_if_due to run their synchronous database work via asyncio.to_thread, ensuring both the sweep insert and periodic prune_old_checks DELETE execute off the event loop while preserving their existing order and behavior.
385-396: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueConsider retrying the connection on an explicit restart.
_connection_attemptedis latched forever, so once Postgres is unavailable at startup, a laterPATCH /node/settings(which callsstart()again) still cannot enable monitoring — only a process restart can. Allowing the reconnect on an explicit restart would make recovery operator-friendly.🤖 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 `@hub_adapter/service_health.py` around lines 385 - 396, The start() connection flow currently prevents recovery after an initial failure because _connection_attempted remains latched; reset that state on an explicit restart such as PATCH /node/settings before invoking start(), so start() retries _connect() and can enable monitoring when Postgres becomes available, while preserving the existing one-attempt behavior during normal operation.tests/test_service_health.py (1)
225-288: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAggregation helpers are never exercised.
summarize_range,fetch_last_checks,fetch_checks,record_sweepandprune_old_checksare mocked in every test, so the uptime math and theDISTINCT ONordering infetch_last_checksare untested. Note SQLite cannot stand in here (DISTINCT ONis Postgres-only), so this likely needs a marked integration test. Want me to draft one?🤖 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 `@tests/test_service_health.py` around lines 225 - 288, Add a marked PostgreSQL integration test covering the real aggregation helpers summarize_range, fetch_last_checks, fetch_checks, record_sweep, and prune_old_checks instead of mocking them. Seed representative checks, execute the helpers against the test database, and assert uptime calculations plus fetch_last_checks selecting the newest row per service according to its DISTINCT ON ordering; keep unit mocks only for sweep orchestration behavior.tests/router_tests/test_health.py (1)
81-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
ClassVarto satisfy Ruff RUF012.🔧 Proposed fix
+from typing import ClassVar + class TestOpenApiStatusEnums: ... - EXPECTED_ENUMS = { + EXPECTED_ENUMS: ClassVar[dict[str, list[str]]] = { "HealthStatus": ["OK", "WARNING", "ERROR", "CRITICAL"], "ServiceCheckStatus": ["OK", "ERROR"], "ServiceMonitoringStatus": ["ACTIVE", "DISABLED"], } - EXPECTED_REFS = { + EXPECTED_REFS: ClassVar[dict[tuple[str, str], str]] = { ("HealthCheck", "status"): "HealthStatus", ("ServiceHealthPoint", "status"): "ServiceCheckStatus", ("ServiceHealthSummary", "status"): "ServiceMonitoringStatus", }🤖 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 `@tests/router_tests/test_health.py` around lines 81 - 91, Annotate the mutable class-level constants EXPECTED_ENUMS and EXPECTED_REFS with ClassVar to satisfy Ruff RUF012, preserving their existing values and structure.Source: Linters/SAST tools
🤖 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 `@hub_adapter/routers/health.py`:
- Around line 165-181: Update async get_health_downstream_services_history so
the synchronous Peewee calls summarize_range, fetch_last_checks, and conditional
fetch_checks execute via the project’s thread-offload mechanism rather than
directly on the event loop. Preserve their arguments, include_checks behavior,
result assignments, and existing PeeweeException-to-503 handling.
In `@hub_adapter/service_health.py`:
- Around line 85-99: Update probe_service’s request error handling to catch all
httpx2.HTTPError failures, including httpx2.InvalidURL, and convert them into
the existing 503 ERROR result with an informative message and log entry. Ensure
probe_all’s asyncio.gather preserves the full sweep by using
return_exceptions=True if non-httpx2 exceptions can still escape.
In `@README.md`:
- Around line 140-159: Update the “Service health monitoring” section in
README.md to hyphenate “per service” as “per-service” in the description of
uptime and latency statistics. Leave the surrounding documentation unchanged.
In `@tests/test_service_health.py`:
- Around line 312-321: Update test_reports_that_monitoring_is_disabled to patch
the endpoint’s imported binding,
hub_adapter.routers.health.service_health_monitor, rather than
hub_adapter.managers.service_health_monitor, so the route uses the mocked
monitor. Preserve the existing response assertions.
---
Nitpick comments:
In `@hub_adapter/service_health.py`:
- Around line 175-180: Update bind_service_health so it only binds
ServiceHealthCheck and no longer calls create_tables on every context entry.
Move table creation to the _connect initialization path by invoking
bind_service_health with create=True there, and support the create flag so
schema creation remains explicit and occurs once at startup.
- Around line 428-430: Update the async sweep around record_sweep and
_prune_if_due to run their synchronous database work via asyncio.to_thread,
ensuring both the sweep insert and periodic prune_old_checks DELETE execute off
the event loop while preserving their existing order and behavior.
- Around line 385-396: The start() connection flow currently prevents recovery
after an initial failure because _connection_attempted remains latched; reset
that state on an explicit restart such as PATCH /node/settings before invoking
start(), so start() retries _connect() and can enable monitoring when Postgres
becomes available, while preserving the existing one-attempt behavior during
normal operation.
In `@tests/router_tests/test_health.py`:
- Around line 81-91: Annotate the mutable class-level constants EXPECTED_ENUMS
and EXPECTED_REFS with ClassVar to satisfy Ruff RUF012, preserving their
existing values and structure.
In `@tests/test_service_health.py`:
- Around line 225-288: Add a marked PostgreSQL integration test covering the
real aggregation helpers summarize_range, fetch_last_checks, fetch_checks,
record_sweep, and prune_old_checks instead of mocking them. Seed representative
checks, execute the helpers against the test database, and assert uptime
calculations plus fetch_last_checks selecting the newest row per service
according to its DISTINCT ON ordering; keep unit mocks only for sweep
orchestration behavior.
🪄 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 Plus
Run ID: 341b3cdf-7bb1-4b1b-99bd-72432040715b
📒 Files selected for processing (18)
README.mddocker/kong/config/kong.yamldocker/kong/docker-compose.ymldocker/kong/kong.txtdocker/kong/test-realm.jsonhub_adapter/conf.pyhub_adapter/database.pyhub_adapter/managers.pyhub_adapter/routers/health.pyhub_adapter/routers/node.pyhub_adapter/schemas/health.pyhub_adapter/schemas/logs.pyhub_adapter/server.pyhub_adapter/service_health.pytests/router_tests/test_health.pytests/router_tests/test_node.pytests/test_managers.pytests/test_service_health.py
💤 Files with no reviewable changes (3)
- docker/kong/config/kong.yaml
- docker/kong/docker-compose.yml
- docker/kong/kong.txt
1 participant
Summary by CodeRabbit
New Features
Documentation
Bug Fixes