Add SUPPRESS_ACCESS_LOGS setting and logging configuration tests#10
Add SUPPRESS_ACCESS_LOGS setting and logging configuration tests#10
Conversation
- Introduced `SUPPRESS_ACCESS_LOGS` setting to control Uvicorn access logs. - Updated `configure_logging` function to accept the new setting. - Enhanced `QueueServerSettings` to include the new configuration option. - Added unit tests for logging configuration behavior based on the new setting.
📝 WalkthroughWalkthroughThe PR adds a SUPPRESS_ACCESS_LOGS setting (default true) to settings and uses a new fq_server.logging.configure_logging to optionally disable Uvicorn access logs; it updates docs, tests, and adds Qlty tooling configs and a Changes
Sequence Diagram(s)sequenceDiagram
participant ASGI as "ASGI entrypoint\n(asgi.py)"
participant Settings as "QueueServerSettings\n(fq_server/settings.py)"
participant LoggerUtil as "fq_server.logging\n(configure_logging)"
participant Uvicorn as "uvicorn.access\n(logger)"
ASGI->>Settings: load settings (including SUPPRESS_ACCESS_LOGS)
ASGI->>LoggerUtil: configure_logging(log_level, suppress_access_logs)
LoggerUtil->>LoggerUtil: resolve log level, maybe basicConfig
LoggerUtil->>Uvicorn: set disabled = suppress_access_logs
LoggerUtil->>ASGI: return (logging configured)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
Adds a new SUPPRESS_ACCESS_LOGS environment setting to disable Uvicorn access logs by default, wires it into the ASGI entrypoint’s logging setup, documents the new option, and introduces tests and Qlty configuration files.
Changes:
- Introduce
SUPPRESS_ACCESS_LOGS(suppress_access_logs) in settings and apply it inasgi.pylogging configuration. - Add unit tests covering settings parsing and access-log suppression behavior.
- Add Qlty code quality configuration files and update ignore patterns.
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
asgi.py |
Extends configure_logging to optionally disable uvicorn.access and wires it to settings at import time. |
fq_server/settings.py |
Adds suppress_access_logs setting with env alias and boolean parsing validator. |
tests/test_config_settings.py |
Adds coverage for default/override behavior of SUPPRESS_ACCESS_LOGS. |
tests/test_logging_config.py |
Adds tests for configure_logging toggling uvicorn.access.disabled. |
README.md |
Documents SUPPRESS_ACCESS_LOGS in the env var table. |
docs/configuration.rst |
Documents SUPPRESS_ACCESS_LOGS and its default. |
.qlty/qlty.toml |
Adds Qlty configuration (generated by qlty init). |
.qlty/configs/.hadolint.yaml |
Adds hadolint ignore configuration. |
.qlty/.gitignore |
Scopes what’s tracked inside .qlty/. |
.gitignore |
Adds .codex to ignored files. |
.codex |
Adds an empty .codex file (currently tracked). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import logging | ||
| import unittest | ||
|
|
||
| from asgi import configure_logging | ||
|
|
||
|
|
There was a problem hiding this comment.
Importing configure_logging from asgi triggers asgi.py module import side effects (QueueServerSettings.from_env(), setup_server(...), and app construction). That makes this unit test heavier than needed and can introduce coupling to external dependencies (e.g., Flowdacity Queue / Redis behavior) unrelated to logging configuration. Consider moving configure_logging into a side-effect-free module (e.g., fq_server/logging.py) and importing it from both asgi.py and this test, or otherwise isolating app/server initialization from imports used by unit tests.
| import logging | |
| import unittest | |
| from asgi import configure_logging | |
| import ast | |
| import logging | |
| import unittest | |
| from pathlib import Path | |
| def _load_configure_logging(): | |
| asgi_path = Path(__file__).resolve().parent.parent / "asgi.py" | |
| module_ast = ast.parse(asgi_path.read_text(encoding="utf-8"), filename=str(asgi_path)) | |
| for node in module_ast.body: | |
| if isinstance(node, ast.FunctionDef) and node.name == "configure_logging": | |
| isolated_module = ast.Module(body=[node], type_ignores=[]) | |
| ast.fix_missing_locations(isolated_module) | |
| namespace = {"logging": logging} | |
| exec(compile(isolated_module, filename=str(asgi_path), mode="exec"), namespace) | |
| return namespace["configure_logging"] | |
| raise AssertionError("configure_logging function not found in asgi.py") | |
| configure_logging = _load_configure_logging() |
| self.original_disabled = self.access_logger.disabled | ||
|
|
||
| def tearDown(self): | ||
| self.access_logger.disabled = self.original_disabled |
There was a problem hiding this comment.
configure_logging() can call logging.basicConfig(...) when the root logger has no handlers, which mutates global logging state for the entire test process. This test currently restores only uvicorn.access.disabled in tearDown, so the root logger handler/level changes may leak into other tests. Consider snapshotting and restoring the root logger’s handlers/level (and any logger state you change) in setUp/tearDown to keep tests isolated.
| self.original_disabled = self.access_logger.disabled | |
| def tearDown(self): | |
| self.access_logger.disabled = self.original_disabled | |
| self.original_disabled = self.access_logger.disabled | |
| self.root_logger = logging.getLogger() | |
| self.original_root_handlers = self.root_logger.handlers[:] | |
| self.original_root_level = self.root_logger.level | |
| def tearDown(self): | |
| self.access_logger.disabled = self.original_disabled | |
| self.root_logger.handlers[:] = self.original_root_handlers | |
| self.root_logger.setLevel(self.original_root_level) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_logging_config.py (1)
6-6: Consider isolatingconfigure_loggingfrom ASGI module side effects for unit tests.Line 6 imports from
asgi, which also initializes app startup objects at module import time. Extracting logging config into a side-effect-free module would keep this test focused and reduce incidental coupling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_logging_config.py` at line 6, The test imports configure_logging from the asgi module which triggers app startup side-effects; extract the logging setup into a side-effect-free module (e.g., a new module like logging_config with a single configure_logging function) and update tests to import configure_logging from that module instead of from asgi; ensure asgi imports the new logging_config.configure_logging where needed so runtime behavior is unchanged while unit tests can import the pure function without initializing app startup objects.tests/test_config_settings.py (1)
69-76: Consider adding one invalid-value test forSUPPRESS_ACCESS_LOGS.A small follow-up could assert rejection of values like
"yes"/"1"to fully cover the validator path for this new field.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_config_settings.py` around lines 69 - 76, Add a test that asserts invalid values for SUPPRESS_ACCESS_LOGS are rejected by the validator: create a new test (e.g., test_queue_server_settings_suppress_access_logs_invalid_value) that calls QueueServerSettings.from_env({"SUPPRESS_ACCESS_LOGS": "yes"}) and/or {"SUPPRESS_ACCESS_LOGS": "1"} and verifies it raises the expected ValidationError (or appropriate exception) rather than returning a boolean for suppress_access_logs; reference the existing tests test_queue_server_settings_suppress_access_logs_default and test_queue_server_settings_suppress_access_logs_override to mirror setup and assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_config_settings.py`:
- Around line 69-76: Add a test that asserts invalid values for
SUPPRESS_ACCESS_LOGS are rejected by the validator: create a new test (e.g.,
test_queue_server_settings_suppress_access_logs_invalid_value) that calls
QueueServerSettings.from_env({"SUPPRESS_ACCESS_LOGS": "yes"}) and/or
{"SUPPRESS_ACCESS_LOGS": "1"} and verifies it raises the expected
ValidationError (or appropriate exception) rather than returning a boolean for
suppress_access_logs; reference the existing tests
test_queue_server_settings_suppress_access_logs_default and
test_queue_server_settings_suppress_access_logs_override to mirror setup and
assertions.
In `@tests/test_logging_config.py`:
- Line 6: The test imports configure_logging from the asgi module which triggers
app startup side-effects; extract the logging setup into a side-effect-free
module (e.g., a new module like logging_config with a single configure_logging
function) and update tests to import configure_logging from that module instead
of from asgi; ensure asgi imports the new logging_config.configure_logging where
needed so runtime behavior is unchanged while unit tests can import the pure
function without initializing app startup objects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 020885cb-0719-4924-a838-60db3ceb591a
📒 Files selected for processing (11)
.codex.gitignore.qlty/.gitignore.qlty/configs/.hadolint.yaml.qlty/qlty.tomlREADME.mdasgi.pydocs/configuration.rstfq_server/settings.pytests/test_config_settings.pytests/test_logging_config.py
…s for improved clarity and maintainability
There was a problem hiding this comment.
🧹 Nitpick comments (2)
fq_server/logging.py (1)
6-10: Consider using theLogLevelNametype for stronger type safety.The function accepts
strbut only works correctly with the five standard log level names. The codebase already definesLogLevelName = Literal["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"]infq_server/settings.py. Using this type would provide better IDE support and catch invalid values at type-check time.💡 Proposed type hint improvement
# Copyright (c) 2025 Flowdacity Development Team. See LICENSE.txt for details. import logging +from typing import Literal + +LogLevelName = Literal["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"] def configure_logging( - log_level: str, + log_level: LogLevelName, suppress_access_logs: bool = True, ) -> None:Alternatively, import the type from settings:
from fq_server.settings import LogLevelName🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fq_server/logging.py` around lines 6 - 10, Change the configure_logging signature to accept the stronger type LogLevelName instead of str: import LogLevelName from fq_server.settings and update the parameter annotation in configure_logging to use that type (keeping suppress_access_logs and the body unchanged), so IDEs and type-checkers will enforce only the five allowed level names when calling configure_logging; ensure you update any callers if needed to satisfy the new type.tests/test_logging_config.py (1)
29-42: Tests cover the core functionality well.The three test cases validate:
- Access logger suppression when enabled
- Access logger re-enabled when suppression is disabled
- Logger level propagation to
fq_serverloggerConsider adding a test for the
basicConfigconditional behavior (root logger configuration when no handlers exist) for more complete coverage.💡 Optional: Additional test for basicConfig behavior
def test_configure_logging_adds_handler_when_none_exist(self): # Remove all root handlers to simulate fresh state for handler in list(self.root_logger.handlers): self.root_logger.removeHandler(handler) configure_logging("DEBUG", suppress_access_logs=False) self.assertGreater(len(self.root_logger.handlers), 0) self.assertEqual(self.root_logger.level, logging.DEBUG)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_logging_config.py` around lines 29 - 42, Add a test that verifies configure_logging invokes logging.basicConfig when there are no root handlers: remove all handlers from the root logger (use the existing self.root_logger), call configure_logging("DEBUG", suppress_access_logs=False), then assert that self.root_logger.handlers has length > 0 and that self.root_logger.level equals logging.DEBUG; reference the existing test helpers/fixtures (self.root_logger, configure_logging, fq_logger, access_logger) to keep consistent setup/teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@fq_server/logging.py`:
- Around line 6-10: Change the configure_logging signature to accept the
stronger type LogLevelName instead of str: import LogLevelName from
fq_server.settings and update the parameter annotation in configure_logging to
use that type (keeping suppress_access_logs and the body unchanged), so IDEs and
type-checkers will enforce only the five allowed level names when calling
configure_logging; ensure you update any callers if needed to satisfy the new
type.
In `@tests/test_logging_config.py`:
- Around line 29-42: Add a test that verifies configure_logging invokes
logging.basicConfig when there are no root handlers: remove all handlers from
the root logger (use the existing self.root_logger), call
configure_logging("DEBUG", suppress_access_logs=False), then assert that
self.root_logger.handlers has length > 0 and that self.root_logger.level equals
logging.DEBUG; reference the existing test helpers/fixtures (self.root_logger,
configure_logging, fq_logger, access_logger) to keep consistent setup/teardown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6842570e-005b-41e9-bdd8-631bb039f536
📒 Files selected for processing (3)
asgi.pyfq_server/logging.pytests/test_logging_config.py
This pull request introduces support for suppressing Uvicorn access logs via a new
SUPPRESS_ACCESS_LOGSconfiguration option, updates documentation to reflect this new setting, and adds related tests. Additionally, it adds Qlty code quality configuration files to the repository.Logging and configuration enhancements:
SUPPRESS_ACCESS_LOGSenvironment variable and correspondingsuppress_access_logsfield toQueueServerSettings, allowing users to suppress Uvicorn access logs by default. The logging configuration inasgi.pywas updated to respect this setting. [1] [2] [3] [4]README.mdanddocs/configuration.rstto describe the newSUPPRESS_ACCESS_LOGSoption, including its default value and behavior. [1] [2] [3]Testing improvements:
suppress_access_logssetting in both the settings loader and the logging configuration. [1] [2]Code quality tooling:
.qlty/.gitignore,.qlty/configs/.hadolint.yaml,.qlty/qlty.toml) to enable and configure code quality checks for the repository. [1] [2] [3]Summary by CodeRabbit
New Features
Documentation
Tests
Chores