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.

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

[Response Ops][Flapping] Rule Specific Flapping - Create/Update API changes #190019

Conversation

JiaweiWu
Copy link
Contributor

@JiaweiWu JiaweiWu commented Aug 7, 2024

Summary

Issue: #190018

Implement rule specific flapping support for create and update Rule API. The new property on the rule is named flapping;

flapping: {
  look_back_window: number;
  status_change_threshold: number;
}

Also make changes in the task runner to use the rule's flapping settings if it exists. Otherwise use the global flapping setting.

To test

  1. Go to x-pack/plugins/triggers_actions_ui/public/common/constants/index.ts and turn IS_RULE_SPECIFIC_FLAPPING_ENABLED to true
  2. Create a rule with a rule specific flapping setting, generate the alert and let it flap
  3. Assert that the flapping is now using the rule specific flapping
  4. Turn space flapping off
  5. Assert that it no longer flaps despite having a rule specific flapping
  6. Try deleting/adding back the rule specific flapping via the UI and verify everything works.

Checklist

@JiaweiWu JiaweiWu added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.16.0 labels Aug 7, 2024
@JiaweiWu JiaweiWu requested a review from a team as a code owner August 7, 2024 04:05
@elasticmachine
Copy link
Contributor

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

@JiaweiWu
Copy link
Contributor Author

@elasticmachine merge upstream

JiaweiWu and others added 4 commits August 15, 2024 20:55
…t --include-path /api/status --include-path /api/alerting/rule/ --include-path /api/alerting/rules --update'
Copy link
Contributor

@ersin-erdal ersin-erdal left a comment

Choose a reason for hiding this comment

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

LGTM.
Tested locally, rule level flapping settings overwrites the global settings if it is enabled

@JiaweiWu
Copy link
Contributor Author

@elasticmachine merge upstream

@elastic-vault-github-plugin-prod elastic-vault-github-plugin-prod bot requested a review from a team as a code owner August 28, 2024 03:17
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

The alert saved object hash changed without a new model version. Are you sure the changes are stable for BWC?

Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

If you're sure no new model version is needed for the change to the SO, then LGTM

@JiaweiWu
Copy link
Contributor Author

@TinaHeiligers Actually I'm not too familiar with how we're dealing with adding new properties to saved objects. So in this PR I'm adding a new un-indexed property to the rule saved object. The property is called flapping.

The alert saved object hash changed without a new model version. Are you sure the changes are stable for BWC?

Here is the commit where I add the new version with the new schema: 99f0926

But please let me know if I missed anything! Thanks!

@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Aug 29, 2024

@TinaHeiligers Actually I'm not too familiar with how we're dealing with adding new properties to saved objects. So in this PR I'm adding a new un-indexed property to the rule saved object. The property is called flapping.

The alert saved object hash changed without a new model version. Are you sure the changes are stable for BWC?

Here is the commit where I add the new version with the new schema: 99f0926

But please let me know if I missed anything! Thanks!

If there isn't a mappings change and the new field isn't in the raw ES doc (it can be in source) and is optional, then it's ok. If, however, you plan on using the field in any UI, I'd rather have a model version v2 to be sure that any rollback to a previous version isn't going to break the UX

@JiaweiWu
Copy link
Contributor Author

JiaweiWu commented Aug 30, 2024

If, however, you plan on using the field in any UI, I'd rather have a model version v2 to be sure that any rollback to a previous version isn't going to break the UX

Yes, it will be used in the UI. By model version, do you just mean what I have added in this commit: 99f0926#diff-12c3ecfb676c8d71cfbd3ca6e4ce29223604dfa58d35a3a108ee6399da34f8daR19 Haha sorry for the dumb question.

@cnasikas
Copy link
Member

cnasikas commented Sep 11, 2024

Oh yeah, I remember that. I had to do it once. Its a pain. Fine by me to do it on another PR! Btw @ymao1 is working in this PR #192404 on a RulesSettingsService that caches the rule's settings. Maybe we can use that instead. Wdyt?

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #17 / FileNameLink renders clickable name if file is image
  • [job] [logs] Jest Tests #17 / FilterPopover renders empty label correctly

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
alerting 234 233 -1
cases 812 811 -1
observability 1056 1055 -1
securitySolution 5764 5763 -1
triggersActionsUi 790 791 +1
total -3

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/alerting-types 219 218 -1
@kbn/alerts-ui-shared 287 290 +3
alerting 843 841 -2
total -0

Async chunks

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

id before after diff
triggersActionsUi 1.6MB 1.6MB -59.0B

Page load bundle

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

id before after diff
alerting 25.6KB 25.2KB -394.0B
triggersActionsUi 121.4KB 121.4KB -17.0B
total -411.0B
Unknown metric groups

API count

id before after diff
@kbn/alerting-types 222 221 -1
@kbn/alerts-ui-shared 303 306 +3
alerting 875 873 -2
total -0

References to deprecated APIs

id before after diff
@kbn/alerting-types 24 26 +2

History

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

JiaweiWu added a commit that referenced this pull request Sep 20, 2024
## Summary

This PR implements the `forwardCompatibility` field for the rule model
version in preparation for the flapping PR
(#190019). We're doing this so we
have the point to revert back to if we need to where the
`forwardCompatibility` is defined.

---------

Co-authored-by: kibanamachine <[email protected]>
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 23, 2024
## Summary

This PR implements the `forwardCompatibility` field for the rule model
version in preparation for the flapping PR
(elastic#190019). We're doing this so we
have the point to revert back to if we need to where the
`forwardCompatibility` is defined.

---------

Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit bb18996)
kibanamachine added a commit that referenced this pull request Sep 23, 2024
… (#193695)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Response Ops][Rules] Rule Model Version Forward Compat
(#193233)](#193233)

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

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

<!--BACKPORT [{"author":{"name":"Jiawei
Wu","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-20T08:50:54Z","message":"[Response
Ops][Rules] Rule Model Version Forward Compat (#193233)\n\n##
Summary\r\n\r\nThis PR implements the `forwardCompatibility` field for
the rule model\r\nversion in preparation for the flapping
PR\r\n(#190019). We're doing this
so we\r\nhave the point to revert back to if we need to where
the\r\n`forwardCompatibility` is
defined.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>","sha":"bb1899653f6fc077d443a600bce6c6f2fc775964","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:ResponseOps","v9.0.0","backport:prev-minor","v8.16.0"],"title":"[Response
Ops][Rules] Rule Model Version Forward
Compat","number":193233,"url":"https://github.com/elastic/kibana/pull/193233","mergeCommit":{"message":"[Response
Ops][Rules] Rule Model Version Forward Compat (#193233)\n\n##
Summary\r\n\r\nThis PR implements the `forwardCompatibility` field for
the rule model\r\nversion in preparation for the flapping
PR\r\n(#190019). We're doing this
so we\r\nhave the point to revert back to if we need to where
the\r\n`forwardCompatibility` is
defined.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>","sha":"bb1899653f6fc077d443a600bce6c6f2fc775964"}},"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/193233","number":193233,"mergeCommit":{"message":"[Response
Ops][Rules] Rule Model Version Forward Compat (#193233)\n\n##
Summary\r\n\r\nThis PR implements the `forwardCompatibility` field for
the rule model\r\nversion in preparation for the flapping
PR\r\n(#190019). We're doing this
so we\r\nhave the point to revert back to if we need to where
the\r\n`forwardCompatibility` is
defined.\r\n\r\n---------\r\n\r\nCo-authored-by: kibanamachine
<[email protected]>","sha":"bb1899653f6fc077d443a600bce6c6f2fc775964"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Jiawei Wu <[email protected]>
@cnasikas cnasikas added v9.0.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:skip This commit does not require backporting labels Sep 25, 2024
@JiaweiWu
Copy link
Contributor Author

JiaweiWu commented Oct 8, 2024

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

Failed CI Steps

Metrics [docs]

‼️ ERROR: no builds found for mergeBase sha [43bc586]

History

@JiaweiWu
Copy link
Contributor Author

JiaweiWu commented Oct 8, 2024

@elasticmachine merge upstream

@JiaweiWu JiaweiWu merged commit edd61f6 into elastic:main Oct 9, 2024
37 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

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

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Oct 9, 2024
…hanges (elastic#190019)

## Summary
Issue: elastic#190018

Implement rule specific flapping support for create and update Rule API.
The new property on the rule is named `flapping`;

```
flapping: {
  look_back_window: number;
  status_change_threshold: number;
}
```

Also make changes in the task runner to use the rule's flapping settings
if it exists. Otherwise use the global flapping setting.

# To test
1. Go to
`x-pack/plugins/triggers_actions_ui/public/common/constants/index.ts`
and turn `IS_RULE_SPECIFIC_FLAPPING_ENABLED` to `true`
2. Create a rule with a rule specific flapping setting, generate the
alert and let it flap
3. Assert that the flapping is now using the rule specific flapping
4. Turn space flapping off
5. Assert that it no longer flaps despite having a rule specific
flapping
6. Try deleting/adding back the rule specific flapping via the UI and
verify everything works.

### Checklist
- [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: Elastic Machine <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit edd61f6)
@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 9, 2024
… API changes (#190019) (#195526)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Response Ops][Flapping] Rule Specific Flapping - Create/Update API
changes (#190019)](#190019)

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

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

<!--BACKPORT [{"author":{"name":"Jiawei
Wu","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-10-09T01:01:45Z","message":"[Response
Ops][Flapping] Rule Specific Flapping - Create/Update API changes
(#190019)\n\n## Summary\r\nIssue:
https://github.com/elastic/kibana/issues/190018\r\n\r\nImplement rule
specific flapping support for create and update Rule API.\r\nThe new
property on the rule is named `flapping`;\r\n\r\n```\r\nflapping: {\r\n
look_back_window: number;\r\n status_change_threshold:
number;\r\n}\r\n```\r\n\r\nAlso make changes in the task runner to use
the rule's flapping settings\r\nif it exists. Otherwise use the global
flapping setting.\r\n\r\n# To test\r\n1. Go
to\r\n`x-pack/plugins/triggers_actions_ui/public/common/constants/index.ts`\r\nand
turn `IS_RULE_SPECIFIC_FLAPPING_ENABLED` to `true`\r\n2. Create a rule
with a rule specific flapping setting, generate the\r\nalert and let it
flap\r\n3. Assert that the flapping is now using the rule specific
flapping\r\n4. Turn space flapping off\r\n5. Assert that it no longer
flaps despite having a rule specific\r\nflapping\r\n6. Try
deleting/adding back the rule specific flapping via the UI and\r\nverify
everything works.\r\n\r\n### Checklist\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\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"edd61f63dbad99fe8da1e503c81db774fcb37e8f","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:ResponseOps","v9.0.0","backport:prev-minor","v8.16.0"],"title":"[Response
Ops][Flapping] Rule Specific Flapping - Create/Update API
changes","number":190019,"url":"https://github.com/elastic/kibana/pull/190019","mergeCommit":{"message":"[Response
Ops][Flapping] Rule Specific Flapping - Create/Update API changes
(#190019)\n\n## Summary\r\nIssue:
https://github.com/elastic/kibana/issues/190018\r\n\r\nImplement rule
specific flapping support for create and update Rule API.\r\nThe new
property on the rule is named `flapping`;\r\n\r\n```\r\nflapping: {\r\n
look_back_window: number;\r\n status_change_threshold:
number;\r\n}\r\n```\r\n\r\nAlso make changes in the task runner to use
the rule's flapping settings\r\nif it exists. Otherwise use the global
flapping setting.\r\n\r\n# To test\r\n1. Go
to\r\n`x-pack/plugins/triggers_actions_ui/public/common/constants/index.ts`\r\nand
turn `IS_RULE_SPECIFIC_FLAPPING_ENABLED` to `true`\r\n2. Create a rule
with a rule specific flapping setting, generate the\r\nalert and let it
flap\r\n3. Assert that the flapping is now using the rule specific
flapping\r\n4. Turn space flapping off\r\n5. Assert that it no longer
flaps despite having a rule specific\r\nflapping\r\n6. Try
deleting/adding back the rule specific flapping via the UI and\r\nverify
everything works.\r\n\r\n### Checklist\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\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"edd61f63dbad99fe8da1e503c81db774fcb37e8f"}},"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/190019","number":190019,"mergeCommit":{"message":"[Response
Ops][Flapping] Rule Specific Flapping - Create/Update API changes
(#190019)\n\n## Summary\r\nIssue:
https://github.com/elastic/kibana/issues/190018\r\n\r\nImplement rule
specific flapping support for create and update Rule API.\r\nThe new
property on the rule is named `flapping`;\r\n\r\n```\r\nflapping: {\r\n
look_back_window: number;\r\n status_change_threshold:
number;\r\n}\r\n```\r\n\r\nAlso make changes in the task runner to use
the rule's flapping settings\r\nif it exists. Otherwise use the global
flapping setting.\r\n\r\n# To test\r\n1. Go
to\r\n`x-pack/plugins/triggers_actions_ui/public/common/constants/index.ts`\r\nand
turn `IS_RULE_SPECIFIC_FLAPPING_ENABLED` to `true`\r\n2. Create a rule
with a rule specific flapping setting, generate the\r\nalert and let it
flap\r\n3. Assert that the flapping is now using the rule specific
flapping\r\n4. Turn space flapping off\r\n5. Assert that it no longer
flaps despite having a rule specific\r\nflapping\r\n6. Try
deleting/adding back the rule specific flapping via the UI and\r\nverify
everything works.\r\n\r\n### Checklist\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\n---------\r\n\r\nCo-authored-by: Elastic Machine
<[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"edd61f63dbad99fe8da1e503c81db774fcb37e8f"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Jiawei Wu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) 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
Development

Successfully merging this pull request may close these issues.

8 participants