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] Add window maintenance column to rule logs and rule tables #155333

Conversation

JiaweiWu
Copy link
Contributor

@JiaweiWu JiaweiWu commented Apr 20, 2023

Summary

Resolves: #153775

This PR adds the maintenance_window_id column to the following tables:

O11Y/Security solutions Alerts table

o11y-alerts-table

Rule details alerts table

rule_details_maintenance_window_ids

Rule run event log

rule_event_log_maintenance_window

To test:

  1. Set ENABLE_MAINTENANCE_WINDOWS to true in x-pack/plugins/alerting/public/plugin.ts
  2. Create 1 or more active maintenance windows in stack management
  3. Create a rule, trigger some alerts
  4. Go to the rule details page alerts table, assert the maintenance window column is there, and renders the maintenance window ids
  5. Go to the rule details page event log table, assert the maintenance window column can enabled, and renders the maintenance window ids
  6. Create a O11Y rule, trigger some alerts
  7. Go to the O11Y alerts table, assert that the maintenance_window_id field can be enabled, and renders the maintenance window ids

Checklist

@JiaweiWu JiaweiWu added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Alerting/RulesManagement Issues related to the Rules Management UX v8.8.0 labels Apr 20, 2023
@lcawl lcawl added the ui-copy Review of UI copy with docs team is recommended label Apr 20, 2023
@mdefazio mdefazio self-requested a review April 20, 2023 11:13
Copy link
Contributor

@mdefazio mdefazio left a comment

Choose a reason for hiding this comment

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

Can we show the MW name rather than the ID? (And not in the square brackets)
Or I suppose a more reasonable question, is the user able to select maintenance_window_name rather than _id?

@XavierM
Copy link
Contributor

XavierM commented Apr 20, 2023

Can we show the MW name rather than the ID? (And not in the square brackets) Or I suppose a more reasonable question, is the user able to select maintenance_window_name rather than _id?

we are going to have to keep the maintenance ids for 8.8, We need to keep the IDs and then we we will do like what we did for cases. Because, we will need to create a bulk_get API on the window maintenance and then we will be able to show the name of the window maintenance. For this feature, we took the same approach that we did for cases. At first, we only had the ids and then we work on showing the case name. We will have the same approach here and add this feature for 8.9.

JiaweiWu and others added 3 commits April 20, 2023 08:40
…/rule_details/components/rule_alert_list.tsx

Co-authored-by: Lisa Cawley <lcawley@elastic.co>
…/rule_details/components/rule_event_log_list_table.tsx

Co-authored-by: Lisa Cawley <lcawley@elastic.co>
@JiaweiWu
Copy link
Contributor Author

@elasticmachine merge upstream

@JiaweiWu JiaweiWu marked this pull request as ready for review April 20, 2023 20:49
@JiaweiWu JiaweiWu requested a review from a team as a code owner April 20, 2023 20:49
@elasticmachine
Copy link
Contributor

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

top_hits: {
size: 1,
_source: {
includes: MAINTENANCE_WINDOW_IDS_FIELD,
Copy link
Contributor

Choose a reason for hiding this comment

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

can you just add MAINTENANCE_WINDOW_IDS_FIELD to the other top_hits aggregation?

@ymao1
Copy link
Contributor

ymao1 commented Apr 21, 2023

This is not directly related to this PR but while testing a rule that has alerts generating actions during a maintenance window, I see this for the triggered/generated actions count:
Screenshot 2023-04-21 at 8 32 28 AM

In my head, I was expecting to see 2 triggered actions and 0 generated, to indicate that 2 actions would have been scheduled if not for the maintenance window. Is that not how it's supposed to work?

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.

Verified I see the maintenance window ids show up in the various tables. Left one comment about optimizing the exec log aggregation but otherwise LGTM

@XavierM
Copy link
Contributor

XavierM commented Apr 21, 2023

This is not directly related to this PR but while testing a rule that has alerts generating actions during a maintenance window, I see this for the triggered/generated actions count: Screenshot 2023-04-21 at 8 32 28 AM

In my head, I was expecting to see 2 triggered actions and 0 generated, to indicate that 2 actions would have been scheduled if not for the maintenance window. Is that not how it's supposed to work?

I agree with @ymao1 on this, it seems weird because it is not representing what we do behind the scene. Honesty/Transparency is always the right answer ;) (#Dad's internet answer)

@mikecote
Copy link
Contributor

mikecote commented Apr 21, 2023

I agree with @ymao1 on this, it seems weird because it is not representing what we do behind the scene. Honesty/Transparency is always the right answer ;)

++ and we should double check that the same doesn't happen with snooze (separately or at the same time).

@XavierM
Copy link
Contributor

XavierM commented Apr 21, 2023

@ymao1, @JiaweiWu, @mikecote and I have a discussion around generated/triggered actions count, at first we all thought that it will be nice to add this count but when we started to see all the overhead to add this metric. Also what will be the expectation of our user around this metric and it might be correct to have 0 on generated/triggered actions count because why even try to generate these action knowing that you won't trigger them.

The Decision is to move forward the way, it is and see if our assumption is correct!

@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
alerting 582 584 +2
triggersActionsUi 511 512 +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
triggersActionsUi 1.4MB 1.4MB +749.0B
Unknown metric groups

API count

id before after diff
alerting 603 605 +2
triggersActionsUi 540 541 +1
total +3

ESLint disabled line counts

id before after diff
securitySolution 394 397 +3

Total ESLint disabled count

id before after diff
securitySolution 474 477 +3

History

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

@JiaweiWu JiaweiWu merged commit 9bb127f into elastic:main Apr 21, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 21, 2023
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 Feature:Alerting/RulesManagement Issues related to the Rules Management UX release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) ui-copy Review of UI copy with docs team is recommended v8.8.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ResponseOps][Window Maintenance] Add window maintenance column to rule logs and rule tables
9 participants