parsers/json_log: detect seconds/ms/μs/ns in numeric timestamps, swallow out-of-range and non-finite values - #26
Open
HrachShah wants to merge 1 commit into
Open
Conversation
…low out-of-range and non-finite values
Reviewer's GuideExtends JSONLogParser’s numeric timestamp handling to robustly interpret seconds/ms/μs/ns by magnitude, and makes invalid or non-finite numeric timestamps return None instead of raising, with accompanying tests and changelog entry. Sequence diagram for numeric timestamp parsing in JSONLogParsersequenceDiagram
participant Caller
participant JSONLogParser
participant _extract_timestamp
participant _numeric_to_datetime
participant datetime_fromtimestamp
Caller->>JSONLogParser: parse(json_line)
JSONLogParser->>_extract_timestamp: _extract_timestamp(data)
_extract_timestamp->>_numeric_to_datetime: _numeric_to_datetime(value)
alt [value is NaN or infinite]
_numeric_to_datetime-->>_extract_timestamp: return None
else [value is finite]
loop up to 4 units
_numeric_to_datetime->>datetime_fromtimestamp: fromtimestamp(seconds)
alt [accepts]
datetime_fromtimestamp-->>_numeric_to_datetime: datetime
_numeric_to_datetime-->>_extract_timestamp: return datetime (break)
else [raises OverflowError/OSError/ValueError]
_numeric_to_datetime->>_numeric_to_datetime: seconds /= 1000.0
end
end
alt [no unit accepted]
_numeric_to_datetime-->>_extract_timestamp: return None
end
end
_extract_timestamp-->>JSONLogParser: timestamp or None
JSONLogParser-->>Caller: entry with timestamp or None
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughRefactors JSON timestamp parsing in JSONLogParser to add a ChangesNumeric timestamp parsing fix
Estimated code review effort: 2 (Simple) | ~15 minutes Sequence Diagram(s)No sequence diagram generated; the change is a localized parsing logic refactor within a single function/helper, not a multi-component interaction flow. 🚥 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
test_parse_json_various_level_namestest was removed without an obvious replacement; if that behavior is still supported, consider keeping or relocating the test to avoid a silent regression in level-field handling. - In
_numeric_to_datetime, the NaN/inf detection would be clearer and more conventional usingmath.isnan/math.isfiniterather thanvalue != valueand explicitfloat('inf')comparisons.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `test_parse_json_various_level_names` test was removed without an obvious replacement; if that behavior is still supported, consider keeping or relocating the test to avoid a silent regression in level-field handling.
- In `_numeric_to_datetime`, the NaN/inf detection would be clearer and more conventional using `math.isnan`/`math.isfinite` rather than `value != value` and explicit `float('inf')` comparisons.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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_parsers.py (1)
84-85: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winKeep the level-field alias coverage —
tests/test_parsers.py:84-85removes the only test that exercisesseverity/loglevelhandling, and no other test covers those aliases. Restore it or add an equivalent check.🤖 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_parsers.py` around lines 84 - 85, The parser test coverage for the level-field aliases was removed, and no other test currently verifies severity/loglevel handling. Restore the deleted assertion in tests/test_parsers.py or add an equivalent test in the same test area that exercises the parser path for severity and loglevel aliases, using the existing parser/test helper symbols to keep the coverage 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/parsers/json_log.py`:
- Around line 84-108: The _numeric_to_datetime helper is converting numeric JSON
timestamps with local time semantics, which makes parsed values depend on the
host timezone. Update the datetime.fromtimestamp path in json_log.py’s
JSONLogParser._numeric_to_datetime to always normalize through UTC while keeping
the returned datetime naive like the rest of the parser, and preserve the
existing magnitude-based fallback loop for
seconds/milliseconds/microseconds/nanoseconds.
---
Outside diff comments:
In `@tests/test_parsers.py`:
- Around line 84-85: The parser test coverage for the level-field aliases was
removed, and no other test currently verifies severity/loglevel handling.
Restore the deleted assertion in tests/test_parsers.py or add an equivalent test
in the same test area that exercises the parser path for severity and loglevel
aliases, using the existing parser/test helper symbols to keep the coverage
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: f9af16b5-0d5b-49a0-96c9-a6a6401419e3
📒 Files selected for processing (3)
CHANGELOG.mdsrc/log_analyzer_cli/parsers/json_log.pytests/test_parsers.py
Comment on lines
+84
to
+108
| @staticmethod | ||
| def _numeric_to_datetime(value: float) -> Optional[datetime]: | ||
| """Convert a numeric timestamp to a datetime. | ||
|
|
||
| JSON log producers disagree on the unit: seconds (Unix epoch), | ||
| milliseconds (Java ``System.currentTimeMillis()``), microseconds | ||
| (``UnixMicro()``), or nanoseconds (``UnixNano()``). The | ||
| heuristic is magnitude-based — anything above the year ~33658 | ||
| boundary can't be a raw second count, so we keep dividing by | ||
| 1000 until the result lands in a sensible range or we run out | ||
| of plausible units. | ||
| """ | ||
| if value != value or value in (float("inf"), float("-inf")): | ||
| return None | ||
|
|
||
| # datetime.fromtimestamp can't represent years far outside the | ||
| # usual range; clamp early so a stray nanosecond value doesn't | ||
| # bubble up as "year 52218064 is out of range". | ||
| seconds = float(value) | ||
| for _ in range(4): | ||
| try: | ||
| return datetime.fromtimestamp(seconds) | ||
| except (OverflowError, OSError, ValueError): | ||
| seconds /= 1000.0 | ||
| return None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check other parsers for tz-aware vs naive datetime handling, and CI timezone config
rg -n 'fromtimestamp|tzinfo|timezone' src/log_analyzer_cli/parsers -A2 -B2
fd -e yml -e yaml . .github | xargs -r rg -n 'TZ|timezone' -iRepository: HrachShah/log-analyzer-cli
Length of output: 1031
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map relevant parser files and datetime usage sites
git ls-files 'src/log_analyzer_cli/**/*.py' 'tests/**/*.py' '.github/**/*.yml' '.github/**/*.yaml' | sed -n '1,200p'
printf '\n--- datetime-related usages ---\n'
rg -n 'datetime\.fromtimestamp|datetime\.utcnow|datetime\.now|timezone\.utc|tzinfo|astimezone|replace\(tzinfo|compare|<=|>=|<|>' src/log_analyzer_cli tests -g '*.py' -A2 -B2
printf '\n--- json_log parser file outline ---\n'
ast-grep outline src/log_analyzer_cli/parsers/json_log.py --view expanded
printf '\n--- relevant tests around timestamp parsing ---\n'
rg -n 'hour|minute|second|fromtimestamp|timestamp|timezone|utc' tests/test_parsers.py -A3 -B3
printf '\n--- CI timezone settings ---\n'
rg -n 'TZ|timezone|UTC' .github -g '*.yml' -g '*.yaml' -i -A2 -B2Repository: HrachShah/log-analyzer-cli
Length of output: 40470
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from datetime import datetime, timezone
ts = 1647780800
print("utc:", datetime.fromtimestamp(ts, tz=timezone.utc))
print("local:", datetime.fromtimestamp(ts))
PY
printf '\n--- parse_timestamp and filters ---\n'
sed -n '1,240p' src/log_analyzer_cli/utils.py
printf '\n--- JSON parser timestamp helpers ---\n'
sed -n '1,180p' src/log_analyzer_cli/parsers/json_log.py
printf '\n--- generic/syslog timestamp helpers ---\n'
sed -n '1,170p' src/log_analyzer_cli/parsers/generic.py
printf '\n'
sed -n '1,180p' src/log_analyzer_cli/parsers/syslog.py
printf '\n'
sed -n '1,160p' src/log_analyzer_cli/parsers/apache.pyRepository: HrachShah/log-analyzer-cli
Length of output: 22187
Normalize numeric JSON timestamps to UTC at src/log_analyzer_cli/parsers/json_log.py:105. datetime.fromtimestamp(seconds) uses the host’s local timezone, so the same epoch value parses to different wall-clock times on non-UTC machines. That makes the numeric JSON path environment-dependent and breaks the new timestamp assertions outside UTC. Keep this conversion in UTC while preserving the parser’s current naive-datetime style.
🤖 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 84 - 108, The
_numeric_to_datetime helper is converting numeric JSON timestamps with local
time semantics, which makes parsed values depend on the host timezone. Update
the datetime.fromtimestamp path in json_log.py’s
JSONLogParser._numeric_to_datetime to always normalize through UTC while keeping
the returned datetime naive like the rest of the parser, and preserve the
existing magnitude-based fallback loop for
seconds/milliseconds/microseconds/nanoseconds.
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
JSONLogParser._extract_timestamponly handled two numeric units: anythingabove
1e12was treated as milliseconds, anything at or below as rawseconds. Microsecond and nanosecond producers (Go's
time.Now().UnixMicro()and
UnixNano(), Python'stime.time_ns(), Java'sSystem.nanoTime())landed outside the millisecond branch, got passed directly to
datetime.fromtimestamp, and crashed withOverflowError/ValueError: year 52218064 is out of range— which then bubbled all theway out of
parse()and turned the entire log line into a parse failure.It also accepted
NaNand ±infbecauseinf > 1e12is True andinf / 1000is stillinf, which then crashed downstream code thatexpected a real datetime.
Fix
JSONLogParser._numeric_to_datetimehelper that converts bymagnitude: keep dividing by 1000 until
datetime.fromtimestampacceptsthe result, or until we've tried all four units (seconds, ms, μs, ns).
Noneso the timestamp field issimply absent instead of crashing.
representation and we return
None(was: raised).Tests
Five new cases in
tests/test_parsers.py::TestJSONNumericTimestampUnits:+inf,-inf,NaN,null, and string-typed timestamp fields allparse successfully and return
entry.timestamp is None(was: raised)Summary by Sourcery
Handle numeric JSON log timestamps across multiple units and make invalid numeric values fail gracefully instead of raising.
Bug Fixes:
Documentation:
Tests:
Summary by CodeRabbit
Bug Fixes
Tests