fix: log level not reconfigurable at runtime Fix #58 #63
Conversation
WalkthroughAdds an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related issues
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/qdrant-loader-core/src/qdrant_loader_core/logging.py (1)
1-435: Module size exceeds configured limit (435 lines > 400) and breaks the pipelineThe test/coverage pipeline is currently failing with:
core module
logging.pyhas 435 lines > 400The new
reconfigureimplementation pushed this file over the module-size threshold. To get the checks passing again, you’ll need to reduce the line count here, e.g.:
- Extract helper classes and functions such as
QdrantVersionFilter,ApplicationFilter,RedactionFilter,CleanFormatter, and perhaps_redact_processorinto a dedicated module (e.g.,logging_filters.py) and import them here, or- Factor some of the duplicated “choose renderer + configure structlog” logic into a shared private helper in another module, if that meaningfully reduces lines.
Without trimming this file below the limit (<= 400 lines) the CI will keep failing.
🧹 Nitpick comments (2)
packages/qdrant-loader/src/qdrant_loader/cli/commands/config_cmd.py (1)
23-27: Propagatinglog_levelinto reconfigure looks correctPassing
level=log_levelhere cleanly fixes the “re-invocation ignores CLI log level” bug while preserving the existing fallback path for older cores. Also consider updating the CLI/docs to clarify that--log-levelis now honored even when logging was previously initialized.packages/qdrant-loader/src/qdrant_loader/cli/commands/init_cmd.py (1)
42-56: Consider also passinglevelin the firstreconfigurefor consistencyThe new
LoggingConfig.reconfigure(file=log_file, level=log_level)at line 91 ensures the workspace-aware logger respects the CLI log level, which is great.However, in the earlier block:
if getattr(LoggingConfig, "reconfigure", None): if getattr(LoggingConfig, "_initialized", False): LoggingConfig.reconfigure(file="qdrant-loader.log") # no level here else: LoggingConfig.setup(level=log_level, format="console", file="qdrant-loader.log")if logging was already initialized before this command, logs emitted between
logger = LoggingConfig.get_logger(__name__)and the later reconfigure may still use the old level. For full consistency with the other commands and with the second reconfigure call, you may want:- if getattr(LoggingConfig, "_initialized", False): # type: ignore[attr-defined] - LoggingConfig.reconfigure(file="qdrant-loader.log") # type: ignore[attr-defined] + if getattr(LoggingConfig, "_initialized", False): # type: ignore[attr-defined] + LoggingConfig.reconfigure(file="qdrant-loader.log", level=log_level) # type: ignore[attr-defined]Also applies to: 84-92
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/qdrant-loader-core/src/qdrant_loader_core/logging.py(2 hunks)packages/qdrant-loader/src/qdrant_loader/cli/commands/config.py(1 hunks)packages/qdrant-loader/src/qdrant_loader/cli/commands/config_cmd.py(1 hunks)packages/qdrant-loader/src/qdrant_loader/cli/commands/ingest_cmd.py(1 hunks)packages/qdrant-loader/src/qdrant_loader/cli/commands/init_cmd.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/back-end-dev.mdc)
**/*.py: Use Python 3.x for back-end implementation and code examples
When building LLM/AI workflows, prefer and follow best practices of PydanticAI or LangChain
Use Qdrant as the vector database with performant indexing and query patterns
Design and implement RESTful endpoints with clear resource modeling and proper status codes
Implement authentication/authorization using OAuth2 or JWT; follow secure session and token handling
Files:
packages/qdrant-loader/src/qdrant_loader/cli/commands/config.pypackages/qdrant-loader-core/src/qdrant_loader_core/logging.pypackages/qdrant-loader/src/qdrant_loader/cli/commands/init_cmd.pypackages/qdrant-loader/src/qdrant_loader/cli/commands/ingest_cmd.pypackages/qdrant-loader/src/qdrant_loader/cli/commands/config_cmd.py
🧬 Code graph analysis (5)
packages/qdrant-loader/src/qdrant_loader/cli/commands/config.py (1)
packages/qdrant-loader-core/src/qdrant_loader_core/logging.py (2)
LoggingConfig(190-435)reconfigure(360-435)
packages/qdrant-loader-core/src/qdrant_loader_core/logging.py (1)
packages/qdrant-loader-mcp-server/src/qdrant_loader_mcp_server/utils/logging.py (1)
reconfigure(135-162)
packages/qdrant-loader/src/qdrant_loader/cli/commands/init_cmd.py (2)
packages/qdrant-loader-core/src/qdrant_loader_core/logging.py (2)
LoggingConfig(190-435)reconfigure(360-435)packages/qdrant-loader-mcp-server/src/qdrant_loader_mcp_server/utils/logging.py (2)
LoggingConfig(55-162)reconfigure(135-162)
packages/qdrant-loader/src/qdrant_loader/cli/commands/ingest_cmd.py (2)
packages/qdrant-loader-core/src/qdrant_loader_core/logging.py (1)
reconfigure(360-435)packages/qdrant-loader-mcp-server/src/qdrant_loader_mcp_server/utils/logging.py (1)
reconfigure(135-162)
packages/qdrant-loader/src/qdrant_loader/cli/commands/config_cmd.py (2)
packages/qdrant-loader-core/src/qdrant_loader_core/logging.py (2)
LoggingConfig(190-435)reconfigure(360-435)packages/qdrant-loader-mcp-server/src/qdrant_loader_mcp_server/utils/logging.py (2)
LoggingConfig(55-162)reconfigure(135-162)
🪛 GitHub Actions: Test and Coverage
packages/qdrant-loader-core/src/qdrant_loader_core/logging.py
[error] 1-435: Module size check failed: core module 'logging.py' has 435 lines > 400. Command failed: pytest tests/ --cov=src --cov-report=xml:../../coverage-core.xml --cov-report=html:../../htmlcov-core -v
🔇 Additional comments (3)
packages/qdrant-loader/src/qdrant_loader/cli/commands/ingest_cmd.py (1)
52-57: Ingest now correctly reuses logging with updated levelUsing
LoggingConfig.reconfigure(file=log_file, level=log_level)in the already-initialized branch aligns with the new core API and ensures--log-levelis respected on re-run without disturbing the legacy fallback path.packages/qdrant-loader/src/qdrant_loader/cli/commands/config.py (1)
39-55: Config helper now correctly reconfigures with the requested log levelAdding
level=log_levelto theLoggingConfig.reconfigurecall in the initialized path makes this helper consistent with the other CLI commands and solves the “log level ignored after prior setup” issue while keeping the compatibility branch intact.packages/qdrant-loader-core/src/qdrant_loader_core/logging.py (1)
360-409:reconfigure(file=..., level=...)implementation looks sound for CLI useThe updated
LoggingConfig.reconfigure:
- Accepts an optional
leveland correctly maps it tonumeric_level, updating the root logger and structlog wrapper while preserving existing processors and format selection from_current_config.- Raises a clear
ValueErrorfor invalid level values, mirroring the behavior ofsetup().- Keeps the existing file-handler replacement semantics intact and updates
_current_configconsistently with the new(level, fmt, file, clean_output, suppress_qdrant_warnings, disable_console)tuple shape.Given current call sites always pass both
fileandlevel, this should behave as intended and fix the runtime log-level reconfiguration bug without breaking older call sites that pass onlyfile.If you want extra safety, you could add a small regression test that:
- Calls
LoggingConfig.setup(level="INFO", ...),- Emits a DEBUG log (and asserts it’s filtered),
- Calls
LoggingConfig.reconfigure(level="DEBUG", file=...),- Emits a DEBUG log and asserts it’s now visible, using structlog’s configured wrapper.
Also applies to: 431-435
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/qdrant-loader-core/src/qdrant_loader_core/logging.py (1)
1-435: Pipeline failure: File exceeds 400-line limit (435 lines).The pipeline reports this module exceeds the size limit. Consider extracting the
RedactionFilterclass and_redact_processorfunction into a separateredaction.pymodule to reduce the file size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/qdrant-loader-core/src/qdrant_loader_core/logging.py(2 hunks)packages/qdrant-loader/src/qdrant_loader/cli/commands/init_cmd.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/qdrant-loader/src/qdrant_loader/cli/commands/init_cmd.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.cursor/rules/back-end-dev.mdc)
**/*.py: Use Python 3.x for back-end implementation and code examples
When building LLM/AI workflows, prefer and follow best practices of PydanticAI or LangChain
Use Qdrant as the vector database with performant indexing and query patterns
Design and implement RESTful endpoints with clear resource modeling and proper status codes
Implement authentication/authorization using OAuth2 or JWT; follow secure session and token handling
Files:
packages/qdrant-loader-core/src/qdrant_loader_core/logging.py
🧬 Code graph analysis (1)
packages/qdrant-loader-core/src/qdrant_loader_core/logging.py (1)
packages/qdrant-loader-mcp-server/src/qdrant_loader_mcp_server/utils/logging.py (1)
reconfigure(135-162)
🪛 GitHub Actions: Test and Coverage
packages/qdrant-loader-core/src/qdrant_loader_core/logging.py
[error] 1-435: Core module 'logging.py' exceeds size limit: 435 lines > 400.
🔇 Additional comments (2)
packages/qdrant-loader-core/src/qdrant_loader_core/logging.py (2)
360-369: LGTM on method signature and docstring.The optional
levelparameter with clear documentation is a clean approach to fix the runtime reconfiguration issue.
431-435: LGTM on config tuple update.The unpacking here correctly handles all 6 tuple elements, and the new level is properly persisted using
level.upper() if level is not None else old_level.
| if level is not None: | ||
| try: | ||
| numeric_level = getattr(logging, level.upper()) | ||
| root_logger.setLevel(numeric_level) | ||
|
|
||
| # Update structlog wrapper to use new level | ||
| if cls._current_config is not None: | ||
| _, fmt, clean_output, suppress_qdrant_warnings, disable_console = cls._current_config | ||
|
|
||
| # Choose timestamp format and final renderer | ||
| if clean_output and fmt == "console": | ||
| ts_fmt = "%H:%M:%S" | ||
| final_renderer = structlog.dev.ConsoleRenderer(colors=True) | ||
| else: | ||
| ts_fmt = "iso" | ||
| final_renderer = ( | ||
| structlog.processors.JSONRenderer() | ||
| if fmt == "json" | ||
| else structlog.dev.ConsoleRenderer(colors=True) | ||
| ) | ||
|
|
||
| # Reconfigure structlog with new level | ||
| structlog.configure( | ||
| processors=[ | ||
| structlog.stdlib.filter_by_level, | ||
| structlog.stdlib.add_logger_name, | ||
| structlog.stdlib.add_log_level, | ||
| structlog.processors.TimeStamper(fmt=ts_fmt), | ||
| _redact_processor, | ||
| final_renderer, | ||
| ], | ||
| wrapper_class=structlog.make_filtering_bound_logger(numeric_level), | ||
| logger_factory=LoggerFactory(), | ||
| cache_logger_on_first_use=False, | ||
| ) | ||
| except AttributeError: | ||
| raise ValueError(f"Invalid log level: {level}") from None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's examine the logging.py file structure and find the unpacking bug context
wc -l packages/qdrant-loader-core/src/qdrant_loader_core/logging.pyRepository: martin-papy/qdrant-loader
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Read the relevant section of the file including context before line 373
sed -n '350,420p' packages/qdrant-loader-core/src/qdrant_loader_core/logging.pyRepository: martin-papy/qdrant-loader
Length of output: 3106
🏁 Script executed:
#!/bin/bash
# Search for where _current_config is set to understand the tuple structure
rg -n "_current_config\s*=" packages/qdrant-loader-core/src/qdrant_loader_core/logging.py -B2 -A2Repository: martin-papy/qdrant-loader
Length of output: 653
🏁 Script executed:
#!/bin/bash
# Search for reconfigure calls and level parameter passing
rg -n "reconfigure\(" packages/qdrant-loader-core/src/qdrant_loader_core/logging.py -C3Repository: martin-papy/qdrant-loader
Length of output: 405
🏁 Script executed:
#!/bin/bash
# Let's get the exact unpacking at line 382 and surrounding context
sed -n '375,390p' packages/qdrant-loader-core/src/qdrant_loader_core/logging.py | cat -nRepository: martin-papy/qdrant-loader
Length of output: 984
🏁 Script executed:
#!/bin/bash
# Check git history for recent changes to this unpacking
cd packages/qdrant-loader-core/src/qdrant_loader_core && \
git log --oneline -20 logging.pyRepository: martin-papy/qdrant-loader
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Look at git diff to see what changed in the unpacking
cd packages/qdrant-loader-core/src/qdrant_loader_core && \
git diff HEAD~5 logging.py | grep -A10 -B10 "_current_config" | head -80Repository: martin-papy/qdrant-loader
Length of output: 255
🏁 Script executed:
#!/bin/bash
# Check the specific git commit that supposedly fixed the issue
cd packages/qdrant-loader-core/src/qdrant_loader_core && \
git show 08369cb --statRepository: martin-papy/qdrant-loader
Length of output: 48060
🏁 Script executed:
#!/bin/bash
# Look at the changes in that commit
cd packages/qdrant-loader-core/src/qdrant_loader_core && \
git show 08369cb | head -200Repository: martin-papy/qdrant-loader
Length of output: 13428
🏁 Script executed:
#!/bin/bash
# Verify current state - check if unpacking matches tuple size
cd packages/qdrant-loader-core/src/qdrant_loader_core && \
python3 << 'EOF'
# Count elements in tuple structure
lines_351 = "current_tuple = (level, fmt, file, clean_output, suppress_qdrant_warnings, disable_console)"
lines_435 = "old_level, fmt, file, clean_output, suppress_qdrant_warnings, disable_console = cls._current_config"
lines_382 = "_, fmt, clean_output, suppress_qdrant_warnings, disable_console = cls._current_config"
tuple_elements_351 = 6 # level, fmt, file, clean_output, suppress_qdrant_warnings, disable_console
unpack_elements_382 = 5 # _, fmt, clean_output, suppress_qdrant_warnings, disable_console
unpack_elements_435 = 6 # old_level, fmt, file, clean_output, suppress_qdrant_warnings, disable_console
print(f"Tuple elements (line 351): {tuple_elements_351}")
print(f"Unpack elements (line 382): {unpack_elements_382}")
print(f"Unpack elements (line 435): {unpack_elements_435}")
print(f"\nLine 382 MISMATCH: {tuple_elements_351} != {unpack_elements_382}")
EOFRepository: martin-papy/qdrant-loader
Length of output: 155
🏁 Script executed:
#!/bin/bash
# Let's verify the exact line and surrounding context one more time to be absolutely certain
sed -n '375,390p' packages/qdrant-loader-core/src/qdrant_loader_core/logging.py | cat -nRepository: martin-papy/qdrant-loader
Length of output: 984
The tuple unpacking at line 382 is incomplete and will cause a ValueError at runtime.
The _current_config tuple contains 6 elements: (level, fmt, file, clean_output, suppress_qdrant_warnings, disable_console). Line 382 attempts to unpack only 5 elements, omitting the file element:
_, fmt, clean_output, suppress_qdrant_warnings, disable_console = cls._current_configThis will fail with ValueError: not enough values to unpack (expected 6, got 5) when level is not None. Compare with line 435 in the same method, which correctly unpacks all 6 elements:
old_level, fmt, file, clean_output, suppress_qdrant_warnings, disable_console = cls._current_configRequired fix: Update line 382 to include file in the unpacking:
_, fmt, file, clean_output, suppress_qdrant_warnings, disable_console = cls._current_config🤖 Prompt for AI Agents
In packages/qdrant-loader-core/src/qdrant_loader_core/logging.py around lines
373 to 409, the tuple unpacking for cls._current_config on line ~382 omits the
`file` element causing a ValueError; update the unpacking to include `file` so
it reads: _, fmt, file, clean_output, suppress_qdrant_warnings, disable_console
= cls._current_config, preserving the rest of the reconfiguration logic.
| if cls._current_config is not None: | ||
| _, fmt, clean_output, suppress_qdrant_warnings, disable_console = cls._current_config | ||
|
|
||
| # Choose timestamp format and final renderer | ||
| if clean_output and fmt == "console": | ||
| ts_fmt = "%H:%M:%S" | ||
| final_renderer = structlog.dev.ConsoleRenderer(colors=True) | ||
| else: | ||
| ts_fmt = "iso" | ||
| final_renderer = ( | ||
| structlog.processors.JSONRenderer() | ||
| if fmt == "json" | ||
| else structlog.dev.ConsoleRenderer(colors=True) | ||
| ) |
There was a problem hiding this comment.
Critical: Tuple unpacking mismatch will cause ValueError at runtime.
The _current_config tuple has 6 elements (level, fmt, file, clean_output, suppress_qdrant_warnings, disable_console), but line 380 unpacks into only 5 variables. This will raise ValueError: too many values to unpack.
Additionally, even if Python didn't error, the variable bindings would be incorrect—clean_output would receive the file value, etc.
Apply this diff to fix the unpacking:
if cls._current_config is not None:
- _, fmt, clean_output, suppress_qdrant_warnings, disable_console = cls._current_config
+ _, fmt, _, clean_output, suppress_qdrant_warnings, disable_console = cls._current_config📝 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.
| if cls._current_config is not None: | |
| _, fmt, clean_output, suppress_qdrant_warnings, disable_console = cls._current_config | |
| # Choose timestamp format and final renderer | |
| if clean_output and fmt == "console": | |
| ts_fmt = "%H:%M:%S" | |
| final_renderer = structlog.dev.ConsoleRenderer(colors=True) | |
| else: | |
| ts_fmt = "iso" | |
| final_renderer = ( | |
| structlog.processors.JSONRenderer() | |
| if fmt == "json" | |
| else structlog.dev.ConsoleRenderer(colors=True) | |
| ) | |
| if cls._current_config is not None: | |
| _, fmt, _, clean_output, suppress_qdrant_warnings, disable_console = cls._current_config | |
| # Choose timestamp format and final renderer | |
| if clean_output and fmt == "console": | |
| ts_fmt = "%H:%M:%S" | |
| final_renderer = structlog.dev.ConsoleRenderer(colors=True) | |
| else: | |
| ts_fmt = "iso" | |
| final_renderer = ( | |
| structlog.processors.JSONRenderer() | |
| if fmt == "json" | |
| else structlog.dev.ConsoleRenderer(colors=True) | |
| ) |
🤖 Prompt for AI Agents
In packages/qdrant-loader-core/src/qdrant_loader_core/logging.py around lines
379 to 392, the code unpacks cls._current_config into only five variables
causing a ValueError and incorrect bindings; update the unpacking to six
variables matching the stored tuple (level, fmt, file, clean_output,
suppress_qdrant_warnings, disable_console) so subsequent uses of fmt and
clean_output refer to the correct values, and ensure any later references to the
file, suppress_qdrant_warnings, or disable_console variables use these newly
unpacked names.
Pull Request
Summary
The
--log-level DEBUGCLI option is ignored when logging has already been initializedType of change
Docs Impact (required for any code or docs changes)
docs/?python website/build.py+python website/check_links.py)Testing
Run the command below and observe the debug mode:
Checklist
pytest -v)Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.