pick the earliest-occurring level keyword in detect_log_level - #23
Open
HrachShah wants to merge 4 commits into
Open
pick the earliest-occurring level keyword in detect_log_level#23HrachShah wants to merge 4 commits into
HrachShah wants to merge 4 commits into
Conversation
detect_log_level walked a hardcoded list of (regex, level) tuples in CRITICAL > ERROR > WARNING > INFO > DEBUG > TRACE order and returned the first patternprecedence — whichever keyword appears first in the line is the intent of the line. Without this, '2024-01-15 10:23:45 WARNING cannot connect to CRITICAL service' was classified as CRITICAL even though the line is a WARNING. The fix runs every level regex, finds the leftmost match across all of them, and returns the level that owns that match. A new tests/test_utils.py pins the new contract: the leftmost keyword wins, embedded levels in later words do not override, and words like 'ERROR_RATE' or 'CRITICAL_ERROR' (which contain the level as a prefix of a longer identifier) are correctly ignored because the pattern uses \b boundaries.
Reviewer's GuideUpdate log level detection to choose the earliest-occurring level keyword in a line instead of a fixed severity order, and add unit tests covering the new behavior and existing edge cases. Flow diagram for updated detect_log_level logicflowchart TD
A[detect_log_level line] --> B[Convert line to uppercase line_upper]
B --> C[Initialize earliest_level = None]
C --> D[Initialize earliest_index = infinity]
D --> E[Iterate level_patterns]
E --> F[re.finditer pattern line_upper]
F --> G{Any matches?}
G -->|No| H{More patterns?}
H -->|Yes| E
H -->|No| I{earliest_level is not None?}
G -->|Yes| J[Take first_match.start]
J --> K{first_match.start < earliest_index?}
K -->|Yes| L[Update earliest_index and earliest_level]
K -->|No| H
L --> H
I -->|Yes| M[Return earliest_level]
I -->|No| N[Return UNKNOWN]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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:
- You don't need to materialize all matches with
list(re.finditer(...)); usingnext(re.finditer(...), None)and checking that single result would avoid unnecessary allocations while still letting you compare positions. - Consider avoiding
float('inf')forearliest_indexand instead initializing it toNoneand adjusting the comparison logic, which can make the intent clearer and removes reliance on magic sentinel values.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You don't need to materialize all matches with `list(re.finditer(...))`; using `next(re.finditer(...), None)` and checking that single result would avoid unnecessary allocations while still letting you compare positions.
- Consider avoiding `float('inf')` for `earliest_index` and instead initializing it to `None` and adjusting the comparison logic, which can make the intent clearer and removes reliance on magic sentinel values.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
📝 WalkthroughWalkthroughThe changes validate JSON parser inputs, expand timezone-aware timestamp formats, and update log-level detection to choose the earliest keyword match. Unit tests cover non-record JSON values, timestamp offsets, UTC markers, keyword precedence, case handling, and word boundaries. ChangesLog parsing behavior
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/log_analyzer_cli/utils.py (1)
148-154: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOptimize first-match discovery to prevent scanning the entire line.
Using
list(re.finditer(...))forces the regex engine to parse the entire line for all occurrences and allocates a list, even though only the first match is needed. Usingre.search(...)achieves the exact same result but stops scanning at the first match, significantly improving performance on long log lines.♻️ Proposed refactor
- for pattern, level in level_patterns: - matches = list(re.finditer(pattern, line_upper)) - if matches: - first_match = matches[0] - if first_match.start() < earliest_index: - earliest_index = first_match.start() - earliest_level = level + for pattern, level in level_patterns: + match = re.search(pattern, line_upper) + if match and match.start() < earliest_index: + earliest_index = match.start() + earliest_level = level(Optional: For even greater performance, consider combining the keywords into a single module-level compiled regex like
re.compile(r'\b(CRITICAL|ERROR|...)\b')to find the absolute earliest match in a single pass without iterating through multiple patterns.)🤖 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 148 - 154, Replace list(re.finditer(pattern, line_upper)) in the level-pattern matching logic with re.search(pattern, line_upper), then use the returned match directly to update earliest_index and earliest_level while preserving the existing earliest-match behavior.
🤖 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.
Nitpick comments:
In `@src/log_analyzer_cli/utils.py`:
- Around line 148-154: Replace list(re.finditer(pattern, line_upper)) in the
level-pattern matching logic with re.search(pattern, line_upper), then use the
returned match directly to update earliest_index and earliest_level while
preserving the existing earliest-match behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d78550e-bb6f-40b9-8dc3-0a5904ae7fcc
📒 Files selected for processing (4)
src/log_analyzer_cli/parsers/json_log.pysrc/log_analyzer_cli/utils.pytests/test_parsers.pytests/test_utils.py
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.
What
utils.detect_log_levelreturns the first regex match in a hardcodedCRITICAL > ERROR > WARNING > INFO > DEBUG > TRACE order, regardless of
where the keyword actually appears in the line.
Repro
Summary by Sourcery
Update log level detection to choose the earliest-occurring level keyword in a log line and add unit tests to cover the new behavior and edge cases.
Bug Fixes:
Tests:
Summary by CodeRabbit
Bug Fixes
Znotation.Tests