allows Alerts to recover gracefully from Executor errors #53688

gmmorris · 2019-12-20T12:16:25Z

Summary

Prevents errors in Alert Executors from forcing their underlying tasks into a zombie state.
resolves #53603

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~This was checked for cross-browser compatibility, including a check against IE11~~
~~Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-12-20T12:17:02Z

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

kibanamachine · 2019-12-23T11:44:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 628b4c9

History

💚 Build #16924 succeeded e47f645
💚 Build #16890 succeeded f2971df

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

kibanamachine · 2019-12-23T18:35:31Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: d5991af

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

gmmorris · 2019-12-30T10:38:50Z

@elasticmachine merge upstream

mikecote

A few nits but changes LGTM! Thanks for adding more tests!

x-pack/test/alerting_api_integration/common/lib/alert_utils.ts

x-pack/test/alerting_api_integration/common/lib/test_assertions.ts

x-pack/test/alerting_api_integration/spaces_only/tests/alerting/alerts.ts

gmmorris · 2020-01-02T08:42:49Z

@elasticmachine merge upstream

elasticmachine · 2020-01-02T08:42:51Z

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

LGTM, left a few comments

pmuellr · 2020-01-02T22:57:45Z

x-pack/legacy/plugins/alerting/server/lib/task_runner_factory.ts

      getBasePath,
    } = this.taskRunnerContext!;

    return {


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 ...

Yeah, agreed, I'll extract it 👍

pmuellr · 2020-01-02T23:26:23Z

x-pack/legacy/plugins/alerting/server/lib/result_type.ts

+  }
+}
+
+export function map<T, E, Resolution>(


map doesn't seem like a great name for this function; processResult(), handleResult(), something like that?

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 [email protected] (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

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)

kibanamachine · 2020-01-06T12:34:32Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 2c163ea

History

💚 Build #18012 succeeded 8c34d4a
💚 Build #17800 succeeded 5a2bbe1
💚 Build #17768 succeeded 0a7dcf3
💔 Build #17752 failed d8a07e1
💔 Build #17740 failed 8fdc4b0

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

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)

…4018) Prevents errors in Alert Executors from forcing their underlying tasks into a zombie state.

refactored alert task runner

f2971df

gmmorris added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v7.6.0 v8.0.0 labels Dec 20, 2019

gmmorris added 2 commits December 20, 2019 15:25

handle exceptions in alert executor

e47f645

log error in alert executor

ae37c2d

gmmorris changed the title ~~[DRAFT] allows Alerts to recover gracefully from Executor errors~~ allows Alerts to recover gracefully from Executor errors Dec 23, 2019

gmmorris marked this pull request as ready for review December 23, 2019 10:19

Merge branch 'master' into alerting/zombie-tasks

628b4c9

gmmorris mentioned this pull request Dec 23, 2019

[Discuss] Resurrect Zombie Alert tasks #53603

Closed

elastic deleted a comment from elasticmachine Dec 23, 2019

wrap alert validation in task runner to prevent failing the task

d5991af

Merge branch 'master' into alerting/zombie-tasks

6ce8199

mikecote approved these changes Dec 31, 2019

View reviewed changes

gmmorris added 2 commits January 2, 2020 08:44

cleaned up naming

d1ef6ef

pmuellr approved these changes Jan 3, 2020

View reviewed changes

gmmorris added 4 commits January 3, 2020 10:20

split up alerting lib folder

0ae7220

split TaskRunner tests from Factory

8fdc4b0

removed unused imports

d8a07e1

mikecote reviewed Jan 3, 2020

View reviewed changes

gmmorris added 2 commits January 3, 2020 14:02

moved alerts_client_factory

0a7dcf3

Merge branch 'master' into alerting/zombie-tasks

5a2bbe1

* 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)

gmmorris added 2 commits January 6, 2020 10:00

Merge branch 'master' into alerting/zombie-tasks

2c163ea

* 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)

gmmorris merged commit 785b916 into elastic:master Jan 6, 2020

gmmorris added a commit to gmmorris/kibana that referenced this pull request Jan 6, 2020

allows Alerts to recover gracefully from Executor errors (elastic#53688)

871687a

Prevents errors in Alert Executors from forcing their underlying tasks into a zombie state.

gmmorris mentioned this pull request Jan 6, 2020

[7.x] allows Alerts to recover gracefully from Executor errors (#53688) #54018

Merged

gmmorris added a commit that referenced this pull request Jan 6, 2020

allows Alerts to recover gracefully from Executor errors (#53688) (#5…

447c15d

…4018) Prevents errors in Alert Executors from forcing their underlying tasks into a zombie state.

gmmorris added the backported label Jan 6, 2020

mikecote mentioned this pull request Apr 7, 2020

extraneous error/warning messages when deleting alerts #42477

Closed

allows Alerts to recover gracefully from Executor errors #53688

allows Alerts to recover gracefully from Executor errors #53688

Uh oh!

Conversation

gmmorris commented Dec 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

For maintainers

Uh oh!

elasticmachine commented Dec 20, 2019

Uh oh!

kibanamachine commented Dec 23, 2019

💚 Build Succeeded

History

Uh oh!

kibanamachine commented Dec 23, 2019

💚 Build Succeeded

Uh oh!

gmmorris commented Dec 30, 2019

Uh oh!

mikecote left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gmmorris commented Jan 2, 2020

Uh oh!

elasticmachine commented Jan 2, 2020

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

pmuellr Jan 2, 2020

Choose a reason for hiding this comment

Uh oh!

gmmorris Jan 3, 2020

Choose a reason for hiding this comment

Uh oh!

pmuellr Jan 2, 2020

Choose a reason for hiding this comment

Uh oh!

gmmorris Jan 3, 2020

Choose a reason for hiding this comment

Uh oh!

mikecote left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 6, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gmmorris commented Dec 20, 2019 •

edited

Loading