[Task Manager] Cleans up legacy plugin structure #80381

gmmorris · 2020-10-13T17:00:34Z

Summary

This PR addresses a list of legacy code debt the plugin has incurred over the past year due to extensive changes in its internals and the adoption of the Kibana Platform.

It includes:

The TaskManager class has been split into several independent components: TaskTypeDictionary, TaskPollingLifecycle, TaskScheduling, Middleware. This has made it easier to understand the roles of the different parts and makes it easier to plug them into the observability work.
The exposed mocks have been corrected to correctly express the Kibana Platform api
The lifecycle has been corrected to remove the need for intermediary streames/promises which we're needed when we first introduced the setup/start lifecycle to support legacy.
The Logger mocks have been replaced with the platform's coreMocks implementation
The integration tests now test the plugin's actual public api (instead of the internals).
The Legacy Elasticsearch client has been replaced with the typed client in response to the deprecation notice.
Typing has been narrowed to prevent the type field from conflicting with the key in the TaskDictionary. This could have caused the displayed type on a task to differ from the type used in the Dictionary itself (this broke a test during refactoring and could have caused a bug in production code if left).

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

* master: (115 commits) [ML] Transforms/DF Analytics: Fix data grid column sorting. (elastic#80618) added brace import to vis editor (elastic#80652) Fix error rate sorting in services list (elastic#80764) Emit info log when using custom registry URL (elastic#80768) [Reporting] Config Schema Validation for rules[N].protocol strings (elastic#80766) Add Storybook a11y addon (elastic#80069) Fix anomaly alert selection text (elastic#80746) [Security Solution] [Maps] Kibana index pattern, comma bug fix (elastic#80208) [kbn/optimizer] tweak split chunks options (elastic#80444) update template to use the new team label (elastic#80748) [Security Solution] Fix the Field dropdown in Timeline data providers resets when scrolled (elastic#80718) Adjusts observability alerting perms to require "all" (elastic#79896) [Security Solutions][Detection Engine] Fixes pre-packaged rules which contain exception lists to not overwrite user defined lists (elastic#80592) [data.ui] Fix flaky test & lazy loading rendering artifacts. (elastic#80612) Licensed feature usage for connectors (elastic#77679) [Security Solution] Cypress template creation (elastic#80180) [APM] Hide service if only data is from ML (elastic#80145) Fix role mappings test for ESS (elastic#80604) [Maps] Add support for envelope (elastic#80614) [Security Solution] Update button text according to status (elastic#80389) ...

elasticmachine · 2020-10-16T15:36:47Z

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

smith

APM changes look good.

elasticmachine · 2020-10-16T15:39:57Z

Pinging @elastic/apm-ui (Team:apm)

patrykkopycinski

SIEM/Endpoint changes LGTM!

azasypkin

Security changes LGTM with a green CI, thanks!

* master: (43 commits) [ML] Transforms: Fix tab ids for expanded row. (elastic#80666) server logs config paths to use for runner (elastic#52980) Fix audit logger logging to console even when disabled (elastic#80928) skip flaky suite (elastic#80929) Added Enterprise Search config to kibana-docker (elastic#80872) skip flaky suite (elastic#80914) [keystore_cli] parse values as JSON before adding to keystore (elastic#80848) [Ingest Manager] Fix for comparing versions with -SNAPSHOT suffix (elastic#80742) ECS audit logging (elastic#74640) [Uptime] Add client-side unit tests for remaining synthetics code (elastic#80215) [Security_Solution][Resolver] Promote z-index on node labels (elastic#80854) Move renderHeaderActions back into mount useEffect + update tests (elastic#80861) [Reporting] Document Network Policy configuration (elastic#80431) [Reporting] Add contextual documentation for CSV Max Bytes setting (elastic#80782) Add catch for Enterprise Search sending back a 401 response instead of redirect (elastic#80757) [Actions] Back Button on Add Connector Flyout (elastic#80160) removing `kibana_datatable` in favor of `datatable` (elastic#80548) [Alerting UI] Updating 'Add new' wording (elastic#80509) [Docs] Document Encrypted Saved Objects functionality. (elastic#80183) [Discover] fix auto-refresh (elastic#80635) ...

pmuellr

LGTM, added some non-blocking comments and questions

Also, tried this out with my 100 alerts firing 4 instances every second test, seemed to work fine

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

pmuellr · 2020-10-19T14:35:46Z

x-pack/plugins/task_manager/server/plugin.ts

+   * @returns void
+   */
+  private assertStillInSetup(operation: string) {
+    if (this.taskPollingLifecycle?.isStarted) {


I like this kind of assertion, but it does require that I go look at taskPollingLifecycle to see when isStarted is true. And then there's some wiggle room after all the plugins have gone through setup where maybe something could slip in here before isStarted turns true - seems highly unlikely. Would it be clearer to just have an instance var here called isStarted set in the start() method? Or does core somehow provide some kind of helper here?

Core doesn't provide anything sadly, I discussed it with Platform last week.
The reason I went with this instead of adding a variable (I did actually have one at one point) was because they could go out of sync and it actually made sense for me for the lifecycle to be the source of truth.

It's actually a pain because the main goal here to prevent types/middle being added after the poller started... it's not *really about the plugin lifecycle.

pmuellr · 2020-10-19T15:00:40Z

x-pack/plugins/task_manager/server/polling_lifecycle.ts

+      }
+      return docs;
+    } catch (ex) {
+      if (identifyEsError(ex).includes('cannot execute [inline] scripts')) {


Have we looked into whether we can do this in a cleaner way? Eg, somehow query ES during start and check to see if it supports inline scripts, and just don't start there. Either checking some ES config / capability, or running some no-op'ish query that uses an inline script.

Not really... but this wasn't added in this PR, just moved.
Happy to revisit in a different issue if you'd like 🤷

pmuellr · 2020-10-19T15:13:33Z

x-pack/plugins/task_manager/server/task_scheduling.ts

+                      taskLifecycleStatus === TaskStatus.Running ||
+                      taskLifecycleStatus === TaskStatus.Claiming
+                    ) {
+                      return new Error(`Failed to run task "${taskId}" as it is currently running`);


Is this really an error? I think the path to get here is that you do a runNow(), but the task is already claimed or running, so it "fails". I think the only use of runNow() in alerting is in alert.update(), and in that case we don't really care when the alert runs, if it happened to be running when the update() was called, that's ok. Looking at that call site, it appears we generate an error message there, not sure it's even required.

Not that it should be fixed in this PR, but am wondering ...

@mikecote and I actually just discussed this today - we do think this is needed as we'll likely want to add a "test alert" in the UI in the future and this would be displayed to the user.

In anycase - this wasn't added in this PR, just moved.
Happy to revisit in a different issue if you'd like 🤷

x-pack/plugins/task_manager/server/task_scheduling.ts

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

x-pack/plugins/task_manager/server/test_utils/index.ts

Co-authored-by: Patrick Mueller <pmuellr@gmail.com>

timroes

Looked at Kibana App changes only. Code looks good, didn't check out and test.

mikecote

Changes LGTM, great work! Few small comments

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

mikecote · 2020-10-19T18:29:01Z

x-pack/plugins/task_manager/server/task_scheduling.mock.ts

+    ensureScheduled: jest.fn(),
+    schedule: jest.fn(),
+    runNow: jest.fn(),
+  } as unknown) as jest.Mocked<TaskScheduling>;


Converting to unknown will allow the object above to become a partial of what TaskScheduling is. Doing something like the following would solve the concern:

const mocked: jest.Mocked<TaskScheduling > = { ensureScheduled: jest.fn(), schedule: jest.fn(), runNow: jest.fn(), }; return mocked;

Nope, sadly this dosn't work because of the private fields.
If you try what you posted above you get this error:

Type '{ ensureScheduled: jest.Mock<any, any>; schedule: jest.Mock<any, any>; runNow: jest.Mock<any, any>; }' is not assignable to type 'Mocked<TaskScheduling>'. Type '{ ensureScheduled: Mock<any, any>; schedule: Mock<any, any>; runNow: Mock<any, any>; }' is missing the following properties from type 'TaskScheduling': store, taskPollingLifecycle, logger, middleware, awaitTaskRunResult ts(2322)

The only way I've found to get around this is the cast to unknown.
In some places we use PublicMethodsOf<> to get around it but then it complains when you try to pass in the actual thing 🤦

Have you found any other solution to this?

PublicMethodsOf<> is what I've seen solve the problem elsewhere. I'm surprised it would complain about passing the actual thing in some places. I may spike something to see what's up.

mikecote · 2020-10-19T18:29:33Z

x-pack/plugins/task_manager/server/task_store.mock.ts

@@ -25,6 +34,7 @@ export const taskStoreMock = {
      maxAttempts,
      index,
      taskManagerId,
+      events,
    } as unknown) as jest.Mocked<TaskStore>;


Similar here regarding converting to unknown type.

x-pack/plugins/task_manager/server/plugin.ts

mikecote · 2020-10-19T18:36:05Z

x-pack/plugins/task_manager/server/polling_lifecycle.mock.ts

+        return opts.events$ ?? of();
+      },
+      stop: jest.fn(),
+    } as unknown) as jest.Mocked<TaskPollingLifecycle>;


Similar comment regarding converting to unknown.

…morris/kibana into task-manager/refactor-oversized-class * 'task-manager/refactor-oversized-class' of github.com:gmmorris/kibana: Update x-pack/plugins/task_manager/server/polling_lifecycle.test.ts

kibanamachine · 2020-10-20T11:16:28Z

💚 Build Succeeded

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

Metrics [docs]

✅ unchanged

History

💚 Build #82528 succeeded 7d764b9
💚 Build #82368 succeeded c30d71b
💚 Build #82212 succeeded 1c21c6b
💔 Build #82163 failed 77d4450
💔 Build #82150 failed 42e041f

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

This PR addresses a list of legacy code debt the plugin has incurred over the past year due to extensive changes in its internals and the adoption of the Kibana Platform. It includes: 1. The `TaskManager` class has been split into several independent components: `TaskTypeDictionary`, `TaskPollingLifecycle`, `TaskScheduling`, `Middleware`. This has made it easier to understand the roles of the different parts and makes it easier to plug them into the observability work. 2. The exposed `mocks` have been corrected to correctly express the Kibana Platform api 3. The lifecycle has been corrected to remove the need for intermediary streames/promises which we're needed when we first introduced the `setup`/`start` lifecycle to support legacy. 4. The Logger mocks have been replaced with the platform's `coreMocks` implementation 5. The integration tests now test the plugin's actual public api (instead of the internals). 6. The Legacy Elasticsearch client has been replaced with the typed client in response to the deprecation notice. 7. Typing has been narrowed to prevent the `type` field from conflicting with the key in the `TaskDictionary`. This could have caused the displayed `type` on a task to differ from the `type` used in the Dictionary itself (this broke a test during refactoring and could have caused a bug in production code if left).

