feat: remove useless log #3028
feat: remove useless log #3028
Conversation
📝 WalkthroughWalkthroughAdded an IgnoreApiBuildFilter logging.Filter in two Django settings modules, registered it in each LOGGING["filters"], and applied it to the console handler and to the file handler (when enabled) to suppress 404 request_finished logs for the /api/build endpoint. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ 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). (9)
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 |
eric-intuitem
left a comment
eric-intuitem
left a comment
There was a problem hiding this comment.
This removes all logs at info and warning level. What we want is to avoid 1 specific log.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
enterprise/backend/enterprise_core/settings.py (1)
41-50: Consider extracting filter to shared module to eliminate duplication.The
IgnoreApiBuildFilterclass is duplicated betweenbackend/ciso_assistant/settings.pyandenterprise/backend/enterprise_core/settings.py. While settings files are often self-contained, extracting this filter to a shared logging utility module would improve maintainability.The same concerns from the community edition apply here:
- Verify that the
requestattribute contains a string path (not an HttpRequest object)- The substring match
"/api/build" in requestcould match unintended pathsExample refactor:
Create
backend/ciso_assistant/logging_utils.py:import logging class IgnoreApiBuildFilter(logging.Filter): """Filter out request_finished logs for /api/build endpoint with 404 status.""" def filter(self, record): request = getattr(record, "request", None) code = getattr(record, "code", None) if code == 404 and request and request.endswith("/api/build"): return False return TrueThen import in both settings files:
from ciso_assistant.logging_utils import IgnoreApiBuildFilter
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/ciso_assistant/settings.py(2 hunks)enterprise/backend/enterprise_core/settings.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
enterprise/backend/enterprise_core/settings.py (1)
backend/ciso_assistant/settings.py (2)
IgnoreApiBuildFilter(44-52)filter(47-52)
backend/ciso_assistant/settings.py (1)
enterprise/backend/enterprise_core/settings.py (2)
IgnoreApiBuildFilter(41-49)filter(44-49)
⏰ 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). (7)
- GitHub Check: ruff-community (3.12)
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (python)
🔇 Additional comments (4)
backend/ciso_assistant/settings.py (2)
68-77: LGTM - Filter registration and application is correct.The filter is properly registered and applied to the console handler using Django's logging configuration format.
90-97: Verify if file handler should also filter /api/build logs.The
ignore_api_buildfilter is only applied to the console handler. WhenLOG_OUTFILEis set, the file handler will still log 404 responses for/api/build. Confirm whether this is intentional (keeping complete audit logs in files) or if the filter should also be applied to the file handler.enterprise/backend/enterprise_core/settings.py (2)
65-74: LGTM - Filter configuration matches community edition.The filter registration and application is correctly configured, consistent with the implementation in
backend/ciso_assistant/settings.py.
82-89: Same file handler concern as community edition.As with
backend/ciso_assistant/settings.py, the file handler does not include theignore_api_buildfilter. Verify that this behavior is consistent and intentional across both the community and enterprise editions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/ciso_assistant/settings.py (1)
44-56: Note: Inconsistency with enterprise version.The enterprise version at
enterprise/backend/enterprise_core/settings.py(lines 40-48) still uses the old substring matching approach ("/api/build" in request) that was fixed here. Consider applying the sameendswith()fix to the enterprise version to avoid false positives.Would you like me to open an issue to track updating the enterprise version with the same fix?
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/ciso_assistant/settings.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/ciso_assistant/settings.py (1)
enterprise/backend/enterprise_core/settings.py (2)
IgnoreApiBuildFilter(41-49)filter(44-49)
⏰ 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). (9)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: build (3.12)
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
backend/ciso_assistant/settings.py (2)
72-76: LGTM!The filter is correctly registered in the logging configuration.
44-56: Theendswith()approach won't catch requests with query parameters, while the enterprise version's substring matching would.The current filter matches
/api/buildand/api/build/but will miss requests like/api/build?version=1. While this is unlikely in practice (since the endpoint doesn't exist), the enterprise version uses substring matching ("/api/build" in request), which handles this case. Consider whether to align with the enterprise approach or document why the narrower match is sufficient for the community version.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/ciso_assistant/settings.py(3 hunks)enterprise/backend/enterprise_core/settings.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/ciso_assistant/settings.py (1)
enterprise/backend/enterprise_core/settings.py (2)
IgnoreApiBuildFilter(41-49)filter(44-49)
enterprise/backend/enterprise_core/settings.py (1)
backend/ciso_assistant/settings.py (2)
IgnoreApiBuildFilter(44-56)filter(47-56)
⏰ 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). (9)
- GitHub Check: startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: build_community_frontend
- GitHub Check: build_enterprise_frontend
- GitHub Check: test (3.12)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
backend/ciso_assistant/settings.py (1)
44-56: LGTM! Filter implementation is correct.The filter correctly uses
endswith()checks to precisely match the/api/buildendpoint, avoiding false positives with similar paths. The filter is properly registered and applied to both console and file handlers.
nas-tabchiche
left a comment
nas-tabchiche
left a comment
There was a problem hiding this comment.
LGTM. To merge once CI is green
Added config to silence the django-structlog’s middleware request
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.