Health Probes: For llm_call endpoints - #1021
Conversation
📝 WalkthroughWalkthroughAdds configurable text, TTS, and STT health probes with concurrent execution, Sentry check-ins, a traced Celery task, and a hidden superuser-only cron endpoint registered with the cron invoker. ChangesHealth probe execution
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
OpenAPI changes ⚪ No API surface changesNote This PR does not modify the API contract.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
backend/app/services/health_probes.py (1)
211-220: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a uniqueness assertion to the
__main__self-check.The
__main__block validates modalities and config building but doesn't check for duplicate probes. Addingassert len(_PROBES) == len(set(_PROBES))would have caught the duplicate at lines 40-41.♻️ Proposed addition
if __name__ == "__main__": assert _PROBES, "probe list must not be empty" + assert len(_PROBES) == len(set(_PROBES)), "duplicate probe entries detected" modalities = {p.modality for p in _PROBES}🤖 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 `@backend/app/services/health_probes.py` around lines 211 - 220, Add a duplicate-probe validation to the __main__ self-check in health_probes.py: after asserting _PROBES is nonempty, assert len(_PROBES) == len(set(_PROBES)) before validating modalities and config construction.backend/app/tests/services/test_health_probes.py (1)
131-138: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the obscure generator-throw lambda with a named function for consistency.
lambda **_k: (_ for _ in ()).throw(RuntimeError("bad"))is unreadable. TheValueErrortest above (line 117) uses a clear named function_boom— follow the same pattern here.♻️ Proposed fix
- monkeypatch.setattr( - health_probes, - "get_llm_provider", - lambda **_k: (_ for _ in ()).throw(RuntimeError("bad")), - ) + def _boom(**_kwargs): + raise RuntimeError("bad") + + monkeypatch.setattr(health_probes, "get_llm_provider", _boom)🤖 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 `@backend/app/tests/services/test_health_probes.py` around lines 131 - 138, Replace the generator-throw lambda in test_get_llm_provider_runtime_error_marks_client_init_failed with a local named function, such as _boom, that accepts arbitrary keyword arguments and raises RuntimeError("bad"), then monkeypatch get_llm_provider with that function, matching the pattern used by the ValueError test.backend/app/celery/tasks/job_execution.py (1)
411-423: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using
dict[str, Any]instead of baredictfor return types.The upstream
run_probesreturnsdict[str, Any]. Using-> dict[str, Any]on bothrun_health_probesand the nested_doclosure would match that contract and improve type precision.♻️ Proposed type hint refinement
-def run_health_probes(self, trace_id: str = DEFAULT_TRACE_ID) -> dict: +def run_health_probes(self, trace_id: str = DEFAULT_TRACE_ID) -> dict[str, Any]: from sqlmodel import Session from app.core.db import engine from app.services.health_probes import run_probes _set_trace(trace_id) - def _do() -> dict: + def _do() -> dict[str, Any]: with Session(engine) as session: return run_probes(session=session) return _run_with_otel_parent(self, _do)🤖 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 `@backend/app/celery/tasks/job_execution.py` around lines 411 - 423, Update the return annotations of both `run_health_probes` and its nested `_do` closure from bare `dict` to `dict[str, Any]`, importing `Any` as needed, so they match the `run_probes` return contract.backend/app/api/routes/cron.py (2)
35-47: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared monitor config values into constants.
checkin_margin,failure_issue_threshold, andrecovery_thresholdare duplicated betweenEVALUATION_CRON_MONITOR_CONFIGand the newHEALTH_PROBES_CRON_MONITOR_CONFIGwith identical values. As per coding guidelines, repeated literals should be extracted into constants.♻️ Proposed refactor
+CRON_CHECKIN_MARGIN = 2 +CRON_FAILURE_ISSUE_THRESHOLD = 2 +CRON_RECOVERY_THRESHOLD = 1 + EVALUATION_CRON_MONITOR_CONFIG: MonitorConfig = { "schedule": { "type": "interval", "value": settings.CRON_INTERVAL_MINUTES, "unit": "minute", }, "timezone": "UTC", - "checkin_margin": 2, + "checkin_margin": CRON_CHECKIN_MARGIN, "max_runtime": 2 * settings.CRON_INTERVAL_MINUTES, - "failure_issue_threshold": 2, - "recovery_threshold": 1, + "failure_issue_threshold": CRON_FAILURE_ISSUE_THRESHOLD, + "recovery_threshold": CRON_RECOVERY_THRESHOLD, } HEALTH_PROBES_CRON_MONITOR_CONFIG: MonitorConfig = { "schedule": { "type": "interval", "value": settings.HEALTH_PROBE_INTERVAL_MINUTES, "unit": "minute", }, "timezone": "UTC", - "checkin_margin": 2, + "checkin_margin": CRON_CHECKIN_MARGIN, "max_runtime": 2 * settings.HEALTH_PROBE_INTERVAL_MINUTES, - "failure_issue_threshold": 2, - "recovery_threshold": 1, + "failure_issue_threshold": CRON_FAILURE_ISSUE_THRESHOLD, + "recovery_threshold": CRON_RECOVERY_THRESHOLD, }🤖 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 `@backend/app/api/routes/cron.py` around lines 35 - 47, Extract the duplicated monitor settings into shared constants and use them in both EVALUATION_CRON_MONITOR_CONFIG and HEALTH_PROBES_CRON_MONITOR_CONFIG. Replace the repeated checkin_margin, failure_issue_threshold, and recovery_threshold literals with the new constants while preserving their current values.Source: Coding guidelines
148-151: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd error handling for consistency with other cron endpoints.
evaluation_cron_jobandpending_jobs_cron_jobboth wrap their logic in try/except, logging the error and capturing it in Sentry before re-raising.health_probes_cron_jobhas no such guard — ifrun_health_probes.delay()fails (e.g., broker unavailable), the exception propagates without structured logging or explicit Sentry capture.🛡️ Proposed error handling
def health_probes_cron_job() -> dict: logger.info("[health_probes_cron_job] Cron job invoked — enqueueing task") - async_result = run_health_probes.delay() - return {"enqueued": True, "task_id": async_result.id} + try: + async_result = run_health_probes.delay() + return {"enqueued": True, "task_id": async_result.id} + except Exception as e: + logger.error( + f"[health_probes_cron_job] Error enqueueing health probes: {e}", + exc_info=True, + ) + sentry_sdk.capture_exception(e) + raise🤖 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 `@backend/app/api/routes/cron.py` around lines 148 - 151, Wrap the logic in health_probes_cron_job with a try/except matching evaluation_cron_job and pending_jobs_cron_job: log the exception with context, capture it in Sentry, then re-raise it. Keep the existing enqueue and response behavior inside the try block.
🤖 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 `@backend/app/services/health_probes.py`:
- Around line 40-41: Remove the duplicate Probe entry for provider "google",
model "gemini-2.5-flash", and modality "text" from the probe configuration, or
replace it with the intended distinct model while preserving the existing Probe
structure.
- Around line 134-147: Wrap the _build_config_and_input(probe) call in a
dedicated try-except that catches validation and decoding errors, records an
appropriate per-probe error in result, and returns it without propagating to
ThreadPoolExecutor.map. Preserve the existing stt_audio_not_configured handling
for a None result, while keeping provider.execute’s existing exception handling
separate.
In `@backend/app/tests/celery/test_run_health_probes.py`:
- Line 12: Add the missing None return annotations to
_NonClosingSession.__init__ and both test functions, preserving their existing
parameters and behavior.
---
Nitpick comments:
In `@backend/app/api/routes/cron.py`:
- Around line 35-47: Extract the duplicated monitor settings into shared
constants and use them in both EVALUATION_CRON_MONITOR_CONFIG and
HEALTH_PROBES_CRON_MONITOR_CONFIG. Replace the repeated checkin_margin,
failure_issue_threshold, and recovery_threshold literals with the new constants
while preserving their current values.
- Around line 148-151: Wrap the logic in health_probes_cron_job with a
try/except matching evaluation_cron_job and pending_jobs_cron_job: log the
exception with context, capture it in Sentry, then re-raise it. Keep the
existing enqueue and response behavior inside the try block.
In `@backend/app/celery/tasks/job_execution.py`:
- Around line 411-423: Update the return annotations of both `run_health_probes`
and its nested `_do` closure from bare `dict` to `dict[str, Any]`, importing
`Any` as needed, so they match the `run_probes` return contract.
In `@backend/app/services/health_probes.py`:
- Around line 211-220: Add a duplicate-probe validation to the __main__
self-check in health_probes.py: after asserting _PROBES is nonempty, assert
len(_PROBES) == len(set(_PROBES)) before validating modalities and config
construction.
In `@backend/app/tests/services/test_health_probes.py`:
- Around line 131-138: Replace the generator-throw lambda in
test_get_llm_provider_runtime_error_marks_client_init_failed with a local named
function, such as _boom, that accepts arbitrary keyword arguments and raises
RuntimeError("bad"), then monkeypatch get_llm_provider with that function,
matching the pattern used by the ValueError test.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4862ba33-39e8-4bae-b167-cbf6f204e8c9
📒 Files selected for processing (9)
backend/app/api/routes/cron.pybackend/app/celery/tasks/job_execution.pybackend/app/core/config.pybackend/app/core/telemetry.pybackend/app/services/health_probes.pybackend/app/tests/api/routes/test_cron_health_probes.pybackend/app/tests/celery/test_run_health_probes.pybackend/app/tests/services/test_health_probes.pyscripts/python/invoke-cron.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…bes' into feat/api-health-probes
There was a problem hiding this comment.
I have approved the PR so it doesn't block the merge, but I added comments that should be addressed before this goes in:
Database sessions should never be opened at the task level. They should be short-lived and opened only within the child functions that perform the database operations.
| HEALTH_PROBES_CRON_MONITOR_CONFIG: MonitorConfig = { | ||
| "schedule": { | ||
| "type": "interval", | ||
| # not required eventbridge is aleady 5 mins. So not required. |
| _set_trace(trace_id) | ||
|
|
||
| def _do() -> dict: | ||
| with Session(engine) as session: |
There was a problem hiding this comment.
Opening a DB session at the task level isn't a good approach because it keeps the connection open for the entire task duration. Sessions should be short-lived. Instead, open and close the session within the child functions where the database operations actually happen.
| _PROBE_MAX_TOKENS = 1 | ||
| _PROBE_WORKERS = 4 | ||
|
|
||
| # "Hello" ~1sec |
There was a problem hiding this comment.
Instead of inlining the Base64 payload in the source file, consider storing it in a separate file and reading it at runtime. This keeps the source code clean, improves maintainability, and avoids checking large static blobs into the code.
like this u can do
with open("tests/assets/hello.wav", "rb") as f:
audio_b64 = base64.b64encode(f.read()).decode()
|
|
||
| _PROBES: list[Probe] = [ | ||
| # Text | ||
| Probe(provider="openai", model="gpt-4o-mini", modality="text"), |
There was a problem hiding this comment.
use anthropic provider as well to test
kartpop
left a comment
There was a problem hiding this comment.
found at least two major issues - please fix
| _PROBE_MAX_TOKENS = 1 | ||
| _PROBE_WORKERS = 4 | ||
|
|
||
| # "Hello" ~1sec |
| def _build_config_and_input( | ||
| probe: Probe, | ||
| ) -> tuple[KaapiCompletionConfig, str | AudioRef] | None: | ||
| if probe.modality == "text": | ||
| cfg = KaapiCompletionConfig.model_validate( | ||
| { | ||
| "provider": probe.provider, | ||
| "type": "text", | ||
| "params": { | ||
| "model": probe.model, | ||
| "temperature": 0.0, | ||
| "max_output_tokens": _PROBE_MAX_TOKENS, | ||
| }, | ||
| } | ||
| ) | ||
| return cfg, _PROBE_INPUT | ||
|
|
||
| if probe.modality == "tts": | ||
| cfg = KaapiCompletionConfig.model_validate( | ||
| { | ||
| "provider": probe.provider, | ||
| "type": "tts", | ||
| "params": {"model": probe.model}, | ||
| } | ||
| ) | ||
| return cfg, _PROBE_INPUT | ||
|
|
||
| # stt | ||
| if not _STT_AUDIO_B64: | ||
| return None | ||
| cfg = KaapiCompletionConfig.model_validate( | ||
| { | ||
| "provider": probe.provider, | ||
| "type": "stt", | ||
| "params": {"model": probe.model}, | ||
| } | ||
| ) | ||
| audio = AudioRef( | ||
| bytes_=base64.b64decode(_STT_AUDIO_B64), | ||
| mime_type=_STT_AUDIO_MIME, | ||
| ) | ||
| return cfg, audio |
There was a problem hiding this comment.
not sure this will work!!!
shouldn't transform_kaapi_config_to_native be called like it gets called in the llm_call flow?
passing KappiCompletionConfig will probably send malformed requests - did we test this?
There was a problem hiding this comment.
yes this was working manually. I will retest and confirm
| @router.get( | ||
| "/cron/health-probes", | ||
| include_in_schema=False, | ||
| dependencies=[Depends(require_permission(Permission.SUPERUSER))], | ||
| ) | ||
| @sentry_sdk.monitor( | ||
| monitor_slug="health-probes-cron-job", | ||
| monitor_config=HEALTH_PROBES_CRON_MONITOR_CONFIG, | ||
| ) | ||
| def health_probes_cron_job() -> dict: | ||
| logger.info("[health_probes_cron_job] Cron job invoked — enqueueing task") | ||
| async_result = run_health_probes.delay() | ||
| return {"enqueued": True, "task_id": async_result.id} |
There was a problem hiding this comment.
this just checks whether the task is enqueued right? is it doing anything useful? i think this will mark SUCCESS irrespective of what happens in the celery task
@vprashrex can you confirm?
there should be a sentry monitor on the task also, because the real work is happening inside the celery task.
There was a problem hiding this comment.
@sentry_sdk.monitor is currently on the route, so it reports OK as soon as the Celery task is queued, not when it actually finishes. Probe failures only appear via logger.error, which isn't real monitoring and misses warning/skipped cases. Need to move the check-in into run_health_probes using capture_checkin, so the monitor reports OK only when all probes succeed and ERROR otherwise.
|
I have updated the PR label from |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
backend/app/tests/services/test_health_probes.py (1)
21-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd missing
-> Nonereturn type hints on__init__methods.
_NonClosingSession.__init__(line 25) and_FakeProvider.__init__(lines 41-46) lack return type annotations.As per coding guidelines, "provide narrow type hints for every function parameter and return value."
✏️ Proposed fix
- def __init__(self, session: Session): + def __init__(self, session: Session) -> None: self._session = sessiondef __init__( self, response: Any = None, error: str | None = None, raise_exc: Exception | None = None, - ): + ) -> None:🤖 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 `@backend/app/tests/services/test_health_probes.py` around lines 21 - 50, Add the explicit -> None return annotation to the __init__ methods of _NonClosingSession and _FakeProvider, preserving their existing parameters and initialization logic.Source: Coding guidelines
backend/app/services/health_probes.py (1)
160-241: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrevious critical concern resolved — config build now inside try-except in
_prepare_probes.
_build_config_and_inputis now called inside a dedicated try-except (lines 227-236) rather than unguarded inside_run_probe, so aValidationError/decode failure on one probe no longer aborts the whole run.Separately, several probe outcomes use hard-coded string literals (
"client_init_failed","stt_audio_not_configured","no_response","not_prepared") that are also asserted against by string equality in the test suite. Consider a smallProbeErrorEnum(or module-level constants) shared between this module and its tests to avoid silent drift between producer and consumer strings.As per coding guidelines, "Do not use magic values; extract repeated literals into constants, enums, or settings" and "Use an
Enumsuffix for enum class names."♻️ Proposed refactor
+class ProbeErrorEnum(str, Enum): + CLIENT_INIT_FAILED = "client_init_failed" + STT_AUDIO_NOT_CONFIGURED = "stt_audio_not_configured" + NO_RESPONSE = "no_response" + NOT_PREPARED = "not_prepared"🤖 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 `@backend/app/services/health_probes.py` around lines 160 - 241, Extract the repeated probe error literals used by _run_probe and _prepare_probes—"client_init_failed", "stt_audio_not_configured", "no_response", and "not_prepared"—into shared module-level constants or a ProbeErrorEnum, using the required Enum suffix if defining an enum. Replace the producer-side literals with those shared symbols and update tests to assert against the same symbols while preserving the existing serialized string values.Source: Coding guidelines
backend/app/celery/tasks/job_execution.py (1)
248-266: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing type hints on
run_prompt_improvement.Return type and
**kwargsare untyped, unlike the siblingrun_health_probestask.As per coding guidelines, "provide narrow type hints for every function parameter and return value; do not use
-> Anyas a substitute for a specific annotation."✏️ Proposed fix
-def run_prompt_improvement(self, project_id: int, job_id: str, trace_id: str, **kwargs): +def run_prompt_improvement( + self, project_id: int, job_id: str, trace_id: str, **kwargs: Any +) -> dict:🤖 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 `@backend/app/celery/tasks/job_execution.py` around lines 248 - 266, Update run_prompt_improvement with narrow type annotations for its **kwargs parameter and return value, matching the typing approach used by the sibling run_health_probes task; avoid using Any and preserve the existing task execution behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@backend/app/celery/tasks/job_execution.py`:
- Around line 248-266: Update run_prompt_improvement with narrow type
annotations for its **kwargs parameter and return value, matching the typing
approach used by the sibling run_health_probes task; avoid using Any and
preserve the existing task execution behavior.
In `@backend/app/services/health_probes.py`:
- Around line 160-241: Extract the repeated probe error literals used by
_run_probe and _prepare_probes—"client_init_failed", "stt_audio_not_configured",
"no_response", and "not_prepared"—into shared module-level constants or a
ProbeErrorEnum, using the required Enum suffix if defining an enum. Replace the
producer-side literals with those shared symbols and update tests to assert
against the same symbols while preserving the existing serialized string values.
In `@backend/app/tests/services/test_health_probes.py`:
- Around line 21-50: Add the explicit -> None return annotation to the __init__
methods of _NonClosingSession and _FakeProvider, preserving their existing
parameters and initialization logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 468b52fd-540d-451e-bdb0-74d9f25c4b25
⛔ Files ignored due to path filters (1)
backend/app/assets/health_probe_hello.oggis excluded by!**/*.ogg
📒 Files selected for processing (7)
backend/app/api/routes/cron.pybackend/app/celery/tasks/job_execution.pybackend/app/core/config.pybackend/app/services/health_probes.pybackend/app/tests/api/routes/test_cron_health_probes.pybackend/app/tests/celery/test_run_health_probes.pybackend/app/tests/services/test_health_probes.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/tests/api/routes/test_cron_health_probes.py
- backend/app/core/config.py
Issue #987
Summary
A cron job that wakes up every 5 mins and make API calls with a small payload to every major models. If any error occurs i.e inference for the model is down it shows up in Sentry Issues. The call traces are logged in sentry too.
Added automated health probes for configured text, speech-to-text, and text-to-speech providers.
Models count 13
The job happens inside Celery task.
env addition
The superuser org_id and project_id against whose creds are to be used to make the provider calls. This to be set during deployment.
Checklist
Before submitting a pull request, please ensure that you mark these task.
fastapi run --reload app/main.pyordocker compose upin the repository root and test.Summary by CodeRabbit
New Features
Bug Fixes
Tests