* master: (64 commits) Rename Security Solution Bug Template (elastic#81187) Update links (elastic#81125) Specify format for date range query (elastic#81025) [Alerting] Improve toast when alert is created (elastic#80327) [UX] Add empty states (elastic#80904) Add TS config for kibana_legacy (elastic#80992) [Telemetry] Add method to enable endpoint security data usage example (elastic#80940) [Alerting] Add scoped cluster client to alerts and actions services (elastic#80794) Fix reactRouterNavigate when used with a string (elastic#80520) [Security Solution] [Detections] Read privileges for dependencies (elastic#80852) [ML] Fixing exclude frequent in advanced wizard (elastic#81121) Fix security solution template label (elastic#80976) [DOCS] Update index management docs (elastic#80893) [APM] Error rate on service list page is not in sync with the value at the transaction page (elastic#80814) skip flaky suite (elastic#81072) [Task Manager] Cleans up legacy plugin structure (elastic#80381) Support unsigned_long fields (elastic#81115) [Form lib] Export internal state instead of raw state (elastic#80842) [Lens] Add toast notification when visualization is saved (elastic#80788) Index pattern edit field formatter API (elastic#78352) ...

gmmorris added 7 commits October 13, 2020 18:00

extract task dictionary from TM

311103e

fixed typing

8396812

extract middleware and store from taskManager

177b6b9

extracted store APIs from TM

3d2cde8

removed redundant lifecycle checks

afcdfd6

extract scheduling from task_manager

bbd445a

renamed task_manager to polling_lifecycle

02afe23

gmmorris changed the title ~~extract task dictionary from TM~~ [Task Manager] Cleans up legacy plugin structure Oct 15, 2020

gmmorris added 6 commits October 15, 2020 19:07

last bit og logger

320ea57

removed usage of deprecated legacy es client

b0275e0

renamed variables to match new names of components

aa669e6

made type optional

42e041f

removed type from tasks

77d4450

gmmorris added Feature:Task Manager 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.11.0 v8.0.0 labels Oct 16, 2020

simplified middleware

4e08766

gmmorris marked this pull request as ready for review October 16, 2020 15:36

gmmorris requested review from a team as code owners October 16, 2020 15:36

gmmorris requested a review from a team October 16, 2020 15:36

gmmorris requested review from a team as code owners October 16, 2020 15:36

smith approved these changes Oct 16, 2020

View reviewed changes

botelastic bot added the Team:APM All issues that need APM UI Team support label Oct 16, 2020

removed types from the tasks in actions and alerts

1c21c6b

patrykkopycinski approved these changes Oct 16, 2020

View reviewed changes

azasypkin approved these changes Oct 16, 2020

View reviewed changes

gmmorris mentioned this pull request Oct 19, 2020

[Task Manager] Mark task as failed if maxAttempts has been met. #80681

Merged

1 task

pmuellr approved these changes Oct 19, 2020

View reviewed changes

Update x-pack/plugins/task_manager/server/polling_lifecycle.test.ts

7d764b9

Co-authored-by: Patrick Mueller <pmuellr@gmail.com>

timroes reviewed Oct 19, 2020

View reviewed changes

mikecote approved these changes Oct 19, 2020

View reviewed changes

gmmorris added 2 commits October 20, 2020 09:38

removed unneeded context name from logger

61a670a

removed unneeded type in definitions

386ff0b

timroes approved these changes Oct 20, 2020

View reviewed changes

gmmorris added 4 commits October 20, 2020 10:15

use mock logger to avoid casting

f9f9094

Merge branch 'task-manager/refactor-oversized-class' of github.com:gm…

4622ef5

…morris/kibana into task-manager/refactor-oversized-class * 'task-manager/refactor-oversized-class' of github.com:gmmorris/kibana: Update x-pack/plugins/task_manager/server/polling_lifecycle.test.ts

broke awaitTaskRunResult down

06ea05a

removed unneeded mock data

3812990

gmmorris merged commit 5460ad7 into elastic:master Oct 20, 2020

gmmorris mentioned this pull request Oct 20, 2020

[7.x] [Task Manager] Cleans up legacy plugin structure (#80381) #81139

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Task Manager] Cleans up legacy plugin structure #80381

[Task Manager] Cleans up legacy plugin structure #80381

gmmorris commented Oct 13, 2020 •

edited

Loading

elasticmachine commented Oct 16, 2020

smith left a comment

elasticmachine commented Oct 16, 2020

patrykkopycinski left a comment

azasypkin left a comment

pmuellr left a comment

pmuellr Oct 19, 2020

gmmorris Oct 19, 2020

pmuellr Oct 19, 2020

gmmorris Oct 19, 2020

pmuellr Oct 19, 2020

gmmorris Oct 19, 2020

timroes left a comment

mikecote left a comment

mikecote Oct 19, 2020

gmmorris Oct 20, 2020 •

edited

Loading

mikecote Oct 20, 2020

mikecote Oct 19, 2020

mikecote Oct 19, 2020

kibanamachine commented Oct 20, 2020

[Task Manager] Cleans up legacy plugin structure #80381

[Task Manager] Cleans up legacy plugin structure #80381

Conversation

gmmorris commented Oct 13, 2020 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented Oct 16, 2020

smith left a comment

Choose a reason for hiding this comment

elasticmachine commented Oct 16, 2020

patrykkopycinski left a comment

Choose a reason for hiding this comment

azasypkin left a comment

Choose a reason for hiding this comment

pmuellr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timroes left a comment

Choose a reason for hiding this comment

mikecote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmmorris Oct 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Oct 20, 2020

💚 Build Succeeded

Metrics [docs]

History

gmmorris commented Oct 13, 2020 •

edited

Loading

gmmorris Oct 20, 2020 •

edited

Loading