Repository navigation
fix(logging): only skip the default 24 hour filter for timestamp restrictions - #18714
abhishekacharya200 wants to merge 1 commit into
Conversation
…rictions list_entries skipped its default timestamp filter whenever the substring 'timestamp' appeared anywhere in the filter, such as in jsonPayload.timestamp_ms or a text search. Only skip it when the filter compares the top-level timestamp or receiveTimestamp field. Fixes googleapis#17338
There was a problem hiding this comment.
Code Review
This pull request improves the timestamp filtering logic in google-cloud-logging by introducing a regular expression (_TIME_RESTRICTION_RE) to identify top-level timestamp or receiveTimestamp fields, preventing false positives on nested fields like jsonPayload.timestamp. Unit tests were added to verify this behavior. A review comment suggests refactoring the unit tests to avoid using datetime.strptime with the entire filter string, which could fail if the filter contains % characters.
| timestamp = datetime.strptime( | ||
| out_filter, in_filter + ' AND timestamp>="' + self._time_format() + '"' | ||
| ) |
There was a problem hiding this comment.
Using datetime.strptime with the entire in_filter as part of the format string is fragile. If in_filter contains any % characters (which are common in search queries, e.g., percent-encoded characters or wildcards), strptime will attempt to interpret them as format directives and raise a ValueError.
Instead, extract the timestamp substring from out_filter first, and then parse only the timestamp portion.
| timestamp = datetime.strptime( | |
| out_filter, in_filter + ' AND timestamp>="' + self._time_format() + '"' | |
| ) | |
| prefix = in_filter + ' AND timestamp>="' | |
| self.assertTrue(out_filter.startswith(prefix)) | |
| self.assertTrue(out_filter.endswith('"')) | |
| timestamp_str = out_filter[len(prefix):-1] | |
| timestamp = datetime.strptime(timestamp_str, self._time_format()) |
list_entriesskips its default 24 hourtimestamp>=filter if the wordtimestampappears anywhere in the user's filter. So filters such asjsonPayload.timestamp_ms>0,textPayload:"timestamp"orlabels.event_timestamp="x"silently search the whole log history.This change skips the default only when the filter compares the top-level
timestamporreceiveTimestampfield (for exampletimestamp>="..."orNOT timestamp<"..."). The check is still case-insensitive. I keptreceiveTimestampin the check so that filters on it keep working as they do now.Added unit tests for both cases.
Fixes #17338 🦕