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

[RAM] Adds revision to alerts schema #151388

Merged
merged 103 commits into from
Mar 29, 2023

Conversation

spong
Copy link
Member

@spong spong commented Feb 15, 2023

Summary

Follow up from #147398, which adds revision to the alerts schema so the rule's current revision is included when creating alerts.

In Security Solution:

In Observability:

Note: this was originally a branched off #147398, so the large commit list is resulting from there as Github doesn't seem to re-write after after a rebase w/ main and a force push.

Checklist

Delete any items that are not applicable to this PR.

spong added 30 commits December 12, 2022 15:01
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@mohamedhamed-ahmed mohamedhamed-ahmed left a comment

Choose a reason for hiding this comment

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

LGTM, on behalf of Obs Infra UI team

Copy link
Contributor

@CoenWarmer CoenWarmer left a comment

Choose a reason for hiding this comment

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

AO Changes LGTM!

Copy link
Contributor

@XavierM XavierM left a comment

Choose a reason for hiding this comment

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

It looks like o11y (@simianhacker) is ok to add this new field on the alert index and response ops is ok with it. I am still wondering the use case/feature for adding revision in the alert index. Can you give me some light? @paulewing

thinking a little bit more about it, I will know that this alert has been detected with this rule revisions and we might be able to explain the reason of why this happen or not thanks to our detection engineer that are updating our pre-package rules. All good!

@spong
Copy link
Member Author

spong commented Mar 24, 2023

It looks like o11y (@simianhacker) is ok to add this new field on the alert index and response ops is ok with it. ~I am still wondering the use case/feature for adding revision in the alert index. Can you give me some light? @paulewing ~

thinking a little bit more about it, I will know that this alert has been detected with this rule revisions and we might be able to explain the reason of why this happen or not thanks to our detection engineer that are updating our pre-package rules. All good!

Yeah, the traceability through the system enables some nice opportunities like:

  • Surfacing all alerts/executions (event-log PR coming... :) for a given revision. e.g. perhaps there was a bad rule configuration and the user needs to identify those alerts and the time window they need to re-evaluate
  • Comparing perf & effectiveness between subsequent revisions of the same rule
  • Clarity in debugging rule executions not resulting in alerts (on the event-log side), being able to trace back to the actual query from a specific rule revision, etc

Obviously more of this power comes when storing (or at least writing to the audit log) user rule revision changes, but this is a start and will be helpful in support scenarios from the get-go.

@spong spong mentioned this pull request Mar 24, 2023
1 task
Copy link
Contributor

@dplumlee dplumlee left a comment

Choose a reason for hiding this comment

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

alerts area files lgtm!

Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

Perfect, thank you @spong!

I tested it locally and reviewed the diff. Everything LGTM. The only super minor issue I noticed is this unknown field in the alert details flyout:

Screenshot 2023-03-28 at 19 10 51

I thought maybe the alerts index mappings were not updated, but after checking them I found the revision field in the technical component template and in the concrete alerts index. In the end, I fixed this by reloading the page, so this feels like something related to caching fields data. We can probably safely ignore this.

Copy link
Contributor

@yctercero yctercero left a comment

Choose a reason for hiding this comment

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

security-solution-platform codeowners review - LGTM!

@spong spong enabled auto-merge (squash) March 28, 2023 19:42
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Rules, Alerts and Exceptions ResponseOps Cypress Tests on Security Solution / Detection rules, bulk edit of rule actions Restricted action privileges User with no privileges can't add rule actions
  • [job] [logs] Security Solution Tests #4 / Row renderers Selected renderer can be disabled and enabled

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 98 99 +1

Async chunks

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

id before after diff
apm 3.8MB 3.8MB +42.0B
observability 1.1MB 1.1MB +42.0B
securitySolution 15.8MB 15.8MB +451.0B
triggersActionsUi 1.4MB 1.4MB +42.0B
total +577.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
ruleRegistry 13 14 +1

Page load bundle

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

id before after diff
apm 32.0KB 32.0KB +55.0B
observability 89.1KB 89.2KB +55.0B
triggersActionsUi 83.6KB 83.7KB +55.0B
total +165.0B
Unknown metric groups

API count

id before after diff
@kbn/rule-data-utils 101 102 +1

ESLint disabled line counts

id before after diff
securitySolution 433 436 +3

Total ESLint disabled count

id before after diff
securitySolution 513 516 +3

History

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

cc @spong

@spong spong merged commit e41cc7a into elastic:main Mar 29, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Mar 29, 2023
@spong spong deleted the adds-revision-to-alerts-schema branch March 29, 2023 19:28
jgowdyelastic pushed a commit to jgowdyelastic/kibana that referenced this pull request Mar 30, 2023
## Summary

Follow up from elastic#147398, which adds
`revision` to the alerts schema so the rule's current revision is
included when creating alerts.

In Security Solution:
<p align="center">
<img width="500"
src="https://user-images.githubusercontent.com/2946766/227386305-c8afe295-b79b-4b28-838a-cc3bed0f3eda.png"
/>
</p>

In Observability:
<p align="center">
<img width="500"
src="https://user-images.githubusercontent.com/2946766/227577019-05307860-e0e3-4e1e-b4cf-604bdb52afdf.png"
/>
</p>



Note: this was originally a branched off
elastic#147398, so the large commit list
is resulting from there as Github doesn't seem to re-write after after a
rebase w/ `main` and a force push.


### Checklist

Delete any items that are not applicable to this PR.

- [ ]
~[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials~
* Base docs to be added for
elastic#147398
- [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
spong added a commit that referenced this pull request Apr 22, 2023
## Summary

Follow on from #151388 &
#147398, which includes the rule's
current `revision` when writing to the kibana event-log.

Note: Added as `kibana.alert.rule.revision` instead of as ECS field
`rule.version` as the [ECS
docs](https://www.elastic.co/guide/en/ecs/current/ecs-rule.html#field-rule-version)
conflate `version` & `revision` and figured it was best to be explicit.
If we do indeed want to use `rule.version` I'll make the change.


<p align="center">
<img width="500"
src="https://user-images.githubusercontent.com/2946766/233216775-f371f412-dcf6-4ef7-a396-84ec853eebbb.png"
/>
</p> 



### Checklist

Delete any items that are not applicable to this PR.

- [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
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 enhancement New value added to drive a business result Feature:Alerting/Alerts-as-Data Issues related to Alerts-as-data and RuleRegistry Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework release_note:skip Skip the PR/issue when compiling release notes Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants