chore: make posthog optional (use mock if not env not set) #249
Conversation
WalkthroughThe changes integrate an Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API
participant Controller
participant AnalyticsService
Client->>API: Request (e.g., create conversation)
API->>Controller: Call with analytics_service injected
Controller->>AnalyticsService: capture_event("event_name", properties)
Controller-->>API: Return response
API-->>Client: Response with analytics integrated
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
app/core/dependencies.py (2)
15-17: Remove unnecessary empty lines.There are unnecessary empty lines between the comment and the function definition.
Apply this diff to improve readability:
# Analytics service configuration - def init_analytics_service() -> AnalyticsService:
19-20: Improve environment variable handling.The current implementation uses empty string as default which requires an additional check. Consider using
Noneas default to simplify the logic.Apply this diff to improve the code:
- POSTHOG_API_KEY = os.getenv("POSTHOG_API_KEY") or "" - POSTHOG_HOST = os.getenv("POSTHOG_HOST") or "" + POSTHOG_API_KEY = os.getenv("POSTHOG_API_KEY") + POSTHOG_HOST = os.getenv("POSTHOG_HOST") - if POSTHOG_API_KEY != "" and POSTHOG_HOST != "": + if POSTHOG_API_KEY and POSTHOG_HOST:app/modules/utils/analytics_service.py (2)
2-2: Remove unused import.The
osmodule is imported but never used.Apply this diff to remove the unused import:
-import os🧰 Tools
🪛 Ruff (0.8.2)
2-2:
osimported but unusedRemove unused import:
os(F401)
43-48: Enhance error handling in PosthogAnalyticsService.The current error handling catches all exceptions and only logs a warning. Consider:
- Logging the event details that failed.
- Using a more specific exception type.
Apply this diff to improve error handling:
def capture_event(self, user_id: str, event_name: str, properties: dict): try: self.posthog.capture(user_id, event=event_name, properties=properties) - except Exception as e: - logger.warning(f"Failed to send event: {e}") + except (ConnectionError, TimeoutError) as e: + logger.warning( + f"Failed to send event: {event_name} for user: {user_id} " + f"with properties: {properties}. Error: {e}" + )app/modules/auth/auth_router.py (1)
80-88: Simplify the uid string formatting.The f-string formatting is unnecessary since uid is already a string.
- f"{uid}", + uid,app/modules/conversations/conversations_router.py (1)
40-174: Refactor dependency injection to avoid function calls in argument defaults.The current implementation triggers B008 warnings for using
Dependsfunction calls in argument defaults. While this works, it's recommended to perform the function calls within the function body.Apply this pattern to refactor the dependency injection:
@staticmethod @router.post("/conversations/", response_model=CreateConversationResponse) async def create_conversation( conversation: CreateConversationRequest, db: Session, analytics_service: AnalyticsService, user: dict, ): """ Example refactored method showing the pattern to apply to all methods. Dependencies are injected in the function body. """ db = Depends(get_db)() analytics_service = Depends(get_analytics_service)() user = Depends(AuthService.check_auth)() user_id = user["user_id"] user_email = user["email"] controller = ConversationController(db, user_id, user_email, analytics_service) return await controller.create_conversation(conversation)🧰 Tools
🪛 Ruff (0.8.2)
40-40: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
41-41: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
55-55: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
56-56: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
57-57: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
73-73: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
74-74: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
75-75: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
88-88: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
89-89: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
90-90: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
122-122: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
123-123: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
124-124: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
145-145: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
146-146: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
147-147: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
158-158: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
159-159: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
160-160: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
172-172: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
173-173: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
174-174: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
app/modules/key_management/secret_manager.py (1)
66-66: Address B008 warnings for FastAPI dependency injection.The use of
Depends()in parameter defaults is discouraged as it can lead to unexpected behavior. Instead, declare dependencies at the router level or move them inside the function.Consider refactoring the dependency injection pattern:
-def create_secret( - request: CreateSecretRequest, - user=Depends(AuthService.check_auth), - analytics_service: AnalyticsService = Depends(get_analytics_service), - db: Session = Depends(get_db), -): +def create_secret( + request: CreateSecretRequest, + user: dict, + analytics_service: AnalyticsService, + db: Session, +):And declare dependencies at the router level:
@router.post("/secrets") async def create_secret_endpoint( request: CreateSecretRequest, user: dict = Depends(AuthService.check_auth), analytics_service: AnalyticsService = Depends(get_analytics_service), db: Session = Depends(get_db), ): return SecretManager.create_secret(request, user, analytics_service, db)Also applies to: 189-189, 261-261, 282-282
🧰 Tools
🪛 Ruff (0.8.2)
66-66: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
app/modules/conversations/conversation/conversation_service.py (1)
151-157: Improve code formatting for better readability.The conditional check could be formatted more consistently.
Consider this formatting:
- if ( - state["agent_id"] in agent_list - and agent_list[state["agent_id"]] != "SYSTEM" - ): + if state["agent_id"] in agent_list and agent_list[state["agent_id"]] != "SYSTEM":
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
app/api/router.py(4 hunks)app/celery/tasks/parsing_tasks.py(2 hunks)app/core/dependencies.py(1 hunks)app/main.py(2 hunks)app/modules/auth/auth_router.py(3 hunks)app/modules/conversations/conversation/conversation_controller.py(1 hunks)app/modules/conversations/conversation/conversation_service.py(12 hunks)app/modules/conversations/conversations_router.py(9 hunks)app/modules/intelligence/provider/provider_router.py(2 hunks)app/modules/intelligence/provider/provider_service.py(1 hunks)app/modules/key_management/secret_manager.py(9 hunks)app/modules/parsing/graph_construction/parsing_controller.py(9 hunks)app/modules/parsing/graph_construction/parsing_router.py(2 hunks)app/modules/parsing/graph_construction/parsing_service.py(4 hunks)app/modules/utils/analytics_service.py(1 hunks)app/modules/utils/posthog_helper.py(0 hunks)
💤 Files with no reviewable changes (1)
- app/modules/utils/posthog_helper.py
🧰 Additional context used
🪛 Ruff (0.8.2)
app/main.py
35-35: app.core.dependencies.init_analytics_service imported but unused
Remove unused import: app.core.dependencies.init_analytics_service
(F401)
app/modules/intelligence/provider/provider_router.py
32-32: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
33-33: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
app/modules/parsing/graph_construction/parsing_router.py
19-19: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
app/modules/auth/auth_router.py
46-46: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
47-47: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
app/api/router.py
59-59: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
79-79: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
80-80: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
81-81: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
102-102: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
103-103: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
app/modules/utils/analytics_service.py
2-2: os imported but unused
Remove unused import: os
(F401)
app/modules/key_management/secret_manager.py
66-66: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
189-189: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
261-261: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
262-262: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
282-282: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
app/modules/conversations/conversations_router.py
40-40: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
41-41: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
56-56: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
57-57: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
74-74: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
75-75: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
88-88: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
123-123: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
124-124: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
146-146: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
147-147: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
158-158: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
159-159: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
160-160: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
173-173: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
174-174: Do not perform function call Depends in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (18)
app/modules/intelligence/provider/provider_router.py (2)
11-11: LGTM!Clean import of analytics service dependencies.
29-41: LGTM!Good integration of analytics service to track provider changes. The event is captured after successful provider change and includes relevant provider information.
🧰 Tools
🪛 Ruff (0.8.2)
31-31: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
32-32: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
33-33: Do not perform function call
Dependsin argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
app/modules/auth/auth_router.py (1)
17-17: LGTM!Clean import of analytics service dependencies.
app/api/router.py (2)
23-23: LGTM!Clean import of analytics service dependencies.
72-73: LGTM!Good integration of analytics service across conversation and parsing endpoints. The service is properly passed to respective controllers.
Also applies to: 83-85, 110-112
app/modules/conversations/conversation/conversation_controller.py (2)
24-24: LGTM!Clean import of analytics service dependencies.
28-41: LGTM!Good integration of analytics service in the controller. The service is properly stored and passed to the conversation service.
app/main.py (1)
42-46: LGTM!The
lifespancontext manager is correctly implemented usingasynccontextmanagerto initialize app state.app/modules/conversations/conversations_router.py (1)
29-29: LGTM!The import of
AnalyticsServiceandget_analytics_serviceis correctly added.app/modules/parsing/graph_construction/parsing_controller.py (2)
168-176: LGTM!The analytics event capture is correctly implemented with consistent event name and payload structure.
237-245: LGTM!The analytics event capture is correctly implemented with consistent event name and payload structure.
app/modules/intelligence/provider/provider_service.py (1)
80-80: LGTM!The return value enhancement improves the API response by including the provider name.
app/modules/key_management/secret_manager.py (1)
99-103: LGTM! Analytics integration looks good.The analytics service integration is consistent across all operations, capturing relevant events with appropriate metadata.
Also applies to: 209-213, 249-253, 267-274, 292-294
app/modules/parsing/graph_construction/parsing_service.py (2)
36-37: LGTM! Constructor changes look good.The analytics service is properly injected through the constructor and stored as an instance variable.
Also applies to: 43-43
189-193: LGTM! Analytics integration is consistent.The analytics service is used appropriately to track project status events with relevant metadata.
Also applies to: 205-209, 217-221
app/modules/conversations/conversation/conversation_service.py (3)
281-282: LGTM! Constructor changes look good.The analytics service is properly injected through the constructor and stored as an instance variable.
Also applies to: 291-291
294-316: LGTM! Create method changes are consistent.The create method properly handles the analytics service parameter and passes it to the constructor.
422-430: LGTM! Analytics integration is consistent.The analytics service is used appropriately to track conversation events with relevant metadata.
Also applies to: 479-483, 610-614, 767-771
| @@ -14,6 +16,7 @@ | |||
| async def parse_directory( | |||
| repo_details: ParsingRequest, | |||
| db: Session = Depends(get_db), | |||
| analytics_service: AnalyticsService = Depends(get_analytics_service), | |||
| user=Depends(AuthService.check_auth), | |||
| ): | |||
| return await ParsingController.parse_directory(repo_details, db, user) | |||
There was a problem hiding this comment.
Pass analytics_service to ParsingController.parse_directory.
The analytics_service parameter is added but not passed to the controller method.
Apply this diff to fix the issue:
- return await ParsingController.parse_directory(repo_details, db, user)
+ return await ParsingController.parse_directory(repo_details, db, analytics_service, user)📝 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.
| return await ParsingController.parse_directory(repo_details, db, user) | |
| return await ParsingController.parse_directory(repo_details, db, analytics_service, user) |
app/celery/tasks/parsing_tasks.py
Outdated
| @@ -42,7 +43,8 @@ def process_parsing( | |||
| ) -> None: | |||
| logger.info(f"Task received: Starting parsing process for project {project_id}") | |||
| try: | |||
| parsing_service = ParsingService(self.db, user_id) | |||
| analytics_service = get_analytics_service() | |||
There was a problem hiding this comment.
Fix analytics service initialization in Celery task.
The get_analytics_service function requires a FastAPI request object, but it's being called in a Celery task where there is no request context. This will cause a runtime error.
Consider these solutions:
- Initialize analytics service directly:
- analytics_service = get_analytics_service()
+ analytics_service = init_analytics_service()- Or better, initialize it once at task startup and store in task instance:
class BaseTask(Task):
_db = None
+ _analytics_service = None
@property
def db(self):
if self._db is None:
self._db = SessionLocal()
return self._db
+ @property
+ def analytics_service(self):
+ if self._analytics_service is None:
+ self._analytics_service = init_analytics_service()
+ return self._analytics_service
+
def after_return(self, *args, **kwargs):
if self._db is not None:
self._db.close()
self._db = None
+ self._analytics_service = None
@celery_app.task(...)
def process_parsing(...):
try:
- analytics_service = get_analytics_service()
+ analytics_service = self.analytics_service📝 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.
| analytics_service = get_analytics_service() | |
| class BaseTask(Task): | |
| _db = None | |
| _analytics_service = None | |
| @property | |
| def db(self): | |
| if self._db is None: | |
| self._db = SessionLocal() | |
| return self._db | |
| @property | |
| def analytics_service(self): | |
| if self._analytics_service is None: | |
| self._analytics_service = init_analytics_service() | |
| return self._analytics_service | |
| def after_return(self, *args, **kwargs): | |
| if self._db is not None: | |
| self._db.close() | |
| self._db = None | |
| self._analytics_service = None | |
| @celery_app.task(...) | |
| def process_parsing(...): | |
| try: | |
| analytics_service = self.analytics_service | |
| # ... rest of the task implementation |
| from contextlib import asynccontextmanager | ||
| from app.core.dependencies import init_state, init_analytics_service |
There was a problem hiding this comment.
Remove unused import.
The init_analytics_service import is not used in this file.
Apply this diff to remove the unused import:
-from app.core.dependencies import init_state, init_analytics_service
+from app.core.dependencies import init_state📝 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.
| from contextlib import asynccontextmanager | |
| from app.core.dependencies import init_state, init_analytics_service | |
| from contextlib import asynccontextmanager | |
| from app.core.dependencies import init_state |
🧰 Tools
🪛 Ruff (0.8.2)
35-35: app.core.dependencies.init_analytics_service imported but unused
Remove unused import: app.core.dependencies.init_analytics_service
(F401)
|
Summary by CodeRabbit
New Features
Bug Fixes
Chores
PostHogClientto simplify the codebase and improve maintainability.