Completed tasks Dependency installation — Added logfire[fastapi,pydantic]>=1.0.0 to pyproject.toml Logging configuration — Created src/logging_config.py with environment-aware setup Settings model — Updated src/config.py with Logfire configuration options#3
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces comprehensive observability and structured logging capabilities via Pydantic Logfire integration, adds extensive operational and architectural documentation, enhances service instrumentation with timing and context-aware logging, and removes test artifacts and outdated planning files. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
✨ 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 |
|
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| configure_fastapi() | ||
| configure_pydantic() |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Pull request overview
This pull request adds comprehensive observability to the Facebook Messenger AI Bot using Pydantic Logfire, including structured logging, request tracing with correlation IDs, and performance monitoring across all services.
Changes:
- Added Logfire observability integration with FastAPI and Pydantic instrumentation
- Created centralized logging configuration with environment-aware setup and PII masking utilities
- Implemented correlation ID middleware for distributed request tracing
- Instrumented all services (Copilot, Agent, Scraper, Facebook, Repository) with structured logging and timing metrics
- Added test fixtures and comprehensive logging tests
Reviewed changes
Copilot reviewed 50 out of 55 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Added logfire[fastapi,pydantic]>=1.0.0 dependency |
| uv.lock | Updated lock file with new dependencies (asgiref, opentelemetry packages) |
| src/logging_config.py | New centralized logging configuration with Logfire setup, PII masking utilities |
| src/config.py | Added Logfire configuration options (token, log level, PII masking, request logging flags) |
| src/main.py | Integrated Logfire initialization in app lifespan and added correlation ID middleware |
| src/middleware/correlation_id.py | New middleware for request tracing with correlation IDs |
| src/services/*.py | Added structured logging to all services with timing, errors, and metrics |
| src/db/repository.py | Added database operation logging with timing and error tracking |
| tests/conftest.py | Added logfire_capture and mock_logfire test fixtures |
| tests/unit/test_logging.py | New comprehensive tests for logging across all services |
| docs/LOGFIRE.md | New comprehensive documentation for Logfire integration |
| docs/RUNBOOK.md | New operational troubleshooting guide with Logfire procedures |
| Various documentation | Updated ARCHITECTURE.md, AGENTS.md, TESTING.md, README.md with Logfire references |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| configure_fastapi() | ||
| configure_pydantic() |
There was a problem hiding this comment.
The imports from logfire.integrations.fastapi import configure_fastapi and from logfire.integrations.pydantic import configure_pydantic are called without any arguments on lines 40-41. According to the Logfire documentation for version 1.0.0+, these integration functions may require parameters or the API may have changed. The correct usage should be verified against the actual Logfire 1.0.0+ API documentation. Consider checking if these functions need the FastAPI app instance or other configuration parameters to be passed.
| with logfire.context(correlation_id=correlation_id): | ||
| response = await call_next(request) | ||
|
|
||
| # Add correlation ID to response headers | ||
| response.headers[self.header_name] = correlation_id | ||
|
|
||
| return response |
There was a problem hiding this comment.
The logfire.context() usage on line 55 creates a context manager that should properly handle entering and exiting. However, if an exception occurs during request processing (within call_next), the correlation ID context may not be properly cleaned up if logfire's context manager doesn't handle exceptions correctly. Consider wrapping this in a try-finally block or verifying that logfire's context manager is exception-safe to prevent context leaks.
| def redact_tokens(data: dict[str, Any]) -> dict[str, Any]: | ||
| """ | ||
| Redact authentication tokens and API keys from log data. | ||
|
|
||
| Args: | ||
| data: Dictionary that may contain sensitive tokens | ||
|
|
||
| Returns: | ||
| Dictionary with tokens redacted | ||
| """ | ||
| redacted = data.copy() | ||
| sensitive_keys = [ | ||
| "token", | ||
| "access_token", | ||
| "api_key", | ||
| "secret", | ||
| "password", | ||
| "authorization", | ||
| "auth", | ||
| ] | ||
|
|
||
| for key in sensitive_keys: | ||
| if key in redacted: | ||
| if isinstance(redacted[key], str): | ||
| redacted[key] = mask_pii(redacted[key]) | ||
| elif isinstance(redacted[key], dict): | ||
| redacted[key] = redact_tokens(redacted[key]) | ||
|
|
||
| return redacted |
There was a problem hiding this comment.
The redact_tokens function performs a shallow copy with data.copy() and then recursively calls itself for nested dictionaries. This approach doesn't create a deep copy, which means that nested dictionaries and lists that are not explicitly processed will still reference the original data structures. If data contains nested lists or other complex structures with sensitive tokens, they won't be redacted. Consider using copy.deepcopy() before processing or explicitly handling all nested structures including lists.
| def capture_info(*args, **kwargs): | ||
| captured_logs.append(("info", args, kwargs)) | ||
| return original_info(*args, **kwargs) | ||
|
|
||
| def capture_warn(*args, **kwargs): | ||
| captured_logs.append(("warn", args, kwargs)) | ||
| return original_warn(*args, **kwargs) | ||
|
|
||
| def capture_error(*args, **kwargs): | ||
| captured_logs.append(("error", args, kwargs)) | ||
| return original_error(*args, **kwargs) | ||
|
|
||
| with patch("logfire.info", side_effect=capture_info), \ | ||
| patch("logfire.warn", side_effect=capture_warn), \ | ||
| patch("logfire.error", side_effect=capture_error): | ||
| yield captured_logs |
There was a problem hiding this comment.
The logfire_capture fixture patches logfire functions globally but still calls the original functions via side_effect. This means logs are both captured AND sent to the actual logfire backend during tests. For true test isolation, consider either using return_value instead of side_effect to prevent actual logging, or ensure tests run with logfire disabled. The current implementation may cause tests to fail if logfire is not properly configured or if the backend is unavailable.
| chat_logs = [ | ||
| log for log in logfire_capture | ||
| if "chat" in str(log[1]).lower() or "chat" in str(kwargs).lower() | ||
| for kwargs in [log[2]] | ||
| ] |
There was a problem hiding this comment.
The list comprehension on lines 54-58 uses a potentially confusing pattern with for kwargs in [log[2]]. This creates a single-element list just to unpack it in the comprehension, which is unnecessarily complex. Consider simplifying to: chat_logs = [log for log in logfire_capture if "chat" in str(log[1]).lower() or "chat" in str(log[2]).lower()] for better readability.
| logfire_enable_pii_masking: bool = Field( | ||
| default=True, | ||
| description="Enable PII masking in logs" | ||
| ) | ||
| logfire_enable_request_logging: bool = Field( | ||
| default=True, | ||
| description="Enable HTTP request/response logging" | ||
| ) |
There was a problem hiding this comment.
The logfire_enable_pii_masking and logfire_enable_request_logging configuration options are defined in the Settings model but are not actually used anywhere in the codebase. The setup_logfire() function doesn't check these settings before enabling instrumentation, and the PII masking utilities (mask_pii, redact_tokens) are defined but never automatically applied. These settings should either be removed or the implementation should be completed to actually respect these configuration flags.
| """Centralized logging configuration with Pydantic Logfire integration.""" | ||
|
|
||
| import logging | ||
| import os |
There was a problem hiding this comment.
The import statement import os on line 4 is unused in this file. The os module is not referenced anywhere in the logging_config.py file. Remove this unused import to keep the code clean.
| return_value="<html><body>Test content with many words " * 100 + "</body></html>" | ||
| ) | ||
|
|
||
| chunks = await scrape_website("https://example.com") |
There was a problem hiding this comment.
Variable chunks is not used.
| """Tests for structured logging with Logfire.""" | ||
|
|
||
| import pytest | ||
| from unittest.mock import AsyncMock, MagicMock, patch |
There was a problem hiding this comment.
Import of 'AsyncMock' is not used.
Import of 'patch' is not used.
Import of 'MagicMock' is not used.
Dependency installation — Added logfire[fastapi,pydantic]>=1.0.0 to pyproject.toml Logging configuration — Created src/logging_config.py with environment-aware setup Settings model — Updated src/config.py with Logfire configuration options FastAPI integration — Modified src/main.py to initialize Logfire in the lifespan Correlation ID middleware — Created src/middleware/correlation_id.py for request tracing Service instrumentation — Added structured logging to: CopilotService — health checks, API calls, timing, errors MessengerAgentService — message processing, confidence, escalation ScraperService — scraping metrics, chunking stats, content hashing FacebookService — message sends, API responses, error handling Repository — database operations with timing Test fixtures — Added logfire_capture and mock_logfire fixtures in tests/conftest.py Logging tests — Created tests/unit/test_logging.py with tests for all services Environment configuration — Updated .env.exDocumentation — Created LOGFIRE.md with setup and usage guide Features Structured JSON logging for production Request tracing with correlation IDs Performance monitoring with automatic timing PII masking utilities for security Environment-aware configuration (local vs production) FastAPI and Pydantic instrumentation Test fixtures for log verification
jreakin
force-pushed
the
integrate-logfire
branch
from
29c93d6 to
91446c3
Compare
Dependency installation — Added logfire[fastapi,pydantic]>=1.0.0 to pyproject.toml
Logging configuration — Created src/logging_config.py with environment-aware setup
Settings model — Updated src/config.py with Logfire configuration options
FastAPI integration — Modified src/main.py to initialize Logfire in the lifespan
Correlation ID middleware — Created src/middleware/correlation_id.py for request tracing
Service instrumentation — Added structured logging to:
CopilotService — health checks, API calls, timing, errors
MessengerAgentService — message processing, confidence, escalation
ScraperService — scraping metrics, chunking stats, content hashing
FacebookService — message sends, API responses, error handling
Repository — database operations with timing
Test fixtures — Added logfire_capture and mock_logfire fixtures in tests/conftest.py
Logging tests — Created tests/unit/test_logging.py with tests for all services
Environment configuration — Updated .env.exDocumentation — Created LOGFIRE.md with setup and usage guide
Features
Structured JSON logging for production
Request tracing with correlation IDs
Performance monitoring with automatic timing
PII masking utilities for security
Environment-aware configuration (local vs production)
FastAPI and Pydantic instrumentation
Test fixtures for log verification
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.