[Security Solution][Alerts] improves performance of new terms multi fields #145167
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
vitaliidm
added
performance
Team: SecuritySolution
…ithub.com/vitaliidm/kibana into alerts/new_terms_perfromance_improvements
…-ref HEAD~1..HEAD --fix'
…ithub.com/vitaliidm/kibana into alerts/new_terms_perfromance_improvements
juliaElastic
approved these changes
Nov 16, 2022
…ook.js --ref HEAD~1..HEAD --fix'" This reverts commit 5308301.
…-ref HEAD~1..HEAD --fix'
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
botelastic
bot
added
the
Team:Fleet
|
Pinging @elastic/fleet (Team:Fleet) |
vitaliidm
added
release_note:skip
x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/new_terms/utils.test.ts
Outdated
Show resolved
Hide resolved
x-pack/plugins/security_solution/server/lib/detection_engine/rule_types/new_terms/utils.test.ts
Outdated
Show resolved
Hide resolved
| export const getNewTermsRuntimeMappings = ( | ||
| newTermsFields: string[] | ||
| newTermsFields: string[], | ||
| values?: Record<string, Record<string, boolean>> |
There was a problem hiding this comment.
values seems like it should be required since the runtime field now depends on it to work correctly. Both createFieldValuesMap and getNewTermsRuntimeMappings also depend on the number of new terms fields - could it be easier to pass buckets in to getNewTermsRuntimeMappings and call createFieldValuesMap after the if (newTermsFields.length <= 1) check in this function?
| @@ -193,6 +194,7 @@ export const createNewTermsAlertType = ( | |||
| } | |||
| const bucketsForField = searchResultWithAggs.aggregations.new_terms.buckets; | |||
| const includeValues = transformBucketsToValues(params.newTermsFields, bucketsForField); | |||
| const fieldsValuesMap = createFieldValuesMap(params.newTermsFields, bucketsForField); | |||
There was a problem hiding this comment.
nit: could make this something like
const newTermsRuntimeMappings = getNewTermsRuntimeMappings(...);
and reuse the same runtime mappings variable for phase 2 and 3 searches
| @@ -27,4 +27,3 @@ The new terms rule type reuses the singleSearchAfter function which implements t | |||
| ## Limitations and future enhancements | |||
|
|
|||
| - Value list exceptions are not supported at the moment. Commit ead04ce removes an experimental method I tried for evaluating value list exceptions. | |||
| - Runtime field supports only 100 emitted values. So for large arrays or combination of values greater than 100, results may not be exhaustive. This applies only to new terms with multiple fields | |||
There was a problem hiding this comment.
This limitation could still be hit, right? It's just less likely now
There was a problem hiding this comment.
Updated README with recent findings
| runtimeMappings: getNewTermsRuntimeMappings(['user.name', 'user.id']), | ||
| runtimeMappings: getNewTermsRuntimeMappings(['user.name', 'user.id'], { | ||
| 'user.name': { 'user-0': true }, | ||
| 'user.id': { '0': true }, |
There was a problem hiding this comment.
Could you add a test to verify that numeric fields like user.id are encoded the same way by transformBucketsToValues as they are by the runtime field? A full rule preview execution searching for first_doc seems like the most robust way to check, but extracting the expectedEncodedValues constant and using it in both this test and a test of transformBucketsToValues could also work.
Since we rely on the encoding of values in Kibana to match up with the encoding in Elasticsearch, we should be extra careful to verify that they match up for both string and numeric source data types.
There was a problem hiding this comment.
Values in painless script and in Kibana are converted to string before encoding.
Plus, there is a set of tests for various types: as this one https://github.com/elastic/kibana/blob/main/x-pack/test/detection_engine_api_integration/security_and_spaces/rule_execution_logic/new_terms.ts#L330-L344
It basically plays role as integration test between transformBucketsToValues and runtime script, as if encoding differs, there wouldn't be generated alerts. So, I think we covered here for this and similar use cases
…ule_types/new_terms/utils.test.ts Co-authored-by: Marshall Main <55718608+marshallmain@users.noreply.github.com>
…ule_types/new_terms/utils.test.ts Co-authored-by: Marshall Main <55718608+marshallmain@users.noreply.github.com>
💛 Build succeeded, but was flaky
Failed CI StepsTest FailuresMetrics [docs]Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @vitaliidm |
marshallmain
approved these changes
Nov 17, 2022
kibanamachine
pushed a commit
to kibanamachine/kibana
that referenced
this pull request
Nov 18, 2022
…ields (elastic#145167) ## Summary This PR improves performance of new terms multiple fields implementation released in elastic#143943 In comparison with single value fields it's faster in 2-2.5 times In comparison with array field the win even more significant: 3-4 times ### Technical implementation: Value for runtime field is emitted only if any of new terms fields present in `include` clause of terms aggregation. It's achieved by passing a values map of new terms fields to the runtime script and checking new terms values in it. So, there is a significant performance wins if runtime field is not matched with includes values: - new terms fields are not encoded in base64 within runtime script - runtime script doesn't emit any values - runtime field doesn't have any value to be compared against during aggregation, as its empty - it eliminates possible unyielding execution branches early: if one of items in array of first new terms field is not present in values map, no need to run through the rest of combinations As a result, this implementation overcomes the issue with non exhaustive results due to the large number of emitted fields. ES doesn't allow emitting more than 100 values in the runtime script, so if the number of all combinations in new terms fields is greater than 100, only the first 100 combinations will be used for terms aggregation. With this new implementation only matched fields will be emitting. Even if its number is greater than 100, we will hit circuit breaker in Security Solution: as rule run can't generate more than 100 alerts ### Performance measurements Implementation | Shards | Docs per shard | Simultaneous Rule Executions | Fields cardinality | Rule Execution Time (improved) | Rule Execution Time (old) -- | -- | -- | -- | -- | -- | -- Terms 1 field | 10 | 900,000 | 1 | 100,000 | 7s | Terms 2 fields | 10 | 900,000 | 1 | 100,000 | 17s | 33s Terms 2 fields | 10 | 900,000 | 2 | 100,000 | 19s | Terms 3 fields | 10 | 900,000 | 1 | 100,000 | 18s | 46s Terms 3 fields | 10 | 900,000 | 2 | 100,000 | 20s | | | | | | | Terms 1 field | 20 | 900,000 | 1 | 100,000 | 10.5s | Terms 2 fields | 20 | 900,000 | 1 | 100,000 | 28s | 55s Terms 2 fields | 20 | 900,000 | 2 | 100,000 | 28.5s | 56s Terms 3 fields | 20 | 900,000 | 1 | 100,000 | 30s | 75s Terms 3 fields | 20 | 900,000 | 2 | 100,000 | 31s | 75s | | | | | | Terms 1 field | 10 | 1,800,000 | 1 | 100,000 | 7s | Terms 2 fields | 10 | 1,800,000 | 1 | 100,000 | 24s | 50s Terms 3 fields | 10 | 1,800,000 | 1 | 100,000 | 26s | 68s | | | | | | array of unique values length 10 | | | | | | Terms 1 field | 10 | 900,000 | 1 | 100,000 | 9.5s | Terms 2 fields | 10 | 900,000 | 1 | 100,000 | 75s | 3.5m Terms 3 fields | 10 | 900,000 | 1 | 100,000 | 83s | 6m ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Marshall Main <55718608+marshallmain@users.noreply.github.com> (cherry picked from commit b985ec1)
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
kibanamachine
added a commit
that referenced
this pull request
Nov 18, 2022
…ulti fields (#145167) (#145697) # Backport This will backport the following commits from `main` to `8.6`: - [[Security Solution][Alerts] improves performance of new terms multi fields (#145167)](#145167) <!--- Backport version: 8.9.7 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Vitalii Dmyterko","email":"92328789+vitaliidm@users.noreply.github.com"},"sourceCommit":{"committedDate":"2022-11-18T09:02:15Z","message":"[Security Solution][Alerts] improves performance of new terms multi fields (#145167)\n\n## Summary\r\n\r\nThis PR improves performance of new terms multiple fields implementation\r\nreleased in https://github.com/elastic/kibana/pull/143943\r\nIn comparison with single value fields it's faster in 2-2.5 times\r\nIn comparison with array field the win even more significant: 3-4 times\r\n\r\n### Technical implementation:\r\nValue for runtime field is emitted only if any of new terms fields\r\npresent in `include` clause of terms aggregation. It's achieved by\r\npassing a values map of new terms fields to the runtime script and\r\nchecking new terms values in it. So, there is a significant performance\r\nwins if runtime field is not matched with includes values:\r\n - new terms fields are not encoded in base64 within runtime script\r\n - runtime script doesn't emit any values\r\n- runtime field doesn't have any value to be compared against during\r\naggregation, as its empty\r\n- it eliminates possible unyielding execution branches early: if one of\r\nitems in array of first new terms field is not present in values map, no\r\nneed to run through the rest of combinations\r\n\r\nAs a result, this implementation overcomes the issue with non exhaustive\r\nresults due to the large number of emitted fields.\r\nES doesn't allow emitting more than 100 values in the runtime script, so\r\nif the number of all combinations in new terms fields is greater than\r\n100, only the first 100 combinations will be used for terms aggregation.\r\nWith this new implementation only matched fields will be emitting. Even\r\nif its number is greater than 100, we will hit circuit breaker in\r\nSecurity Solution: as rule run can't generate more than 100 alerts\r\n\r\n### Performance measurements\r\n\r\n\r\nImplementation | Shards | Docs per shard | Simultaneous Rule Executions\r\n| Fields cardinality | Rule Execution Time (improved) | Rule Execution\r\nTime (old)\r\n-- | -- | -- | -- | -- | -- | --\r\nTerms 1 field | 10 | 900,000 | 1 | 100,000 | 7s | \r\nTerms 2 fields | 10 | 900,000 | 1 | 100,000 | 17s | 33s\r\nTerms 2 fields | 10 | 900,000 | 2 | 100,000 | 19s | \r\nTerms 3 fields | 10 | 900,000 | 1 | 100,000 | 18s | 46s\r\nTerms 3 fields | 10 | 900,000 | 2 | 100,000 | 20s | \r\n | | | | | | \r\nTerms 1 field | 20 | 900,000 | 1 | 100,000 | 10.5s | \r\nTerms 2 fields | 20 | 900,000 | 1 | 100,000 | 28s | 55s\r\nTerms 2 fields | 20 | 900,000 | 2 | 100,000 | 28.5s | 56s\r\nTerms 3 fields | 20 | 900,000 | 1 | 100,000 | 30s | 75s\r\nTerms 3 fields | 20 | 900,000 | 2 | 100,000 | 31s | 75s\r\n | | | | | | \r\nTerms 1 field | 10 | 1,800,000 | 1 | 100,000 | 7s | \r\nTerms 2 fields | 10 | 1,800,000 | 1 | 100,000 | 24s | 50s\r\nTerms 3 fields | 10 | 1,800,000 | 1 | 100,000 | 26s | 68s\r\n | | | | | | \r\narray of unique values length 10 | | | | | | \r\nTerms 1 field | 10 | 900,000 | 1 | 100,000 | 9.5s | \r\nTerms 2 fields | 10 | 900,000 | 1 | 100,000 | 75s | 3.5m\r\nTerms 3 fields | 10 | 900,000 | 1 | 100,000 | 83s | 6m\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by: Marshall Main <55718608+marshallmain@users.noreply.github.com>","sha":"b985ec1735d432bf4cf13fb4a4610dabb5c987c6","branchLabelMapping":{"^v8.7.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["performance","release_note:skip","Team: SecuritySolution","Team:Detection Alerts","backport:prev-minor","v8.6.0","v8.7.0"],"number":145167,"url":"https://github.com/elastic/kibana/pull/145167","mergeCommit":{"message":"[Security Solution][Alerts] improves performance of new terms multi fields (#145167)\n\n## Summary\r\n\r\nThis PR improves performance of new terms multiple fields implementation\r\nreleased in https://github.com/elastic/kibana/pull/143943\r\nIn comparison with single value fields it's faster in 2-2.5 times\r\nIn comparison with array field the win even more significant: 3-4 times\r\n\r\n### Technical implementation:\r\nValue for runtime field is emitted only if any of new terms fields\r\npresent in `include` clause of terms aggregation. It's achieved by\r\npassing a values map of new terms fields to the runtime script and\r\nchecking new terms values in it. So, there is a significant performance\r\nwins if runtime field is not matched with includes values:\r\n - new terms fields are not encoded in base64 within runtime script\r\n - runtime script doesn't emit any values\r\n- runtime field doesn't have any value to be compared against during\r\naggregation, as its empty\r\n- it eliminates possible unyielding execution branches early: if one of\r\nitems in array of first new terms field is not present in values map, no\r\nneed to run through the rest of combinations\r\n\r\nAs a result, this implementation overcomes the issue with non exhaustive\r\nresults due to the large number of emitted fields.\r\nES doesn't allow emitting more than 100 values in the runtime script, so\r\nif the number of all combinations in new terms fields is greater than\r\n100, only the first 100 combinations will be used for terms aggregation.\r\nWith this new implementation only matched fields will be emitting. Even\r\nif its number is greater than 100, we will hit circuit breaker in\r\nSecurity Solution: as rule run can't generate more than 100 alerts\r\n\r\n### Performance measurements\r\n\r\n\r\nImplementation | Shards | Docs per shard | Simultaneous Rule Executions\r\n| Fields cardinality | Rule Execution Time (improved) | Rule Execution\r\nTime (old)\r\n-- | -- | -- | -- | -- | -- | --\r\nTerms 1 field | 10 | 900,000 | 1 | 100,000 | 7s | \r\nTerms 2 fields | 10 | 900,000 | 1 | 100,000 | 17s | 33s\r\nTerms 2 fields | 10 | 900,000 | 2 | 100,000 | 19s | \r\nTerms 3 fields | 10 | 900,000 | 1 | 100,000 | 18s | 46s\r\nTerms 3 fields | 10 | 900,000 | 2 | 100,000 | 20s | \r\n | | | | | | \r\nTerms 1 field | 20 | 900,000 | 1 | 100,000 | 10.5s | \r\nTerms 2 fields | 20 | 900,000 | 1 | 100,000 | 28s | 55s\r\nTerms 2 fields | 20 | 900,000 | 2 | 100,000 | 28.5s | 56s\r\nTerms 3 fields | 20 | 900,000 | 1 | 100,000 | 30s | 75s\r\nTerms 3 fields | 20 | 900,000 | 2 | 100,000 | 31s | 75s\r\n | | | | | | \r\nTerms 1 field | 10 | 1,800,000 | 1 | 100,000 | 7s | \r\nTerms 2 fields | 10 | 1,800,000 | 1 | 100,000 | 24s | 50s\r\nTerms 3 fields | 10 | 1,800,000 | 1 | 100,000 | 26s | 68s\r\n | | | | | | \r\narray of unique values length 10 | | | | | | \r\nTerms 1 field | 10 | 900,000 | 1 | 100,000 | 9.5s | \r\nTerms 2 fields | 10 | 900,000 | 1 | 100,000 | 75s | 3.5m\r\nTerms 3 fields | 10 | 900,000 | 1 | 100,000 | 83s | 6m\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by: Marshall Main <55718608+marshallmain@users.noreply.github.com>","sha":"b985ec1735d432bf4cf13fb4a4610dabb5c987c6"}},"sourceBranch":"main","suggestedTargetBranches":["8.6"],"targetPullRequestStates":[{"branch":"8.6","label":"v8.6.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.7.0","labelRegex":"^v8.7.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/145167","number":145167,"mergeCommit":{"message":"[Security Solution][Alerts] improves performance of new terms multi fields (#145167)\n\n## Summary\r\n\r\nThis PR improves performance of new terms multiple fields implementation\r\nreleased in https://github.com/elastic/kibana/pull/143943\r\nIn comparison with single value fields it's faster in 2-2.5 times\r\nIn comparison with array field the win even more significant: 3-4 times\r\n\r\n### Technical implementation:\r\nValue for runtime field is emitted only if any of new terms fields\r\npresent in `include` clause of terms aggregation. It's achieved by\r\npassing a values map of new terms fields to the runtime script and\r\nchecking new terms values in it. So, there is a significant performance\r\nwins if runtime field is not matched with includes values:\r\n - new terms fields are not encoded in base64 within runtime script\r\n - runtime script doesn't emit any values\r\n- runtime field doesn't have any value to be compared against during\r\naggregation, as its empty\r\n- it eliminates possible unyielding execution branches early: if one of\r\nitems in array of first new terms field is not present in values map, no\r\nneed to run through the rest of combinations\r\n\r\nAs a result, this implementation overcomes the issue with non exhaustive\r\nresults due to the large number of emitted fields.\r\nES doesn't allow emitting more than 100 values in the runtime script, so\r\nif the number of all combinations in new terms fields is greater than\r\n100, only the first 100 combinations will be used for terms aggregation.\r\nWith this new implementation only matched fields will be emitting. Even\r\nif its number is greater than 100, we will hit circuit breaker in\r\nSecurity Solution: as rule run can't generate more than 100 alerts\r\n\r\n### Performance measurements\r\n\r\n\r\nImplementation | Shards | Docs per shard | Simultaneous Rule Executions\r\n| Fields cardinality | Rule Execution Time (improved) | Rule Execution\r\nTime (old)\r\n-- | -- | -- | -- | -- | -- | --\r\nTerms 1 field | 10 | 900,000 | 1 | 100,000 | 7s | \r\nTerms 2 fields | 10 | 900,000 | 1 | 100,000 | 17s | 33s\r\nTerms 2 fields | 10 | 900,000 | 2 | 100,000 | 19s | \r\nTerms 3 fields | 10 | 900,000 | 1 | 100,000 | 18s | 46s\r\nTerms 3 fields | 10 | 900,000 | 2 | 100,000 | 20s | \r\n | | | | | | \r\nTerms 1 field | 20 | 900,000 | 1 | 100,000 | 10.5s | \r\nTerms 2 fields | 20 | 900,000 | 1 | 100,000 | 28s | 55s\r\nTerms 2 fields | 20 | 900,000 | 2 | 100,000 | 28.5s | 56s\r\nTerms 3 fields | 20 | 900,000 | 1 | 100,000 | 30s | 75s\r\nTerms 3 fields | 20 | 900,000 | 2 | 100,000 | 31s | 75s\r\n | | | | | | \r\nTerms 1 field | 10 | 1,800,000 | 1 | 100,000 | 7s | \r\nTerms 2 fields | 10 | 1,800,000 | 1 | 100,000 | 24s | 50s\r\nTerms 3 fields | 10 | 1,800,000 | 1 | 100,000 | 26s | 68s\r\n | | | | | | \r\narray of unique values length 10 | | | | | | \r\nTerms 1 field | 10 | 900,000 | 1 | 100,000 | 9.5s | \r\nTerms 2 fields | 10 | 900,000 | 1 | 100,000 | 75s | 3.5m\r\nTerms 3 fields | 10 | 900,000 | 1 | 100,000 | 83s | 6m\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [x] [Unit or functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere updated or added to match the most common scenarios\r\n\r\nCo-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>\r\nCo-authored-by: Marshall Main <55718608+marshallmain@users.noreply.github.com>","sha":"b985ec1735d432bf4cf13fb4a4610dabb5c987c6"}}]}] BACKPORT--> Co-authored-by: Vitalii Dmyterko <92328789+vitaliidm@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR improves performance of new terms multiple fields implementation released in #143943
In comparison with single value fields it's faster in 2-2.5 times
In comparison with array field the win even more significant: 3-4 times
Technical implementation:
Value for runtime field is emitted only if any of new terms fields present in
includeclause of terms aggregation. It's achieved by passing a values map of new terms fields to the runtime script and checking new terms values in it. So, there is a significant performance wins if runtime field is not matched with includes values:As a result, this implementation overcomes the issue with non exhaustive results due to the large number of emitted fields.
ES doesn't allow emitting more than 100 values in the runtime script, so if the number of all combinations in new terms fields is greater than 100, only the first 100 combinations will be used for terms aggregation.
With this new implementation only matched fields will be emitting. Even if its number is greater than 100, we will hit circuit breaker in Security Solution: as rule run can't generate more than 100 alerts
Performance measurements
Checklist
Delete any items that are not applicable to this PR.