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.

Sign up for GitHub

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

Jump to bottom

[RAM] add maintenance window banner #163516

Merged
merged 25 commits into from
Aug 17, 2023
Merged

[RAM] add maintenance window banner #163516

merged 25 commits into from
Aug 17, 2023

Conversation

guskovaue
Copy link
Contributor

@guskovaue guskovaue commented Aug 9, 2023 •
edited by kibanamachine
Loading

Summary

Solves: #163465

Add maintenance window banner to Rules list and Alerts list in O11y and Management.

Screenshot 2023-08-09 at 13 03 50 Screenshot 2023-08-09 at 13 05 10

@guskovaue guskovaue self-assigned this Aug 9, 2023
@guskovaue guskovaue 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) v8.10.0 labels Aug 9, 2023
@guskovaue guskovaue marked this pull request as ready for review August 10, 2023 14:47
@guskovaue guskovaue requested review from a team as code owners August 10, 2023 14:47
@elasticmachine
Copy link
Contributor

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

@XavierM
Copy link
Contributor

XavierM commented Aug 10, 2023

@mdefazio for you to be aware

jcger
jcger reviewed Aug 10, 2023
View reviewed changes
...test/functional_with_es_ssl/apps/triggers_actions_ui/rules_list/maintenance_window_banner.ts Outdated Show resolved Hide resolved
...test/functional_with_es_ssl/apps/triggers_actions_ui/rules_list/maintenance_window_banner.ts Outdated Show resolved Hide resolved
x-pack/plugins/observability/public/pages/alerts/alerts.tsx Show resolved Hide resolved
...test/functional_with_es_ssl/apps/triggers_actions_ui/rules_list/maintenance_window_banner.ts Outdated
import { generateUniqueKey } from '../../../lib/get_test_data';
import { createMaintenanceWindow, createObjectRemover } from '../maintenance_windows/utils';

export default ({ getPageObjects, getService }: FtrProviderContext) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we might achieve the desired test confidence using a Jest test, which could run faster and might reduce flakiness. What do you think about considering a Jest test for this instead of an e2e test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jcger eb5c988

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just do it like that 7a89261 without too much complication and maybe avoid e2e test. what do you think?

@mdefazio
Copy link
Contributor

Thanks for the heads up. Issue for improvements: #163497

@lcawl
Copy link
Contributor

lcawl commented Aug 11, 2023

Though https://github.com/elastic/kibana/blob/main/packages/kbn-alerts-ui-shared/src/maintenance_window_callout/index.tsx isn't included in this PR, IMO we can shorten the callout. For example, to "Rule notifications are stopped during the maintenance window". If you want me to add that change to this PR, lmk!

@lcawl lcawl added the ui-copy Review of UI copy with docs team is recommended label Aug 11, 2023
jcger
jcger approved these changes Aug 14, 2023
View reviewed changes
x-pack/plugins/observability/public/pages/alerts/alerts.test.tsx Outdated
it('should render an alerts page template', async () => {
const wrapper = await setup();
expect(wrapper.getByTestId('alertsPage')).toBeInTheDocument();
await act(async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's weird you need act here, maybe when can get through why you needed it when I come back

@maryam-saeidi
Copy link
Member

maryam-saeidi commented Aug 15, 2023 •
edited
Loading

@guskovaue How can I test this feature locally?

Update
For creating a maintenance window, we can do it via the stack management page.

@XavierM XavierM enabled auto-merge (squash) August 15, 2023 12:11
maryam-saeidi
maryam-saeidi approved these changes Aug 15, 2023
View reviewed changes
Copy link
Member

@maryam-saeidi maryam-saeidi 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 have some questions, but they are not blockers or even necessarily related to this PR.

As shown below, Alert 1 was created before the MW, Alert 2 was created during that time.

image

My questions are:

  1. Shouldn't we see the maintenance window for Alert 1 too, how is the logic of making this decision?
  2. What is the logic of showing the MW banner? Is it only shown during an MW is active?
  3. Is there a plan to make the MW clickable?
  4. If an MW is changed, how does it impact the existing alerts related to that MW?
  5. It seems it does not matter if the alert's rule has a connector and, actually any notification is impacted during that time window. Wouldn't it be better to add the MW to an alert only when a notification is impacted by that MW?

x-pack/plugins/observability/public/pages/alerts/alerts.test.tsx Outdated

await act(async () => {
const wrapper = await setup();
expect(await wrapper.findByText('Maintenance window is running')).toBeInTheDocument();
Copy link
Member

Choose a reason for hiding this comment

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

nit: Wouldn't it be better to use data-test-subj instead of finding an element with text?

@XavierM
Copy link
Contributor

XavierM commented Aug 15, 2023 •
edited
Loading

  1. Shouldn't we see the maintenance window for Alert 1 too, how is the logic of making this decision?

We only show the window maintenance when has been created during a window maintenance.

  1. What is the logic of showing the MW banner? Is it only shown during an MW is active?

Yes correct it will only show when a MW is active

  1. Is there a plan to make the MW clickable?

there is an issue and discussion to improve the banner -> #163497

  1. If an MW is changed, how does it impact the existing alerts related to that MW?

It does not, we only care that this alert has been created during a WM

  1. It seems it does not matter if the alert's rule has a connector and, actually any notification is impacted during that time
    window. Wouldn't it be better to add the MW to an alert only when a notification is impacted by that MW?

you are correct, @shanisagiv1 do you think that we should do that?

@JiaweiWu
Copy link
Contributor

@elasticmachine merge upstream

@JiaweiWu
Copy link
Contributor

@elasticmachine merge upstream

@JiaweiWu
Copy link
Contributor

@elasticmachine merge upstream

@JiaweiWu
Copy link
Contributor

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

expected head sha didn’t match current head ref.

@shanisagiv1
Copy link

It seems it does not matter if the alert's rule has a connector and, actually any notification is impacted during that time
window. Wouldn't it be better to add the MW to an alert only when a notification is impacted by that MW?
you are correct, @shanisagiv1 do you think that we should do that?

I think it's better to show the MW even when there are no set triggered actions. so users won't waste time investigating these alerts since it's not "actionable", and should be considered as false alarms
@XavierM @maryam-saeidi

@maryam-saeidi maryam-saeidi mentioned this pull request Aug 16, 2023
Closed
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Async chunks

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

id before after diff
observability 1023.8KB 1.0MB +2.2KB
securitySolution 15.7MB 15.7MB +44.0B
triggersActionsUi 1.4MB 1.5MB +3.9KB
total +6.2KB

History

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

cc @guskovaue

@XavierM XavierM merged commit b3122cb into elastic:main Aug 17, 2023
@kibanamachine kibanamachine mentioned this pull request Aug 17, 2023
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.10

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 17, 2023
@guskovaue
[RAM] add maintenance window banner (elastic#163516)
6d31dd1 
## Summary

Solves: elastic#163465

Add maintenance window banner to Rules list and Alerts list in O11y and
Management.

<img width="1334" alt="Screenshot 2023-08-09 at 13 03 50"
src="https://github.com/elastic/kibana/assets/26089545/de0708b1-db2a-4517-91aa-a3d6b3e62b44">

<img width="1350" alt="Screenshot 2023-08-09 at 13 05 10"
src="https://github.com/elastic/kibana/assets/26089545/9f7c488d-e992-4807-a60e-3c077b623b4e">

---------

Co-authored-by: Xavier Mouligneau <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit b3122cb)
kibanamachine added a commit that referenced this pull request Aug 17, 2023
@kibanamachine @guskovaue
[8.10] [RAM] add maintenance window banner (#163516) (#164161)
18e8844 
# Backport

This will backport the following commits from `main` to `8.10`:
- [[RAM] add maintenance window banner
(#163516)](#163516)

<!--- Backport version: 8.9.7 -->

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

<!--BACKPORT
[{"author":{"name":"Julia","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-08-17T11:04:56Z","message":"[RAM]
add maintenance window banner (#163516)\n\n## Summary\r\n\r\nSolves:
https://github.com/elastic/kibana/issues/163465\r\n\r\nAdd maintenance
window banner to Rules list and Alerts list in O11y
and\r\nManagement.\r\n\r\n<img width=\"1334\" alt=\"Screenshot
2023-08-09 at 13 03
50\"\r\nsrc=\"https://github.com/elastic/kibana/assets/26089545/de0708b1-db2a-4517-91aa-a3d6b3e62b44\">\r\n\r\n<img
width=\"1350\" alt=\"Screenshot 2023-08-09 at 13 05
10\"\r\nsrc=\"https://github.com/elastic/kibana/assets/26089545/9f7c488d-e992-4807-a60e-3c077b623b4e\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Xavier Mouligneau <[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"b3122cb65670929eb5e274d8eba61bc17ca2af4f","branchLabelMapping":{"^v8.10.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:ResponseOps","ui-copy","v8.10.0","v8.11.0"],"number":163516,"url":"https://github.com/elastic/kibana/pull/163516","mergeCommit":{"message":"[RAM]
add maintenance window banner (#163516)\n\n## Summary\r\n\r\nSolves:
https://github.com/elastic/kibana/issues/163465\r\n\r\nAdd maintenance
window banner to Rules list and Alerts list in O11y
and\r\nManagement.\r\n\r\n<img width=\"1334\" alt=\"Screenshot
2023-08-09 at 13 03
50\"\r\nsrc=\"https://github.com/elastic/kibana/assets/26089545/de0708b1-db2a-4517-91aa-a3d6b3e62b44\">\r\n\r\n<img
width=\"1350\" alt=\"Screenshot 2023-08-09 at 13 05
10\"\r\nsrc=\"https://github.com/elastic/kibana/assets/26089545/9f7c488d-e992-4807-a60e-3c077b623b4e\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Xavier Mouligneau <[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"b3122cb65670929eb5e274d8eba61bc17ca2af4f"}},"sourceBranch":"main","suggestedTargetBranches":["8.11"],"targetPullRequestStates":[{"branch":"main","label":"v8.10.0","labelRegex":"^v8.10.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/163516","number":163516,"mergeCommit":{"message":"[RAM]
add maintenance window banner (#163516)\n\n## Summary\r\n\r\nSolves:
https://github.com/elastic/kibana/issues/163465\r\n\r\nAdd maintenance
window banner to Rules list and Alerts list in O11y
and\r\nManagement.\r\n\r\n<img width=\"1334\" alt=\"Screenshot
2023-08-09 at 13 03
50\"\r\nsrc=\"https://github.com/elastic/kibana/assets/26089545/de0708b1-db2a-4517-91aa-a3d6b3e62b44\">\r\n\r\n<img
width=\"1350\" alt=\"Screenshot 2023-08-09 at 13 05
10\"\r\nsrc=\"https://github.com/elastic/kibana/assets/26089545/9f7c488d-e992-4807-a60e-3c077b623b4e\">\r\n\r\n---------\r\n\r\nCo-authored-by:
Xavier Mouligneau <[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"b3122cb65670929eb5e274d8eba61bc17ca2af4f"}},{"branch":"8.11","label":"v8.11.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Julia <[email protected]>
This was referenced Oct 1, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XavierM XavierM XavierM left review comments

@jcger jcger jcger approved these changes

@maryam-saeidi maryam-saeidi maryam-saeidi approved these changes

Assignees

@guskovaue guskovaue

Labels
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.10.0 v8.11.0
Projects
No open projects
AppEx: ResponseOps - Rules & Alerts Management
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.