-
Notifications
You must be signed in to change notification settings - Fork 65
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
perf!: getEntries filters 24 hour timestamp by default #955
Conversation
Codecov Report
@@ Coverage Diff @@
## master #955 +/- ##
=======================================
Coverage 98.38% 98.39%
=======================================
Files 18 18
Lines 12977 12985 +8
Branches 412 414 +2
=======================================
+ Hits 12768 12776 +8
Misses 207 207
Partials 2 2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless this is changing the API surface, I'd avoid indicating that it's a breaking change with !
, as this will result in a major version bump.
@bcoe possible to resolve the change request so this one can be merged as well? |
// By default, sort entries by descending timestamp | ||
let reqOpts = extend({orderBy: 'timestamp desc'}, options); | ||
|
||
// By default, filter entries to last 24 hours only |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the previous behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, it would get as many entries as possible until a max threshold was reached. It took about 10 minutes. The default behavior across CLI is actually to filter by 24 hours, as well as what's recommended in documentation.
We were getting a lot of support tickets across different language repos on this.
This fix reduces query time to a few seconds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I just wanted to get that information to determine if it's a breaking change, which it sounds like it is. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great question, this actually got a lot of debate. After some back and forth in the chats, we decided to mark it as breaking. and bundle it with another breaking fix that just got merged last night
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks!
Fixes #924
This fix/feat is applied across client libraries in Go, Python, Java and Node, e.g.
googleapis/google-cloud-go#3120
googleapis/python-logging#73