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

Jump to bottom

[KQL] Fix performance issue with nested subqueries #181208

Merged
merged 12 commits into from
Apr 27, 2024
Merged

[KQL] Fix performance issue with nested subqueries #181208

merged 12 commits into from
Apr 27, 2024

Conversation

lukasolson
Copy link
Member

@lukasolson lukasolson commented Apr 18, 2024 •
edited
Loading

Summary

Resolves #143335.

Some history: A similar issue was reported a few years back (#76811). The solution (#93319) was to use the --cache PEG.js parameter when generating the parser. Back when this was added, we were still manually building the parser on demand when it was changed. Eventually we added support for dynamically building the parser during the build process (#145615). I'm not sure where along the process the cache parameter got lost but it didn't appear to be used when we switched.

This PR re-adds this parameter which increases performance considerably (metrics shown in ops/sec):

Before using cache:

  ● kuery AST API › fromKueryExpression › performance › with simple expression
    Received:   7110.68990544415

  ● kuery AST API › fromKueryExpression › performance › with complex expression
    Received:   40.51361746242248

  ● kuery AST API › fromKueryExpression › performance › with many subqueries
    Received:   17.071767133068473

After using cache:

  ● kuery AST API › fromKueryExpression › performance › with simple expression
    Received:   8275.49109867502

  ● kuery AST API › fromKueryExpression › performance › with complex expression
    Received:   447.0459218892934

  ● kuery AST API › fromKueryExpression › performance › with many subqueries
    Received:   115852.43643466769

Checklist

@lukasolson lukasolson added performance Feature:KQL KQL Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.15.0 labels Apr 18, 2024
@lukasolson lukasolson self-assigned this Apr 18, 2024
@lukasolson
Copy link
Member Author

/ci

1 similar comment
@lukasolson
Copy link
Member Author

/ci

@lukasolson lukasolson force-pushed the kql/nested-subquery branch from c68e70f to bc5b485 Compare April 23, 2024 06:37
@lukasolson
Copy link
Member Author

/ci

@lukasolson
Copy link
Member Author

/ci

@lukasolson
Copy link
Member Author

/ci

@lukasolson lukasolson marked this pull request as ready for review April 24, 2024 22:16
@lukasolson lukasolson requested a review from a team as a code owner April 24, 2024 22:16
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

lukasolson
lukasolson commented Apr 24, 2024
View reviewed changes
packages/kbn-es-query/src/kuery/ast/ast.test.ts
@@ -16,8 +16,7 @@ import { nodeTypes } from '../node_types';
import { DataViewBase } from '../../..';
import { KueryNode } from '../types';
import { fields } from '../../filters/stubs';

jest.mock('../grammar');
Copy link
Member Author

Choose a reason for hiding this comment

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

This grammar file was not being updated as part of the build, and it doesn't need to be mocked any longer anyway.

lukasolson
lukasolson commented Apr 24, 2024
View reviewed changes
packages/kbn-es-query/src/kuery/grammar/__mocks__/grammar.js Outdated
Copy link
Member Author

Choose a reason for hiding this comment

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

This file was only used in the test above, and we no longer need it mocked.

lukasolson
lukasolson commented Apr 24, 2024
View reviewed changes
test/api_integration/apis/saved_objects/find.ts
});
expect(resp.body.error).to.be('Bad Request');
expect(resp.body.statusCode).to.be(400);
expect(resp.body.message).to.match(/KQLSyntaxError[\s\S]+Bad Request/);
Copy link
Member Author

Choose a reason for hiding this comment

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

This test is "KQL syntax error should return 400 with Bad Request" - With the changes in this PR, the format of the error messages in these tests changed slightly. We don't care about the exact content of the message.

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #10 / Lens Field Item should pass add filter callback and pass result to filter manager
  • [job] [logs] Jest Tests #10 / Lens Field Item should request field stats every time the button is clicked
  • [job] [logs] Jest Tests #9 / useCreateAttachments calls the api when invoked with the correct parameters

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
kbnUiSharedDeps-srcJs 2.9MB 2.9MB +2.8KB
Unknown metric groups

ESLint disabled in files

id before after diff
@kbn/es-query 1 0 -1

Total ESLint disabled count

id before after diff
@kbn/es-query 9 8 -1

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @lukasolson

mattkime
mattkime approved these changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

Code changes look good, works well - nice performance increase!

@lukasolson lukasolson merged commit c4df36e into elastic:main Apr 27, 2024
20 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattkime mattkime mattkime approved these changes

Assignees

@lukasolson lukasolson

Labels
backport:skip This commit does not require backporting Feature:KQL KQL performance release_note:fix Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.15.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

KQL parsing stuck when query has multiple brackets
5 participants
@lukasolson @elasticmachine @kibana-ci @mattkime @kibanamachine