TaskManager tasks scheduled without attempting to run

[kobelb]

Summary

Prior to this change, every call to schedule was calling attemptToRun which was pushing an item into the task poller's pollRequest$. This means that multiple frequent calls to schedule were all trying to cause tasks to be claimed, until the TaskPoller hits the capacity of the TaskPool and starts filtering those items out and preventing any more tasks from trying to be claimed. This unintentionally violates the general flow control which the TaskPoller tries to implement.

The solution implemented in this PR just removes the call to attemptToRun. This means that when a task is first scheduled, it only has the potential to run during the next normal polling interval. If the consumer of TaskManager wishes for this to be run immediately, they have the option of then using runNow.

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

"Release Note: Fixing bug where calls to schedule a task with task manager would cause jobs to be claimed more frequently than the configured poll interval"

[kobelb]

@elasticmachine merge upstream

[elasticmachine]

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

[gmmorris]

This unintentionally violates the general flow control which the TaskPoller tries to implement.

Haha, actually, that was the intention, in order to nudge TM into action sooner.
concatMap is supposed to be blocking (it creates backpressure preventing new work from being pushed down the queue) and so a busy TM will drop these requests but a TM that isn't working will be nudged into work sooner.

I'm not sure I understand the problem this solves.. perhaps I'm missing something here.

[gmmorris]

Had a chat with @kobelb and the context of this change is clearer now so LGTM, but we shouldn't merge this into 7.7 as it might have an impact on the validity of the performance tests done by siem - best to merge this into master and 7.8 to give ourselves the chance to measure the impact.

[kobelb]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: d7cafa2

History

• 💚 Build #37703 succeeded f138852
• 💔 Build #37684 failed 75310f9

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

[kobelb]

@gmmorris I did some additional testing today to compare the performance before and after the changes introduced by this PR. Using a ton of basic SIEM rules I created, I was seeing comparable tasks being run per minute. When SIEM was initially scheduling all of the tasks, I was seeing a higher throughput without these changes, which is expected. Once the scheduling was done, both before and after we were looking at right under 130 tasks per minute.

[gmmorris]

@gmmorris I did some additional testing today to compare the performance before and after the changes introduced by this PR. Using a ton of basic SIEM rules I created, I was seeing comparable tasks being run per minute. When SIEM was initially scheduling all of the tasks, I was seeing a higher throughput without these changes, which is expected. Once the scheduling was done, both before and after we were looking at right under 130 tasks per minute.

Yeah, as you said... as expected. 🤔

Seems like if the overall throughput is basically the same, the added complexity (pre these changes) probably isn't worth it. 👍