[Search Sessions] Implement cancel on search session monitoring task, fetch and process sessions page by page #96321

Dosant · 2021-04-06T15:15:06Z

Summary

Partially addressed #96131

Cancel sessions monitoring task if it takes too long
Also expose trackingTimeout config
Fetches and processes sessions page by page now. Previously we first fetched all the sessions but then processed them.

Checklist

Unit or functional tests were updated or added to match the most common scenarios
After PR is merged, expose new setting in cloud config

For maintainers

This was checked for breaking API changes and was labeled appropriately

Dosant · 2021-04-07T14:06:57Z

x-pack/plugins/data_enhanced/config.ts

+       * trackingTimeout controls for how long task manager waits for search session monitoring task to complete before considering it timed out,
+       * If tasks timeouts it receives cancel signal and next task starts in "trackingInterval" time
+       */
+      trackingTimeout: schema.duration({ defaultValue: '5m' }),


'5m' is the current default in task manager. I don't change the default and expose a way to override it.

Dosant · 2021-04-07T14:07:24Z

x-pack/plugins/data_enhanced/server/search/session/check_running_sessions.ts

  deps: CheckRunningSessionsDeps,
  config: SearchSessionsConfig
-): Promise<void> {
+): Observable<void> {


Just changing from Promise to Observable to be able to abort

Dosant · 2021-04-07T14:09:00Z

x-pack/plugins/data_enhanced/server/search/session/monitoring_task.ts

@@ -48,12 +52,17 @@ function searchSessionRunner(
            logger,
          },
          sessionConfig
-        );
+        )
+          .pipe(takeUntil(aborted$))


The cancelation idea is the following:

We can use pageConfig to process sessions page by page. When abort is triggered we will process the last received page and won't fetch for the one that are left

elasticmachine · 2021-04-07T15:09:46Z

Pinging @elastic/kibana-app-services (Team:AppServices)

Dosant · 2021-04-08T08:05:26Z

@elasticmachine merge upstream

lukasolson

Couple of minor nits below, but overall LGTM

lukasolson · 2021-04-08T23:19:21Z

x-pack/plugins/data_enhanced/server/search/session/monitoring_task.ts

