-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Logs UI] Fix alerts recovery #87369
[Logs UI] Fix alerts recovery #87369
Conversation
…y recognised by the framework
Pinging @elastic/logs-metrics-ui (Team:logs-metrics-ui) |
(ℹ️ Test fixes are incoming) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like all combinations of { count, ratio }
and { ungrouped, grouped }
work. Great job tracking the problems down 👏
* Ensure that if alert instances are instantiated they are used in a way recognised by the framework
* Ensure that if alert instances are instantiated they are used in a way recognised by the framework
💔 Build Failed
Failed CI Steps
Test FailuresChrome X-Pack UI Functional Tests.x-pack/test/functional_with_es_ssl/apps/uptime/alert_flyout·ts.Uptime app with real-world data uptime alerts overview page alert flyout controls posts an alert, verifies its presence, and deletes the alertStandard Out
Stack Trace
Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/monitoring/elasticsearch/nodes·js.Monitoring app Elasticsearch nodes listing with offline node should sort by diskStandard Out
Stack Trace
Chrome X-Pack UI Functional Tests.x-pack/test/functional/apps/ml/data_frame_analytics/cloning·ts.machine learning data frame analytics jobs cloning supported by UI form classification job supported by the form opens the existing job in the data frame analytics job wizardStandard Out
Stack Trace
and 5 more failures, only showing the first 3. Metrics [docs]Distributable file count
History
To update your PR or re-run it, just comment with: |
Summary
This PR fixes #86507.
The introduction of the recovery mechanism has caused some subtle, but bad, conflicts with the way we were using alert instances.
At it's core recovered instances are those that have been instantiated and have no scheduled actions: https://github.com/elastic/kibana/blob/master/x-pack/plugins/alerts/server/task_runner/task_runner.ts#L283, those with scheduled actions are passed through to further executions so it can be inferred which ones have recovered.
This PR removes the "faux recovery" mechanism we had in place (we were setting an
OK
state, but it wasn't actually used). Usage like this breaks the whole recovery mechanism.I've also removed the
ERROR
usage. The eager instantiation of the alert instance would cause this bug alone, but even without being eager it's pointless as we don't surface it.With this you should get the expected output:
with no constant
recovered
state.Testing
Notify every
onRun only on status change