[Search Sessions] Monitoring hardening part 1 #96196

lizozom · 2021-04-04T16:14:12Z

Summary

Implements some of the more urgent items from #96131

Decrease default page size to 100
Set default strategy
Don't create session objects when feature is disabled
Clear monitoring task if it was disabled (after kibana restart)
Use concatMap to serialize session checkup

Checklist

Delete any items that are not applicable to this PR.

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
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately

Set default strategy Don't create sessions when disabled Clear monitoring task when disabled Use concatMap to serialize session checkup

elasticmachine · 2021-04-04T16:14:52Z

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

…toring-hardening

lukasolson

Overall, changes LGTM! A couple of minor things below. Also, just to understand, were we seeing the entries without "strategy" when the feature was disabled? Or how can I verify the before/after behavior to make sure this fixes it?

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

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

@@ -154,7 +154,7 @@ export async function checkRunningSessions(
  try {
    await getAllSavedSearchSessions$(deps, config)
      .pipe(
-        mergeMap(async (runningSearchSessionsResponse) => {
+        concatMap(async (runningSearchSessionsResponse) => {


I mentioned this in the other issue as well, but what are we hoping to accomplish by processing these serially rather than in parallel? The main issue of concern seemed to be that the update request is too large, so the only benefits I can see with serially are a decrease in CPU/memory, but I'm not sure if that will outweigh the fact that this change may also make the process take much longer.

The whole idea of paging the sessions was to update the items in reasonable amounts and in orderly fashion.
With mergeMap we weren't doing that.

I think that serializing bulks of 100 search sessions makes sense, but it's going to be hard to quantify the impact.
Not 100% the same (more related to reducing the page size), but the article @Dosant linked convinced me https://eng.lifion.com/promise-allpocalypse-cfb6741298a7?gi=2c3ce0dba6ea

@lukasolson, I think there is also a difference between grouping per 100 requests or sending all of them to ES? with concatMap we spread out the load.

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

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

…vice.ts Co-authored-by: Lukas Olson <olson.lukas@gmail.com>

lizozom · 2021-04-06T09:28:20Z

@lukasolson IMO searches without a strategy were initiated server side by TSVB

Dosant · 2021-04-06T11:43:19Z

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

+        const [coreStart, pluginsStart] = await core.getStartServices();
+        if (!sessionConfig.enabled) {
+          logger.info('Search sessions are disabled. Clearing task.');
+          await pluginsStart.taskManager.removeIfExists(SEARCH_SESSIONS_TASK_ID);


@gmmorris (or someone else who another task manager expert :D ), could you please review? (If this already hasn't been discussed somewhere) 🙏

oooh I do not know about this 😬
What removeIfExists will do here is delete the SO that backs the task that is currently running.
I'd expect that to cause this task execution to fail....

I suspect you'll see that if you add an end to end test that verifies this behaviour.

What are we trying to achieve here?
Are you looking to respond reactively to a config change?
A better approach would be to do this from outside the task execution - subscribe to the config change and delete the task. And, in the task executor, skip the code path you don't want to execute if config is disabled but the task was rerun before you got a chance to remove it.

This is what we try to achieve:

Assume Kibana is running with this feature ON

Restart Kibana with the feature OFF

We want to be sure that we don't run the task when the feature is OFF

Currently, this is not happening in master.

@gmmorris Updated.
Since changing this setting restarts the server, I'm now unschedulng the task on the setupMonitoring function.

Dosant

Code LGTM, will test

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

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

@lizozom, FYI, I have a pr that implements cancel on a task definition here
I started it from your pr. We can wait for yours to go in first or merge those together. I am fine with both

Dosant

LGTM. Tested pageSize and concatMap change.
I am not sure we've finished this discussion: https://github.com/elastic/kibana/pull/96196/files#r607388697, but I think concatMap is the right thing here to do to spread out the load on es. In the cases where it gets too slow - pageSize can be increased.

I'd wait for someone from alerting to code review remove task change

…bana into sessions/monitoring-hardening

…toring-hardening

gmmorris

The task lifecycle LGTM

I haven't tested it locally, as I'm not quite familiar with search sessions, but the flow in the task scheduling looks good 👍.

kibanamachine · 2021-04-07T12:36:38Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #117479 succeeded a3ac094
💚 Build #117206 succeeded fd68d01
💚 Build #117195 succeeded 5e488cc
💔 Build #117187 failed b5e74dd
💔 Build #117172 failed 0cb55f6

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

cc @lizozom

* Decrease default pageSize to 100 Set default strategy Don't create sessions when disabled Clear monitoring task when disabled Use concatMap to serialize session checkup * ts * ts * ts * Update x-pack/plugins/data_enhanced/server/search/session/session_service.ts Co-authored-by: Lukas Olson <olson.lukas@gmail.com> * Search sessions are disabled * Clear task on server start Co-authored-by: Lukas Olson <olson.lukas@gmail.com>

kibanamachine · 2021-04-07T12:39:34Z

💚 Backport successful

✅ 7.12 / #96399
✅ 7.x / #96400

The backport PRs will be merged automatically after passing CI.

* Decrease default pageSize to 100 Set default strategy Don't create sessions when disabled Clear monitoring task when disabled Use concatMap to serialize session checkup * ts * ts * ts * Update x-pack/plugins/data_enhanced/server/search/session/session_service.ts Co-authored-by: Lukas Olson <olson.lukas@gmail.com> * Search sessions are disabled * Clear task on server start Co-authored-by: Lukas Olson <olson.lukas@gmail.com> Co-authored-by: Liza Katz <lizka.k@gmail.com> Co-authored-by: Lukas Olson <olson.lukas@gmail.com>

Liza K added 2 commits April 4, 2021 19:12

Decrease default pageSize to 100

c51d5fd

Set default strategy Don't create sessions when disabled Clear monitoring task when disabled Use concatMap to serialize session checkup

ts

0cb55f6

lizozom self-assigned this Apr 4, 2021

lizozom requested a review from a team as a code owner April 4, 2021 16:14

lizozom added auto-backport Deprecated - use backport:version if exact versions are needed bug Fixes for quality problems that affect the customer experience v7.12.1 v7.13.0 v8.0.0 Team:AppServices labels Apr 4, 2021

lizozom added the release_note:skip Skip the PR/issue when compiling release notes label Apr 4, 2021

lizozom changed the title ~~[Sessions] Monitoring hardening part 1~~ [Search Sessions] Monitoring hardening part 1 Apr 4, 2021

Liza K and others added 4 commits April 5, 2021 10:58

ts

af9f767

Merge branch 'master' of github.com:elastic/kibana into sessions/moni…

b5e74dd

…toring-hardening

ts

5e488cc

Merge branch 'master' into sessions/monitoring-hardening

fd68d01

lukasolson reviewed Apr 5, 2021

View reviewed changes

lizozom and others added 2 commits April 6, 2021 12:25

Update x-pack/plugins/data_enhanced/server/search/session/session_ser…

fb72103

…vice.ts Co-authored-by: Lukas Olson <olson.lukas@gmail.com>

Search sessions are disabled

a3ac094

lizozom requested a review from lukasolson April 6, 2021 09:28

Dosant reviewed Apr 6, 2021

View reviewed changes

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

Dosant approved these changes Apr 7, 2021

View reviewed changes

Liza K added 3 commits April 7, 2021 13:26

Clear task on server start

4ad1c4a

Merge branch 'sessions/monitoring-hardening' of github.com:lizozom/ki…

6922cee

…bana into sessions/monitoring-hardening

Merge branch 'master' of github.com:elastic/kibana into sessions/moni…

511ada5

…toring-hardening

lizozom requested a review from gmmorris April 7, 2021 10:36

gmmorris approved these changes Apr 7, 2021

View reviewed changes

lizozom merged commit 7584b72 into elastic:master Apr 7, 2021

kibanamachine mentioned this pull request Apr 7, 2021

[7.12] [Search Sessions] Monitoring hardening part 1 (#96196) #96399

Merged

kibanamachine mentioned this pull request Apr 7, 2021

[7.x] [Search Sessions] Monitoring hardening part 1 (#96196) #96400

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search Sessions] Monitoring hardening part 1 #96196

[Search Sessions] Monitoring hardening part 1 #96196

lizozom commented Apr 4, 2021

elasticmachine commented Apr 4, 2021

lukasolson left a comment

lukasolson Apr 5, 2021

lizozom Apr 6, 2021

Dosant Apr 6, 2021

lizozom commented Apr 6, 2021

Dosant Apr 6, 2021 •

edited

Loading

gmmorris Apr 7, 2021

Dosant Apr 7, 2021 •

edited

Loading

lizozom Apr 7, 2021

gmmorris Apr 7, 2021

Dosant left a comment

Dosant commented Apr 6, 2021

Dosant left a comment

gmmorris left a comment

kibanamachine commented Apr 7, 2021

kibanamachine commented Apr 7, 2021

[Search Sessions] Monitoring hardening part 1 #96196

[Search Sessions] Monitoring hardening part 1 #96196

Conversation

lizozom commented Apr 4, 2021

Summary

Checklist

For maintainers

elasticmachine commented Apr 4, 2021

lukasolson left a comment

Choose a reason for hiding this comment

lukasolson Apr 5, 2021

Choose a reason for hiding this comment

lizozom Apr 6, 2021

Choose a reason for hiding this comment

Dosant Apr 6, 2021

Choose a reason for hiding this comment

lizozom commented Apr 6, 2021

Dosant Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

gmmorris Apr 7, 2021

Choose a reason for hiding this comment

Dosant Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

lizozom Apr 7, 2021

Choose a reason for hiding this comment

gmmorris Apr 7, 2021

Choose a reason for hiding this comment

Dosant left a comment

Choose a reason for hiding this comment

Dosant commented Apr 6, 2021

Dosant left a comment

Choose a reason for hiding this comment

gmmorris left a comment

Choose a reason for hiding this comment

kibanamachine commented Apr 7, 2021

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented Apr 7, 2021

💚 Backport successful

Dosant Apr 6, 2021 •

edited

Loading

Dosant Apr 7, 2021 •

edited

Loading