adds ability to fetch Alert and Alert Instance state #56625

gmmorris · 2020-02-03T14:25:22Z

Summary

Enables access to the Alert State, which allows us to see which current Alert Instances are active.

This includes:

Addition of a get api on Task Manager
Typing and validation on Serialisation & Deserialisation of the State of an Alert's underlying Task
Addition of the getAlertState api on AlertsClient

closes #48442

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

* master: (42 commits) Move kuery_autocomplete ⇒ NP (elastic#56607) [ML] Functional tests - stabilize job row and analytics result view assertions (elastic#56595) [Discover] Inline angular directives only used in this plugin (elastic#56119) [Discover] Migrate get_sort.js test from mocha to TypeScript (elastic#56011) [SIEM] Enable flow_target_select_connected unit tests (elastic#55618) Start consuming np logging config (elastic#56480) [SIEM] Add eslint-plugin-react-perf (elastic#55960) Mention changed SAML ACS endpoint URL in breaking changes doc. (elastic#56613) Add `getServerInfo` API to http setup contract (elastic#56636) Updates Monitoring alert Jest snapshots Kibana property config migrations (elastic#55937) Vislib replacement toggle (elastic#56439) [Uptime] Add unit tests for QueryContext time calculation (elastic#56671) [SIEM][Detection Engine] Critical blocker, fixes pre-packaged rule miscounts Upgrade EUI to v18.3.0 (elastic#56228) [Maps] Fix server log (elastic#56679) [SIEM] Fixes FTUE when APM node is present (elastic#56574) [Reporting/FieldFormats] expose `setFieldFormats` and call from ReportingPlugin.start (elastic#56563) Update EMS API urls for production (elastic#56657) Ability to delete alerts even when AAD is out of sync (elastic#56543) ...

elasticmachine · 2020-02-05T06:52:07Z

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

* master: (23 commits) Properly handle password change for users authenticated with provider other than `basic`. (elastic#55206) Improve pull request template proposal (elastic#56756) Only change handlers as the element changes (elastic#56782) [SIEM][Detection Engine] Final final rule changes (elastic#56806) [SIEM][Detection Engine] critical blocker, wrong ilm policy, need to match beats ilm policy Move ui/agg_types in to shim data plugin (elastic#56353) [SIEM] Fixes Signals count spinner (elastic#56797) [docs] Update upgrade version path (elastic#56658) [Canvas] Use unique Id for Canvas Embeddables (elastic#56783) [Rollups] Adjust max width for job detail panel (elastic#56674) Prevent http client from converting our form data (elastic#56772) Disable creating alerts client instances when ESO plugin is using an ephemeral encryption key (elastic#56676) Bumps terser-webpack-plugin to 2.3.4 (elastic#56662) Advanced settings component registry ⇒ kibana platform plugin (elastic#55940) [Endpoint] EMT-67: add kql support for endpoint list (elastic#56328) Implement UI for Create Alert form (elastic#55232) Fix: Filter pill base coloring (elastic#56761) fix open close signal on detail page (elastic#56757) [Search service] Move loadingCount to sync search strategy (elastic#56335) Rollup TSVB integration: Add test and fix warning text (elastic#56639) ...

pmuellr

LGTM, made a lot of comments, none critical

x-pack/legacy/plugins/alerting/README.md

x-pack/legacy/plugins/alerting/server/alert_instance/alert_instance.test.ts

pmuellr · 2020-02-05T13:48:04Z

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

+      // decode
+      value => {
+        const decoded = new Date(value);
+        return isNaN(decoded.getTime()) ? t.failure(valueToDecode, context) : t.success(decoded);


I don't think the getTime() is needed on the decoded.getTime(), but perhaps it's a bit arcane without it; I always try it in a REPL tot make sure, eg ... :-)

> isNaN(new Date('foo')) true

So, REPL says you're right, but Typescript disagrees.

It insists new Date() returns a Date, which is true, and so can't be passed as a number to isNaN, which is also true.
The REPL is relying on the coercion that JS automatically applies, but TS dosn't allow you to get away with that.

x-pack/legacy/plugins/alerting/server/alert_instance/alert_instance.ts

x-pack/legacy/plugins/alerting/server/task_runner/alert_task_instance.ts

pmuellr · 2020-02-05T15:58:58Z

x-pack/legacy/plugins/alerting/server/task_runner/task_runner.ts

@@ -192,8 +199,11 @@ export class TaskRunner {
    }

    return {
-      alertTypeState: updatedAlertTypeState,
-      alertInstances: instancesWithScheduledActions,
+      alertTypeState: updatedAlertTypeState || undefined,


I can't quite tell how close to ES this is, but remember that if we "update" a doc instead of "re-index" it, you would have to pass a null to "delete" the existing property in the ES index. I guess we should probably make sure the io-ts encode() is handling this, if needed.

ah, interesting.
This isn't actually a change, as it was a void before, which is an undefined from a function that doesn't return anything.
This has only now become an issue because we're actually typing this exchange!

Any idea what the expected behaviour would be? Would an AlertExecutor that returns no state be expected to wipe out previous state or keep what was there before?

I might split this possible change off of this PR as it feels like it's worth a discussion on its own.
As it stands, an executor won't be able to return null as we haven't marked that as a valid return type (unless they aren't using TS... 🤦‍♂ ) so feels like a separate conversation.

pmuellr · 2020-02-05T16:09:59Z

x-pack/plugins/task_manager/server/create_task_manager.test.ts

-            "waitUntilStarted": [MockFunction],
-          }
-        `);
+      TaskManager {


I'm curious if this test is even useful. It doesn't bring me joy :-)

That's a fair question, it's left overs from Legacy IMHO, but until we migrate everything to TS and NP, feels like this is still worth keeping around?

x-pack/test/alerting_api_integration/security_and_spaces/tests/alerting/get_alert_state.ts

pmuellr · 2020-02-05T16:28:48Z

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

+      expect(alertInstances.length).to.eql(response.body.alertTypeState.runCount);
+      alertInstances.forEach(([key, value], index) => {
+        expect(key).to.eql(`instance-${index}`);
+        expect(value.state).to.eql({ instanceStateValue: true });


I wonder if the instanceStateValue should be something different, to be on the safe side. Perhaps the index value of the instance + runCount.

x-pack/test/plugin_api_integration/plugins/task_manager/init_routes.js

mikecote

Changes LGTM outside of @pmuellr's comments 👍

…t-state * upstream/master: (96 commits) top nav ts arg support (elastic#56984) [SIEM][detection engine] Limit network rules to filebeat source semantics (elastic#57130) Add docs for alerting and action settings (elastic#57035) Add Test to Verify Endpoint App Landing Page (elastic#57129) Update `markdown-to-jsx` (`6.9.3` → `6.11.0`) and `url-parse` (`1.4.4` → `1.4.7`) dependencies. (elastic#57126) chore(NA): removes use of parallel option in the terser minimizer (elastic#57077) [ML] New Platform server shim: update file data visualizer routes to use new platform router (elastic#56972) Specifying valid licenses for the Graph feature (elastic#55911) [APM][docs] Add troubleshooting for non-indexed fields (elastic#54948) [ML] DF Analytics creation: update schema definition for create route (elastic#56979) Remove Kibana a11y guide in favor of EUI (elastic#57021) [Logs UI] Set streamLive false in URL state when arriving from link-to (elastic#56329) [docs] Fix spaces api example json (elastic#50411) Add new config for filebeat index name (elastic#56920) [Metrics-UI] Fix toolbar popover for metrics table row (elastic#56796) Saved Objects testing (elastic#56965) Disabled categorization stats validation (elastic#57087) [Rollups] Server NP migration (elastic#55606) [Metrics UI] Limit group by selector to only 2 fields (elastic#56800) fix auto closing new vis modal when navigating to lens or when navigating away with browser history (elastic#56998) ...

Added missing api on mock TM

kibanamachine · 2020-02-09T23:02:23Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 1cf3ffa

History

💔 Build #25450 failed 06cc5f6
💚 Build #24538 succeeded ad6d76e
💔 Build #24534 failed a34c0cb
💚 Build #24516 succeeded 91e28e8
💔 Build #24334 failed 28e7c7e

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

Enables access to the Alert State, which allows us to see which current Alert Instances are active. This includes: 1. Addition of a `get` api on Task Manager 2. Typing and validation on Serialisation & Deserialisation of the State of an Alert's underlying Task 3. Addition of the `getAlertState` api on AlertsClient

gmmorris added 3 commits February 3, 2020 14:23

expose get api on Task Manager

6e8ec2e

added types and validation around the state of an AlertTypeTask

834d07f

gmmorris changed the title ~~expose get api on Task Manager~~ adds ability to fetch Alert and Alert Instance state Feb 4, 2020

fixed type issues

91e28e8

gmmorris added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.7.0 v8.0.0 labels Feb 5, 2020

gmmorris added 2 commits February 5, 2020 10:13

added getAlertState on AlertClient and exposed on api

45cc53e

gmmorris marked this pull request as ready for review February 5, 2020 10:20

gmmorris added 2 commits February 5, 2020 10:35

added docs

96be465

added docs for TM

ad6d76e

pmuellr approved these changes Feb 5, 2020

View reviewed changes

mikecote approved these changes Feb 6, 2020

View reviewed changes

gmmorris mentioned this pull request Feb 9, 2020

Should we rename lastScheduledActions? #57173

Closed

gmmorris added 2 commits February 10, 2020 09:28

fixed typos

06cc5f6

gmmorris requested a review from a team as a code owner February 9, 2020 20:29

Update mocks.ts

1cf3ffa

Added missing api on mock TM

gmmorris merged commit a02232d into elastic:master Feb 9, 2020

gmmorris mentioned this pull request Feb 9, 2020

[7.x] adds ability to fetch Alert and Alert Instance state (#56625) #57174

Merged

gmmorris deleted the alerting/get-alert-state branch February 9, 2020 23:13

gmmorris mentioned this pull request Feb 13, 2020

[Discuss] Should an Alert Executor that returns void actually wipe out the existing state? #57631

Closed

gmmorris added the backported label Mar 20, 2020

mikecote added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Apr 15, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds ability to fetch Alert and Alert Instance state #56625

adds ability to fetch Alert and Alert Instance state #56625

gmmorris commented Feb 3, 2020 •

edited

Loading

elasticmachine commented Feb 5, 2020

pmuellr left a comment

pmuellr Feb 5, 2020

gmmorris Feb 9, 2020 •

edited

Loading

pmuellr Feb 5, 2020

gmmorris Feb 9, 2020

pmuellr Feb 5, 2020

gmmorris Feb 9, 2020

pmuellr Feb 5, 2020

mikecote left a comment

kibanamachine commented Feb 9, 2020

adds ability to fetch Alert and Alert Instance state #56625

adds ability to fetch Alert and Alert Instance state #56625

Conversation

gmmorris commented Feb 3, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Feb 5, 2020

pmuellr left a comment

Choose a reason for hiding this comment

pmuellr Feb 5, 2020

Choose a reason for hiding this comment

gmmorris Feb 9, 2020 • edited Loading

Choose a reason for hiding this comment

pmuellr Feb 5, 2020

Choose a reason for hiding this comment

gmmorris Feb 9, 2020

Choose a reason for hiding this comment

pmuellr Feb 5, 2020

Choose a reason for hiding this comment

gmmorris Feb 9, 2020

Choose a reason for hiding this comment

pmuellr Feb 5, 2020

Choose a reason for hiding this comment

mikecote left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 9, 2020

💚 Build Succeeded

History

gmmorris commented Feb 3, 2020 •

edited

Loading

gmmorris Feb 9, 2020 •

edited

Loading