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(logging): Entries has a 24H default filter #3120

Merged
merged 7 commits into from Apr 7, 2021
Merged

fix(logging): Entries has a 24H default filter #3120

merged 7 commits into from Apr 7, 2021

Conversation

0xSage
Copy link
Contributor

@0xSage 0xSage commented Oct 31, 2020

Would like to push this through as a minor feat, and a nonbreaking change. User's existing code will actually not be broken by the PR. Users will simply retrieve less logs upon running listEntries. Previously, users would have hit API quota anyway. This is more a bug fix than a new feature

fixes: #3088

BREAKING CHANGE: significantly speeds up call to list log entries
@0xSage 0xSage added type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. api: logging Issues related to the Cloud Logging API. priority: p2 Moderately-important priority. Fix may not be included in next release. labels Oct 31, 2020
@0xSage 0xSage requested a review from a team as a code owner October 31, 2020 16:58
@0xSage 0xSage self-assigned this Oct 31, 2020
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 31, 2020
@0xSage 0xSage requested a review from codyoss October 31, 2020 16:58
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

I have a couple of questions about these changes, raised internally.

@0xSage
Copy link
Contributor Author

0xSage commented Nov 2, 2020

After discussion: we'll batch this change with future improvements

@0xSage 0xSage added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Nov 2, 2020
logging/logadmin/logadmin.go Outdated Show resolved Hide resolved
logging/logadmin/logadmin.go Show resolved Hide resolved
@0xSage 0xSage requested a review from a team as a code owner November 16, 2020 17:24
@0xSage 0xSage changed the title feat(logging)!: Entries has a 24H default filter feat(logging): Entries has a 24H default filter Apr 6, 2021
@0xSage 0xSage removed the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Apr 6, 2021
@0xSage 0xSage changed the title feat(logging): Entries has a 24H default filter fix(logging): Entries has a 24H default filter Apr 6, 2021
@0xSage 0xSage requested a review from codyoss April 6, 2021 21:55
Copy link
Member

@codyoss codyoss left a comment

Choose a reason for hiding this comment

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

Seems reasonable and a more desirable behavior. It looks like this was also considered a non-breaking change in the node libraries.

@0xSage 0xSage added the automerge Merge the pull request once unit tests and other checks pass. label Apr 7, 2021
@0xSage 0xSage merged commit b32eb82 into master Apr 7, 2021
@0xSage 0xSage deleted the fix3088 branch April 7, 2021 17:31
@gcf-merge-on-green gcf-merge-on-green bot removed the automerge Merge the pull request once unit tests and other checks pass. label Apr 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: logging Issues related to the Cloud Logging API. cla: yes This human has signed the Contributor License Agreement. priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

logging: logAdmin listLogEntries to filter by last 24 hours by default
3 participants