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

[RAC] Populate common rule fields in alert helpers #108679

Merged
merged 25 commits into from
Aug 26, 2021

Conversation

weltenwort
Copy link
Member

@weltenwort weltenwort commented Aug 16, 2021

📝 Summary

This marks some technical alert-as-data fields as required and changes the alert status values from open/closed to active/recovered. It also updates the lifecycle and persistent rule type helpers to consistently populate several common alert fields with values from the alerting framework.

closes #108161

🕵️ Review notes

  • Since the persistent rule type helper almost exclusively passes through very loosely typed documents I was unable to adapt any downstream-changes that might be required due to it. @elastic/security-threat-hunting would you be able to adapt to it in your code-base in a follow-up or should I remove that change from the PR?

@weltenwort weltenwort added bug Fixes for quality problems that affect the customer experience v8.0.0 Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed Theme: rac label obsolete Feature:RAC label obsolete v7.15.0 v7.16.0 labels Aug 16, 2021
@weltenwort weltenwort self-assigned this Aug 16, 2021
@weltenwort
Copy link
Member Author

@elastic/security-threat-hunting these changes might impact your persistent alert type code, but I was unable to determine the exact changes required. Please let me know what you think of this. 😇

[ALERT_RULE_NAME]: options.rule.name,
[ALERT_RULE_PRODUCER]: options.rule.producer,
[ALERT_RULE_TYPE_ID]: options.rule.ruleTypeId,
[ALERT_RULE_UUID]: options.alertId,
Copy link
Contributor

@marshallmain marshallmain Aug 16, 2021

Choose a reason for hiding this comment

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

We'll probably want to change this to ALERT_RULE_ID and remove ALERT_RULE_UUID entirely throughout the codebase to align with the schema

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I'll check if it's used anywhere. (probably not)

Copy link
Contributor

Choose a reason for hiding this comment

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

Per slack conversation, let's stick with ALERT_RULE_UUID here and plan to remove ALERT_RULE_ID elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

awesome, thanks for following up

@mgiota mgiota marked this pull request as ready for review August 23, 2021 23:24
@@ -391,7 +391,7 @@ export default function ApiTest({ getService }: FtrProviderContext) {
"apm.transaction_error_rate",
],
"kibana.alert.status": Array [
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not part of this PR, but shouldn't we use constants for the alert fields here instead of hardcoding them?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a jest inline snapshot. Manually interpolating the constants here would render the auto-updating of the snapshot mute, wouldn't it?

Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

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

LGTM! I left a few comments.

Let me know if it makes sense to import the common alert fields from the get_common_alert_fields you created instead of '@kbn/rule-data-utils'. Isn't it the reason why this file was created? Do I miss something here? This would require quite a few changes though.

I tested it locally and it works fine. Notice in my screenshot the empty cells. They are empty because these entries still have the open value. This wouldn't happen to our users, but maybe we could fall back to an empty dash ?
Screenshot 2021-08-24 at 01 14 43

@mgiota mgiota marked this pull request as draft August 24, 2021 05:56
@weltenwort
Copy link
Member Author

weltenwort commented Aug 24, 2021

Thanks for taking a look at this PR.

Notice in my screenshot the empty cells. They are empty because these entries still have the open value.

I think we don't need to be backwards-compatible to the previous unfinished schema.

Copy link
Contributor

@mgiota mgiota left a comment

Choose a reason for hiding this comment

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

LGTM

@mgiota mgiota marked this pull request as ready for review August 24, 2021 19:23
@mgiota mgiota requested a review from marshallmain August 24, 2021 19:23
@weltenwort weltenwort enabled auto-merge (squash) August 25, 2021 15:49
Copy link
Contributor

@marshallmain marshallmain 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 and rule registry changes LGTM

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1622 1623 +1
infra 970 971 +1
observability 340 341 +1
securitySolution 2392 2393 +1
timelines 328 329 +1
uptime 668 669 +1
total +6

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
ruleRegistry 118 109 -9

Async chunks

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

id before after diff
apm 4.4MB 4.4MB +624.0B
infra 1.7MB 1.7MB +10.0B
observability 553.4KB 554.2KB +842.0B
securitySolution 6.5MB 6.5MB +576.0B
timelines 424.3KB 426.6KB +2.3KB
uptime 1.0MB 1.0MB +3.0B
total +4.3KB

Page load bundle

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

id before after diff
infra 149.8KB 150.3KB +574.0B
uptime 36.2KB 36.7KB +574.0B
total +1.1KB
Unknown metric groups

API count

id before after diff
ruleRegistry 141 132 -9

History

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

cc @weltenwort

@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.15
7.x

The backport PRs will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Aug 26, 2021
Co-authored-by: mgiota <[email protected]>

Co-authored-by: Felix Stürmer <[email protected]>
Co-authored-by: mgiota <[email protected]>
kibanamachine added a commit that referenced this pull request Aug 26, 2021
Co-authored-by: mgiota <[email protected]>

Co-authored-by: Felix Stürmer <[email protected]>
Co-authored-by: mgiota <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience Feature:RAC label obsolete release_note:skip Skip the PR/issue when compiling release notes Team:Infra Monitoring UI - DEPRECATED DEPRECATED - Label for the Infra Monitoring UI team. Use Team:obs-ux-infra_services Theme: rac label obsolete v7.15.0 v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RAC] Populate common alert fields from alerting framework data
7 participants