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

[ML] Preserve field formatters between rule executions #178621

Merged
merged 6 commits into from
Mar 13, 2024
Merged

[ML] Preserve field formatters between rule executions #178621

merged 6 commits into from
Mar 13, 2024

Conversation

darnautov
Copy link
Contributor

@darnautov darnautov commented Mar 13, 2024 •
edited by kibanamachine
Loading

Summary

This PR adds caching for field formatters used for typcial and actual values in the alerting context.
Before the Data View service was initialized on each rule execution and called the field_caps API, which could cause performance issues, especially for large clusters and with a fairly frequent rule check interval.

Checklist

@darnautov darnautov added release_note:fix :ml Feature:Alerting/RuleTypes Issues related to specific Alerting Rules Types Team:ML Team label for ML (also use :ml) v8.13.0 v8.14.0 labels Mar 13, 2024
@darnautov darnautov requested a review from peteharverson March 13, 2024 12:30
@darnautov darnautov self-assigned this Mar 13, 2024
@darnautov darnautov requested a review from a team as a code owner March 13, 2024 12:30
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

peteharverson
peteharverson approved these changes Mar 13, 2024
View reviewed changes
Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

Tested and LGTM.

As discussed, unrelated to the changes here, the re-renders in the rule flyout when editing the rule is affecting usability.

edit_rule.mov

@darnautov
Copy link
Contributor Author

@elasticmachine merge upstream

jgowdyelastic
jgowdyelastic approved these changes Mar 13, 2024
View reviewed changes
Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

Code LGTM

peteharverson
peteharverson approved these changes Mar 13, 2024
View reviewed changes
Copy link
Contributor

@peteharverson peteharverson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Latest changes in 97c56d0 LGTM fixing the rerender issues mentioned in #178621 (review).

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
ml 3.7MB 3.7MB -9.0B

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
ml 96 97 +1

History

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

cc @darnautov

@darnautov darnautov merged commit 51235a9 into elastic:main Mar 13, 2024
16 checks passed
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 8.13 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 178621

Questions ?

Please refer to the Backport tool documentation

@darnautov darnautov deleted the ml-alerting-cache-field-caps branch March 14, 2024 10:09
@darnautov darnautov mentioned this pull request Mar 14, 2024
Merged
@darnautov
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
✅ 8.13

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

darnautov added a commit to darnautov/kibana that referenced this pull request Mar 14, 2024
@darnautov
[ML] Preserve field formatters between rule executions (elastic#178621)
5df2e8c 
(cherry picked from commit 51235a9)

# Conflicts:
#	x-pack/plugins/ml/server/lib/alerts/alerting_service.ts
#	x-pack/plugins/ml/server/lib/alerts/register_anomaly_detection_alert_type.ts
darnautov added a commit that referenced this pull request Mar 14, 2024
@darnautov
[8.13] [ML] Preserve field formatters between rule executions (#178621)…
3ec767c 
… (#178716)

# Backport

This will backport the following commits from `main` to `8.13`:
- [[ML] Preserve field formatters between rule executions
(#178621)](#178621)

<!--- Backport version: 8.9.8 -->

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

<!--BACKPORT [{"author":{"name":"Dima
Arnautov","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-03-13T17:35:35Z","message":"[ML]
Preserve field formatters between rule executions
(#178621)","sha":"51235a9c7a53d1a8ef9340f01c028389b9dda61e","branchLabelMapping":{"^v8.14.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:fix",":ml","Feature:Alerting/RuleTypes","Team:ML","v8.13.0","v8.14.0"],"number":178621,"url":"https://github.com/elastic/kibana/pull/178621","mergeCommit":{"message":"[ML]
Preserve field formatters between rule executions
(#178621)","sha":"51235a9c7a53d1a8ef9340f01c028389b9dda61e"}},"sourceBranch":"main","suggestedTargetBranches":["8.13"],"targetPullRequestStates":[{"branch":"8.13","label":"v8.13.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.14.0","labelRegex":"^v8.14.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/178621","number":178621,"mergeCommit":{"message":"[ML]
Preserve field formatters between rule executions
(#178621)","sha":"51235a9c7a53d1a8ef9340f01c028389b9dda61e"}}]}]
BACKPORT-->
@walterra walterra mentioned this pull request Apr 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peteharverson peteharverson peteharverson approved these changes

@jgowdyelastic jgowdyelastic jgowdyelastic approved these changes

Assignees

@darnautov darnautov

Labels
Feature:Alerting/RuleTypes Issues related to specific Alerting Rules Types :ml release_note:fix Team:ML Team label for ML (also use :ml) v8.13.0 v8.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@darnautov @elasticmachine @kibana-ci @kibanamachine @peteharverson @jgowdyelastic