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

[Response Ops][Alerting] Remove rules client from alerting task runner #194300

Merged
merged 6 commits into from
Oct 3, 2024
Merged

[Response Ops][Alerting] Remove rules client from alerting task runner #194300

merged 6 commits into from
Oct 3, 2024
+511 −493

Conversation

ymao1
Copy link
Contributor

@ymao1 ymao1 commented Sep 27, 2024 •
edited by kibanamachine
Loading

Summary

Initializing the rules client with every rule execution causes 2 requests to Elasticsearch, a POST /_security/user/_has_privileges request and a GET/.kibana_<version>/_doc/space:<spaceId> request. We only use the rules client to proxy two methods getAlertFromRaw and clearExpiredSnoozes. Both of these methods can be converted into library functions that can be used without the rules client. This PR makes that conversion.

ymao1
ymao1 commented Sep 27, 2024
View reviewed changes
x-pack/plugins/alerting/server/task_runner/lib/clear_expired_snoozes.ts Outdated
return true;
}
})
: [];

if (snoozeSchedule.length === rule.snoozeSchedule?.length) return;

const updateAttributes = updateMeta(context, {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This updateMeta function adds some fields if we're updating the apiKey or apiKeyOwner fields, which we're not.

ymao1
ymao1 commented Sep 27, 2024
View reviewed changes
x-pack/plugins/alerting/server/task_runner/lib/clear_expired_snoozes.ts Outdated
snoozeSchedule,
updatedBy: await context.getUserName(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since it's the alerting framework updating the saved object, we should not need to identify a username for this update, we don't do it when updating the rule SO after each rule execution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also removed updatedAt since we don't update that value when we make other partial updates to the rule during rule execution and this limits the number of fields we allow to update via ES.

@ymao1 ymao1 changed the title Alerting/no rules client [Response Ops][Alerting] Remove rules client from alerting task runner Sep 27, 2024
@ymao1 ymao1 self-assigned this Sep 30, 2024
@ymao1 ymao1 added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v9.0.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.16.0 labels Sep 30, 2024
@ymao1 ymao1 marked this pull request as ready for review September 30, 2024 16:47
@ymao1 ymao1 requested a review from a team as a code owner September 30, 2024 16:47
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

pmuellr
pmuellr approved these changes Oct 2, 2024
View reviewed changes
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

LGTM

@ymao1
Copy link
Contributor Author

ymao1 commented Oct 3, 2024

@elasticmachine merge upstream

guskovaue
guskovaue approved these changes Oct 3, 2024
View reviewed changes
Copy link
Contributor

@guskovaue guskovaue left a comment

Choose a reason for hiding this comment

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

Great improvements!

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #94 / Reporting Functional Tests with Security enabled Access to Management > Reporting Download report user can access download link for export type that is no longer supported

Metrics [docs]

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
alerting 52 50 -2

History

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

cc @ymao1

@ymao1 ymao1 merged commit 326f813 into elastic:main Oct 3, 2024
44 checks passed
@ymao1 ymao1 deleted the alerting/no-rules-client branch October 3, 2024 15:53
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11165341287

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 3, 2024
@ymao1
[Response Ops][Alerting] Remove rules client from alerting task runner (
2367fa0 
elastic#194300)

## Summary

Initializing the rules client with every rule execution causes 2
requests to Elasticsearch, a `POST /_security/user/_has_privileges`
request and a `GET/.kibana_<version>/_doc/space:<spaceId>` request. We
only use the rules client to proxy two methods `getAlertFromRaw` and
`clearExpiredSnoozes`. Both of these methods can be converted into
library functions that can be used without the rules client. This PR
makes that conversion.

---------

Co-authored-by: Elastic Machine <[email protected]>
(cherry picked from commit 326f813)
@kibanamachine kibanamachine mentioned this pull request Oct 3, 2024
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.x

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 Oct 3, 2024
@kibanamachine @ymao1
[8.x] [Response Ops][Alerting] Remove rules client from alerting task…
23e1bdf 
… runner (#194300) (#194858)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Response Ops][Alerting] Remove rules client from alerting task
runner (#194300)](#194300)

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

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

<!--BACKPORT [{"author":{"name":"Ying
Mao","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-03T15:53:06Z","message":"[Response
Ops][Alerting] Remove rules client from alerting task runner
(#194300)\n\n## Summary\r\n\r\nInitializing the rules client with every
rule execution causes 2\r\nrequests to Elasticsearch, a `POST
/_security/user/_has_privileges`\r\nrequest and a
`GET/.kibana_<version>/_doc/space:<spaceId>` request. We\r\nonly use the
rules client to proxy two methods `getAlertFromRaw`
and\r\n`clearExpiredSnoozes`. Both of these methods can be converted
into\r\nlibrary functions that can be used without the rules client.
This PR\r\nmakes that
conversion.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"326f81398472e160fec254c05541aa2a588a7f70","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Alerting","release_note:skip","Team:ResponseOps","v9.0.0","backport:prev-minor","v8.16.0"],"title":"[Response
Ops][Alerting] Remove rules client from alerting task
runner","number":194300,"url":"https://github.com/elastic/kibana/pull/194300","mergeCommit":{"message":"[Response
Ops][Alerting] Remove rules client from alerting task runner
(#194300)\n\n## Summary\r\n\r\nInitializing the rules client with every
rule execution causes 2\r\nrequests to Elasticsearch, a `POST
/_security/user/_has_privileges`\r\nrequest and a
`GET/.kibana_<version>/_doc/space:<spaceId>` request. We\r\nonly use the
rules client to proxy two methods `getAlertFromRaw`
and\r\n`clearExpiredSnoozes`. Both of these methods can be converted
into\r\nlibrary functions that can be used without the rules client.
This PR\r\nmakes that
conversion.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"326f81398472e160fec254c05541aa2a588a7f70"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/194300","number":194300,"mergeCommit":{"message":"[Response
Ops][Alerting] Remove rules client from alerting task runner
(#194300)\n\n## Summary\r\n\r\nInitializing the rules client with every
rule execution causes 2\r\nrequests to Elasticsearch, a `POST
/_security/user/_has_privileges`\r\nrequest and a
`GET/.kibana_<version>/_doc/space:<spaceId>` request. We\r\nonly use the
rules client to proxy two methods `getAlertFromRaw`
and\r\n`clearExpiredSnoozes`. Both of these methods can be converted
into\r\nlibrary functions that can be used without the rules client.
This PR\r\nmakes that
conversion.\r\n\r\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>","sha":"326f81398472e160fec254c05541aa2a588a7f70"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Ying Mao <[email protected]>
tiansivive pushed a commit to tiansivive/kibana that referenced this pull request Oct 7, 2024
@ymao1 @elasticmachine @tiansivive
[Response Ops][Alerting] Remove rules client from alerting task runner (
57298d1 
elastic#194300)

## Summary

Initializing the rules client with every rule execution causes 2
requests to Elasticsearch, a `POST /_security/user/_has_privileges`
request and a `GET/.kibana_<version>/_doc/space:<spaceId>` request. We
only use the rules client to proxy two methods `getAlertFromRaw` and
`clearExpiredSnoozes`. Both of these methods can be converted into
library functions that can be used without the rules client. This PR
makes that conversion.

---------

Co-authored-by: Elastic Machine <[email protected]>
tiansivive pushed a commit to tiansivive/kibana that referenced this pull request Oct 7, 2024
@ymao1 @elasticmachine @tiansivive
[Response Ops][Alerting] Remove rules client from alerting task runner (
a42f535 
elastic#194300)

## Summary

Initializing the rules client with every rule execution causes 2
requests to Elasticsearch, a `POST /_security/user/_has_privileges`
request and a `GET/.kibana_<version>/_doc/space:<spaceId>` request. We
only use the rules client to proxy two methods `getAlertFromRaw` and
`clearExpiredSnoozes`. Both of these methods can be converted into
library functions that can be used without the rules client. This PR
makes that conversion.

---------

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pmuellr pmuellr pmuellr approved these changes

@guskovaue guskovaue guskovaue approved these changes

Assignees

@ymao1 ymao1

Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.16.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ymao1 @elasticmachine @kibana-ci @kibanamachine @pmuellr @guskovaue