chore: production readiness hardening (deps, logging, Sentry, DB pool)#4
Closed
sarangs1621 wants to merge 2 commits into
Closed
chore: production readiness hardening (deps, logging, Sentry, DB pool)#4sarangs1621 wants to merge 2 commits into
sarangs1621 wants to merge 2 commits into
Conversation
- Bump pytest/pytest-asyncio and fastapi/starlette to clear all pip-audit findings (starlette 1.x required two HTTP status constant renames). - Add configurable structured logging (text/json) so existing fail-open warnings are visible in production. - Add optional Sentry error tracking, no-op unless SENTRY_DSN is set. - Make the SQLAlchemy async engine connection pool configurable, with pre-ping and recycle enabled by default for managed Postgres providers. - Document new settings and drop the now-resolved dependency vulnerability note from the README. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR adds production observability and deployment infrastructure. It extends configuration for logging (with JSON support), Sentry error tracking, and SQLAlchemy connection pool tuning; creates centralized logging and Sentry modules with idempotent initialization; introduces Render deployment manifests for web, worker, and beat services; and adds a local development scheduler. Dependencies are updated and git ignores extended for frontend and Celery artifacts. ChangesObservability, Performance Tuning, and Deployment
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Adds render.yaml describing the API, worker, beat, and Postgres services for a Render deployment, plus run_local_scheduler.py for running the dispatch tasks without Redis/Celery during local dev. Documents SMTP and production environment variables in .env.example and ignores frontend build artifacts and local celerybeat schedule files.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@app/core/config.py`:
- Line 25: SENTRY_TRACES_SAMPLE_RATE (and the other unconstrained numeric
runtime settings around lines 38-41) must be validated at config load; add
explicit bounds checks (e.g., for sample rates enforce 0.0 <=
SENTRY_TRACES_SAMPLE_RATE <= 1.0, and apply sensible min/max for any other
numeric settings) in the config initialization path (e.g., __post_init__ of the
Config/dataclass or pydantic validators) and raise a clear ValueError with the
variable name and invalid value if the check fails so startup fails fast with a
descriptive message.
In `@app/core/logging.py`:
- Around line 50-53: The root logger unconditionally calls
root_logger.addHandler(handler) which can duplicate logs; update the
initialization in app/core/logging.py to detect existing equivalent handlers
before adding: inspect logging.getLogger()'s handlers (root_logger.handlers) and
only add the new handler if there is no existing handler of the same type and
configuration (e.g., same class or same stream/formatter), so modify the logic
around root_logger, handler, addHandler and settings.LOG_LEVEL to perform this
presence check and skip addHandler when an equivalent handler is already
attached.
In `@render.yaml`:
- Around line 20-24: The manifest references a Redis service named
"sentinel-redis" in the REDIS_URL fromService blocks (type: redis, property:
connectionURI) but that service is not declared; either add a service
declaration for sentinel-redis with the expected redis/sentinel configuration or
update each fromService reference (lines using REDIS_URL) to point to an
existing declared Redis service name; ensure the service name you choose matches
all uses (the three fromService entries) and provides the connectionURI property
expected by the consumers.
In `@run_local_scheduler.py`:
- Around line 26-31: Split the single try/except so a failure in
dispatch_due_checks.delay() doesn't prevent
dispatch_pending_notifications.delay() from running: wrap each call
(dispatch_due_checks.delay() and dispatch_pending_notifications.delay()) in its
own try/except and log errors separately (preserving the original exception
handling behavior), and keep the last_check = now assignment after both attempts
so the cycle advances only after both dispatches have been attempted.
In `@tests/test_logging.py`:
- Around line 31-42: The tests (e.g., test_configure_logging_is_idempotent)
restore root_logger.handlers but fail to restore root_logger.level after calling
configure_logging(), leaking global logging level into other tests; update the
teardown/finally blocks in this test (and the similar test at lines 44-55) to
capture root_logger.level before calling configure_logging() and restore that
saved level in the finally block so the root logger's level is returned to its
original value after the 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e6170c49-cdbc-4976-a1b5-2b8dbd68237b
📒 Files selected for processing (16)
.env.example.gitignoreREADME.mdapp/core/celery_app.pyapp/core/config.pyapp/core/logging.pyapp/core/security_headers.pyapp/core/sentry.pyapp/db/session.pyapp/main.pyrender.yamlrequirements-dev.txtrequirements.txtrun_local_scheduler.pytests/test_logging.pytests/test_sentry.py
|
|
||
| # Error tracking (optional). Leave unset to disable Sentry entirely. | ||
| SENTRY_DSN: str | None = None | ||
| SENTRY_TRACES_SAMPLE_RATE: float = 0.0 |
There was a problem hiding this comment.
Add bounds validation for new numeric runtime settings.
Line 25 and Lines 38-41 accept unconstrained numeric values from env, so invalid values can slip through and break startup/runtime behavior instead of failing fast at config load.
Suggested fix
-from pydantic import field_validator, model_validator
+from pydantic import Field, field_validator, model_validator
@@
- SENTRY_TRACES_SAMPLE_RATE: float = 0.0
+ SENTRY_TRACES_SAMPLE_RATE: float = Field(default=0.0, ge=0.0, le=1.0)
@@
- DB_POOL_SIZE: int = 5
- DB_MAX_OVERFLOW: int = 10
- DB_POOL_TIMEOUT: int = 30
- DB_POOL_RECYCLE_SECONDS: int = 1800
+ DB_POOL_SIZE: int = Field(default=5, ge=1)
+ DB_MAX_OVERFLOW: int = Field(default=10, ge=0)
+ DB_POOL_TIMEOUT: int = Field(default=30, ge=1)
+ DB_POOL_RECYCLE_SECONDS: int = Field(default=1800, ge=0)Also applies to: 38-42
🤖 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 `@app/core/config.py` at line 25, SENTRY_TRACES_SAMPLE_RATE (and the other
unconstrained numeric runtime settings around lines 38-41) must be validated at
config load; add explicit bounds checks (e.g., for sample rates enforce 0.0 <=
SENTRY_TRACES_SAMPLE_RATE <= 1.0, and apply sensible min/max for any other
numeric settings) in the config initialization path (e.g., __post_init__ of the
Config/dataclass or pydantic validators) and raise a clear ValueError with the
variable name and invalid value if the check fails so startup fails fast with a
descriptive message.
Comment on lines
+50
to
+53
| root_logger = logging.getLogger() | ||
| root_logger.setLevel(settings.LOG_LEVEL) | ||
| root_logger.addHandler(handler) | ||
|
|
There was a problem hiding this comment.
Avoid unconditional root handler append (duplicate log emission risk).
At Line 52, root_logger.addHandler(handler) always adds another handler even when a root stream handler already exists, which can duplicate every log record in real runtimes that preconfigure logging.
Proposed fix
def configure_logging() -> None:
@@
root_logger = logging.getLogger()
root_logger.setLevel(settings.LOG_LEVEL)
- root_logger.addHandler(handler)
+ # Replace existing root handlers to ensure a single, deterministic format.
+ root_logger.handlers.clear()
+ root_logger.addHandler(handler)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| root_logger = logging.getLogger() | |
| root_logger.setLevel(settings.LOG_LEVEL) | |
| root_logger.addHandler(handler) | |
| root_logger = logging.getLogger() | |
| root_logger.setLevel(settings.LOG_LEVEL) | |
| # Replace existing root handlers to ensure a single, deterministic format. | |
| root_logger.handlers.clear() | |
| root_logger.addHandler(handler) |
🤖 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 `@app/core/logging.py` around lines 50 - 53, The root logger unconditionally
calls root_logger.addHandler(handler) which can duplicate logs; update the
initialization in app/core/logging.py to detect existing equivalent handlers
before adding: inspect logging.getLogger()'s handlers (root_logger.handlers) and
only add the new handler if there is no existing handler of the same type and
configuration (e.g., same class or same stream/formatter), so modify the logic
around root_logger, handler, addHandler and settings.LOG_LEVEL to perform this
presence check and skip addHandler when an equivalent handler is already
attached.
Comment on lines
+20
to
+24
| - key: REDIS_URL | ||
| fromService: | ||
| type: redis | ||
| name: sentinel-redis | ||
| property: connectionURI |
There was a problem hiding this comment.
sentinel-redis is referenced but not declared in this blueprint.
Lines 20-24, 52-56, and 81-85 all depend on a Redis service named sentinel-redis, but this manifest does not define one. That creates a deployment-time wiring failure for all three services.
Suggested fix
services:
@@
- type: worker
name: sentinel-beat
@@
- key: PYTHON_VERSION
value: "3.12.0"
+
+ - type: redis
+ name: sentinel-redis
+ plan: free
+ ipAllowList: []Also applies to: 52-56, 81-85
🤖 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 `@render.yaml` around lines 20 - 24, The manifest references a Redis service
named "sentinel-redis" in the REDIS_URL fromService blocks (type: redis,
property: connectionURI) but that service is not declared; either add a service
declaration for sentinel-redis with the expected redis/sentinel configuration or
update each fromService reference (lines using REDIS_URL) to point to an
existing declared Redis service name; ensure the service name you choose matches
all uses (the three fromService entries) and provides the connectionURI property
expected by the consumers.
Comment on lines
+12
to
+13
| print("Starting Sentinel Local Scheduler (No Redis required)...") | ||
| print("Using eager task execution.") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Locate scheduler/Celery config files ==="
fd -i 'run_local_scheduler.py|celery_app.py|config.py|tasks.py'
echo
echo "=== Find eager-mode and delay usage ==="
rg -nP --type=py '\b(task_always_eager|task_eager_propagates|CELERY_TASK_ALWAYS_EAGER|\.delay\s*\()'
echo
echo "=== Focus on scheduler + worker dispatch chain ==="
rg -nP --type=py '\bdispatch_due_checks\b|\bdispatch_pending_notifications\b|\bdispatch_metric_aggregation\b|\.delay\s*\(' run_local_scheduler.py app/workers/tasks.pyRepository: sarangs1621/Sentinel
Length of output: 2079
Make the “No Redis required / eager execution” claim conditional on Celery eager mode
run_local_scheduler.py prints “No Redis required” / “Using eager task execution” unconditionally (lines 12-13), but it dispatches Celery tasks via .delay() (lines 27-28, 36), which requires the broker unless eager mode is enabled. The Celery app uses task_always_eager=settings.CELERY_TASK_ALWAYS_EAGER (app/core/celery_app.py), and the default is CELERY_TASK_ALWAYS_EAGER: bool = False (app/core/config.py). The nested dispatcher tasks in app/workers/tasks.py also call .delay(), so brokerless behavior is not guaranteed.
Update this entrypoint to either force CELERY_TASK_ALWAYS_EAGER=True before Celery app creation (and align the log messages), or fail fast with a clear error when CELERY_TASK_ALWAYS_EAGER is false.
Comment on lines
+26
to
+31
| try: | ||
| dispatch_due_checks.delay() | ||
| dispatch_pending_notifications.delay() | ||
| except Exception as e: | ||
| print(f"Error during dispatch: {e}") | ||
| last_check = now |
There was a problem hiding this comment.
Split dispatch error handling so one failing task doesn’t block the other.
At Line 26–31, if dispatch_due_checks.delay() fails, dispatch_pending_notifications.delay() is skipped for that cycle, and last_check still advances. That creates avoidable notification delays.
Suggested fix
if now - last_check >= check_interval:
print(f"[{time.strftime('%H:%M:%S')}] Dispatching checks and notifications...")
- try:
- dispatch_due_checks.delay()
- dispatch_pending_notifications.delay()
- except Exception as e:
- print(f"Error during dispatch: {e}")
+ try:
+ dispatch_due_checks.delay()
+ except Exception as e:
+ print(f"Error dispatching due checks: {e}")
+ try:
+ dispatch_pending_notifications.delay()
+ except Exception as e:
+ print(f"Error dispatching pending notifications: {e}")
last_check = now🤖 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 `@run_local_scheduler.py` around lines 26 - 31, Split the single try/except so
a failure in dispatch_due_checks.delay() doesn't prevent
dispatch_pending_notifications.delay() from running: wrap each call
(dispatch_due_checks.delay() and dispatch_pending_notifications.delay()) in its
own try/except and log errors separately (preserving the original exception
handling behavior), and keep the last_check = now assignment after both attempts
so the cycle advances only after both dispatches have been attempted.
Comment on lines
+31
to
+42
| def test_configure_logging_is_idempotent(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| root_logger = logging.getLogger() | ||
| monkeypatch.setattr(app_logging, "_configured", False) | ||
| handlers_before = list(root_logger.handlers) | ||
|
|
||
| try: | ||
| configure_logging() | ||
| configure_logging() | ||
| assert len(root_logger.handlers) == len(handlers_before) + 1 | ||
| finally: | ||
| root_logger.handlers = handlers_before | ||
|
|
There was a problem hiding this comment.
Restore root logger level in teardown to prevent cross-test state leakage.
These tests restore handlers but not root_logger.level; since configure_logging() mutates it, later tests can observe altered global logging state.
Proposed fix
def test_configure_logging_is_idempotent(monkeypatch: pytest.MonkeyPatch) -> None:
root_logger = logging.getLogger()
+ level_before = root_logger.level
@@
finally:
root_logger.handlers = handlers_before
+ root_logger.setLevel(level_before)
def test_configure_logging_uses_json_formatter_when_configured(monkeypatch: pytest.MonkeyPatch) -> None:
root_logger = logging.getLogger()
+ level_before = root_logger.level
@@
finally:
root_logger.handlers = handlers_before
+ root_logger.setLevel(level_before)Also applies to: 44-55
🤖 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_logging.py` around lines 31 - 42, The tests (e.g.,
test_configure_logging_is_idempotent) restore root_logger.handlers but fail to
restore root_logger.level after calling configure_logging(), leaking global
logging level into other tests; update the teardown/finally blocks in this test
(and the similar test at lines 44-55) to capture root_logger.level before
calling configure_logging() and restore that saved level in the finally block so
the root logger's level is returned to its original value after the test.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pip-auditfindings; updates two now-deprecated Starlette HTTP status constants accordingly.LOG_LEVEL/LOG_FORMAT, text or JSON) viaapp/core/logging.py, wired into both the FastAPI app and Celery workers.app/core/sentry.py), gated bySENTRY_DSN- no-op when unset.DB_POOL_SIZE,DB_MAX_OVERFLOW,DB_POOL_TIMEOUT,DB_POOL_RECYCLE_SECONDS,DB_POOL_PRE_PING), with pre-ping and 30-min recycle enabled by default for managed Postgres providers..env.exampleand a new README "Configuration" section; removes the now-resolved "pre-existing dependency vulnerabilities" note.Test plan
pytest- 209 passed, 96.54% coverage (>= 95% gate)ruff check .- all checks passedmypy app- no issues in 91 source filespip-audit -r requirements.txt -r requirements-dev.txt- no known vulnerabilities-W error::DeprecationWarning- no warnings🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes