allows Alerts to recover gracefully from Executor errors#53688
allows Alerts to recover gracefully from Executor errors#53688gmmorris merged 16 commits intoelastic:masterfrom
Conversation
|
Pinging @elastic/kibana-alerting-services (Team:Alerting Services) |
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
💚 Build SucceededTo update your PR or re-run it, just comment with: |
|
@elasticmachine merge upstream |
mikecote
left a comment
There was a problem hiding this comment.
A few nits but changes LGTM! Thanks for adding more tests!
x-pack/test/alerting_api_integration/common/lib/test_assertions.ts
Outdated
Show resolved
Hide resolved
x-pack/test/alerting_api_integration/spaces_only/tests/alerting/alerts.ts
Show resolved
Hide resolved
|
@elasticmachine merge upstream |
|
merge conflict between base and head |
* master: Bump year in NOTICE.txt Add kibanamachine support to Github PR comments (elastic#53852) Add tests to ensure AAD isn't broken after performing a change on an alert / action (elastic#53333) Skip failing test suite [Vega] Sample [Flights] Airport Connections (Hover Over Airport) visualization not working (elastic#53799) Do not remount applications between page navigations (elastic#53851) [Canvas] Refactor Canvas to no longer use componentWillReceiveProps (elastic#52129) [Canvas] Migrate usage collector to NP plugin (elastic#53303)
pmuellr
left a comment
There was a problem hiding this comment.
LGTM, left a few comments
| @@ -59,9 +63,7 @@ export class TaskRunnerFactory { | |||
| } = this.taskRunnerContext!; | |||
|
|
|||
| return { | |||
There was a problem hiding this comment.
nit: the remainder of the file is the definition of a class (basically), inlined. Feels like it would be a little easier to grok if it was an actual class. Sadly, our lint rules disallow defining more than one class per file, so it would need to be in a separate file, which seems like it might be less grokkable. Perhaps defining the returned object in a new top-level function would remove some indentation and be an easier read ...
There was a problem hiding this comment.
Yeah, agreed, I'll extract it 👍
| } | ||
| } | ||
|
|
||
| export function map<T, E, Resolution>( |
There was a problem hiding this comment.
map doesn't seem like a great name for this function; processResult(), handleResult(), something like that?
There was a problem hiding this comment.
It's actually a standard name for this operation in FP... it's mapping the Result type, be it an Ok or an Err into a specific type.
And TBH I don't feel process and handle tell me anything about what is happening here.
* master: Rename `/api/security/oidc` to `/api/security/oidc/callback`. (elastic#53886) Updating transitive dependencies to use handlebars@4.5.3 (elastic#53899) [Reporting/Tests] consolidate functional test configs (elastic#52671) [Reporting] Correct the docvalue_fields params in the search query Download CSV from Dashboard Panel (elastic#52833) [Test/Newsfeed] Re-enable test and add news item to be filtered (elastic#53905) cleanup server-log action (elastic#53326) [Uptime] Delete uptime eslint rule skip (elastic#50912) [skip-ci] Expression Lifecycle Docs (elastic#51494) [Endpoint] add react router to endpoint app (elastic#53808) [SIEM][Detection Engine] Silence 409 errors on signal creation (elastic#53859) [Maps] get max_result_window and max_inner_result_window from index settings (elastic#53500) [ML] New Platform server shim: update analytics routes to use new platform router (elastic#53521) fixes typo on engine detection page (elastic#53877) [Maps] push mapbox value extraction from VectorStyle and into DynamicStyleProperty (elastic#53806) Fix suggested value for time_zone in range query (elastic#53841) Clean up generic hooks, use react-use instead (elastic#53822)
mikecote
left a comment
There was a problem hiding this comment.
Folder split looks good, just one file I don't think belongs in the alert_instance folder: alerts_client_factory.ts. It doesn't use any sibling files and alerts are different than alert instance.
* master: [SR] Enable component integration tests (elastic#53893) Move index patterns: src/legacy/core_plugins/data 👉 src/plugins/data (elastic#53794) moved Task Manager server code under "server" directory (elastic#53777)
* master: increase delay to make sure license refetched (elastic#53882) Allow custom NP plugin paths in production (elastic#53562) [Maps] show custom color ramps in legend (elastic#53780) [Lens] Expression type on document can be null (elastic#53883) [SIEM] [Detection engine] Add user permission to detection engine (elastic#53778) Update dependency @elastic/charts to v16.0.2 (elastic#52619) Set consistent EOL symbol in core API docs (elastic#53815) [Logs UI] Refactor query bar state to hooks (elastic#52656) [Maps] pass getFieldFormatter to DynamicTextProperty (elastic#53937) Invalidate alert API Key when generating a new one (elastic#53732) [Logs UI] HTTP API for log entries (elastic#53798) [kbn/pm] add caching to bootstrap (elastic#53622) adds createdAt and updatedAt fields to alerting (elastic#53793)
* master: adds strict types to Alerting Client (elastic#53821) [Dashboard] Empty screen redesign (elastic#53681) Migrate config deprecations and `ShieldUser` functionality to the New Platform (elastic#53768)
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
Prevents errors in Alert Executors from forcing their underlying tasks into a zombie state.
* master: allows Alerts to recover gracefully from Executor errors (elastic#53688) [Console] Fix OSS build (elastic#53885) migrate xsrf / version-check / custom-headers handlers to NP (elastic#53684) use NP deprecations in uiSettings (elastic#53755) adds strict types to Alerting Client (elastic#53821) [Dashboard] Empty screen redesign (elastic#53681) Migrate config deprecations and `ShieldUser` functionality to the New Platform (elastic#53768)
Summary
Prevents errors in Alert Executors from forcing their underlying tasks into a zombie state.
resolves #53603
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