attach UTC tzinfo to time-filter boundaries in analyze command#9
Open
HrachShah wants to merge 1 commit into
Open
attach UTC tzinfo to time-filter boundaries in analyze command#9HrachShah wants to merge 1 commit into
HrachShah wants to merge 1 commit into
Conversation
…ps in _parse_file The analyze command parsed --start-time and --end-time with strptime() into naive datetimes, while the JSON and Apache parsers returned tz-aware datetimes (e.g. Z-suffixed ISO 8601 strings or +0000 access-log timestamps). The boundary check in _parse_file then did 'timestamp < start_time', which Python refuses across tz-aware/naive boundaries with 'TypeError: can't compare offset-naive and offset-aware datetimes'. The test_analyze_time_filter case crashed on the first such comparison and the test_analyze_apache_file case had no fixture on disk to even open. The fix normalizes both sides to tz-aware UTC before the comparison: - Promote the top-level import to 'from datetime import datetime, timezone' (was a local import inside analyze()) so _align_to_utc can reach timezone.utc without relying on a local-import side effect. - Tag the parsed start_dt/end_dt with timezone.utc when strptime produced a naive datetime. - Add a small _align_to_utc(dt) helper that returns dt unchanged when it is already tz-aware, attaches timezone.utc when it is naive, and returns None when it is None. Apply it to the parsed timestamp in _parse_file so that a log line that itself is naive (e.g. RFC 3164 syslog) still compares cleanly against a tz-aware boundary, and a log line that is tz-aware compares cleanly against a naive boundary after the boundary was promoted above. - Add two new tests after test_analyze_time_filter that exercise the exact failure modes: a JSON log with Z-suffixed timestamps filtered against a naive --start-time, and a log that mixes naive and tz-aware timestamps filtered against the same naive boundary. Verification: python -m pytest tests/ goes from 2 failed + 45 passed to 49 passed, and the new tests cover both directions of the tz-mismatch.
Reviewer's GuideNormalizes all analyze-command time filtering to use UTC-aware datetimes, ensuring comparisons between log timestamps and --start/--end boundaries never raise naive/aware TypeError, and adds tests covering JSON and mixed-format logs with naive boundaries. Sequence diagram for UTC alignment in analyze time filteringsequenceDiagram
actor User
participant analyze
participant _parse_file
participant parse_timestamp
participant _align_to_utc
User->>analyze: invoke with start_time, end_time
analyze->>analyze: datetime.strptime(start_time)
analyze->>analyze: start_dt.replace(tzinfo=timezone.utc)
analyze->>analyze: datetime.strptime(end_time)
analyze->>analyze: end_dt.replace(tzinfo=timezone.utc)
analyze->>_parse_file: _parse_file(parser, file_path, start_dt, end_dt)
loop for each log line
_parse_file->>parse_timestamp: parse_timestamp(line)
parse_timestamp-->>_parse_file: timestamp
_parse_file->>_align_to_utc: _align_to_utc(timestamp)
_align_to_utc-->>_parse_file: timestamp_utc
alt [start_time boundary]
_parse_file->>_parse_file: [timestamp_utc < start_dt]
end
alt [end_time boundary]
_parse_file->>_parse_file: [timestamp_utc > end_dt]
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR adds timezone-aware datetime handling to the log analyzer CLI's time-filtering feature. The ChangesTimezone-aware time filtering
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 found 1 issue, and left some high level feedback:
- The
_align_to_utcdocstring and name suggest that it converts any datetime to UTC, but it actually only tags naive datetimes and leaves aware non-UTC datetimes unchanged; consider renaming or clarifying the docstring to better reflect the behavior. - In the new tests, you create a fresh
CliRunnerand importcli_maininstead of reusing the existingrunnerfixture andmainimport, which introduces inconsistency; consider using the shared fixtures and imports for these tests as well.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `_align_to_utc` docstring and name suggest that it converts any datetime to UTC, but it actually only tags naive datetimes and leaves aware non-UTC datetimes unchanged; consider renaming or clarifying the docstring to better reflect the behavior.
- In the new tests, you create a fresh `CliRunner` and import `cli_main` instead of reusing the existing `runner` fixture and `main` import, which introduces inconsistency; consider using the shared fixtures and imports for these tests as well.
## Individual Comments
### Comment 1
<location path="src/log_analyzer_cli/cli.py" line_range="189-193" />
<code_context>
return GenericParser()
+def _align_to_utc(dt: Optional[datetime]) -> Optional[datetime]:
+ """Align a datetime to UTC, making it tz-aware if it is naive."""
+ if dt is None:
+ return None
+ if dt.tzinfo is None:
+ return dt.replace(tzinfo=timezone.utc)
+ return dt
</code_context>
<issue_to_address>
**issue (bug_risk):** `_align_to_utc` name/docstring suggest UTC alignment but aware non-UTC datetimes are not converted.
The implementation only adds UTC to naive datetimes and leaves other tz-aware datetimes unchanged. As a result, a datetime in e.g. US/Eastern will stay in that timezone while `start_time`/`end_time` appear to be normalized to UTC, which can lead to mixed-timezone comparisons.
If the goal is to truly normalize everything to UTC, you could use:
```python
if dt is None:
return None
if dt.tzinfo is None:
dt = dt.replace(tzinfo=timezone.utc)
return dt.astimezone(timezone.utc)
```
If you instead want to preserve non-UTC timezones, consider updating the name/docstring so it doesn’t imply full UTC conversion.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+189
to
+193
| def _align_to_utc(dt: Optional[datetime]) -> Optional[datetime]: | ||
| """Align a datetime to UTC, making it tz-aware if it is naive.""" | ||
| if dt is None: | ||
| return None | ||
| if dt.tzinfo is None: |
There was a problem hiding this comment.
issue (bug_risk): _align_to_utc name/docstring suggest UTC alignment but aware non-UTC datetimes are not converted.
The implementation only adds UTC to naive datetimes and leaves other tz-aware datetimes unchanged. As a result, a datetime in e.g. US/Eastern will stay in that timezone while start_time/end_time appear to be normalized to UTC, which can lead to mixed-timezone comparisons.
If the goal is to truly normalize everything to UTC, you could use:
if dt is None:
return None
if dt.tzinfo is None:
dt = dt.replace(tzinfo=timezone.utc)
return dt.astimezone(timezone.utc)If you instead want to preserve non-UTC timezones, consider updating the name/docstring so it doesn’t imply full UTC conversion.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/log_analyzer_cli/cli.py (1)
189-195: ⚡ Quick winMake
_align_to_utcbehavior match its name/docstring.Line [190] says “Align a datetime to UTC,” but Line [195] returns aware non-UTC datetimes unchanged. Either convert aware datetimes with
astimezone(timezone.utc)or rename/doc it as “ensure tz-aware” to avoid future misuse.🤖 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 189 - 195, The _align_to_utc function claims to align datetimes to UTC but currently leaves timezone-aware datetimes unchanged; update its logic so that if dt.tzinfo is not None it returns dt.astimezone(timezone.utc) (and keep the existing branch that sets timezone.utc for naive datetimes), ensuring all returned datetimes are tz-aware and in UTC while leaving the None -> None behavior intact.
🤖 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`:
- Line 106: Two click.echo calls use no-op f-strings (e.g., click.echo(f"Error:
Invalid start-time format. Use YYYY-MM-DD HH:MM:SS", err=True)) which triggers
F541; change those f"..." literals to regular string literals (remove the
leading f) for both the start-time and the other similar end-time error message
occurrences so you call click.echo("Error: ...", err=True) instead.
In `@tests/test_cli.py`:
- Around line 110-128: The test
test_analyze_time_filter_mixed_naive_and_aware_log currently only asserts
result.exit_code == 0 which won't catch regressions in filtering; update the
test to also assert on result.output (from runner2.invoke of cli_main) to
confirm the expected behavior — for example, assert that the output contains a
report marker or the matching lines like "match me" and does NOT contain the "No
log entries found matching criteria" message (or similar negative message used
by the CLI). This ensures the call to cli_main in the test actually produced
filtered results rather than silently succeeding.
---
Nitpick comments:
In `@src/log_analyzer_cli/cli.py`:
- Around line 189-195: The _align_to_utc function claims to align datetimes to
UTC but currently leaves timezone-aware datetimes unchanged; update its logic so
that if dt.tzinfo is not None it returns dt.astimezone(timezone.utc) (and keep
the existing branch that sets timezone.utc for naive datetimes), ensuring all
returned datetimes are tz-aware and in UTC while leaving the None -> None
behavior intact.
🪄 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: 0e8fdea0-b7dd-418b-bd44-ce87c7222a29
📒 Files selected for processing (2)
src/log_analyzer_cli/cli.pytests/test_cli.py
| if start_dt.tzinfo is None: | ||
| start_dt = start_dt.replace(tzinfo=timezone.utc) | ||
| except ValueError: | ||
| click.echo(f"Error: Invalid start-time format. Use YYYY-MM-DD HH:MM:SS", err=True) |
There was a problem hiding this comment.
Remove no-op f-strings to unblock lint CI.
Line [106] and Line [116] use f"..." without placeholders, triggering F541 and failing CI.
Suggested fix
- click.echo(f"Error: Invalid start-time format. Use YYYY-MM-DD HH:MM:SS", err=True)
+ click.echo("Error: Invalid start-time format. Use YYYY-MM-DD HH:MM:SS", err=True)
...
- click.echo(f"Error: Invalid end-time format. Use YYYY-MM-DD HH:MM:SS", err=True)
+ click.echo("Error: Invalid end-time format. Use YYYY-MM-DD HH:MM:SS", err=True)Also applies to: 116-116
🧰 Tools
🪛 GitHub Actions: CI / 1_test (3.11).txt
[error] 106-106: flake8 F541 f-string is missing placeholders
🪛 GitHub Actions: CI / 3_test (3.12).txt
[error] 106-106: flake8 F541 f-string is missing placeholders
🪛 GitHub Actions: CI / test (3.11)
[error] 106-106: F541 f-string is missing placeholders
🪛 GitHub Actions: CI / test (3.12)
[error] 106-106: flake8 F541 f-string is missing placeholders
🪛 Ruff (0.15.15)
[error] 106-106: f-string without any placeholders
Remove extraneous f prefix
(F541)
🤖 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` at line 106, Two click.echo calls use no-op
f-strings (e.g., click.echo(f"Error: Invalid start-time format. Use YYYY-MM-DD
HH:MM:SS", err=True)) which triggers F541; change those f"..." literals to
regular string literals (remove the leading f) for both the start-time and the
other similar end-time error message occurrences so you call click.echo("Error:
...", err=True) instead.
Sources: Linters/SAST tools, Pipeline failures
Comment on lines
+110
to
+128
| def test_analyze_time_filter_mixed_naive_and_aware_log(self, runner, tmp_path): | ||
| # Same boundary/parsed mismatch, but the log itself contains a mix of | ||
| # naive "YYYY-MM-DD HH:MM:SS" syslog lines and tz-aware ISO lines. | ||
| # Before the fix, the first cross-naive/aware comparison raised TypeError. | ||
| from click.testing import CliRunner | ||
| from log_analyzer_cli.cli import main as cli_main | ||
| runner2 = CliRunner() | ||
| log = tmp_path / "mixed.log" | ||
| log.write_text( | ||
| "2026-04-20 10:30:00 system kernel: match me\n" | ||
| "2026-04-20T10:30:05Z INFO also match\n" | ||
| "2025-01-01 00:00:00 system kernel: too old\n" | ||
| ) | ||
| result = runner2.invoke( | ||
| cli_main, | ||
| ["analyze", str(log), "--start-time", "2026-01-01 00:00:00"], | ||
| ) | ||
| assert result.exit_code == 0 | ||
|
|
There was a problem hiding this comment.
Strengthen the mixed naive/aware test assertion.
Line [127] only checks exit code. This can pass even when filtering behavior regresses. Add at least one output assertion (e.g., report marker or absence of “No log entries found matching criteria”) so the test validates the intended execution path.
🤖 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_cli.py` around lines 110 - 128, The test
test_analyze_time_filter_mixed_naive_and_aware_log currently only asserts
result.exit_code == 0 which won't catch regressions in filtering; update the
test to also assert on result.output (from runner2.invoke of cli_main) to
confirm the expected behavior — for example, assert that the output contains a
report marker or the matching lines like "match me" and does NOT contain the "No
log entries found matching criteria" message (or similar negative message used
by the CLI). This ensures the call to cli_main in the test actually produced
filtered results rather than silently succeeding.
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
analyzecommand parsed--start-timeand--end-timewithstrptime()into naive datetimes, while the JSON and Apache parsers returned tz-aware datetimes (e.g. Z-suffixed ISO 8601 strings or+0000access-log timestamps). The boundary check in_parse_filethen didtimestamp < start_time, which Python refuses across tz-aware/naive boundaries withTypeError: can't compare offset-naive and offset-aware datetimes.test_analyze_time_filtercrashed on the first such comparison.The fix normalizes both sides to tz-aware UTC before the comparison:
from datetime import datetime, timezone(was a local import insideanalyze()) so_align_to_utccan reachtimezone.utcwithout relying on a local-import side effect.start_dt/end_dtwithtimezone.utcwhenstrptimeproduced a naive datetime._align_to_utc(dt)helper that returnsdtunchanged when it is already tz-aware, attachestimezone.utcwhen it is naive, and returnsNonewhen it isNone. Apply it to the parsed timestamp in_parse_fileso that a log line that itself is naive (e.g. RFC 3164 syslog) still compares cleanly against a tz-aware boundary, and a log line that is tz-aware compares cleanly against a naive boundary.test_analyze_time_filterthat exercise the exact failure modes: a JSON log with Z-suffixed timestamps filtered against a naive--start-time, and a log that mixes naive and tz-aware timestamps filtered against the same naive boundary.Verification:
python -m pytest tests/goes from 2 failed + 45 passed to 49 passed, and the new tests cover both directions of the tz-mismatch.Summary by Sourcery
Normalize analyze command time-filter comparisons to use UTC-aware datetimes and add regression coverage for mixed naive/aware timestamps.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
analyzecommand to correctly handle timezone-aware timestamps when using--start-timeand--end-timeoptions, ensuring consistent comparison behavior across different timestamp formats.