Skip to content

Comments

[Security Solution][Telemetry] Paginate ES queries#177263

Merged
szaffarano merged 29 commits intomainfrom
szaffarano/receiver-review
Apr 4, 2024
Merged

[Security Solution][Telemetry] Paginate ES queries#177263
szaffarano merged 29 commits intomainfrom
szaffarano/receiver-review

Conversation

@szaffarano
Copy link
Contributor

@szaffarano szaffarano commented Feb 20, 2024 •
edited
Loading

Summary

Paginate ES queries

This PR adds the ITelemetryReceiver#paginate method to execute Elasticsearch queries using a PIT to paginate the responses. The number of documents per page is calculated using a sample set of ITelemetryReceiver#numDocsToSample documents for the given query to get an estimated average, and finally, ITelemetryReceiver#maxPageSizeBytes is divided by this average value.

It's possible to configure the behavior with the following parameters:

  • ITelemetryReceiver#numDocsToSample: Defaults to 10 documents.
  • ITelemetryReceiver#maxPageSizeBytes: Defaults to min("2% of host's total memory", 80MiB).

Endpoint Meta Task

One of the heaviest ES queries this task executes pulls a whole endpoint metrics document per agent. Since some clusters may (literally) have thousands of agents, it means the query response could potentially be expensive. Improvements:

  • Change all queries only to include the needed document fields instead of the whole document
  • Change the endpoint metrics query to return a list of document IDs instead of the document itself, and then run a second query using the paginate mechanism (see above for further details) to get the documents before sending the telemetry data.
  • Small refactorings trying to simplify the task logic

WIP

  • Expose numDocsToSample and maxPageSizeBytes as part of the telemetry-buffer-and-batch-sizes-v1 artifact to allow dynamics updates.
  • Use ITelemetryReceiver#paginate across the telemetry tasks.
  • Add more integration tests.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@szaffarano szaffarano requested review from a team as code owners February 20, 2024 10:29
@szaffarano szaffarano added Feature:Telemetry release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. labels Feb 20, 2024
@elasticmachine
Copy link
Contributor

elasticmachine commented Feb 20, 2024

Pinging @elastic/security-solution (Team: SecuritySolution)

@szaffarano szaffarano changed the title [Security Solution][Telemetry] Improve ES queries [Security Solution][Telemetry] Paginate ES queries Feb 20, 2024
donaherc
donaherc approved these changes Mar 20, 2024
View reviewed changes
Copy link
Contributor

@donaherc donaherc left a comment

Choose a reason for hiding this comment

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

Generally LGTM, had a couple questions.

@szaffarano szaffarano requested a review from a team as a code owner March 20, 2024 13:12
@szaffarano szaffarano requested a review from JDKurma March 22, 2024 14:43
JDKurma
JDKurma approved these changes Mar 28, 2024
View reviewed changes
Copy link
Contributor

@JDKurma JDKurma left a comment

Choose a reason for hiding this comment

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

LGTM!

@szaffarano szaffarano enabled auto-merge (squash) April 2, 2024 11:10
@banderror banderror requested review from a team, banderror and dhurley14 April 2, 2024 11:55
@szaffarano szaffarano disabled auto-merge April 2, 2024 12:46
@kibana-ci
Copy link

