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

Add flapping state object and interface in AAD index and Event Log #143920

Merged
merged 18 commits into from
Nov 7, 2022

Conversation

ersin-erdal
Copy link
Contributor

@ersin-erdal ersin-erdal commented Oct 25, 2022

resolves: #143442

This PR intends to create the flapping state object/interfaces in AAD, Event Log and alert summary EP. Then we can add the code to set/unset the state.

@ersin-erdal ersin-erdal added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) release_note:feature Makes this part of the condensed release notes v8.6.0 labels Oct 25, 2022
@ersin-erdal ersin-erdal changed the title Flapping WIP Add flapping state object and interface in ADD index and Event Log Nov 1, 2022
@ersin-erdal ersin-erdal marked this pull request as ready for review November 1, 2022 11:26
@ersin-erdal ersin-erdal requested review from a team as code owners November 1, 2022 11:26
@elasticmachine
Copy link
Contributor

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

@@ -37,6 +37,7 @@ import {
TAGS,
TIMESTAMP,
VERSION,
// ALERT_FLAPPING,
Copy link
Contributor Author

@ersin-erdal ersin-erdal Nov 1, 2022

Choose a reason for hiding this comment

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

i didn't delete this on purpose. May help the next developer to figure out how to get the flapping object's path for AAD.

@ersin-erdal ersin-erdal changed the title Add flapping state object and interface in ADD index and Event Log Add flapping state object and interface in AAD index and Event Log Nov 1, 2022
Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

LGTM! Verified I see this new field in the alerts-as-data mapping, the event log mapping and returned in the alert summary defaulting to false

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, but one request.

For the uuid PR, I added some function tests in

  • x-pack/test/alerting_api_integration/spaces_only/tests/alerting/event_log.ts
  • x-pack/test/alerting_api_integration/spaces_only/tests/alerting/event_log_alerts.ts

These turned out to be super-useful! I found a case were we were generating a recovered event that I had missed setting the uuid. For this PR, I think we'd just check that the flapping property is boolean, in similar cases as the tests check them.

@ersin-erdal
Copy link
Contributor Author

ersin-erdal commented Nov 4, 2022

LGTM, but one request.

For the uuid PR, I added some function tests in

  • x-pack/test/alerting_api_integration/spaces_only/tests/alerting/event_log.ts
  • x-pack/test/alerting_api_integration/spaces_only/tests/alerting/event_log_alerts.ts

These turned out to be super-useful! I found a case were we were generating a recovered event that I had missed setting the uuid. For this PR, I think we'd just check that the flapping property is boolean, in similar cases as the tests check them.

Unfortunately we can't test "flapping" with those integration tests. Rule execution does not add any flapping data to the logs yet. Would you like me to add a default "false" to all alert logs? Then we can test those false values.

BTW i moved flapping under kibana.alert back.

@mikecote
Copy link
Contributor

mikecote commented Nov 4, 2022

Unfortunately we can't test "flapping" with those integration tests. Rule execution does not add any flapping data to the logs yet. Would you like me to add a default "false" to all alert logs? Then we can test those false values.

If we add any tests, lets keep it lightweight as @doakalexi is working on setting true/false appropriately and it'll come with its own series of tests (#143443). Defaulting to false isn't a bad idea to avoid implying undefined is false.

@pmuellr
Copy link
Member

pmuellr commented Nov 4, 2022

Unfortunately we can't test "flapping" with those integration tests. Rule execution does not add any flapping data to the logs yet. Would you like me to add a default "false" to all alert logs? Then we can test those false values.

I checked ECS to see if there was guidance for boolean fields, and kinda funny I found this issue elastic/ecs#100 where they deferred guidance till they had some boolean fields. They didn't then, but they do now. Didn't find anything else.

I don't have a sense for if it's really better to explicitly store the false value, except to say if we do always expect to send one of either true or false, that's easy to test for, and similar to the test I added (if it's an -*instance action, it MUST have an alert.uuid, and if it's not, it MUST NOT". It caught a case where it wasn't being set. So at least it's easier for us to test that we're always setting it (or not) :-)

BTW i moved flapping under kibana.alert back.

Cool, I think that's probably the right thing 🤞🏻

@ersin-erdal
Copy link
Contributor Author

I added default "false" flapping value to the alert logs

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

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/rule-data-utils 82 83 +1
alerting 370 371 +1
triggersActionsUi 491 492 +1
total +3

Async chunks

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

id before after diff
observability 490.6KB 490.7KB +96.0B
securitySolution 9.6MB 9.6MB +501.0B
triggersActionsUi 660.0KB 660.3KB +338.0B
total +935.0B

Page load bundle

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

id before after diff
apm 29.9KB 29.9KB +25.0B
cases 126.5KB 126.6KB +25.0B
infra 85.3KB 85.3KB +25.0B
synthetics 24.7KB 24.8KB +25.0B
timelines 138.5KB 138.5KB +25.0B
total +125.0B
Unknown metric groups

API count

id before after diff
@kbn/rule-data-utils 85 86 +1
alerting 379 380 +1
triggersActionsUi 520 521 +1
total +3

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 58 64 +6
osquery 108 113 +5
securitySolution 440 446 +6
total +19

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 66 72 +6
osquery 109 115 +6
securitySolution 517 523 +6
total +20

History

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

Copy link
Contributor

@kdelemme kdelemme left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻

@ersin-erdal ersin-erdal merged commit c5bcfd6 into elastic:main Nov 7, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 7, 2022
@ersin-erdal ersin-erdal deleted the 143445-flapping branch November 7, 2022 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:feature Makes this part of the condensed release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ResponseOps][flapping] add external flapping state
10 participants