@@ -29,7 +32,8 @@ interface SearchSessionTaskDeps {
 function searchSessionRunner(
  core: CoreSetup<DataEnhancedStartDependencies>,
  { logger, config }: SearchSessionTaskDeps
-) {
+): TaskRunCreatorFunction {
+  const aborted$ = new Subject<void>();


Since we have one global aborted$ for all instances of this task, doesn't this mean that if one instance hits the timeout, any others in-progress will also be aborted? What if we moved the initialization of aborted$ between lines 37/38?

I moved it inside the task factory. Now we for sure have a single aborted$ instance per task

I still would like @elastic/kibana-alerting-services to review how I use their API 🙏

This doesn't look global to me, actually.
A TaskRunner is creates whenever a task is picked up by Kibana, so each task will have its own abort observable, which will be GCed when that task completes.

Looking at this usage, this PR LGTM 👍

x-pack/plugins/data_enhanced/server/search/session/check_running_sessions.test.ts

Dosant · 2021-04-12T07:48:10Z

@elasticmachine merge upstream

x-pack/plugins/data_enhanced/server/search/session/monitoring_task.ts

lizozom

I tested and I see the behavior is correct, mostly that pages are fetched and updated correctly, but I didn't test any negative scenarios. I don't have time to dive deeper today. If this is urgent, could you ask @lukasolson to take another look?

x-pack/plugins/data_enhanced/server/search/session/monitoring_task.ts

…ssion-processing

kibanamachine · 2021-04-12T19:20:22Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #118696 succeeded 8f7d456
💚 Build #118674 succeeded 9dd59cb
💔 Build #118666 failed 7267ea6
💔 Build #118634 failed a3ba3e2
💚 Build #118616 succeeded 9f52707

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

… fetch and process sessions page by page (elastic#96321)

… fetch and process sessions page by page (#96321) (#96875)

… fetch and process sessions page by page (#96321) (#96874)

gmmorris · 2021-04-13T11:07:17Z

x-pack/plugins/data_enhanced/server/search/session/monitoring_task.ts

@@ -39,6 +43,8 @@ function searchSessionRunner(
          logger.debug('Search sessions are disabled. Skipping task.');
          return;
        }
+        if (aborted$.getValue()) return;


This looks OK, but it's worth clarifying that if (for some reason) you return here before Task Manager actually calls cancel, then you'll be wiping out your State as TM will think you've just completed the task without returning any state.

It doesn't look like you're using task state, so I think this is fine, but I felt it was worth flagging

Thanks for the flag!

Dosant mentioned this pull request Apr 6, 2021

[Search Sessions] Monitoring hardening part 1 #96196

Merged

9 tasks

Dosant closed this Apr 6, 2021

Dosant deleted the dev/cancel-session-processing branch April 6, 2021 15:41

Dosant reopened this Apr 6, 2021

cancel tasks if running too long

73ad72f

Dosant force-pushed the dev/cancel-session-processing branch from b6f8d1b to 73ad72f Compare April 7, 2021 14:04

Dosant changed the title ~~Dev/cancel session processing~~ [Search Sessions] Cancel sessions monitoring task if takes too long Apr 7, 2021

Dosant commented Apr 7, 2021

View reviewed changes

Dosant added bug Fixes for quality problems that affect the customer experience Feature:Search Querying infrastructure in Kibana Team:AppServices v7.12.1 v7.13.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Apr 7, 2021

Dosant marked this pull request as ready for review April 7, 2021 15:09

Dosant requested a review from a team as a code owner April 7, 2021 15:09

Dosant requested review from lizozom, lukasolson and gmmorris April 7, 2021 15:09

Merge branch 'master' into dev/cancel-session-processing

c0911f5

Dosant requested a review from a team April 8, 2021 08:14

lukasolson approved these changes Apr 8, 2021

View reviewed changes

kibanamachine and others added 3 commits April 12, 2021 03:48

Merge branch 'master' into dev/cancel-session-processing

3153cd4

jest.useFakeTimers in test

9f52707

improve

a3ba3e2

Dosant added 3 commits April 12, 2021 11:32

fix ts

e1caadf

fix behaviour subject regression

1fa00d4

fetch session one by one

7267ea6

Dosant changed the title ~~[Search Sessions] Cancel sessions monitoring task if takes too long~~ [Search Sessions] Implement cancel on search session monitoring task, fetch and process sessions page by page Apr 12, 2021

Dosant added 3 commits April 12, 2021 13:35

fix ts

edfda35

process older sessions first

9dd59cb

add more unit tests

8f7d456

Dosant commented Apr 12, 2021

View reviewed changes

x-pack/plugins/data_enhanced/server/search/session/monitoring_task.ts Show resolved Hide resolved

lizozom approved these changes Apr 12, 2021

View reviewed changes

x-pack/plugins/data_enhanced/server/search/session/monitoring_task.ts Outdated Show resolved Hide resolved

Dosant added 2 commits April 12, 2021 19:16

trackingTimeout -> monitoringTaskTimeout

a93fdd2

Merge branch 'master' of github.com:elastic/kibana into dev/cancel-se…

237b573

…ssion-processing

Dosant merged commit c4b3dfd into elastic:master Apr 12, 2021

Dosant added a commit to Dosant/kibana that referenced this pull request Apr 12, 2021

[Search Sessions] Implement cancel on search session monitoring task,…

a0fc0f2

… fetch and process sessions page by page (elastic#96321)

Dosant mentioned this pull request Apr 12, 2021

[7.x] [Search Sessions] Implement cancel on search session monitoring task, fetch and process sessions page by page (#96321) #96874

Merged

Dosant added a commit to Dosant/kibana that referenced this pull request Apr 12, 2021

[Search Sessions] Implement cancel on search session monitoring task,…

c926926

… fetch and process sessions page by page (elastic#96321)

Dosant mentioned this pull request Apr 12, 2021

[7.12] [Search Sessions] Implement cancel on search session monitoring task, fetch and process sessions page by page (#96321) #96875

Merged

Dosant added a commit that referenced this pull request Apr 12, 2021

[Search Sessions] Implement cancel on search session monitoring task,…

1eacc79

… fetch and process sessions page by page (#96321) (#96875)

Dosant added a commit that referenced this pull request Apr 12, 2021

[Search Sessions] Implement cancel on search session monitoring task,…

87549c6

… fetch and process sessions page by page (#96321) (#96874)

gmmorris reviewed Apr 13, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search Sessions] Implement cancel on search session monitoring task, fetch and process sessions page by page #96321

[Search Sessions] Implement cancel on search session monitoring task, fetch and process sessions page by page #96321

Dosant commented Apr 6, 2021 •

edited

Loading

Dosant Apr 7, 2021

Dosant Apr 7, 2021

Dosant Apr 7, 2021

elasticmachine commented Apr 7, 2021

Dosant commented Apr 8, 2021

lukasolson left a comment

lukasolson Apr 8, 2021

Dosant Apr 12, 2021 •

edited

Loading

gmmorris Apr 13, 2021

Dosant commented Apr 12, 2021

lizozom left a comment

kibanamachine commented Apr 12, 2021

gmmorris Apr 13, 2021

Dosant Apr 13, 2021

[Search Sessions] Implement cancel on search session monitoring task, fetch and process sessions page by page #96321

[Search Sessions] Implement cancel on search session monitoring task, fetch and process sessions page by page #96321

Conversation

Dosant commented Apr 6, 2021 • edited Loading

Summary

Checklist

For maintainers

Dosant Apr 7, 2021

Choose a reason for hiding this comment

Dosant Apr 7, 2021

Choose a reason for hiding this comment

Dosant Apr 7, 2021

Choose a reason for hiding this comment

elasticmachine commented Apr 7, 2021

Dosant commented Apr 8, 2021

lukasolson left a comment

Choose a reason for hiding this comment

lukasolson Apr 8, 2021

Choose a reason for hiding this comment

Dosant Apr 12, 2021 • edited Loading

Choose a reason for hiding this comment

gmmorris Apr 13, 2021

Choose a reason for hiding this comment

Dosant commented Apr 12, 2021

lizozom left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 12, 2021

💚 Build Succeeded

Metrics [docs]

History

gmmorris Apr 13, 2021

Choose a reason for hiding this comment

Dosant Apr 13, 2021

Choose a reason for hiding this comment

Dosant commented Apr 6, 2021 •

edited

Loading

Dosant Apr 12, 2021 •

edited

Loading