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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Dec 1, 2020
Merged

perf!: getEntries filters 24 hour timestamp by default #955

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
918f454
feat!: getEntries filters 24 hour timestamp by default
Nov 24, 2020
aac80c9
chore: clean up comments
Nov 24, 2020
891f63a
test: fix tests breaking on millisecond difference
Nov 24, 2020
ecb9623
Merge branch 'master' into fix924
freelerobot Nov 25, 2020
e55949c
Merge branch 'master' into fix924
freelerobot Nov 25, 2020
a98ac67
Merge branch 'master' into fix924
freelerobot Nov 30, 2020
c35937d
Merge branch 'master' into fix924
freelerobot Dec 1, 2020
42bc8d2
Merge branch 'master' into fix924
freelerobot Dec 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 14 additions & 6 deletions src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -552,12 +552,20 @@ class Logging {
opts?: GetEntriesRequest | GetEntriesCallback
): Promise<GetEntriesResponse> {
const options = opts ? (opts as GetEntriesRequest) : {};
const reqOpts = extend(
{
orderBy: 'timestamp desc',
},
options
);

// By default, sort entries by descending timestamp
let reqOpts = extend({orderBy: 'timestamp desc'}, options);

// By default, filter entries to last 24 hours only
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Great, thanks!

const time = new Date();
time.setDate(time.getDate() - 1);
const timeFilter = `timestamp >= "${time.toISOString()}"`;
if (!options.filter) {
reqOpts = extend({filter: timeFilter}, reqOpts);
} else if (!options.filter.includes('timestamp')) {
reqOpts.filter += ` AND ${timeFilter}`;
}

reqOpts.resourceNames = arrify(reqOpts.resourceNames!);
this.projectId = await this.auth.getProjectId();
const resourceName = 'projects/' + this.projectId;
Expand Down
41 changes: 36 additions & 5 deletions test/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,26 +477,29 @@ describe('Logging', () => {
logging.auth.getProjectId = async () => PROJECT_ID;
});

it('should exec without options', async () => {
it('should exec without options (with defaults)', async () => {
logging.loggingService.listLogEntries = async (
reqOpts: {},
// eslint-disable-next-line @typescript-eslint/no-explicit-any
reqOpts: any,
gaxOpts: {}
) => {
assert.deepStrictEqual(reqOpts, {
filter: reqOpts?.filter,
orderBy: 'timestamp desc',
resourceNames: ['projects/' + logging.projectId],
});
assert.deepStrictEqual(gaxOpts, {
autoPaginate: undefined,
});
assert.ok(reqOpts?.filter.includes('timestamp'));
return [[]];
};

await logging.getEntries();
});

it('should accept options', async () => {
const options = {filter: 'test'};
it('should accept options (and not overwrite timestamp)', async () => {
const options = {filter: 'timestamp > "2020-11-11T15:01:23.045123456Z"'};

logging.loggingService.listLogEntries = async (
reqOpts: {},
Expand All @@ -505,7 +508,7 @@ describe('Logging', () => {
assert.deepStrictEqual(
reqOpts,
extend(options, {
filter: 'test',
filter: 'timestamp > "2020-11-11T15:01:23.045123456Z"',
orderBy: 'timestamp desc',
resourceNames: ['projects/' + logging.projectId],
})
Expand All @@ -520,6 +523,32 @@ describe('Logging', () => {
await logging.getEntries(options);
});

it('should append default timestamp to existing filters', async () => {
const options = {filter: 'test'};

logging.loggingService.listLogEntries = async (
// eslint-disable-next-line @typescript-eslint/no-explicit-any
reqOpts: any,
gaxOpts: {}
) => {
assert.deepStrictEqual(
reqOpts,
extend(options, {
filter: reqOpts?.filter,
orderBy: 'timestamp desc',
resourceNames: ['projects/' + logging.projectId],
})
);
assert.deepStrictEqual(gaxOpts, {
autoPaginate: undefined,
});
assert.ok(reqOpts?.filter.includes('test AND timestamp'));
return [[]];
};

await logging.getEntries(options);
});

it('should not push the same resourceName again', async () => {
const options = {
resourceNames: ['projects/' + logging.projectId],
Expand Down Expand Up @@ -567,12 +596,14 @@ describe('Logging', () => {
assert.deepStrictEqual(reqOpts, {
a: 'b',
c: 'd',
filter: reqOpts?.filter,
orderBy: 'timestamp desc',
resourceNames: ['projects/' + logging.projectId],
});
// eslint-disable-next-line @typescript-eslint/no-explicit-any
assert.strictEqual((reqOpts as any).gaxOptions, undefined);
assert.deepStrictEqual(gaxOpts, options.gaxOptions);
assert.ok(reqOpts?.filter.includes('timestamp'));
return [[]];
};

Expand Down