feat: reduce http requests logs #1863
Conversation
WalkthroughA new function, Changes
Sequence Diagram(s)sequenceDiagram
participant Django
participant Logger
participant lower_request_log_level
Django->>Logger: Emit log event (e.g., request_started/request_finished)
Logger->>lower_request_log_level: Process event_dict
lower_request_log_level->>Logger: Modify log level to debug (if applicable), print event_dict
Logger-->>Django: Continue logging with updated event_dict
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 2
🧹 Nitpick comments (2)
backend/ciso_assistant/settings.py (2)
91-91: Consider making request debug logging optionalAdding this processor to the main structlog configuration means all request logs will be printed and downgraded to debug level in all environments.
If this is intended for temporary debugging, consider making it conditional:
structlog.stdlib.add_logger_name, structlog.stdlib.add_log_level, - lower_request_log_level, + # Only include the processor if DEBUG_REQUESTS is enabled + *([] if os.environ.get("DEBUG_REQUESTS", "False") != "True" else [lower_request_log_level]), structlog.stdlib.PositionalArgumentsFormatter(),Alternatively, you could implement this as middleware if it's intended for development use only.
43-91: Document the purpose of thelower_request_log_levelfunctionThe function lacks documentation explaining its purpose and when it should be used.
Add a docstring to clarify the function's intended use:
def lower_request_log_level(logger, method_name, event_dict): + """ + Intercepts request_started and request_finished events and lowers their log level to debug. + + This function helps debug request processing by ensuring Django's request logs are + visible even when the application's log level is set higher than DEBUG. + + Args: + logger: The logger instance + method_name: The logging method being called + event_dict: The structlog event dictionary + + Returns: + Modified event dictionary with adjusted log level for request events + """ if event_dict.get("event") in {"request_started", "request_finished"}: print(event_dict) event_dict["level"] = "debug" return event_dict
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/ciso_assistant/settings.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: ruff (3.12)
- GitHub Check: Analyze (javascript-typescript)
| def lower_request_log_level(logger, method_name, event_dict): | ||
| if event_dict.get("event") in {"request_started", "request_finished"}: | ||
| print(event_dict) | ||
| event_dict["level"] = "debug" | ||
| return event_dict |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify logging performance impact
The current implementation adds a print statement to every request, which could affect performance in high-traffic scenarios.
🏁 Script executed:
#!/bin/bash
# Check if we have any middleware that would generate request_started/request_finished events
# We're looking for the RequestMiddleware from django_structlog
echo "Checking for RequestMiddleware in the middleware configuration:"
# Check for RequestMiddleware in MIDDLEWARE
grep -n "RequestMiddleware" backend/ciso_assistant/settings.py
echo "Checking Django settings for log level:"
# Check the configured log level to understand the environment
grep -n "LOG_LEVEL" backend/ciso_assistant/settings.pyLength of output: 545
Performance Impact: Adjust Logging Behavior
The implementation currently uses a print statement in the lower_request_log_level function for request_started and request_finished events. With the django_structlog.middlewares.RequestMiddleware active, this hard-coded output may degrade performance under high traffic. Consider replacing the print(event_dict) with a logging call (e.g., logger.debug(event_dict)) that respects the LOG_LEVEL configuration to control output in production environments.
🛠️ Refactor suggestion
Reconsider using print() for request logging in production
The function lower_request_log_level prints entire event dictionaries to stdout, which could:
- Create excessive console output in high-traffic environments
- Potentially expose sensitive request information
- Impact performance when processing many requests
Consider making this debug behavior configurable with an environment variable and using the structured logger instead of print:
-def lower_request_log_level(logger, method_name, event_dict):
- if event_dict.get("event") in {"request_started", "request_finished"}:
- print(event_dict)
- event_dict["level"] = "debug"
- return event_dict
+def lower_request_log_level(logger, method_name, event_dict):
+ if event_dict.get("event") in {"request_started", "request_finished"}:
+ # Only modify log level when DEBUG_REQUESTS env var is enabled
+ if os.environ.get("DEBUG_REQUESTS", "False") == "True":
+ # Use existing logger instead of print
+ if LOG_LEVEL != "debug":
+ structlog.getLogger(__name__).debug(
+ "Forcing request log to debug level",
+ **event_dict
+ )
+ event_dict["level"] = "debug"
+ return event_dict📝 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.
| def lower_request_log_level(logger, method_name, event_dict): | |
| if event_dict.get("event") in {"request_started", "request_finished"}: | |
| print(event_dict) | |
| event_dict["level"] = "debug" | |
| return event_dict | |
| def lower_request_log_level(logger, method_name, event_dict): | |
| if event_dict.get("event") in {"request_started", "request_finished"}: | |
| # Only modify log level when DEBUG_REQUESTS env var is enabled | |
| if os.environ.get("DEBUG_REQUESTS", "False") == "True": | |
| # Use existing logger instead of print | |
| if LOG_LEVEL != "debug": | |
| structlog.getLogger(__name__).debug( | |
| "Forcing request log to debug level", | |
| **event_dict | |
| ) | |
| event_dict["level"] = "debug" | |
| return event_dict |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/ciso_assistant/settings.py (1)
43-47: Address the unresolved performance and security concerns from previous reviewsThe implementation still uses
print()statements and exposes full event dictionaries, which creates the same performance and security issues previously identified.Additionally, the function name suggests "lowering" log levels, but setting the level to "debug" may actually increase log verbosity if the current level is higher (e.g., "info" or "warning"). This contradicts the PR objective of reducing HTTP request logs.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 43-43: Missing function or method docstring
(C0116)
[warning] 43-43: Redefining name 'logger' from outer scope (line 104)
(W0621)
[warning] 43-43: Unused argument 'logger'
(W0613)
[warning] 43-43: Unused argument 'method_name'
(W0613)
🧹 Nitpick comments (1)
backend/ciso_assistant/settings.py (1)
43-43: Fix static analysis issuesThe function has several code quality issues flagged by static analysis:
-def lower_request_log_level(logger, method_name, event_dict): +def lower_request_log_level(logger, method_name, event_dict): + """ + Processor to modify log level for HTTP request events. + + Args: + logger: Logger instance (unused but required by structlog processor interface) + method_name: Method name (unused but required by structlog processor interface) + event_dict: Event dictionary containing log data + + Returns: + Modified event dictionary + """Consider prefixing unused parameters with underscore to indicate they're intentionally unused:
-def lower_request_log_level(logger, method_name, event_dict): +def lower_request_log_level(_logger, _method_name, event_dict):🧰 Tools
🪛 Pylint (3.3.7)
[convention] 43-43: Missing function or method docstring
(C0116)
[warning] 43-43: Redefining name 'logger' from outer scope (line 104)
(W0621)
[warning] 43-43: Unused argument 'logger'
(W0613)
[warning] 43-43: Unused argument 'method_name'
(W0613)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/ciso_assistant/settings.py(2 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/ciso_assistant/settings.py
[convention] 43-43: Missing function or method docstring
(C0116)
[warning] 43-43: Redefining name 'logger' from outer scope (line 104)
(W0621)
[warning] 43-43: Unused argument 'logger'
(W0613)
[warning] 43-43: Unused argument 'method_name'
(W0613)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
🔇 Additional comments (2)
backend/ciso_assistant/settings.py (2)
44-46: Clarify the intended behavior vs. PR objectiveThe current implementation forces request events to "debug" level and prints them unconditionally. This appears to increase log verbosity rather than reduce it as stated in the PR objective.
Please clarify:
- Is the goal to reduce log volume by moving requests to debug level (only visible when LOG_LEVEL="DEBUG")?
- Or is the goal to ensure request logs are always visible regardless of LOG_LEVEL?
If the goal is reduction, consider renaming the function to better reflect its purpose (e.g.,
demote_request_log_level).
91-91: Verify processor positioning in the chainThe function is correctly positioned after
add_log_leveland beforePositionalArgumentsFormatter, which allows it to modify the log level before final formatting.
Summary by CodeRabbit