Fix log follow mode without range header #5347
Conversation
agners
force-pushed
the
fix-log-follow-mode
branch
from
77dac3d to
941c16a
Compare
📝 WalkthroughWalkthroughThe changes in this pull request focus on the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant APIHost
Client->>APIHost: Request logs with follow=true
APIHost-->>Client: Stream logs (range_header: entries=:-{lines-1}:)
Client->>APIHost: Request logs with follow=false
APIHost-->>Client: Return fixed number of logs (range_header: entries=:-{lines-1}:{lines})
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🔇 Additional comments (4)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
agners
force-pushed
the
fix-log-follow-mode
branch
2 times, most recently
from
b41a3e0 to
308ac4e
Compare
When using a range with an explicit num_entries systemd-journal-gatewayd will not contiue even when the follow parameter is passed. Fixes an issue introduced with #5334.
agners
force-pushed
the
fix-log-follow-mode
branch
from
308ac4e to
b65d363
Compare
![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
🧹 Outside diff range and nitpick comments (2)
supervisor/api/host.py (1)
242-248: LGTM: Improved range_header constructionThe changes effectively address the issue with the log follow mode. The
range_headerconstruction now correctly handles thefollowparameter, allowing for continuous log streaming whenfollowis True. Additionally, setting a minimum of two lines prevents potential edge cases with single-line responses.For improved readability, consider extracting the
range_headerconstruction into a separate method. This would make the code more maintainable and easier to understand.Here's a suggested refactor:
def _construct_range_header(self, lines: int, follow: bool) -> str: entries = f":-{lines - 1}:" num_entries = "" if follow else str(lines) return f"entries={entries}{num_entries}" # In the method body: range_header = self._construct_range_header(lines, follow)This refactoring would make the logic more explicit and easier to maintain.
tests/api/test_host.py (1)
Line range hint
1-453: Overall assessment: Changes are focused and effectiveThe modifications in this file are minimal but crucial for addressing the issue with the log follow mode. The addition of the
DEFAULT_RANGE_FOLLOWconstant and its usage in thetest_advanced_logsfunction directly support the PR's objective. These changes ensure that the new behavior is properly tested without affecting other test cases.To further improve confidence in these changes:
- Consider adding a new test case that specifically verifies the behavior of the log follow mode with and without a range header.
- Ensure that the corresponding changes in the actual implementation (likely in
supervisor/api/host.py) align with these test modifications.Would you like me to outline a potential new test case for the log follow mode?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- supervisor/api/host.py (1 hunks)
- tests/api/test_host.py (2 hunks)
🧰 Additional context used
🔇 Additional comments (5)
supervisor/api/host.py (3)
Line range hint
1-1: LGTM: Improved type hintingThe addition of the return type annotation
-> web.StreamResponseenhances code readability and provides better type information for theadvanced_logsmethod.
Line range hint
231-241: LGTM: Improved handling oflinesparameterThe changes to ensure a minimum of two lines when processing the
linesparameter is a good defensive programming practice. It prevents potential issues with single-line responses and edge cases.The added comment provides a clear explanation for the reasoning behind this change, which is helpful for future maintainers.
Line range hint
1-268: Overall assessment: PR objectives achievedThis PR successfully addresses the issue with the log follow mode without range header. The changes in the
advanced_logs_handlermethod effectively allow for continuous log streaming when thefollowparameter is True, while maintaining the previous behavior for non-follow requests.Key improvements:
- The
range_headerconstruction now correctly handles thefollowparameter.- A minimum of two lines is ensured when processing the
linesparameter, preventing potential edge cases.- The code is well-documented with clear comments explaining the reasoning behind the changes.
These changes align well with the PR objectives and should resolve the issue introduced by the previous change (#5333). The implementation is solid, with only minor suggestions for refactoring to improve readability.
tests/api/test_host.py (2)
18-18: LGTM: New constant for follow mode range headerThe addition of
DEFAULT_RANGE_FOLLOWconstant is appropriate. It defines a range header specifically for the log follow mode, which aligns with the PR's objective of fixing the log follow mode without a range header.
237-237: LGTM: Updated test case for log follow modeThe change from
DEFAULT_RANGEtoDEFAULT_RANGE_FOLLOWin the assertion is correct and necessary. This update ensures that the test case accurately verifies the behavior of the log follow mode with the new range header format. The modification aligns perfectly with the PR's objective of fixing the log follow mode.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
supervisor/api/host.py (1)
242-242: LGTM! Consider a minor improvement for consistency.The changes effectively address the issue with the log follow mode when using a range with an explicit
num_entries. The modifications allow for continuous log streaming whenfollowisTrue, while preserving the original behavior for non-follow mode.For consistency, consider using parentheses for the ternary operation on line 242, similar to the multi-line format used in lines 246-248. This would improve readability and maintain a consistent style:
range_header = f"entries=:-{lines-1}:{''.format() if follow else lines}"Also applies to: 246-248
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- supervisor/api/host.py (1 hunks)
- tests/api/test_host.py (2 hunks)
🧰 Additional context used
🔇 Additional comments (4)
supervisor/api/host.py (1)
242-242: Verify the impact on log retrieval functionality.The changes effectively address the issue with the log follow mode. The modifications are minimal and focused, which reduces the risk of unintended side effects. However, it's important to verify that these changes don't negatively impact other log retrieval scenarios.
Please run the following tests to ensure the changes work as expected:
Test log retrieval with follow mode enabled:
Test log retrieval with a specific number of lines:
Test log retrieval with both follow mode and a specific number of lines:
Ensure that each scenario behaves correctly, particularly that the follow mode continues to stream logs and that specifying a number of lines returns the expected amount of data.
Also applies to: 246-248
tests/api/test_host.py (3)
18-18: LGTM: New constant for follow mode range.The addition of
DEFAULT_RANGE_FOLLOWconstant is appropriate for distinguishing between regular and follow mode ranges in the tests. This aligns with the PR objective to fix the log follow mode.
237-237: LGTM: Updated test for follow mode.The use of
DEFAULT_RANGE_FOLLOWin thetest_advanced_logsfunction for the/host/logs/followendpoint is correct. This change ensures that the test accurately reflects the expected behavior of the follow mode in the API.
18-18: Overall assessment: Changes are focused and effective.The modifications in this file are well-aligned with the PR objective to fix the log follow mode without a range header. The addition of the
DEFAULT_RANGE_FOLLOWconstant and its usage in thetest_advanced_logsfunction improve the accuracy of testing the follow mode functionality without introducing any breaking changes to existing tests.Also applies to: 237-237
Proposed change
When using a range with an explicit num_entries systemd-journal-gatewayd will not contiue even when the follow parameter is passed.
Fixes an issue introduced with #5334.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
New Features
followparameter.Bug Fixes
Tests