ApacheParser: accept a real (non-empty) user in the COMBINED pattern - #30
Open
HrachShah wants to merge 3 commits into
Open
ApacheParser: accept a real (non-empty) user in the COMBINED pattern#30HrachShah wants to merge 3 commits into
HrachShah wants to merge 3 commits into
Conversation
added 3 commits
July 4, 2026 17:31
…start-time/--end-time comparable
…rt-time/--end-time
The previous compare-tz-aware-to-naive path raised TypeError on
`<=>/<` between a tz-aware log timestamp and a naive CLI bound
("Cannot compare tz-naive and tz-aware datetime-like objects"),
so any line with an offset (e.g. Apache/JSON logs with +00:00)
silently disappeared from filtered output. cli._parse_file already
handles this with `timestamp.replace(tzinfo=None)` when a tz is
attached; mirror that here in the underlying filter_lines helper
so the helper, the CLI, and any direct library caller all agree on
how a tz-aware log entry is compared against a tz-naive bound.
Two new TestFilterLinesTzAware cases pin the behaviour: a
15:00:00+00:00 entry is correctly dropped against a naive 16:00:00
start, and a 17:00:00+00:00 entry is kept.
The user slot in the Apache combined log format is just another token, not a run of dashes — a real production line looks like `192.168.1.10 - frank [..] "GET / HTTP/1.1" 200 ..`. The old `(?P<user>\s+)` only matched the literal `- -` placeholder, so a real-user line was reported as unparseable by COMBINED_PATTERN, parse() silently fell through to COMMON_PATTERN, and the referer and user_agent captured by COMBINED were dropped on the floor. Switch the user slot to `\S+` so COMBINED matches real-world logs and parse() carries referer and user_agent into metadata. A regression test pins the new behaviour against a `frank` user. Full test suite: 76 passed (was 75, +1 new).
Reviewer's GuideUpdates Apache log parsing and timestamp handling to correctly support non-empty user fields, timezone-aware ISO 8601 and Apache-style timestamps (including offsets), and aligns CLI and utility filtering behavior via new tests. Sequence diagram for timestamp parsing and timezone-normalized filteringsequenceDiagram
actor User
participant CLI as cli._parse_file
participant Utils as utils.parse_timestamp
User->>CLI: _parse_file(start_time, end_time)
CLI->>Utils: parse_timestamp(line)
Utils-->>CLI: timestamp
opt timestamp and timestamp.tzinfo
CLI->>CLI: timestamp.replace(tzinfo=None)
end
alt compare_ts < start_time or compare_ts > end_time
CLI-->>User: skip line
else
CLI-->>User: yield parsed line
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughTimestamp parsing gains additional format support (ISO-8601 variants, Apache/common-log with optional timezone offsets). Filtering logic in both cli.py and utils.py now normalizes timezone-aware timestamps to naive before comparing against start/end bounds. The Apache parser's user-field regex was corrected to capture non-whitespace tokens. Tests were added for all changes. ChangesTimestamp Parsing and Timezone-Aware Filtering
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant LogFile
participant ParseTimestamp as parse_timestamp/_try_parse_datetime
participant FilterLines as filter_lines/_parse_file
participant Output
LogFile->>ParseTimestamp: raw timestamp string
ParseTimestamp->>ParseTimestamp: match against expanded format list
ParseTimestamp-->>FilterLines: parsed datetime (naive or tz-aware)
FilterLines->>FilterLines: drop tzinfo if timestamp is tz-aware
FilterLines->>FilterLines: compare normalized timestamp to start_time/end_time
FilterLines-->>Output: included or excluded log line
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The timezone-normalization logic for timestamp comparisons is duplicated between
filter_linesand_parse_file; consider extracting this into a shared helper to avoid divergence and make future changes easier. - In
ApacheParser.COMBINED_PATTERN, switchinguserto\S+means it will no longer match an actually empty user field; if truly empty usernames are possible in your logs, you may want a pattern that accepts both-and empty values while still capturing real usernames.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The timezone-normalization logic for timestamp comparisons is duplicated between `filter_lines` and `_parse_file`; consider extracting this into a shared helper to avoid divergence and make future changes easier.
- In `ApacheParser.COMBINED_PATTERN`, switching `user` to `\S+` means it will no longer match an actually empty user field; if truly empty usernames are possible in your logs, you may want a pattern that accepts both `-` and empty values while still capturing real usernames.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/log_analyzer_cli/utils.py (1)
208-227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared tz-normalization logic to avoid duplication.
The tz-aware-to-naive normalization block is duplicated verbatim in
filter_lineshere and incli._parse_file(lines 217-225). Extracting a small helper (e.g.,_normalize_for_compare(timestamp)) would keep the two paths in sync and reduce the risk of them diverging.♻️ Suggested helper extraction
+def _to_naive(ts: Optional[datetime]) -> Optional[datetime]: + """Drop tzinfo for comparison against naive bounds.""" + return ts.replace(tzinfo=None) if (ts and ts.tzinfo) else ts + # In filter_lines: - if start_time and timestamp: - compare_ts = ( - timestamp.replace(tzinfo=None) - if timestamp.tzinfo - else timestamp - ) - if compare_ts < start_time: - continue - if end_time and timestamp: - compare_ts = ( - timestamp.replace(tzinfo=None) - if timestamp.tzinfo - else timestamp - ) - if compare_ts > end_time: - continue + if start_time and timestamp: + if _to_naive(timestamp) < start_time: + continue + if end_time and timestamp: + if _to_naive(timestamp) > end_time: + continue🤖 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/log_analyzer_cli/utils.py` around lines 208 - 227, The timestamp normalization logic is duplicated in filter_lines and cli._parse_file, so extract it into a shared helper such as _normalize_for_compare(timestamp) in utils.py and use that helper in both comparison paths. Keep the helper behavior identical for tz-aware and tz-naive timestamps, and update the existing start_time/end_time checks to call it instead of repeating the same replace(tzinfo=None) block.
🤖 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.
Inline comments:
In `@tests/test_utils.py`:
- Line 7: Remove the unused imports from the test module to satisfy flake8 F401;
in the test file, delete the unused `pytest` import and the unused
`read_log_file` import so only symbols actually referenced by the tests remain.
---
Nitpick comments:
In `@src/log_analyzer_cli/utils.py`:
- Around line 208-227: The timestamp normalization logic is duplicated in
filter_lines and cli._parse_file, so extract it into a shared helper such as
_normalize_for_compare(timestamp) in utils.py and use that helper in both
comparison paths. Keep the helper behavior identical for tz-aware and tz-naive
timestamps, and update the existing start_time/end_time checks to call it
instead of repeating the same replace(tzinfo=None) block.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45da5255-896b-4dd6-b793-ecbb4bb9a8ee
📒 Files selected for processing (5)
src/log_analyzer_cli/cli.pysrc/log_analyzer_cli/parsers/apache.pysrc/log_analyzer_cli/utils.pytests/test_parsers.pytests/test_utils.py
|
|
||
| from datetime import datetime, timezone, timedelta | ||
|
|
||
| import pytest |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Remove unused imports flagged by flake8.
CI fails with F401: pytest and read_log_file are imported but never used in this test module.
🛡️ Proposed fix
from datetime import datetime, timezone, timedelta
-import pytest
-
from log_analyzer_cli.utils import (
detect_log_level,
filter_lines,
normalize_error_pattern,
parse_timestamp,
- read_log_file,
)Also applies to: 9-9
🧰 Tools
🪛 GitHub Actions: CI / 2_test (3.11).txt
[error] 7-7: flake8 (F401): 'pytest' imported but unused.
🪛 GitHub Actions: CI / 3_test (3.10).txt
[error] 7-7: flake8: F401 'pytest' imported but unused
🪛 GitHub Actions: CI / test (3.10)
[error] 7-7: flake8 (F401) 'pytest' imported but unused
🪛 GitHub Actions: CI / test (3.11)
[error] 7-7: flake8: F401 'pytest' imported but unused
🤖 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 `@tests/test_utils.py` at line 7, Remove the unused imports from the test
module to satisfy flake8 F401; in the test file, delete the unused `pytest`
import and the unused `read_log_file` import so only symbols actually referenced
by the tests remain.
Sources: Linters/SAST tools, Pipeline failures
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Switch the user slot in ApacheParser.COMBINED_PATTERN from \s+ to \S+ so a real production log line with a non-empty user field (e.g. 'frank') is matched by the combined pattern instead of falling through to the common pattern and dropping the referer and user_agent. A new regression test pins the behaviour.
Summary by Sourcery
Improve log parsing robustness for Apache combined logs and timezone-aware timestamps, while aligning CLI and library filtering behavior.
New Features:
Enhancements:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests