[WEB-5071] chore: implement webhook logging to MongoDB and fallback to database #7896
Conversation
…7887) - Added a new function `save_webhook_log` to log webhook requests and responses to MongoDB, with a fallback to the database if the MongoDB save fails. - Updated the `webhook_send_task` to utilize the new logging function. - Modified the `get_webhook_logs_queryset` to order logs by creation date and adjusted the chunk size for iteration.
WalkthroughIntroduces save_webhook_log to persist webhook logs via MongoDB with DB fallback and updates webhook_send_task to use it. Adjusts webhook log cleanup iteration to explicit created_at ordering and reduces batch size from 500 to 100. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant WS as webhook_send_task
participant SL as save_webhook_log
participant MG as MongoConnection.webhook_logs
participant DB as WebhookLog (DB)
WS->>SL: save_webhook_log(webhook, req/resp, retry, event_type)
Note over SL: Build log payload
alt Mongo collection available
SL->>MG: insert_one(payload)
alt Insert OK
MG-->>SL: ack
SL-->>WS: return
else Insert fails
MG--x SL: error
SL->>DB: create(payload)
DB-->>SL: ack/fail
SL-->>WS: return
end
else No collection/connection
SL->>DB: create(payload)
DB-->>SL: ack/fail
SL-->>WS: return
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
There was a problem hiding this comment.
Pull Request Overview
This PR implements webhook logging to MongoDB with database fallback functionality. The changes introduce a new save_webhook_log function that attempts to save webhook logs to MongoDB first, and falls back to the existing database if MongoDB operations fail.
- Extracted webhook logging logic into a reusable
save_webhook_logfunction with MongoDB primary storage and database fallback - Updated webhook task to use the new logging function instead of direct database operations
- Modified webhook logs queryset to include ordering and adjusted chunk size
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| apps/api/plane/bgtasks/webhook_task.py | Added save_webhook_log function with MongoDB/database fallback and updated webhook task to use new logging |
| apps/api/plane/bgtasks/cleanup_task.py | Updated webhook logs queryset with ordering and modified chunk size |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| "workspace_id":str(webhook.workspace_id), | ||
| "webhook":str(webhook.id), | ||
| "event_type":str(event_type), | ||
| "request_method":str(request_method), | ||
| "request_headers":str(request_headers), | ||
| "request_body":str(request_body), | ||
| "response_status":str(response_status), | ||
| "response_headers":str(response_headers), | ||
| "response_body":str(response_body), | ||
| "retry_count":str(retry_count), |
There was a problem hiding this comment.
[nitpick] Inconsistent spacing after colons in dictionary keys. Some keys have spaces after colons while others don't. Standardize to either no spaces or consistent spacing throughout the dictionary.
| "workspace_id":str(webhook.workspace_id), | |
| "webhook":str(webhook.id), | |
| "event_type":str(event_type), | |
| "request_method":str(request_method), | |
| "request_headers":str(request_headers), | |
| "request_body":str(request_body), | |
| "response_status":str(response_status), | |
| "response_headers":str(response_headers), | |
| "response_body":str(response_body), | |
| "retry_count":str(retry_count), | |
| "workspace_id": str(webhook.workspace_id), | |
| "webhook": str(webhook.id), | |
| "event_type": str(event_type), | |
| "request_method": str(request_method), | |
| "request_headers": str(request_headers), | |
| "request_body": str(request_body), | |
| "response_status": str(response_status), | |
| "response_headers": str(response_headers), | |
| "response_body": str(response_body), | |
| "retry_count": str(retry_count), |
Copilot uses AI. Check for mistakes.
| "workspace_id":str(webhook.workspace_id), | ||
| "webhook":str(webhook.id), | ||
| "event_type":str(event_type), | ||
| "request_method":str(request_method), | ||
| "request_headers":str(request_headers), | ||
| "request_body":str(request_body), | ||
| "response_status":str(response_status), | ||
| "response_headers":str(response_headers), | ||
| "response_body":str(response_body), | ||
| "retry_count":str(retry_count), |
There was a problem hiding this comment.
Converting all values to strings may be unnecessary and could impact performance. Consider preserving native data types (int, dict, etc.) for MongoDB storage, as MongoDB can handle these types natively and it would allow for better querying and indexing.
Copilot uses AI. Check for mistakes.
| if not mongo_save_success: | ||
| try: | ||
| # insert the log data into the database | ||
| WebhookLog.objects.create(**log_data) |
There was a problem hiding this comment.
Using the same log_data dictionary for both MongoDB and database operations may cause issues if the field names or data types differ between the two storage systems. The database model might expect different field names or data types than what's prepared for MongoDB.
Copilot uses AI. Check for mistakes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/api/plane/bgtasks/cleanup_task.py (1)
413-414: Consider using a constant for the chunk size.The explicit ordering by
created_atis beneficial for deterministic processing and aligns well with the new centralized logging approach.However, the hardcoded chunk size of
100deviates from the pattern used in other queryset functions (which useBATCH_SIZE). If this smaller chunk size is intentional for webhook logs (e.g., due to larger payloads), consider introducing a dedicated constant likeWEBHOOK_BATCH_SIZEfor maintainability.Apply this diff if you'd like to introduce a dedicated constant:
+WEBHOOK_BATCH_SIZE = 100 + def get_webhook_logs_queryset(): """Get email logs older than cutoff days.""" cutoff_days = int(os.environ.get("HARD_DELETE_AFTER_DAYS", 30)) cutoff_time = timezone.now() - timedelta(days=cutoff_days) logger.info(f"Webhook logs cutoff time: {cutoff_time}") return ( WebhookLog.all_objects.filter(created_at__lte=cutoff_time) .values( "id", "created_at", "workspace_id", "webhook", "event_type", # Request "request_method", "request_headers", "request_body", # Response "response_status", "response_body", "response_headers", "retry_count", ) .order_by("created_at") - .iterator(chunk_size=100) + .iterator(chunk_size=WEBHOOK_BATCH_SIZE) )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/api/plane/bgtasks/cleanup_task.py(1 hunks)apps/api/plane/bgtasks/webhook_task.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
apps/api/plane/bgtasks/webhook_task.py (2)
apps/api/plane/settings/mongo.py (2)
MongoConnection(18-122)get_collection(91-109)apps/api/plane/db/models/webhook.py (2)
Webhook(30-57)WebhookLog(60-86)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Lint API
- GitHub Check: Cursor Bugbot
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
apps/api/plane/bgtasks/webhook_task.py (1)
51-51: LGTM!The import is necessary for MongoDB integration in the new logging function.
| def save_webhook_log( | ||
| webhook: Webhook, | ||
| request_method: str, | ||
| request_headers: str, | ||
| request_body: str, | ||
| response_status: str, | ||
| response_headers: str, | ||
| response_body: str, | ||
| retry_count: int, | ||
| event_type: str, | ||
| ) -> None: | ||
|
|
||
| # webhook_logs | ||
| mongo_collection = MongoConnection.get_collection("webhook_logs") | ||
|
|
||
| log_data = { | ||
| "workspace_id":str(webhook.workspace_id), | ||
| "webhook":str(webhook.id), | ||
| "event_type":str(event_type), | ||
| "request_method":str(request_method), | ||
| "request_headers":str(request_headers), | ||
| "request_body":str(request_body), | ||
| "response_status":str(response_status), | ||
| "response_headers":str(response_headers), | ||
| "response_body":str(response_body), | ||
| "retry_count":str(retry_count), | ||
| } | ||
|
|
||
| mongo_save_success = False | ||
| if mongo_collection is not None: | ||
| try: | ||
| # insert the log data into the mongo collection | ||
| mongo_collection.insert_one(log_data) | ||
| logger.info("Webhook log saved successfully to mongo") | ||
| mongo_save_success = True | ||
| except Exception as e: | ||
| log_exception(e) | ||
| logger.error(f"Failed to save webhook log: {e}") | ||
| mongo_save_success = False | ||
|
|
||
| # if the mongo save is not successful, save the log data into the database | ||
| if not mongo_save_success: | ||
| try: | ||
| # insert the log data into the database | ||
| WebhookLog.objects.create(**log_data) | ||
| logger.info("Webhook log saved successfully to database") | ||
| except Exception as e: | ||
| log_exception(e) | ||
| logger.error(f"Failed to save webhook log: {e}") | ||
|
|
There was a problem hiding this comment.
Critical: Type mismatch will cause database fallback to fail.
The log_data dictionary converts all values to strings (lines 106-115), including retry_count on line 115. However, the WebhookLog model (see apps/api/plane/db/models/webhook.py line 76) defines retry_count as a PositiveSmallIntegerField, which expects an integer, not a string.
When MongoDB persistence fails and the fallback attempts WebhookLog.objects.create(**log_data) on line 134, Django will raise a validation error due to the type mismatch, causing webhook logs to be lost entirely.
Apply this diff to fix the type issue:
log_data = {
- "workspace_id":str(webhook.workspace_id),
- "webhook":str(webhook.id),
- "event_type":str(event_type),
- "request_method":str(request_method),
- "request_headers":str(request_headers),
- "request_body":str(request_body),
- "response_status":str(response_status),
- "response_headers":str(response_headers),
- "response_body":str(response_body),
- "retry_count":str(retry_count),
+ "workspace_id": str(webhook.workspace_id),
+ "webhook": str(webhook.id),
+ "event_type": str(event_type),
+ "request_method": str(request_method),
+ "request_headers": str(request_headers),
+ "request_body": str(request_body),
+ "response_status": str(response_status),
+ "response_headers": str(response_headers),
+ "response_body": str(response_body),
+ "retry_count": retry_count,
}Additionally, for MongoDB storage, you may want to preserve native types. Consider creating two separate dictionaries:
# For MongoDB - preserve types and structure
mongo_log_data = {
"workspace_id": str(webhook.workspace_id),
"webhook": str(webhook.id),
"event_type": event_type,
"request_method": request_method,
"request_headers": request_headers,
"request_body": request_body,
"response_status": response_status,
"response_headers": response_headers,
"response_body": response_body,
"retry_count": retry_count,
}
# For database - ensure field compatibility
db_log_data = {
"workspace_id": webhook.workspace_id,
"webhook": webhook.id,
"event_type": str(event_type),
"request_method": str(request_method),
"request_headers": str(request_headers),
"request_body": str(request_body),
"response_status": str(response_status),
"response_headers": str(response_headers),
"response_body": str(response_body),
"retry_count": retry_count,
}| save_webhook_log( | ||
| webhook=webhook, | ||
| request_method=action, | ||
| request_headers=headers, | ||
| request_body=payload, | ||
| response_status=response.status_code, | ||
| response_headers=response.headers, | ||
| response_body=response.text, | ||
| retry_count=self.request.retries, | ||
| event_type=event, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Ensure proper serialization of complex data types.
The calls to save_webhook_log pass dictionaries and dict-like objects (e.g., headers at line 333 and 347, payload at line 334 and 348) which will be stringified using str() in the logging function. This may produce non-JSON-serializable representations like "{'key': 'value'}" instead of proper JSON.
Consider serializing these to JSON before passing to save_webhook_log:
# Log the webhook request
save_webhook_log(
webhook=webhook,
request_method=action,
- request_headers=headers,
- request_body=payload,
+ request_headers=json.dumps(headers),
+ request_body=json.dumps(payload),
response_status=response.status_code,
- response_headers=response.headers,
+ response_headers=json.dumps(dict(response.headers)),
response_body=response.text,
retry_count=self.request.retries,
event_type=event,
)And similarly for the error path:
# Log the failed webhook request
save_webhook_log(
webhook=webhook,
request_method=action,
- request_headers=headers,
- request_body=payload,
+ request_headers=json.dumps(headers),
+ request_body=json.dumps(payload),
response_status=500,
response_headers="",
response_body=str(e),
retry_count=self.request.retries,
event_type=event,
)Also applies to: 344-354
Description
save_webhook_logto log webhook requests and responses to MongoDB, with a fallback to the database if the MongoDB save fails.webhook_send_taskto utilize the new logging function.get_webhook_logs_querysetto order logs by creation date and adjusted the chunk size for iteration.Type of Change
Test Scenarios
References
WEB-5071
Summary by CodeRabbit