normalize RFC 3339 'Z' UTC suffix in syslog parser - #28
Draft
HrachShah wants to merge 12 commits into
Draft
Conversation
added 4 commits
June 13, 2026 13:32
…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.
A single log file can contain both naive timestamps from a syslog emitter (2025-03-20 10:15:32) and tz-aware ISO timestamps from a JSON logger (2025-03-20T10:15:33Z). The previous code fed the raw entry.timestamp values to sorted(), min()/max(), and the < / > comparisons in _add_to_error_group; Python rejects mixing offset-naive and offset-aware datetimes with 'TypeError: can't compare offset-naive and offset-aware datetimes', which crashed TimeDistribution construction and error-group first/last_seen computation on any such file. The fix introduces a small _normalize_timestamp helper that promotes naive datetimes to UTC (and leaves tz-aware datetimes as-is) on a *local copy* used by sorted() and the < / > comparisons. The original entry.timestamp on each ParsedEntry is left untouched, so callers still see the exact value the parser produced. The new tests in test_analyzer.py cover the helper directly (sorted/min/max), the time_distribution sort path with a mixed list, and the error-group first/last_seen comparison path with the same mix.
JSONLogParser._extract_timestamp passed numeric timestamps straight into datetime.fromtimestamp(), so any value that overflowed the year range (year > 9999, year < 1, or values beyond platform time_t) raised a ValueError/OverflowError and the whole log line was dropped — including its level and message metadata, which are usually the parts an operator actually needs when triaging an incident. Wrap the fromtimestamp call so out-of-range epochs yield a timestamp-less entry instead of crashing. Also exclude bool from the numeric branch since True/False are isinstance(int) in Python and would otherwise be interpreted as epoch 1 / epoch 0. Three new tests cover the far-future, far-past, and float-overflow cases that previously raised uncaught exceptions.
SyslogParser._parse_timestamp accepted the ISO 8601 'Z' suffix via the
PATTERNS regex but then iterated strptime formats that only know %z
('+00:00' / '-05:30' style). A timestamp like '2025-03-20T10:15:32Z'
matched the regex, fell through every format with a ValueError, and
_parse_timestamp returned None. The line was kept (level, message,
host, process all survive) but its timestamp was silently lost, which
makes time-windowed analysis undercount lines from the very entries
that usually carry the most precise wall-clock information.
Normalise the trailing 'Z' to '+00:00' before strptime, mirroring the
same fix already present in JSONLogParser._parse_timestamp_string. The
ts_str variable in the legacy 'RFC 3164' branch (which uses no timezone
suffix) is left untouched.
Two new tests cover the Z suffix with and without fractional seconds,
asserting that the parsed datetime carries a real timezone offset
rather than coming back as a naive None-returned entry.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults 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:
✨ 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 |
Reviewer's GuideThis PR fixes timestamp handling across parsers and analysis: syslog RFC 3339 'Z' UTC suffix is normalized so timestamps are preserved, JSON epoch parsing is hardened against out-of-range values, and analyzer/CLI date-time logic is updated to safely handle mixed naive and timezone-aware datetimes when filtering and computing time distributions/error group bounds, with new tests covering these scenarios. Sequence diagram for updated timestamp parsing and normalizationsequenceDiagram
actor User
participant CLI as CLI_analyze
participant Parser as SyslogParser
participant Analyzer as LogAnalyzer_analyze
User->>CLI: analyze(start_time,end_time,log_format,file)
CLI->>CLI: datetime.strptime(start_time)
CLI->>CLI: datetime.strptime(end_time)
CLI->>CLI: _align_to_utc(start_dt)
CLI->>CLI: _align_to_utc(end_dt)
CLI->>Parser: parse_timestamp(line)
Parser->>Parser: _parse_timestamp(ts_str)
Parser->>Parser: ts_normalized = ts_str[:-1] + "+00:00" [ts_str endswith Z]
Parser->>Parser: datetime.strptime(ts_normalized,fmt)
Parser-->>CLI: timestamp
CLI->>CLI: _align_to_utc(timestamp)
CLI-->>Analyzer: ParsedEntry(timestamp,level,message,...)
Analyzer->>Analyzer: _normalize_timestamp(entry.timestamp)
Analyzer->>Analyzer: sorted(normalized_timestamps)
Analyzer->>Analyzer: _normalize_timestamp(group.first_seen)
Analyzer->>Analyzer: _normalize_timestamp(group.last_seen)
Analyzer-->>User: TimeDistribution and error_groups
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
added 8 commits
July 15, 2026 01:25
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.
Summary
SyslogParser._parse_timestampaccepted the ISO 8601 / RFC 3339 single-letter'Z'UTC suffix via thePATTERNSregex but then iteratedstrptimeformats that only know%z(the+00:00/-05:30style). A timestamp like2025-03-20T10:15:32Zmatched the regex, fell through every format with aValueError, and_parse_timestampreturnedNone. The line was kept (level, message, host, process all survive) but its timestamp was silently lost, which makes time-windowed analysis undercount exactly the entries that usually carry the most precise wall-clock information.The fix normalises the trailing
Zto+00:00beforestrptime, mirroring the same fix already present inJSONLogParser._parse_timestamp_string. The legacy RFC 3164 branch (which uses no timezone suffix) is left untouched:ts_strin that branch is the bare string,ts_normalizedonly matters for the ISO 8601 branch.Two new tests cover the
Zsuffix with and without fractional seconds, asserting the parsed datetime carries a real timezone offset and does not get dropped toNone.Summary by Sourcery
Normalize and robustly handle timestamps across syslog, JSON log parsing, analyzer, and CLI time filtering so RFC 3339 'Z' suffixes, mixed naive/aware datetimes, and extreme epoch values are parsed or skipped safely without crashing or silently dropping entries.
New Features:
Bug Fixes:
Enhancements: