[Search] Session SO polling #84225

lizozom · 2020-11-24T14:59:17Z

Summary

~~Blocked by #84837~~

This PR introduces session polling: each Kibana server tracks search IDS in memory and checks whether a matching Background Session was created for each . If it was, search IDs are synced into the SO and cleared from memory.

This PR handles search expiration, version conflicts and max retries.
This code is enabled only if the dev feature flag is on!!!

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)
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

#84837

This was checked for breaking API changes and was labeled appropriately

lizozom · 2020-11-30T12:18:15Z

@elasticmachine merge upstream

kibanamachine · 2020-11-30T12:18:17Z

merge conflict between base and head

…y-logic

jportner

Code review only: Spaces changes (unit test mocks) LGTM!

legrego

A couple of preliminary questions for you -- I haven't had a chance to pull this down to run it, so I'm sorry for the lack of concrete guidance right now. I'll update my comments with suggestions once I have a chance to run this locally

src/plugins/data/server/search/session/session_service.ts

…y-logic

legrego · 2020-12-01T20:15:19Z

I started leaving some comments, but I'm a little confused. We previously said (IIRC) in the RFC that these sessions would be space-aware, and the saved object that represents the session indicates this. However, the entire BackgroundSessionService is using these saved objects as if they are not space-aware. All sessions are being stored within the default space, even if they belong to a different space.

So, I guess I'm asking: should background sessions be space-aware, or space-agnostic? If Space-aware, then we need to re-work the BackgroundSessionService to actually be space-aware. If Space-agnostic, then the Saved Object definition for this type is incorrect.

…y-logic

legrego

I haven't tested yet, but this looks much better! Thanks for taking the time to make this space-aware

legrego · 2020-12-07T15:54:37Z

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

+ const res = await this.internalSavedObjectsClient.find<BackgroundSessionSavedObjectAttributes>({
+ type: BACKGROUND_SESSION_TYPE,
+ search: activeMappingIds,
+ searchFields: ['sessionId'],


Using searchFields to search for exact matches has proved problematic for other teams (Alerting and ML come to mind).

Have you explored using a KQL filter for this? The alerting team builds theirs programmatically to retrieve documents matching specific criteria:
https://github.com/elastic/kibana/blob/223a18766ad4bc079d461ef145e9b2e3e9fa2469/x-pack%2Fplugins%2Falerts%2Fserver%2Fauthorization%2Falerts_authorization_kuery.ts

Created an issue for this #85252

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

legrego · 2020-12-07T16:14:23Z

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

+ };
+
+ public asScopedProvider = ({ savedObjects }: CoreStart) => {
+ return (request: KibanaRequest) => {


👍 Thanks for introducing the scoped provider.

Since we haven't granted any privileges for this new saved object type, these requests will all fail our authorization checks unless the user is a superuser.

I know we're discussing background session security outside of this PR, but I wanted to raise it anyway in case this wasn't your expectation.

So how should this be correctly addressed?

We'll either take the sub-feature privilege approach, or the top-level feature approach that we've been discussing in the doc that Brandon put together. Once we reach consensus I'll help you with the changes that we'll need to make

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

legrego · 2020-12-07T16:21:12Z

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

+ );
+ }
+
+ // TODO: Generate the `userId` from the realm type/realm name/username


Check with us before doing so. We are working with the ES security team to come up with a unique user identifier that doesn't rely on the realm name.

legrego · 2020-12-07T16:21:55Z

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

+ return session;
+ };
+
+ // TODO: Throw an error if this session doesn't belong to this user


Not sure what your timeline looks like, but we are starting work on OLS Phase 1, which might prove useful here.

@legrego We have a PR up already, would you mind taking a look? #84975

lukasolson

Overall, this LGTM, just a few minor things you can take or leave below. Tested and things seem to be working. I created a terms agg other bucket with a delay of 30s (30s for the initial request and 30s for the follow-up request), then sent to background before the first request completed. I stayed on the page for the second request to be initiated, and behind the scenes the SO was updated properly, and restoring it worked.

lukasolson · 2020-12-07T21:10:57Z

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

 // Get the mapping of request hash/search ID for this session
- const searchMap = this.sessionSearchMap.get(sessionId) ?? new Map<string, string>();
- const idMapping = Object.fromEntries(searchMap.entries());


Is your thinking just that we might as well only do the updating in the monitoring task rather than here as well?

Yes!I think it's best if all updates are done from one place (we also need to be extending expiration in the monitoring task)

If my understanding correct, then after saving a background search worst case all relevant searches will be pushed to this object in 10s?

Wouldn't it lead to weird race condition where user navigates to background session management and experiences incorrect state? Or tries to restores searches before monitoring loop pushed them to SO? This especially could cause flakiness in e2e tests where everything is fast and non deterministic.

lukasolson · 2020-12-07T21:15:05Z

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

 },
 });

 const router = core.http.createRouter();
 registerSessionRoutes(router);
 }

- public start(core: CoreStart) {}
+ public start(core: CoreStart) {
+ this.sessionService.start(core, this.initializerContext.config.create());


At some point, should we make SessionService and BackgroundSessionService actual services (that implement Plugin)? Doesn't need to be part of this PR, just my thoughts.

lukasolson · 2020-12-07T21:22:26Z

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

 import { createRequestHash } from './utils';
+import moment from 'moment';


Nit: I don't think we're doing anything all that complicated with moment in this file other than a couple of date comparisons. In general, I think we should avoid bringing in libraries for these sorts of trivial operations.

We already use moment in the data plugin, so I don't think it should matter. :)
And as the code uses them , I needed the types to match. No?

my 5 cents: I'd also not used moment for this.
Seems like nothing complicated that requires moment, but for me makes harder to reason about and read the code than if it was with regular Date.now.
For example:

curTime.diff(sessionInfo.insertTime)

I don't know if this returns curTime - insertTime or insertTime - curTime without looking into docs

I'd have to use this kind of syntax: new Date(new Date().getTime() - (2*60*1000)); rather than moment().subtract(2, 'm').
I'm really not a fan, but if you insist 🤷

nit a bit shorter: new Date(Date.now() - (2601000));

One q: are there timezones risks in play? Maybe movement handles something in addition related to tz?

…y-logic

… sessions/retry-logic

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

Dosant · 2020-12-08T12:05:23Z

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

 import { createRequestHash } from './utils';
+import moment from 'moment';


my 5 cents: I'd also not used moment for this.
Seems like nothing complicated that requires moment, but for me makes harder to reason about and read the code than if it was with regular Date.now.
For example:

curTime.diff(sessionInfo.insertTime)

I don't know if this returns curTime - insertTime or insertTime - curTime without looking into docs

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

Dosant · 2020-12-08T13:33:04Z

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

 // Get the mapping of request hash/search ID for this session
- const searchMap = this.sessionSearchMap.get(sessionId) ?? new Map<string, string>();
- const idMapping = Object.fromEntries(searchMap.entries());


If my understanding correct, then after saving a background search worst case all relevant searches will be pushed to this object in 10s?

Wouldn't it lead to weird race condition where user navigates to background session management and experiences incorrect state? Or tries to restores searches before monitoring loop pushed them to SO? This especially could cause flakiness in e2e tests where everything is fast and non deterministic.

Dosant

Should we add retry mechanism for this snippet in case we retrieved a session but there is yet no searches with that hash?

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

Lines 336 to 340 in e85da48

 const session = await this.get(sessionId, deps); 

 const requestHash = createRequestHash(searchRequest.params); 

 if (!session.attributes.idMapping.hasOwnProperty(requestHash)) { 

 throw new Error('No search ID in this session matching the given search request'); 

 }

…vice.ts Co-authored-by: Anton Dosov <dosantappdev@gmail.com>

Dosant · 2020-12-08T14:34:12Z

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

+ return sessionSavedObject;
+ });
+
+ const updateResults = await this.internalSavedObjectsClient.bulkUpdate<BackgroundSessionSavedObjectAttributes>(


I wonder if this could throw a conflict error and if we should immediately retry if it did. (I am not sure when SO api throws conflict error, just've seen 409 error before)

As far as I understand, bulkUpdate returns an error per object. You can see those processed in monitorMappedIds, when checking each response item for errors.

…vice.ts Co-authored-by: Anton Dosov <dosantappdev@gmail.com>

…y-logic

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

Dosant

lgtm, didn't test.
re-reviewed interval -> timeout change

kibanamachine · 2020-12-09T11:59:07Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 07fe4bf

Metrics [docs]

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/development-plugin-saved-objects.html#_mappings

id	before	after	diff
`background-session`	7	8	+1

History

💔 Build #92836 failed 51143af
💚 Build #92800 succeeded ffabaa9
💚 Build #92736 succeeded 4876f3d
💚 Build #92651 succeeded e85da48
💚 Build #92470 succeeded 0858b98

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

* Monitor ids * import fix * solve circular dep * eslint * mock circular dep * max retries test * mock circular dep * test * jest <(-:C * jestttttt * [data.search] Move search method inside session service and add tests * merge * Move background session service to data_enhanced plugin * Better logs Save IDs only in monitoring loop * Fix types * Space aware session service * ts * Fix session service saving * merge fix * stable stringify * INMEM_MAX_SESSIONS * INMEM_MAX_SESSIONS * Update x-pack/plugins/data_enhanced/server/search/session/session_service.ts Co-authored-by: Anton Dosov <dosantappdev@gmail.com> * Update x-pack/plugins/data_enhanced/server/search/session/session_service.ts Co-authored-by: Anton Dosov <dosantappdev@gmail.com> * Use setTimeout to schedule monitoring steps * settimeout Co-authored-by: Lukas Olson <olson.lukas@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Anton Dosov <dosantappdev@gmail.com>

…k-field-to-hot-phase * 'master' of github.com:elastic/kibana: (429 commits) simplify popover open state logic (elastic#85379) [Logs UI][Metrics UI] Move actions to the kibana header (elastic#84648) [Search Source] Do not pick scripted fields if * provided (elastic#85133) [Search] Session SO polling (elastic#84225) [Transform] Replace legacy elasticsearch client (elastic#84932) [Uptime]Refactor header and action menu (elastic#83779) Fix agg select external link (elastic#85380) [ILM] Show forcemerge in hot when rollover is searchable snapshot is enabled (elastic#85292) clear using keyboard (elastic#85042) [GS] add tag and dashboard suggestion results (elastic#85144) [ML] API integration tests - skip GetAnomaliesTableData Add ECS field for event.code. (elastic#85109) [Functional][TSVB] Wait for markdown textarea to be cleaned (elastic#85128) skip flaky suite (elastic#62060) skip flaky suite (elastic#85098) Bump highlight.js to v9.18.5 (elastic#84296) Add `server.publicBaseUrl` config (elastic#85075) [Alerting & Actions ] More debug logging (elastic#85149) [Security Solution][Case] Manual attach alert to a case (elastic#82996) Loosen UUID regex to accept uuidv1 or uuidv4 (elastic#85338) ... # Conflicts: # x-pack/plugins/index_lifecycle_management/__jest__/client_integration/edit_policy/edit_policy.helpers.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/hot_phase/hot_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared_fields/index.ts # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/warm_phase/warm_phase.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/edit_policy.tsx # x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/i18n_texts.ts # x-pack/plugins/index_lifecycle_management/server/routes/api/policies/register_create_route.ts

* Monitor ids * import fix * solve circular dep * eslint * mock circular dep * max retries test * mock circular dep * test * jest <(-:C * jestttttt * [data.search] Move search method inside session service and add tests * merge * Move background session service to data_enhanced plugin * Better logs Save IDs only in monitoring loop * Fix types * Space aware session service * ts * Fix session service saving * merge fix * stable stringify * INMEM_MAX_SESSIONS * INMEM_MAX_SESSIONS * Update x-pack/plugins/data_enhanced/server/search/session/session_service.ts Co-authored-by: Anton Dosov <dosantappdev@gmail.com> * Update x-pack/plugins/data_enhanced/server/search/session/session_service.ts Co-authored-by: Anton Dosov <dosantappdev@gmail.com> * Use setTimeout to schedule monitoring steps * settimeout Co-authored-by: Lukas Olson <olson.lukas@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Anton Dosov <dosantappdev@gmail.com> Co-authored-by: Lukas Olson <olson.lukas@gmail.com> Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Anton Dosov <dosantappdev@gmail.com>

Liza K added 4 commits November 24, 2020 16:51

Monitor ids

1c82050

import fix

52032fc

Merge remote-tracking branch 'upstream/master' into sessions/retry-logic

60abe6c

solve circular dep

8a12991

Liza K added 4 commits November 30, 2020 19:49

eslint

d692c1e

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

9a02d9c

…y-logic

mock circular dep

1092c7a

max retries test

500f15d

lizozom marked this pull request as ready for review November 30, 2020 18:00

lizozom requested review from a team as code owners November 30, 2020 18:00

lizozom self-assigned this Nov 30, 2020

lizozom requested a review from a team as a code owner November 30, 2020 18:00

lizozom added v7.11.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:AppArch labels Nov 30, 2020

Liza K added 2 commits November 30, 2020 20:40

mock circular dep

c6fea68

test

e5325e9

lukasolson self-requested a review November 30, 2020 18:50

jportner approved these changes Nov 30, 2020

View reviewed changes

pgayvallet approved these changes Nov 30, 2020

View reviewed changes

legrego reviewed Nov 30, 2020

View reviewed changes

src/plugins/data/server/search/session/session_service.ts Outdated Show resolved Hide resolved

src/plugins/data/server/search/session/session_service.ts Outdated Show resolved Hide resolved

Liza K added 3 commits December 1, 2020 10:41

jest <(-:C

c540fde

jestttttt

5f7db2d

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

80fdf82

…y-logic

legrego self-requested a review December 1, 2020 19:42

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

f40fcca

…y-logic

lizozom changed the title ~~[Search] Session monitoring~~ [Search] Session SO polling Dec 7, 2020

legrego reviewed Dec 7, 2020

View reviewed changes

Liza K added 5 commits December 7, 2020 20:03

Merge remote-tracking branch 'upstream/master' into sessions/retry-logic

c1ba50c

merge fix

b5b0895

stable stringify

f14f042

INMEM_MAX_SESSIONS

5b9782d

INMEM_MAX_SESSIONS

0858b98

lukasolson approved these changes Dec 7, 2020

View reviewed changes

Liza K added 2 commits December 8, 2020 12:00

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

6c9e345

…y-logic

Merge branch 'sessions/retry-logic' of github.com:lizozom/kibana into…

e85da48

… sessions/retry-logic

lizozom mentioned this pull request Dec 8, 2020

[Search] Use filters in SavedObjects find #85252

Closed

Dosant reviewed Dec 8, 2020

View reviewed changes

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

4876f3d

…vice.ts Co-authored-by: Anton Dosov <dosantappdev@gmail.com>

Dosant reviewed Dec 8, 2020

View reviewed changes

lizozom and others added 5 commits December 8, 2020 19:02

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

ffabaa9

…vice.ts Co-authored-by: Anton Dosov <dosantappdev@gmail.com>

Use setTimeout to schedule monitoring steps

63c7d6a

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

51143af

…y-logic

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

bf54579

…y-logic

settimeout

07fe4bf

Dosant reviewed Dec 9, 2020

View reviewed changes

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

Dosant approved these changes Dec 9, 2020

View reviewed changes

lizozom merged commit 0a04835 into elastic:master Dec 9, 2020

lizozom mentioned this pull request Dec 9, 2020

[7.x] [Search] Session SO polling (#84225) #85400

Merged

lizozom mentioned this pull request Dec 9, 2020

[Search] Implement a background search service #61743

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search] Session SO polling #84225

[Search] Session SO polling #84225

lizozom commented Nov 24, 2020 •

edited

Loading

lizozom commented Nov 30, 2020

kibanamachine commented Nov 30, 2020

jportner left a comment

legrego left a comment

legrego commented Dec 1, 2020

legrego left a comment

legrego Dec 7, 2020

lizozom Dec 8, 2020

legrego Dec 7, 2020

lizozom Dec 7, 2020

legrego Dec 7, 2020

legrego Dec 7, 2020

legrego Dec 7, 2020

lukasolson Dec 7, 2020

lukasolson left a comment •

edited

Loading

lukasolson Dec 7, 2020

lizozom Dec 8, 2020

Dosant Dec 8, 2020

lukasolson Dec 7, 2020

lukasolson Dec 7, 2020

lizozom Dec 8, 2020 •

edited

Loading

Dosant Dec 8, 2020 •

edited

Loading

lizozom Dec 8, 2020

Dosant Dec 9, 2020

Dosant Dec 8, 2020 •

edited

Loading

Dosant Dec 8, 2020

Dosant left a comment •

edited

Loading

Dosant Dec 8, 2020

lizozom Dec 8, 2020

Dosant left a comment

kibanamachine commented Dec 9, 2020

		import { createRequestHash } from './utils';
		import moment from 'moment';

	const session = await this.get(sessionId, deps);
	const requestHash = createRequestHash(searchRequest.params);
	if (!session.attributes.idMapping.hasOwnProperty(requestHash)) {
	throw new Error('No search ID in this session matching the given search request');
	}

[Search] Session SO polling #84225

[Search] Session SO polling #84225

Conversation

lizozom commented Nov 24, 2020 • edited Loading

Summary

Checklist

For maintainers

lizozom commented Nov 30, 2020

kibanamachine commented Nov 30, 2020

jportner left a comment

Choose a reason for hiding this comment

legrego left a comment

Choose a reason for hiding this comment

legrego commented Dec 1, 2020

legrego 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasolson left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizozom Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Dosant Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dosant Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dosant left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dosant left a comment

Choose a reason for hiding this comment

kibanamachine commented Dec 9, 2020

💚 Build Succeeded

Metrics [docs]

Saved Objects .kibana field count

History

lizozom commented Nov 24, 2020 •

edited

Loading

lukasolson left a comment •

edited

Loading

lizozom Dec 8, 2020 •

edited

Loading

Dosant Dec 8, 2020 •

edited

Loading

Dosant Dec 8, 2020 •

edited

Loading

Dosant left a comment •

edited

Loading