Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: add default filter settings to list_entries #73

Merged
merged 32 commits into from Oct 19, 2020

Conversation

daniel-sanche
Copy link
Contributor

Fixes #71 🦕

The list_entries functions currently have no filters applied, and the default settings take ~5 minutes for a single query. This PR matches the functionality of gcloud logging read by adding a default 24 hour filter to the query. If the user provides their own filter statement, the two statements will be appended, unless they explicitly set their own timestamp value

@daniel-sanche daniel-sanche requested a review from a team as a code owner October 15, 2020 20:16
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 15, 2020
@daniel-sanche daniel-sanche added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 15, 2020
@daniel-sanche daniel-sanche marked this pull request as draft October 15, 2020 20:16
@daniel-sanche daniel-sanche deleted the branch googleapis:master October 19, 2020 20:45
@daniel-sanche daniel-sanche reopened this Oct 19, 2020
@daniel-sanche daniel-sanche changed the base branch from fix-broken-tests to master October 19, 2020 20:48
@daniel-sanche daniel-sanche marked this pull request as ready for review October 19, 2020 20:48
@daniel-sanche daniel-sanche merged commit 0a1dd94 into googleapis:master Oct 19, 2020
@daniel-sanche daniel-sanche deleted the default-filter branch October 19, 2020 21:33
@@ -242,6 +243,7 @@ def list_entries(
:param filter_:
a filter expression. See
https://cloud.google.com/logging/docs/view/advanced_filters
By default, a 24 hour filter is applied.
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to flag this as a breaking change?

@@ -97,7 +97,7 @@ def default(session, django_dep=('django',)):
)


@nox.session(python=['2.7', '3.5', '3.6', '3.7'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Dropping Python 2.7 support is definitely a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, still getting used to the conventional commit style. Yes, this PR introduces a breaking change (more discussion on the release PR). How do you think we should proceed since it was already merged without the breaking label?

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that we haven't removed the Python :: 2 entry from setup.py yet, we can repair the breakage by doing that in another PR.

@0xSage
Copy link
Contributor

0xSage commented Oct 29, 2020

I'm working on handling this in Go lib as well, but realized that checking "timestamp" not in filter_.lower():will catch if user is filtering for a payload substring that contains "timestamp"

Very edge case but may lead to an issue report down the line

@daniel-sanche
Copy link
Contributor Author

Hmm good point. I opened #86 to discuss the issue. Thanks!

gcf-merge-on-green bot pushed a commit that referenced this pull request Nov 19, 2020
🤖 I have created a release \*beep\* \*boop\* 
---
## [2.0.0](https://www.github.com/googleapis/python-logging/compare/v1.15.1...v2.0.0) (2020-11-19)


### ⚠ BREAKING CHANGES

* Use microgenerator for GAPIC layer. See [UPGRADING.md](https://github.com/googleapis/python-logging/blob/master/UPGRADING.md) for details. (#94)
* removes support for webapp2 and other Python2 specific code

### Features

* pass 'client_options' to super ctor ([#61](https://www.github.com/googleapis/python-logging/issues/61)) ([c4387b3](https://www.github.com/googleapis/python-logging/commit/c4387b307f8f3502fb53ae1f7e1144f6284280a4)), closes [#55](https://www.github.com/googleapis/python-logging/issues/55)
* use microgenerator ([#94](https://www.github.com/googleapis/python-logging/issues/94)) ([ff90fd2](https://www.github.com/googleapis/python-logging/commit/ff90fd2fb54c612fe6ab29708a2d5d984f60dea7))


### Bug Fixes

* add default filter settings to list_entries ([#73](https://www.github.com/googleapis/python-logging/issues/73)) ([0a1dd94](https://www.github.com/googleapis/python-logging/commit/0a1dd94811232634fdb849cb2c85bd44e870642f))
* failing CI tests ([#70](https://www.github.com/googleapis/python-logging/issues/70)) ([96adeed](https://www.github.com/googleapis/python-logging/commit/96adeedbda16a5c21651c356261442478aaa867a))


### Code Refactoring

* remove python2 ([#78](https://www.github.com/googleapis/python-logging/issues/78)) ([bf579e4](https://www.github.com/googleapis/python-logging/commit/bf579e4f871c92391a9f6f87eca931744158e31a))


### Documentation

* update docs ([#77](https://www.github.com/googleapis/python-logging/issues/77)) ([bdd9c44](https://www.github.com/googleapis/python-logging/commit/bdd9c440f29d1fcd6fb9545d8465c63efa6c0cea))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

list_logs hangs
4 participants