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

[Uptime] Fix alerting false positives #75577

Merged

Conversation

justinkambic
Copy link
Contributor

@justinkambic justinkambic commented Aug 20, 2020

Summary

Fixes #74069.

There are circumstances where complex filter logic causes query.bool.filter clauses to be rendered for ES queries. In these cases, the user-provided filter is overwriting filters that we use. It then causes alerts to trigger for monitors that are up.

I've added test cases to account for arrays and objects in this field. The coded that causes these tests to pass does not break any of the existing tests.

EDIT: while this error was discovered in the Monitor Status alert, it will also happen for the Availability query, which is composed in a very similar way. Since it's a small change to fix it, and it will be simpler to track for backporting, we will modify that code in the same patch.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@justinkambic justinkambic added bug Fixes for quality problems that affect the customer experience v8.0.0 Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.10.0 v7.9.1 labels Aug 20, 2020
@justinkambic justinkambic requested a review from a team as a code owner August 20, 2020 16:41
@justinkambic justinkambic self-assigned this Aug 20, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/uptime (Team:uptime)

@justinkambic justinkambic force-pushed the uptime_fix-monitor-status-alert-filters branch from ef45eeb to 61e9c94 Compare August 25, 2020 17:58
Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

Do we also need to fix the issue in x-pack/plugins/uptime/server/lib/requests/get_monitor_availability.ts ?

Copy link
Contributor

@andrewvc andrewvc left a comment

Choose a reason for hiding this comment

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

LGTM WFG

Did some manual testing here too

@justinkambic
Copy link
Contributor Author

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

✅ unchanged

History

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

@justinkambic justinkambic merged commit 4f23f0a into elastic:master Aug 31, 2020
justinkambic added a commit to justinkambic/kibana that referenced this pull request Aug 31, 2020
* Add test and modify code to account for complex filter input.

* Avoid unnecessary nesting.

* Simplify filtering code.

* Use `must` instead of `should`.

* Fix types.

* Refresh snapshots.

* Use `filter` instead of `must`.

* Refactor to improve readability.

* Resolve filtering issue for availability query.

* Add test covering filter fix.

* Removed unused var.

Co-authored-by: Elastic Machine <[email protected]>
justinkambic added a commit to justinkambic/kibana that referenced this pull request Aug 31, 2020
* Add test and modify code to account for complex filter input.

* Avoid unnecessary nesting.

* Simplify filtering code.

* Use `must` instead of `should`.

* Fix types.

* Refresh snapshots.

* Use `filter` instead of `must`.

* Refactor to improve readability.

* Resolve filtering issue for availability query.

* Add test covering filter fix.

* Removed unused var.

Co-authored-by: Elastic Machine <[email protected]>
# Conflicts:
#	x-pack/plugins/uptime/server/lib/requests/get_monitor_availability.ts
#	x-pack/plugins/uptime/server/lib/requests/get_monitor_status.ts
justinkambic added a commit that referenced this pull request Sep 2, 2020
* [Uptime] Fix alerting false positives (#75577)

* Add test and modify code to account for complex filter input.

* Avoid unnecessary nesting.

* Simplify filtering code.

* Use `must` instead of `should`.

* Fix types.

* Refresh snapshots.

* Use `filter` instead of `must`.

* Refactor to improve readability.

* Resolve filtering issue for availability query.

* Add test covering filter fix.

* Removed unused var.

Co-authored-by: Elastic Machine <[email protected]>

* Update broken snapshots.

Co-authored-by: Elastic Machine <[email protected]>
@justinkambic
Copy link
Contributor Author

justinkambic commented Sep 2, 2020

@justinkambic justinkambic deleted the uptime_fix-monitor-status-alert-filters branch September 2, 2020 14:58
@justinkambic justinkambic added v7.9.2 and removed v7.9.1 labels Sep 2, 2020
justinkambic added a commit that referenced this pull request Sep 2, 2020
* [Uptime] Fix alerting false positives (#75577)

* Add test and modify code to account for complex filter input.

* Avoid unnecessary nesting.

* Simplify filtering code.

* Use `must` instead of `should`.

* Fix types.

* Refresh snapshots.

* Use `filter` instead of `must`.

* Refactor to improve readability.

* Resolve filtering issue for availability query.

* Add test covering filter fix.

* Removed unused var.

Co-authored-by: Elastic Machine <[email protected]>
# Conflicts:
#	x-pack/plugins/uptime/server/lib/requests/get_monitor_availability.ts
#	x-pack/plugins/uptime/server/lib/requests/get_monitor_status.ts

* Resolve type difference from master.

* Refresh test snapshot.

* Refresh snapshots.

Co-authored-by: Elastic Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience release_note:fix Team:Uptime - DEPRECATED Synthetics & RUM sub-team of Application Observability v7.9.2 v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Uptime alert for downtime triggered all the time whilst monitor is up
5 participants