normalise timezone-aware log filter comparisons - #33
Open
HrachShah wants to merge 15 commits into
Open
Conversation
added 10 commits
July 26, 2026 09:27
Reviewer's GuideNormalize timestamp comparisons across CLI and utility helpers to handle timezone-aware and naive datetimes safely, tighten JSON log parsing and timestamp extraction, and improve error pattern normalization and regression coverage. Sequence diagram for timezone-normalized log filteringsequenceDiagram
actor Caller
participant filter_lines
participant parse_timestamp
participant _timestamp_before
participant _timestamp_after
participant _align_timestamp_timezone
Caller->>filter_lines: filter_lines(lines, start_time, end_time)
loop for each line
filter_lines->>parse_timestamp: parse_timestamp(line)
parse_timestamp-->>filter_lines: timestamp
alt start_time provided
filter_lines->>_timestamp_before: _timestamp_before(timestamp, start_time)
_timestamp_before->>_align_timestamp_timezone: _align_timestamp_timezone(timestamp, start_time)
_align_timestamp_timezone-->>_timestamp_before: timestamp, start_time
_timestamp_before-->>filter_lines: bool
alt timestamp before start_time
filter_lines-->>Caller: skip line
end
end
alt end_time provided
filter_lines->>_timestamp_after: _timestamp_after(timestamp, end_time)
_timestamp_after->>_align_timestamp_timezone: _align_timestamp_timezone(timestamp, end_time)
_align_timestamp_timezone-->>_timestamp_after: timestamp, end_time
_timestamp_after-->>filter_lines: bool
alt timestamp after end_time
filter_lines-->>Caller: skip line
end
end
filter_lines-->>Caller: yield line_num, line, timestamp, level
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds timezone-aware timestamp alignment, improves JSON timestamp extraction and record validation, updates CLI filtering to use parsed timestamps, validates time ranges, fixes per-entry metadata defaults, and adds regression tests. ChangesTimestamp-aware log filtering
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Input as JSONL input
participant JSONParser as JSON parser
participant CLI as CLI analyzer
participant TimeHelpers as timezone comparison helpers
Input->>JSONParser: parse log line
JSONParser-->>CLI: parsed entry with timestamp
CLI->>TimeHelpers: compare timestamp with boundaries
TimeHelpers-->>CLI: include or exclude entry
CLI-->>Input: JSON analysis output
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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
_align_timestamp_timezone/_timestamp_before/_timestamp_afterhelpers are duplicated in bothcli.pyandutils.py; consider centralizing them in a shared module to avoid divergence and keep timezone handling consistent. - The timezone alignment currently uses
replace(tzinfo=...)without converting between zones; if logs and filters may be in different timezones, consider normalizing to a common timezone (e.g., UTC) withastimezoneto avoid silent offset errors. - JSONLogParser now calls
json.loadsin bothcan_parseandparse, andcan_parserejects empty objects; you might want to either cache the parsed data between the two or relaxcan_parseto avoid redundant parsing and unexpected rejection of valid-but-empty records if that is acceptable input.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_align_timestamp_timezone` / `_timestamp_before` / `_timestamp_after` helpers are duplicated in both `cli.py` and `utils.py`; consider centralizing them in a shared module to avoid divergence and keep timezone handling consistent.
- The timezone alignment currently uses `replace(tzinfo=...)` without converting between zones; if logs and filters may be in different timezones, consider normalizing to a common timezone (e.g., UTC) with `astimezone` to avoid silent offset errors.
- JSONLogParser now calls `json.loads` in both `can_parse` and `parse`, and `can_parse` rejects empty objects; you might want to either cache the parsed data between the two or relax `can_parse` to avoid redundant parsing and unexpected rejection of valid-but-empty records if that is acceptable input.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: 3
🤖 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 `@src/log_analyzer_cli/cli.py`:
- Around line 186-201: Move the from datetime import datetime statement from
inside analyze to the module-level imports in cli.py, so the annotations and
helper functions _align_timestamp_timezone, _timestamp_before, and
_timestamp_after resolve datetime at module scope.
In `@src/log_analyzer_cli/parsers/json_log.py`:
- Around line 82-92: Update the numeric timestamp conversion in the JSON log
parser to use an inclusive millisecond threshold, so exactly 1e12 is divided by
1000 before calling datetime.fromtimestamp. Add a regression test covering the
boundary value 1_000_000_000_000 and verifying it produces the expected UTC
datetime.
In `@tests/test_utils.py`:
- Line 11: Fix the Flake8 formatting violations in the tests/test_utils.py
fixture: split the long lines assignment around lines 11 and 30–33 into
compliant formatting, and remove the extra blank line before line 33. Preserve
the existing fixture values and behavior.
🪄 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 Plus
Run ID: 7128c2a1-585d-4be7-8277-26393311b383
📒 Files selected for processing (6)
src/log_analyzer_cli/cli.pysrc/log_analyzer_cli/parsers/json_log.pysrc/log_analyzer_cli/utils.pytests/test_cli.pytests/test_parsers.pytests/test_utils.py
Comment on lines
+186
to
+201
| def _align_timestamp_timezone(timestamp: datetime, boundary: datetime) -> tuple[datetime, datetime]: | ||
| if timestamp.tzinfo is None and boundary.tzinfo is not None: | ||
| timestamp = timestamp.replace(tzinfo=boundary.tzinfo) | ||
| elif timestamp.tzinfo is not None and boundary.tzinfo is None: | ||
| boundary = boundary.replace(tzinfo=timestamp.tzinfo) | ||
| return timestamp, boundary | ||
|
|
||
|
|
||
| def _timestamp_before(timestamp: datetime, boundary: datetime) -> bool: | ||
| timestamp, boundary = _align_timestamp_timezone(timestamp, boundary) | ||
| return timestamp < boundary | ||
|
|
||
|
|
||
| def _timestamp_after(timestamp: datetime, boundary: datetime) -> bool: | ||
| timestamp, boundary = _align_timestamp_timezone(timestamp, boundary) | ||
| return timestamp > boundary |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 \
'from datetime import datetime|def _align_timestamp_timezone|def _timestamp_before|def _timestamp_after' \
src/log_analyzer_cli/cli.pyRepository: HrachShah/log-analyzer-cli
Length of output: 1178
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if command -v ruff >/dev/null 2>&1; then
ruff check src/log_analyzer_cli/cli.py --select F821
else
python3 - <<'PY'
from pathlib import Path
text = Path("src/log_analyzer_cli/cli.py").read_text()
print("Ruff not available; inspecting imports")
print([line.strip() for line in text.splitlines() if "datetime" in line])
PY
fiRepository: HrachShah/log-analyzer-cli
Length of output: 5161
Import datetime at module scope.
datetime is used by annotations at module scope, but it is only imported inside analyze. Move from datetime import datetime to the module imports to clear the undefined-name check.
🧰 Tools
🪛 Ruff (0.16.0)
[error] 186-186: Undefined name datetime
(F821)
[error] 186-186: Undefined name datetime
(F821)
[error] 186-186: Undefined name datetime
(F821)
[error] 186-186: Undefined name datetime
(F821)
[error] 194-194: Undefined name datetime
(F821)
[error] 194-194: Undefined name datetime
(F821)
[error] 199-199: Undefined name datetime
(F821)
[error] 199-199: Undefined name datetime
(F821)
🤖 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/cli.py` around lines 186 - 201, Move the from datetime
import datetime statement from inside analyze to the module-level imports in
cli.py, so the annotations and helper functions _align_timestamp_timezone,
_timestamp_before, and _timestamp_after resolve datetime at module scope.
Source: Linters/SAST tools
Comment on lines
+82
to
+92
| if isinstance(value, (int, float)) and not isinstance(value, bool): | ||
| try: | ||
| if value > 1e12: | ||
| return datetime.fromtimestamp(value / 1000, tz=timezone.utc) | ||
| return datetime.fromtimestamp(value, tz=timezone.utc) | ||
| except (OverflowError, OSError, ValueError): | ||
| continue | ||
| if isinstance(value, str): | ||
| return self._parse_timestamp_string(value) | ||
| parsed = self._parse_timestamp_string(value) | ||
| if parsed is not None: | ||
| return parsed |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle the millisecond threshold inclusively.
A timestamp value of 1_000_000_000_000 is a valid millisecond epoch value. The strict comparison treats it as seconds, and datetime.fromtimestamp then rejects it. Use >= and add a boundary regression test.
Proposed fix
- if value > 1e12:
+ if value >= 1e12:📝 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.
Suggested change
| if isinstance(value, (int, float)) and not isinstance(value, bool): | |
| try: | |
| if value > 1e12: | |
| return datetime.fromtimestamp(value / 1000, tz=timezone.utc) | |
| return datetime.fromtimestamp(value, tz=timezone.utc) | |
| except (OverflowError, OSError, ValueError): | |
| continue | |
| if isinstance(value, str): | |
| return self._parse_timestamp_string(value) | |
| parsed = self._parse_timestamp_string(value) | |
| if parsed is not None: | |
| return parsed | |
| if isinstance(value, (int, float)) and not isinstance(value, bool): | |
| try: | |
| if value >= 1e12: | |
| return datetime.fromtimestamp(value / 1000, tz=timezone.utc) | |
| return datetime.fromtimestamp(value, tz=timezone.utc) | |
| except (OverflowError, OSError, ValueError): | |
| continue | |
| if isinstance(value, str): | |
| parsed = self._parse_timestamp_string(value) | |
| if parsed is not None: | |
| return parsed |
🤖 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/parsers/json_log.py` around lines 82 - 92, Update the
numeric timestamp conversion in the JSON log parser to use an inclusive
millisecond threshold, so exactly 1e12 is divided by 1000 before calling
datetime.fromtimestamp. Add a regression test covering the boundary value
1_000_000_000_000 and verifying it produces the expected UTC datetime.
|
|
||
|
|
||
| def test_filter_lines_compares_aware_timestamp_with_naive_bounds(): | ||
| lines = iter(["2025-03-20T09:00:00+00:00 INFO early\n", "2025-03-20T11:00:00+00:00 INFO late\n"]) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Fix the Flake8 violations.
Split the lines fixture at Line 11. Remove one excess blank line before Line 33. The current file fails CI.
Proposed fix
- lines = iter(["2025-03-20T09:00:00+00:00 INFO early\n", "2025-03-20T11:00:00+00:00 INFO late\n"])
+ lines = iter(
+ [
+ "2025-03-20T09:00:00+00:00 INFO early\n",
+ "2025-03-20T11:00:00+00:00 INFO late\n",
+ ]
+ )Also applies to: 30-33
🧰 Tools
🪛 GitHub Actions: CI / 1_test (3.11).txt
[error] 11-33: flake8 command failed: lines exceed 100 characters (E501 at line 11) and too many blank lines (E303 at line 33).
🪛 GitHub Actions: CI / 3_test (3.10).txt
[error] 11-33: Flake8 reported a line exceeding 100 characters (E501) at line 11 and too many blank lines (E303) at line 33.
🪛 GitHub Actions: CI / test (3.10)
[error] 11-33: Flake8 reported lines exceeding the 100-character limit (E501) and too many blank lines (E303).
🪛 GitHub Actions: CI / test (3.11)
[error] 11-33: Flake8 reported lines exceeding 100 characters at line 11 (E501) and too many blank lines at line 33 (E303).
🤖 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 11, Fix the Flake8 formatting violations in the
tests/test_utils.py fixture: split the long lines assignment around lines 11 and
30–33 into compliant formatting, and remove the extra blank line before line 33.
Preserve the existing fixture values and behavior.
Source: Pipeline failures
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/log_analyzer_cli/parsers/syslog.py`:
- Around line 121-123: Normalize parsed timezone-aware timestamps using the same
timezone-alignment policy as filter_lines before returning them, preserving the
represented instant and ensuring all values are comparable with naive syslog
timestamps during analyzer sorting. Add a regression test covering mixed RFC
5424 offset-aware and naive formats.
- Around line 121-122: Update the syslog ISO timestamp pattern used by the
parser around the fractional formats so it accepts only one through six
fractional digits, matching datetime.strptime’s %f behavior; preserve timezone
and non-fractional parsing, and add a test covering the selected handling of
inputs with more than six fractional digits.
🪄 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 Plus
Run ID: 062a5444-077e-4f67-8d58-8f55f17400a7
📒 Files selected for processing (2)
src/log_analyzer_cli/parsers/syslog.pytests/test_parsers.py
Comment on lines
+121
to
122
| "%Y-%m-%dT%H:%M:%S.%f%z", | ||
| "%Y-%m-%dT%H:%M:%S.%f", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from datetime import datetime
value = "2025-03-20T10:15:32.1234567+01:00"
try:
datetime.strptime(value, "%Y-%m-%dT%H:%M:%S.%f%z")
except ValueError as exc:
print(f"Confirmed: {exc}")
else:
raise SystemExit("Expected %f to reject more than six fractional digits")
PYRepository: HrachShah/log-analyzer-cli
Length of output: 268
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect the relevant syslog parser section and tests if present.
fd -a 'syslog\.py|test.*syslog|syslog.*test' . || true
printf '\n--- syslog.py ---\n'
wc -l src/log_analyzer_cli/parsers/syslog.py 2>/dev/null || true
sed -n '1,180p' src/log_analyzer_cli/parsers/syslog.py 2>/dev/null || true
printf '\n--- related date parsing tests/usages ---\n'
rg -n "syslog|strptime|datetime|fractional|timestamp=None|%f" src tests 2>/dev/null || trueRepository: HrachShah/log-analyzer-cli
Length of output: 12788
Make syslog ISO fraction parsing match the ISO timestamp pattern.
The second syslog regex pattern accepts arbitrarily many fractional digits, but datetime.strptime(..., "%f") only accepts six. Inputs like 2025-03-20T10:15:32.1234567+01:00 match the line but return timestamp=None. Limit the regex fraction to one through six digits, or normalize longer fractions before parsing. Add a test for the chosen behavior.
🤖 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/parsers/syslog.py` around lines 121 - 122, Update the
syslog ISO timestamp pattern used by the parser around the fractional formats so
it accepts only one through six fractional digits, matching datetime.strptime’s
%f behavior; preserve timezone and non-fractional parsing, and add a test
covering the selected handling of inputs with more than six fractional digits.
Comment on lines
+121
to
123
| "%Y-%m-%dT%H:%M:%S.%f%z", | ||
| "%Y-%m-%dT%H:%M:%S.%f", | ||
| "%Y-%m-%dT%H:%M:%S%z", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Normalize timestamps before downstream sorting.
This format returns timezone-aware datetimes for RFC 5424 records with offsets. Other syslog formats still return naive datetimes. The supplied src/log_analyzer_cli/analyzer.py:63-111 code sorts these values directly, which raises TypeError when a log contains both formats.
Apply the same timezone-alignment policy used by filter_lines before returning or sorting timestamps. Preserve the instant when converting aware values. Add a mixed-format regression test.
🤖 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/parsers/syslog.py` around lines 121 - 123, Normalize
parsed timezone-aware timestamps using the same timezone-alignment policy as
filter_lines before returning them, preserving the represented instant and
ensuring all values are comparable with naive syslog timestamps during analyzer
sorting. Add a regression test covering mixed RFC 5424 offset-aware and naive
formats.
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.
The direct
filter_lineshelper compared timezone-aware parsed timestamps with naive filter boundaries, raisingTypeErrorand dropping matching log records. The CLI path already normalises these comparisons.This brings the helper into line with the CLI for both aware timestamps versus naive bounds and the inverse case, with regression coverage.
Verification:
PYTHONPATH=src python -m pytest -q tests/test_utils.py -o addopts="" --disable-warnings;PYTHONPATH=src python -m compileall -q src;git diff --check.Summary by Sourcery
Normalize timestamp timezone handling in log filtering and JSON parsing, and tighten JSON log record validation with additional edge-case coverage.
New Features:
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit