Skip to content

fix(logging): only skip the default 24 hour filter for timestamp restrictions - #18714

Open
abhishekacharya200 wants to merge 1 commit into
googleapis:mainfrom
abhishekacharya200:fix-logging-default-time-filter
Open

abhishekacharya200 wants to merge 1 commit into
googleapis:mainfrom
abhishekacharya200:fix-logging-default-time-filter

Conversation

@abhishekacharya200

Copy link
Copy Markdown
Contributor
  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

list_entries skips its default 24 hour timestamp>= filter if the word timestamp appears anywhere in the user's filter. So filters such as jsonPayload.timestamp_ms>0, textPayload:"timestamp" or labels.event_timestamp="x" silently search the whole log history.

This change skips the default only when the filter compares the top-level timestamp or receiveTimestamp field (for example timestamp>="..." or NOT timestamp<"..."). The check is still case-insensitive. I kept receiveTimestamp in the check so that filters on it keep working as they do now.

Added unit tests for both cases.

Fixes #17338 🦕

…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

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +240 to +242
timestamp = datetime.strptime(
out_filter, in_filter + ' AND timestamp>="' + self._time_format() + '"'
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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())

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

list_entries: a filter containing the substring "timestamp" silently disables the default 24-hour window

1 participant