kibana-ci commented Apr 2, 2024

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@szaffarano szaffarano merged commit 655b916 into main Apr 4, 2024
@szaffarano szaffarano deleted the szaffarano/receiver-review branch April 4, 2024 08:33
@kibanamachine kibanamachine added v8.14.0 backport:skip This PR does not require backporting labels Apr 4, 2024
@szaffarano szaffarano mentioned this pull request Jul 9, 2024
Merged
3 tasks
szaffarano added a commit that referenced this pull request Jul 12, 2024
@szaffarano
[Telemetry][Security Solution] Use the proper index to query builtin …
a120c51 
…alerts (#187859)

## Summary

#177263 changed the way
`telemetry-prebuilt-rule-alerts` get data from elastic, but it changed
the index used to run the queries. This PR fixes it using the proper
index.
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Jul 12, 2024
@szaffarano
[Telemetry][Security Solution] Use the proper index to query builtin …
d2ea7e7 
…alerts (elastic#187859)

## Summary

elastic#177263 changed the way
`telemetry-prebuilt-rule-alerts` get data from elastic, but it changed
the index used to run the queries. This PR fixes it using the proper
index.

(cherry picked from commit a120c51)
szaffarano added a commit that referenced this pull request Jul 12, 2024
@szaffarano
[Telemetry][Security Solution] Use the proper index to query builtin …
cb3118c 
…alerts (#187859)

#177263 changed the way
`telemetry-prebuilt-rule-alerts` get data from elastic, but it changed
the index used to run the queries. This PR fixes it using the proper
index.

(cherry picked from commit a120c51)
kibanamachine added a commit that referenced this pull request Jul 12, 2024
@kibanamachine @szaffarano
[8.15] [Telemetry][Security Solution] Use the proper index to query b…
e6c0563 
…uiltin alerts (#187859) (#188217)

# Backport

This will backport the following commits from `main` to `8.15`:
- [[Telemetry][Security Solution] Use the proper index to query builtin
alerts (#187859)](#187859)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Sebastián
Zaffarano","email":"sebastian.zaffarano@elastic.co"},"sourceCommit":{"committedDate":"2024-07-12T13:17:43Z","message":"[Telemetry][Security
Solution] Use the proper index to query builtin alerts (#187859)\n\n##
Summary\r\n\r\nhttps://github.com//pull/177263 changed the
way\r\n`telemetry-prebuilt-rule-alerts` get data from elastic, but it
changed\r\nthe index used to run the queries. This PR fixes it using the
proper\r\nindex.","sha":"a120c510b9738aab0fb5f9296515a82f6f0792a6","branchLabelMapping":{"^v8.16.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:
SecuritySolution","v8.14.0","v8.15.0","v8.16.0"],"title":"[Telemetry][Security
Solution] Use the proper index to query builtin
alerts","number":187859,"url":"https://github.com/elastic/kibana/pull/187859","mergeCommit":{"message":"[Telemetry][Security
Solution] Use the proper index to query builtin alerts (#187859)\n\n##
Summary\r\n\r\nhttps://github.com//pull/177263 changed the
way\r\n`telemetry-prebuilt-rule-alerts` get data from elastic, but it
changed\r\nthe index used to run the queries. This PR fixes it using the
proper\r\nindex.","sha":"a120c510b9738aab0fb5f9296515a82f6f0792a6"}},"sourceBranch":"main","suggestedTargetBranches":["8.14","8.15"],"targetPullRequestStates":[{"branch":"8.14","label":"v8.14.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.15","label":"v8.15.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/187859","number":187859,"mergeCommit":{"message":"[Telemetry][Security
Solution] Use the proper index to query builtin alerts (#187859)\n\n##
Summary\r\n\r\nhttps://github.com//pull/177263 changed the
way\r\n`telemetry-prebuilt-rule-alerts` get data from elastic, but it
changed\r\nthe index used to run the queries. This PR fixes it using the
proper\r\nindex.","sha":"a120c510b9738aab0fb5f9296515a82f6f0792a6"}}]}]
BACKPORT-->

Co-authored-by: Sebastián Zaffarano <sebastian.zaffarano@elastic.co>
szaffarano added a commit that referenced this pull request Jul 15, 2024
@szaffarano @elasticmachine
[8.14] [Telemetry][Security Solution] Use the proper index to query b…
655f25e 
…uiltin alerts (#187859) (#188235)

# Backport

This will backport the following commits from `main` to `8.14`:
- [[Telemetry][Security Solution] Use the proper index to query builtin
alerts (#187859)](#187859)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Sebastián
Zaffarano","email":"sebastian.zaffarano@elastic.co"},"sourceCommit":{"committedDate":"2024-07-12T13:17:43Z","message":"[Telemetry][Security
Solution] Use the proper index to query builtin alerts (#187859)\n\n##
Summary\r\n\r\nhttps://github.com//pull/177263 changed the
way\r\n`telemetry-prebuilt-rule-alerts` get data from elastic, but it
changed\r\nthe index used to run the queries. This PR fixes it using the
proper\r\nindex.","sha":"a120c510b9738aab0fb5f9296515a82f6f0792a6","branchLabelMapping":{"^v8.16.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:
SecuritySolution","v8.14.0","v8.15.0","v8.16.0"],"title":"[Telemetry][Security
Solution] Use the proper index to query builtin
alerts","number":187859,"url":"https://github.com/elastic/kibana/pull/187859","mergeCommit":{"message":"[Telemetry][Security
Solution] Use the proper index to query builtin alerts (#187859)\n\n##
Summary\r\n\r\nhttps://github.com//pull/177263 changed the
way\r\n`telemetry-prebuilt-rule-alerts` get data from elastic, but it
changed\r\nthe index used to run the queries. This PR fixes it using the
proper\r\nindex.","sha":"a120c510b9738aab0fb5f9296515a82f6f0792a6"}},"sourceBranch":"main","suggestedTargetBranches":["8.14","8.15"],"targetPullRequestStates":[{"branch":"8.14","label":"v8.14.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.15","label":"v8.15.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/187859","number":187859,"mergeCommit":{"message":"[Telemetry][Security
Solution] Use the proper index to query builtin alerts (#187859)\n\n##
Summary\r\n\r\nhttps://github.com//pull/177263 changed the
way\r\n`telemetry-prebuilt-rule-alerts` get data from elastic, but it
changed\r\nthe index used to run the queries. This PR fixes it using the
proper\r\nindex.","sha":"a120c510b9738aab0fb5f9296515a82f6f0792a6"}}]}]
BACKPORT-->

Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dhurley14 dhurley14 dhurley14 approved these changes

@donaherc donaherc donaherc approved these changes

@JDKurma JDKurma JDKurma approved these changes

@banderror banderror Awaiting requested review from banderror banderror was automatically assigned from elastic/security-detection-rule-management

Assignees

No one assigned

Labels

backport:skip This PR does not require backporting Feature:Telemetry release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.14.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@szaffarano @elasticmachine @kibana-ci @dhurley14 @donaherc @JDKurma @kibanamachine