[alerting] gracefully handle error in initialization of Alert TaskRunner #54335
Conversation
gmmorris
added
Feature:Alerting
Team:ResponseOps
|
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
gmmorris
added
the
release_note:skip
There was a problem hiding this comment.
LGTM, except descriptions of the test cases. They seem either wrong, or it's not clear why the mocked errors are testing what the say they are testing.
I have a feeling this change is going to cause some further issues when merged. Whereas before this, I guess tasks became zombies in some cases, now they won't, but they will likely still fail - if it couldn't decrypt encrypted attributes before, it's not going to be able to now either. Relatedly, we don't seem to report these errors anywhere. So it seems like they're still cases where they are going to be zombie-ish (the task won't run), only instead of not running it anymore, we'll keep running it.
If that's the case, I'm still fine with this, but let's open an issue to discuss. We may want to just rely on the event log (in the future) to be able to have folks notice this, which would give us a place to "log" the error.
x-pack/legacy/plugins/alerting/server/task_runner/task_runner.test.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/alerting/server/task_runner/task_runner.test.ts
Outdated
Show resolved
Hide resolved
x-pack/legacy/plugins/alerting/server/task_runner/task_runner.test.ts
Outdated
Show resolved
Hide resolved
* master: Remove eslint overwrite for src/legacy/core_plugins/kibana (elastic#54222)
* master: (69 commits) [Graph] Fix various a11y issues (elastic#54097) Add ApplicationService app status management (elastic#50223) logs in one time (elastic#54447) Deprecate using `elasticsearch.ssl.certificate` without `elasticsearch.ssl.key` and vice versa (elastic#54392) [Optimizer] Fix a stack overflow with watch_cache when it attempts to delete very large folders. (elastic#54457) Security - Role Mappings UI (elastic#53620) [SIEM] [Detection engine] Permission II (elastic#54292) Allow User to Cleanup Repository from UI (elastic#53047) [Detection engine] Some UX for rule creation (elastic#54471) share specific instances of some ui packages (elastic#54079) [ML] APM modules configs for RUM Javascript and NodeJS (elastic#53792) [APM] Delay rendering invalid license notification (elastic#53924) [Graph] Improve error message on graph requests (elastic#54230) [ILM] Kibana should allow a min_age setting of 0ms in ILM policy phases (elastic#53719) Unit Tests for common/lib (elastic#53736) [Graph] Only show explorable fields (elastic#54101) remove linting rule exception for markdown (elastic#54232) [Monitoring] Fetch shard data more efficiently (elastic#54028) [Maps] Add hiddenLayers option to embeddable map input (elastic#54355) Pass termOrder and hasTermsAgg properties to serializeThresholdWatch function (elastic#54391) ...
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
…ner (elastic#54335) Prevents an edge cases where Alerts can end up in a zombie state. 1. Decrypting attributes throws an error 2. Fetching an Api Key throws an error 3. Getting Services with user permissions throws an error
…ner (elastic#54335) Prevents an edge cases where Alerts can end up in a zombie state. 1. Decrypting attributes throws an error 2. Fetching an Api Key throws an error 3. Getting Services with user permissions throws an error
Summary
Prevents an edge cases where Alerts can end up in a zombie state.
closes #54334
Checklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.This was checked for cross-browser compatibility, including a check against IE11Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n supportDocumentation was added for features that require explanation or tutorialsThis was checked for keyboard-only and screenreader accessibilityFor maintainers