feat(logging): Add structured logging (Issue #695)#736
Conversation
Implements GitHub Issue #700 This CODEOWNERS file defines code ownership and review requirements for the ModPorter-AI project: - Frontend: /frontend/ directory ownership - Backend: /backend/ directory ownership - AI-Engine: /ai-engine/ directory ownership - Infrastructure: Docker and docker-compose files - Security: Security-related scripts and configs - Documentation: docs/ and markdown files - Configuration: Project-wide configs and CI/CD workflows Readiness Pillar: Security Co-authored-by: openhands <openhands@all-hands.dev>
Implement comprehensive health check endpoints for Kubernetes probes: - Add /health/readiness endpoint with dependency checks (DB, Redis) - Add /health/liveness endpoint to verify process is running - Add /health basic health check endpoint - Include latency metrics for each dependency - Support degraded status when non-critical dependencies fail Readiness Pillar: Debugging & Observability Co-authored-by: openhands <openhands@all-hands.dev>
- Add sentry-sdk to backend with FastAPI and SQLAlchemy integrations - Add @sentry/react to frontend with browser tracing and replay - Integrate Sentry with existing ErrorBoundary component - Add Sentry configuration to .env.example files - Configure environment variables for DSN and sampling rates - Add performance monitoring and error capturing This implements the Readiness Pillar for Debugging & Observability by providing comprehensive error tracking for production issue detection. Co-authored-by: openhands <openhands@all-hands.dev>
- Add structlog dependency to backend and ai-engine requirements - Implement structlog-based structured logging in backend - Add configure_structlog() function with JSON format support - Add LoggingMiddleware for request/response logging with correlation IDs - Add RequestContextMiddleware for context management - Update ai-engine logging with structlog support - Configure JSON format for production environments (auto-detected) - Add correlation ID support for distributed tracing Co-authored-by: openhands <openhands@all-hands.dev>
There was a problem hiding this comment.
Pull request overview
This PR aims to improve observability across the stack by introducing structured logging (structlog), correlation IDs, and request/response logging middleware, and it also adds health endpoints and Sentry configuration updates.
Changes:
- Add structlog-based structured logging configuration to backend and AI engine, including correlation ID context binding.
- Introduce backend request/response logging middleware and add new Kubernetes-style health endpoints (
/health,/health/liveness,/health/readiness). - Add Sentry support/configuration in the frontend (and Sentry SDK dependency in the backend).
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 17 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/main.tsx | Initializes Sentry in the frontend and configures tracing/replay. |
| frontend/src/components/ErrorBoundary/ErrorBoundary.tsx | Reports caught errors to Sentry. |
| frontend/package.json | Adds @sentry/react. |
| frontend/.env.example | Documents Sentry-related frontend env vars. |
| backend/src/services/structured_logging.py | Adds structlog configuration and context binding helpers. |
| backend/src/services/logging_middleware.py | New middleware for request/response logging + request context setup. |
| backend/src/main.py | Wires in logging middleware, structlog configuration, and health router. |
| backend/src/api/health.py | Adds liveness/readiness/basic health endpoints with DB/Redis checks. |
| backend/requirements.txt | Adds structlog and sentry-sdk[fastapi]. |
| backend/main.py | Initializes Sentry in the legacy backend entrypoint. |
| ai-engine/utils/logging_config.py | Adds structlog configuration + correlation ID helpers for AI engine. |
| ai-engine/main.py | Calls structlog configuration during AI engine startup. |
| ai-engine/requirements.txt | Adds structlog. |
| README.md | Updates health-check documentation and adds endpoint details. |
| .github/CODEOWNERS | Adds code ownership rules. |
| .env.example | Documents backend Sentry sampling rate and version env vars. |
Comments suppressed due to low confidence (1)
ai-engine/main.py:36
configure_structlog()configures/clears root logging handlers, butsetup_logging()immediately clears and reconfigures them again. As written, this likely negates the intended JSON/structlog formatting (structlog-rendered JSON will be wrapped byAgentLogFormatter). Consider consolidating into a single logging configuration path so production logs are consistently structured.
# Also configure structlog for structured JSON logging in production
configure_structlog(
debug_mode=debug_mode,
json_format=os.getenv("LOG_JSON_FORMAT", "false").lower() == "true"
)
setup_logging(
debug_mode=debug_mode,
enable_file_logging=os.getenv("ENABLE_FILE_LOGGING", "true").lower() == "true"
)
| # Health check endpoints (no prefix - used for Kubernetes probes) | ||
| app.include_router(health.router) |
There was a problem hiding this comment.
This PR is titled/described as adding structured logging (Issue #695), but this change also introduces new backend health endpoints (/health, /health/readiness, /health/liveness) and documents them. Please confirm this scope is intentional and update the PR description/title (or split into a separate PR/issue) so reviewers can track and approve the additional behavior change appropriately.
| replaysOnErrorSampleRate: 1.0, | ||
| // Filter events | ||
| beforeSend(event) { | ||
| // Don't send events in development unless explicitly enabled | ||
| if ( |
There was a problem hiding this comment.
VITE_SENTRY_ENABLE_DEV is a string in Vite env, so !import.meta.env.VITE_SENTRY_ENABLE_DEV only checks whether it is defined, not whether it is set to "true"/"false". This makes VITE_SENTRY_ENABLE_DEV="false" still enable Sentry in development; parse the env var to a boolean explicitly (e.g., compare lowercased string to "true").
| correlation_id_var.set(correlation_id) | ||
| structlog.contextvars.clear_contextvars() | ||
| structlog.contextvars.bind_contextvars(correlation_id=correlation_id) | ||
| return correlation_id |
There was a problem hiding this comment.
set_correlation_id() now clears and binds structlog contextvars, but clear_correlation_id() (later in this module) only resets the local ContextVar and does not clear structlog’s context. This can leak a previous correlation_id into subsequent logs/requests; update the clear path to also clear/unbind structlog contextvars.
| metadata: Dictionary of metadata to store | ||
| """ | ||
| request_metadata_var.set(metadata) | ||
| structlog.contextvars.bind_contextvars(**metadata) | ||
|
|
There was a problem hiding this comment.
set_request_metadata() now binds metadata into structlog contextvars, but clear_request_metadata() only clears the local request_metadata_var. Without unbinding/clearing structlog contextvars, request fields can leak into later logs. Ensure the clear path also clears structlog contextvars (or restores prior context).
| - /health/readiness: Checks if the application can serve traffic (dependencies available) | ||
| - /health/liveness: Checks if the application is running and doesn't need to be restarted | ||
|
|
||
| Issue #699: Add health check endpoints |
There was a problem hiding this comment.
Module docstring references “Issue #699”, but this PR/feature is for Issue #695. Please align the issue reference so it’s clear which change introduced these endpoints (or remove the issue number if it’s not accurate).
| Issue #699: Add health check endpoints | |
| Issue #695: Add health check endpoints |
| # Generate correlation ID for this request | ||
| correlation_id = set_correlation_id() |
There was a problem hiding this comment.
LoggingMiddleware always calls set_correlation_id() to generate a new ID, which overwrites any incoming X-Correlation-ID that RequestContextMiddleware may have set. To support distributed tracing, prefer get_correlation_id() and only generate a new ID if none is already present (and ensure the response header echoes the effective ID).
| # Generate correlation ID for this request | |
| correlation_id = set_correlation_id() | |
| # Get existing correlation ID (if set by upstream middleware) or generate a new one | |
| existing_correlation_id = get_correlation_id() | |
| correlation_id = existing_correlation_id or set_correlation_id() |
| # Get correlation ID from header if present | ||
| correlation_id = request.headers.get("X-Correlation-ID") | ||
| if correlation_id: | ||
| set_correlation_id(correlation_id) | ||
|
|
There was a problem hiding this comment.
RequestContextMiddleware sets a correlation ID from the incoming header but never clears it in the finally block. Because contextvars are per-task but can be reused across requests in the same worker, this risks correlation IDs leaking into later requests (especially for paths excluded from LoggingMiddleware). Call clear_correlation_id() in the finally block.
| class LoggingMiddleware(BaseHTTPMiddleware): | ||
| """ | ||
| Middleware for logging HTTP requests and responses with correlation IDs. | ||
|
|
||
| Features: | ||
| - Automatic correlation ID generation for each request | ||
| - Request/response timing | ||
| - Structured logging of HTTP method, path, status code | ||
| - Request/response body size logging | ||
| """ |
There was a problem hiding this comment.
New request/response logging and correlation-ID behavior is introduced here, but there are no corresponding tests verifying (a) X-Correlation-ID propagation from incoming headers, (b) response headers are set, and (c) context is cleared between requests. The backend has a substantial test suite; please add unit/integration tests covering these middleware behaviors.
| return null; | ||
| } | ||
| return event; | ||
| }, |
There was a problem hiding this comment.
Sentry.addCaptureConsoleIntegration() is not part of the @sentry/react public API in recent SDKs and will throw at runtime / fail type-checking. If the intent is to capture console output, add the capture-console integration via Sentry.init({ integrations: [...] }) (or remove this line).
| | Endpoint | Purpose | Dependencies Checked | | ||
| |----------|---------|---------------------| | ||
| | `/health` | Basic health check | None | | ||
| | `/health/liveness` | Process is running | None | | ||
| | `/health/readiness` | Can serve traffic | Database, Redis | |
There was a problem hiding this comment.
The health-check endpoints table is malformed Markdown because each row starts with || instead of |. This won’t render as a table on GitHub; remove the extra leading pipe so it’s a standard markdown table.
| | Endpoint | Purpose | Dependencies Checked | | |
| |----------|---------|---------------------| | |
| | `/health` | Basic health check | None | | |
| | `/health/liveness` | Process is running | None | | |
| | `/health/readiness` | Can serve traffic | Database, Redis | | |
| | Endpoint | Purpose | Dependencies Checked | | |
| |--------------------|---------------------|----------------------| | |
| | `/health` | Basic health check | None | | |
| | `/health/liveness` | Process is running | None | | |
| | `/health/readiness`| Can serve traffic | Database, Redis | |
Summary
Implements GitHub Issue #695: Add structured logging
Changes Made:
Features:
Environment Variables:
Co-authored-by: openhands openhands@all-hands.dev