Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes the redundant timestamp format configuration and replaces its usage with a new Timestamp field type that generalizes timestamps to the ISO format. Key changes include:
- Updated test expectations in several test files to reflect the new ISO timestamp format without a trailing timezone suffix.
- Modified log processing in both collector and batch handler modules to use datetime.fromisoformat instead of datetime.strptime with a configurable timestamp format.
- Introduction of a new Timestamp field type in the logline handler to encapsulate and validate timestamp parsing.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/prefilter/test_prefilter.py | Updated expected log message timestamps to match new ISO format without the trailing 'Z'. |
| tests/miscellaneous/test_marshmallow.py | Modified test data to remove the redundant timestamp format parameter from the expected JSON. |
| tests/miscellaneous/test_logline_handler.py | Adjusted field types and expectations, including an increased field count due to the new Timestamp field. |
| src/logcollector/collector.py | Replaced strptime-based conversion of timestamps with fromisoformat, affecting how timestamps are parsed. |
| src/base/logline_handler.py | Added the new Timestamp class and updated JSON field extraction to use get_timestamp_as_str for Timestamp fields. |
| config.yaml | Removed the global timestamp_format configuration to enforce ISO timestamp formatting. |
Comments suppressed due to low confidence (5)
tests/prefilter/test_prefilter.py:391
- Test expectation for the timestamp format has been updated to remove the timezone suffix. Please ensure that this change aligns with the intended ISO format output throughout the pipeline.
"begin_timestamp": "2024-05-21T08:31:27.000000Z",
tests/miscellaneous/test_marshmallow.py:12
- The expected timestamp in the test data has been altered to omit the microseconds and trailing 'Z'. Confirm that this revised expectation is intentional and consistent with upstream processing.
"begin_timestamp": "2024-05-21T08:31:27.000000Z",
tests/miscellaneous/test_logline_handler.py:66
- The field count in tests has increased from 3 to 4 due to the addition of the Timestamp field. Ensure that all related tests fully cover the new behavior introduced by this field type.
self.assertEqual(4, sut.number_of_fields)
src/logcollector/collector.py:131
- Using datetime.fromisoformat may return a naive datetime if the input lacks timezone information. Consider ensuring consistent timezone handling across the pipeline to avoid potential issues in datetime comparisons.
timestamp=datetime.datetime.fromisoformat(fields.get("timestamp")),
src/base/logline_handler.py:103
- The Timestamp field’s get_timestamp_as_str method returns an ISO-formatted string without explicit timezone data. Confirm that downstream consumers of this timestamp are prepared to handle naive datetime strings, or consider appending a timezone indicator if needed.
return str(datetime.datetime.strptime(value, self.timestamp_format).isoformat())
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified Files
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #78 +/- ##
==========================================
+ Coverage 98.88% 98.90% +0.01%
==========================================
Files 15 15
Lines 1439 1455 +16
Branches 151 154 +3
==========================================
+ Hits 1423 1439 +16
Misses 9 9
Partials 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
stefanDeveloper
left a comment
There was a problem hiding this comment.
Looks good from my side
The redundant configuration variable for setting the timestamp format has been removed. A new
FieldTypefor timestamps has been added, which takes the timestamp format as input. The timestamp format throughout the pipeline has been generalized to the ISO format and the configured variable only changes the input format.Resolves #69.