fix(logs): hide default service field in dev console - #13624
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR introduces a structlog processor that removes the repetitive ChangesService field filtering in pretty console logs
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 9✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
27a16c8 to
c3c61de
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/backend/tests/unit/test_logger.py (1)
798-811: ⚡ Quick winTest name suggests DEV-specific behavior, but the service hiding is not DEV-dependent.
The test name
test_dev_pretty_console_hides_default_service_fieldand the explicitDEV=Truepatch (line 801) suggest that hiding the service field is DEV-mode specific. However, looking at the logger implementation (line 611), the_hide_default_service_for_pretty_consoleprocessor is added regardless of theDEVflag value. TheDEVflag only controls callsite parameter addition (lines 520-529).Consider either:
- Renaming the test to
test_pretty_console_hides_default_service_field(removing "dev")- Removing the
DEV=Truepatch if it's not required for the behavior under testAlso consider adding test coverage for the
LANGFLOW_LOG_FORMAT=key_valuecase to verify the service field behavior with KeyValueRenderer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/tests/unit/test_logger.py` around lines 798 - 811, The test name and monkeypatch are misleading because the _hide_default_service_for_pretty_console processor is registered by configure(), not gated by DEV; update the test accordingly by either renaming test_dev_pretty_console_hides_default_service_field to test_pretty_console_hides_default_service_field and removing the monkeypatch.setattr(logger_module, "DEV", True) call, or keep the name but drop the DEV patch so the test only asserts the behavior of configure()/ _hide_default_service_for_pretty_console; optionally add a second test that sets LANGFLOW_LOG_FORMAT="key_value" and asserts the service field behavior when using the KeyValueRenderer to cover that code path as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/backend/tests/unit/test_logger.py`:
- Around line 798-811: The test name and monkeypatch are misleading because the
_hide_default_service_for_pretty_console processor is registered by configure(),
not gated by DEV; update the test accordingly by either renaming
test_dev_pretty_console_hides_default_service_field to
test_pretty_console_hides_default_service_field and removing the
monkeypatch.setattr(logger_module, "DEV", True) call, or keep the name but drop
the DEV patch so the test only asserts the behavior of configure()/
_hide_default_service_for_pretty_console; optionally add a second test that sets
LANGFLOW_LOG_FORMAT="key_value" and asserts the service field behavior when
using the KeyValueRenderer to cover that code path as well.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b266181b-a4b5-479d-bcfa-febe5c74d06e
📒 Files selected for processing (2)
src/backend/tests/unit/test_logger.pysrc/lfx/src/lfx/log/logger.py
c3c61de to
71099a9
Compare
|
Addressed the CodeRabbit nitpick: the pretty-console service-field test no longer implies DEV-specific behavior, and the unused DEV patch/import were removed. Re-verified with |
|
CI looks blocked by runner/network setup rather than this PR change: both failed backend jobs stopped in |
…le-log-service-field
Fixes #13615.
Summary
service=langflowfield from human-oriented pretty console logs--devpretty console outputTests
uv run pytest src/backend/tests/unit/test_logger.py -quv run ruff check src/lfx/src/lfx/log/logger.py src/backend/tests/unit/test_logger.pyuv run ruff format --check src/lfx/src/lfx/log/logger.py src/backend/tests/unit/test_logger.pygit diff --checkSummary by CodeRabbit
Tests
Bug Fixes