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鈥檒l 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

@daniel-sanche daniel-sanche commented Oct 15, 2020

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 as a code owner Oct 15, 2020
@google-cla google-cla bot added the cla: yes label Oct 15, 2020
@daniel-sanche daniel-sanche added the do not merge label Oct 15, 2020
@daniel-sanche daniel-sanche marked this pull request as draft Oct 15, 2020
google/cloud/logging/_helpers.py Show resolved Hide resolved
@daniel-sanche daniel-sanche deleted the branch googleapis:master Oct 19, 2020
@daniel-sanche daniel-sanche reopened this Oct 19, 2020
@daniel-sanche daniel-sanche changed the base branch from fix-broken-tests to master Oct 19, 2020
@daniel-sanche daniel-sanche marked this pull request as ready for review Oct 19, 2020
@daniel-sanche daniel-sanche merged commit 0a1dd94 into googleapis:master Oct 19, 2020
7 of 8 checks passed
@daniel-sanche daniel-sanche deleted the default-filter branch Oct 19, 2020
@@ -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

@tseaver tseaver Oct 21, 2020

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

@tseaver tseaver Oct 21, 2020

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

@daniel-sanche daniel-sanche Oct 21, 2020

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

@tseaver tseaver Oct 21, 2020

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 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

@daniel-sanche daniel-sanche commented Oct 30, 2020

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

gcf-merge-on-green bot pushed a commit that referenced this issue 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 do not merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants