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

[ResponseOps][Alerts] Fix muted alerts query using wrong filter #204182

Merged
merged 12 commits into from
Dec 18, 2024
Merged

[ResponseOps][Alerts] Fix muted alerts query using wrong filter #204182

merged 12 commits into from
Dec 18, 2024
+150 −2

Conversation

umbopepato
Copy link
Member

@umbopepato umbopepato commented Dec 13, 2024 •
edited by kibanamachine
Loading

Summary

Reverts the getMutedAlerts filter to use rule ids instead of rule type ids that caused the muted alerts state to be always empty.

To verify

  1. Create a rule that fire alerts
  2. Navigate to Stack management > Alerts
  3. Click on any alert actions menu (•••)
  4. Toggle on and off the alert muted state and check that the action updates accordingly and the slashed bell icon appears in the status cell when muted

Release note

Fix alert mute/unmute action

@umbopepato umbopepato added release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Dec 13, 2024
@umbopepato umbopepato requested a review from a team as a code owner December 13, 2024 09:47
@elasticmachine
Copy link
Contributor

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

cnasikas
cnasikas approved these changes Dec 13, 2024
View reviewed changes
Copy link
Member

@cnasikas cnasikas left a comment

Choose a reason for hiding this comment

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

Tested and LGTM! What do you think of adding an e2e to cover this bug?

adcoelho
adcoelho approved these changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@adcoelho adcoelho left a comment

Choose a reason for hiding this comment

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

Tested, LGTM 👍

...ers_actions_ui/public/application/sections/alerts_table/hooks/apis/get_rules_muted_alerts.ts
return http.post<FindRulesResponse>(INTERNAL_FIND_RULES_URL, {
body: JSON.stringify({
rule_type_ids: params.ruleIds,
filter: JSON.stringify(filterNode),
Copy link
Contributor

Choose a reason for hiding this comment

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

The snapshot in the tests will fail. Could you also add a test where there are multiple ruleIds? The current one only tests ruleIds: ['foo'].

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! 🙂

@elasticmachine
Copy link
Contributor

💔 Build Failed

Failed CI Steps

History

@umbopepato
Copy link
Member Author

Tested and LGTM! What do you think of adding an e2e to cover this bug?

I added an API integration test. The combination of unit (using the correct filter) + integration (correct muted alert instance ids in rules find API) should cover this case pretty well 🙂

@umbopepato umbopepato enabled auto-merge (squash) December 18, 2024 11:45
@umbopepato umbopepato merged commit d5af2e0 into elastic:main Dec 18, 2024
9 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/12393882707

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 8.x Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 204182

Questions ?

Please refer to the Backport tool documentation

JoseLuisGJ pushed a commit to JoseLuisGJ/kibana that referenced this pull request Dec 19, 2024
@umbopepato @JoseLuisGJ
[ResponseOps][Alerts] Fix muted alerts query using wrong filter (elas…
6181172 
…tic#204182)

## Summary

Reverts the `getMutedAlerts` filter to use rule ids instead of rule type
ids that caused the muted alerts state to be always empty.

## To verify

1. Create a rule that fire alerts
2. Navigate to `Stack management > Alerts`
3. Click on any alert actions menu (•••)
4. Toggle on and off the alert muted state and check that the action
updates accordingly and the slashed bell icon appears in the status cell
when muted

## Release note

Fix alert mute/unmute action
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 20, 2024
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 204182 locally

@umbopepato umbopepato mentioned this pull request Dec 23, 2024
Open
@umbopepato
Copy link
Member Author

💚 All backports created successfully

Status Branch Result
✅ 8.x

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

Questions ?

Please refer to the Backport tool documentation

@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

1 similar comment
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cnasikas cnasikas cnasikas approved these changes

@adcoelho adcoelho adcoelho approved these changes

Assignees
No one assigned
Labels
backport missing Added to PRs automatically when the are determined to be missing a backport. backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.18.0 v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@umbopepato @elasticmachine @kibanamachine @adcoelho @cnasikas