Skip to content

Commit

Permalink
[Search] Search Sessions Monitoring Task (#85253)
Browse files Browse the repository at this point in the history
* 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

* initial

* initial

* Fix session service saving

* merge fix

* stable stringify

* INMEM_MAX_SESSIONS

* INMEM_MAX_SESSIONS

* use the status API

* Move task scheduling behind a feature flag

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

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

* Add unit tests

* 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

* Update request_utils.ts

* settimeout

* tiny cleanup

* Core review + use client.asyncSearch.status

* update ts

* fix unit test

* code review fixes

* Save individual search errors on SO

* Don't re-fetch completed or errored searches

* Rename Background Sessions to Search Sessions (with a send to background action)

* doc

* doc

* jest fun

* rename rfc

* translations

* merge fix

* merge fix

* code review

* update so name in features

* Move deleteTaskIfItExists to task manager

* task_manager to ts project

* Move deleteTaskIfItExists to public contract

* mock

* use task store

* ts

* code review

* code review + jest

* Alerting code review

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: restrry <restrry@gmail.com>
  • Loading branch information
5 people committed Jan 11, 2021
1 parent dd85399 commit 3eeec0f
Show file tree
Hide file tree
Showing 25 changed files with 741 additions and 60 deletions.
5 changes: 2 additions & 3 deletions x-pack/plugins/alerts/server/alerts_client/alerts_client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ import {
import { EncryptedSavedObjectsClient } from '../../../encrypted_saved_objects/server';
import { TaskManagerStartContract } from '../../../task_manager/server';
import { taskInstanceToAlertTaskInstance } from '../task_runner/alert_task_instance';
import { deleteTaskIfItExists } from '../lib/delete_task_if_it_exists';
import { RegistryAlertType, UntypedNormalizedAlertType } from '../alert_type_registry';
import { AlertsAuthorization, WriteOperations, ReadOperations } from '../authorization';
import { IEventLogClient } from '../../../../plugins/event_log/server';
Expand Down Expand Up @@ -602,7 +601,7 @@ export class AlertsClient {
const removeResult = await this.unsecuredSavedObjectsClient.delete('alert', id);

await Promise.all([
taskIdToRemove ? deleteTaskIfItExists(this.taskManager, taskIdToRemove) : null,
taskIdToRemove ? this.taskManager.removeIfExists(taskIdToRemove) : null,
apiKeyToInvalidate
? markApiKeyForInvalidation(
{ apiKey: apiKeyToInvalidate },
Expand Down Expand Up @@ -1060,7 +1059,7 @@ export class AlertsClient {

await Promise.all([
attributes.scheduledTaskId
? deleteTaskIfItExists(this.taskManager, attributes.scheduledTaskId)
? this.taskManager.removeIfExists(attributes.scheduledTaskId)
: null,
apiKeyToInvalidate
? await markApiKeyForInvalidation(
Expand Down
10 changes: 5 additions & 5 deletions x-pack/plugins/alerts/server/alerts_client/tests/delete.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ describe('delete()', () => {
const result = await alertsClient.delete({ id: '1' });
expect(result).toEqual({ success: true });
expect(unsecuredSavedObjectsClient.delete).toHaveBeenCalledWith('alert', '1');
expect(taskManager.remove).toHaveBeenCalledWith('task-123');
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(unsecuredSavedObjectsClient.create.mock.calls[0][0]).toBe(
'api_key_pending_invalidation'
);
Expand All @@ -135,7 +135,7 @@ describe('delete()', () => {
const result = await alertsClient.delete({ id: '1' });
expect(result).toEqual({ success: true });
expect(unsecuredSavedObjectsClient.delete).toHaveBeenCalledWith('alert', '1');
expect(taskManager.remove).toHaveBeenCalledWith('task-123');
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
expect(unsecuredSavedObjectsClient.get).toHaveBeenCalledWith('alert', '1');
expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
Expand All @@ -153,7 +153,7 @@ describe('delete()', () => {
});

await alertsClient.delete({ id: '1' });
expect(taskManager.remove).not.toHaveBeenCalled();
expect(taskManager.removeIfExists).not.toHaveBeenCalled();
});

test(`doesn't invalidate API key when apiKey is null`, async () => {
Expand Down Expand Up @@ -217,8 +217,8 @@ describe('delete()', () => {
);
});

test('throws error when taskManager.remove throws an error', async () => {
taskManager.remove.mockRejectedValue(new Error('TM Fail'));
test('throws error when taskManager.removeIfExists throws an error', async () => {
taskManager.removeIfExists.mockRejectedValue(new Error('TM Fail'));

await expect(alertsClient.delete({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
`"TM Fail"`
Expand Down
10 changes: 5 additions & 5 deletions x-pack/plugins/alerts/server/alerts_client/tests/disable.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ describe('disable()', () => {
version: '123',
}
);
expect(taskManager.remove).toHaveBeenCalledWith('task-123');
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(
(unsecuredSavedObjectsClient.create.mock.calls[0][1] as InvalidatePendingApiKey).apiKeyId
).toBe('123');
Expand Down Expand Up @@ -254,7 +254,7 @@ describe('disable()', () => {
version: '123',
}
);
expect(taskManager.remove).toHaveBeenCalledWith('task-123');
expect(taskManager.removeIfExists).toHaveBeenCalledWith('task-123');
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
});

Expand All @@ -280,7 +280,7 @@ describe('disable()', () => {

await alertsClient.disable({ id: '1' });
expect(unsecuredSavedObjectsClient.update).not.toHaveBeenCalled();
expect(taskManager.remove).not.toHaveBeenCalled();
expect(taskManager.removeIfExists).not.toHaveBeenCalled();
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
});

Expand Down Expand Up @@ -314,7 +314,7 @@ describe('disable()', () => {

await alertsClient.disable({ id: '1' });
expect(unsecuredSavedObjectsClient.update).toHaveBeenCalled();
expect(taskManager.remove).toHaveBeenCalled();
expect(taskManager.removeIfExists).toHaveBeenCalled();
expect(unsecuredSavedObjectsClient.create).not.toHaveBeenCalled();
expect(alertsClientParams.logger.error).toHaveBeenCalledWith(
'disable(): Failed to load API key to invalidate on alert 1: Fail'
Expand All @@ -338,7 +338,7 @@ describe('disable()', () => {
});

test('throws when failing to remove task from task manager', async () => {
taskManager.remove.mockRejectedValueOnce(new Error('Failed to remove task'));
taskManager.removeIfExists.mockRejectedValueOnce(new Error('Failed to remove task'));

await expect(alertsClient.disable({ id: '1' })).rejects.toThrowErrorMatchingInlineSnapshot(
`"Failed to remove task"`
Expand Down
1 change: 0 additions & 1 deletion x-pack/plugins/data_enhanced/common/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ export {
IAsyncSearchOptions,
pollSearch,
SearchSessionSavedObjectAttributes,
SearchSessionFindOptions,
SearchSessionStatus,
SearchSessionRequestInfo,
} from './search';
41 changes: 39 additions & 2 deletions x-pack/plugins/data_enhanced/common/search/session/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,55 @@ export interface SearchSessionSavedObjectAttributes {
* App that created the session. e.g 'discover'
*/
appId: string;
/**
* Creation time of the session
*/
created: string;
/**
* Expiration time of the session. Expiration itself is managed by Elasticsearch.
*/
expires: string;
/**
* status
*/
status: string;
/**
* urlGeneratorId
*/
urlGeneratorId: string;
/**
* The application state that was used to create the session.
* Should be used, for example, to re-load an expired search session.
*/
initialState: Record<string, unknown>;
/**
* Application state that should be used to restore the session.
* For example, relative dates are conveted to absolute ones.
*/
restoreState: Record<string, unknown>;
/**
* Mapping of search request hashes to their corresponsing info (async search id, etc.)
*/
idMapping: Record<string, SearchSessionRequestInfo>;
}

export interface SearchSessionRequestInfo {
id: string; // ID of the async search request
strategy: string; // Search strategy used to submit the search request
/**
* ID of the async search request
*/
id: string;
/**
* Search strategy used to submit the search request
*/
strategy: string;
/**
* status
*/
status: string;
/**
* An optional error. Set if status is set to error.
*/
error?: string;
}

export interface SearchSessionFindOptions {
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/data_enhanced/kibana.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
"requiredPlugins": [
"bfetch",
"data",
"features"
"features",
"taskManager"
],
"optionalPlugins": ["kibanaUtils", "usageCollection"],
"server": true,
Expand Down
20 changes: 17 additions & 3 deletions x-pack/plugins/data_enhanced/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/

import { CoreSetup, CoreStart, Logger, Plugin, PluginInitializerContext } from 'kibana/server';
import { TaskManagerSetupContract, TaskManagerStartContract } from '../../task_manager/server';
import {
PluginSetup as DataPluginSetup,
PluginStart as DataPluginStart,
Expand All @@ -24,9 +25,15 @@ import { getUiSettings } from './ui_settings';
interface SetupDependencies {
data: DataPluginSetup;
usageCollection?: UsageCollectionSetup;
taskManager: TaskManagerSetupContract;
}
export interface StartDependencies {
data: DataPluginStart;
taskManager: TaskManagerStartContract;
}

export class EnhancedDataServerPlugin implements Plugin<void, void, SetupDependencies> {
export class EnhancedDataServerPlugin
implements Plugin<void, void, SetupDependencies, StartDependencies> {
private readonly logger: Logger;
private sessionService!: SearchSessionService;

Expand Down Expand Up @@ -65,10 +72,17 @@ export class EnhancedDataServerPlugin implements Plugin<void, void, SetupDepende

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

this.sessionService.setup(core, {
taskManager: deps.taskManager,
});
}

public start(core: CoreStart) {
this.sessionService.start(core, this.initializerContext.config.create());
public start(core: CoreStart, { taskManager }: StartDependencies) {
this.sessionService.start(core, {
taskManager,
config$: this.initializerContext.config.create(),
});
}

public stop() {
Expand Down
Loading

0 comments on commit 3eeec0f

Please sign in to comment.