Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Search Sessions] Monitoring hardening part 1 #96196

Merged
merged 11 commits into from
Apr 7, 2021
Merged

[Search Sessions] Monitoring hardening part 1 #96196

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
c51d5fd
Decrease default pageSize to 100
Apr 4, 2021
0cb55f6
ts
Apr 4, 2021
af9f767
ts
Apr 5, 2021
b5e74dd
Merge branch 'master' of github.com:elastic/kibana into sessions/moni…
Apr 5, 2021
5e488cc
ts
Apr 5, 2021
fd68d01
Merge branch 'master' into sessions/monitoring-hardening
lizozom Apr 5, 2021
fb72103
Update x-pack/plugins/data_enhanced/server/search/session/session_ser…
lizozom Apr 6, 2021
a3ac094
Search sessions are disabled
lizozom Apr 6, 2021
4ad1c4a
Clear task on server start
Apr 7, 2021
6922cee
Merge branch 'sessions/monitoring-hardening' of github.com:lizozom/ki…
Apr 7, 2021
511ada5
Merge branch 'master' of github.com:elastic/kibana into sessions/moni…
Apr 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion x-pack/plugins/data_enhanced/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export const configSchema = schema.object({
* pageSize controls how many search session objects we load at once while monitoring
* session completion
*/
pageSize: schema.number({ defaultValue: 10000 }),
pageSize: schema.number({ defaultValue: 100 }),
/**
* trackingInterval controls how often we track search session objects progress
*/
Expand Down
29 changes: 7 additions & 22 deletions x-pack/plugins/data_enhanced/server/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,7 @@
*/

import { CoreSetup, CoreStart, Logger, Plugin, PluginInitializerContext } from 'kibana/server';
import { TaskManagerSetupContract, TaskManagerStartContract } from '../../task_manager/server';
import {
PluginSetup as DataPluginSetup,
PluginStart as DataPluginStart,
usageProvider,
} from '../../../../src/plugins/data/server';
import { UsageCollectionSetup } from '../../../../src/plugins/usage_collection/server';
import { usageProvider } from '../../../../src/plugins/data/server';
import { ENHANCED_ES_SEARCH_STRATEGY, EQL_SEARCH_STRATEGY } from '../common';
import { registerSessionRoutes } from './routes';
import { searchSessionSavedObjectType } from './saved_objects';
Expand All @@ -22,22 +16,13 @@ import {
eqlSearchStrategyProvider,
} from './search';
import { getUiSettings } from './ui_settings';
import type { DataEnhancedRequestHandlerContext } from './type';
import type {
DataEnhancedRequestHandlerContext,
DataEnhancedSetupDependencies as SetupDependencies,
DataEnhancedStartDependencies as StartDependencies,
} from './type';
import { ConfigSchema } from '../config';
import { registerUsageCollector } from './collectors';
import { SecurityPluginSetup } from '../../security/server';

interface SetupDependencies {
data: DataPluginSetup;
usageCollection?: UsageCollectionSetup;
taskManager: TaskManagerSetupContract;
security?: SecurityPluginSetup;
}

export interface StartDependencies {
data: DataPluginStart;
taskManager: TaskManagerStartContract;
}

export class EnhancedDataServerPlugin
implements Plugin<void, void, SetupDependencies, StartDependencies> {
Expand All @@ -50,7 +35,7 @@ export class EnhancedDataServerPlugin
this.config = this.initializerContext.config.get<ConfigSchema>();
}

public setup(core: CoreSetup<DataPluginStart>, deps: SetupDependencies) {
public setup(core: CoreSetup<StartDependencies>, deps: SetupDependencies) {
const usage = deps.usageCollection ? usageProvider(core) : undefined;

core.uiSettings.register(getUiSettings());
Expand Down
4 changes: 2 additions & 2 deletions x-pack/plugins/data_enhanced/server/search/session/check_running_sessions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {
} from 'kibana/server';
import moment from 'moment';
import { EMPTY, from } from 'rxjs';
import { expand, mergeMap } from 'rxjs/operators';
import { expand, concatMap } from 'rxjs/operators';
import { nodeBuilder } from '../../../../../../src/plugins/data/common';
import {
ENHANCED_ES_SEARCH_STRATEGY,
Expand Down Expand Up @@ -154,7 +154,7 @@ export async function checkRunningSessions(
try {
await getAllSavedSearchSessions$(deps, config)
.pipe(
mergeMap(async (runningSearchSessionsResponse) => {
concatMap(async (runningSearchSessionsResponse) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Dosant marked this conversation as resolved.
Show resolved Hide resolved
if (!runningSearchSessionsResponse.total) return;

logger.debug(`Found ${runningSearchSessionsResponse.total} running sessions`);
Expand Down
18 changes: 15 additions & 3 deletions x-pack/plugins/data_enhanced/server/search/session/monitoring_task.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { checkRunningSessions } from './check_running_sessions';
import { CoreSetup, SavedObjectsClient, Logger } from '../../../../../../src/core/server';
import { ConfigSchema } from '../../../config';
import { SEARCH_SESSION_TYPE } from '../../../common';
import { DataEnhancedStartDependencies } from '../../type';

export const SEARCH_SESSIONS_TASK_TYPE = 'search_sessions_monitor';
export const SEARCH_SESSIONS_TASK_ID = `data_enhanced_${SEARCH_SESSIONS_TASK_TYPE}`;
Expand All @@ -25,12 +26,20 @@ interface SearchSessionTaskDeps {
config: ConfigSchema;
}

function searchSessionRunner(core: CoreSetup, { logger, config }: SearchSessionTaskDeps) {
function searchSessionRunner(
core: CoreSetup<DataEnhancedStartDependencies>,
{ logger, config }: SearchSessionTaskDeps
) {
return ({ taskInstance }: RunContext) => {
return {
async run() {
const sessionConfig = config.search.sessions;
const [coreStart] = await core.getStartServices();
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);
lizozom marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

@Dosant Dosant Apr 6, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@Dosant Dosant Apr 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what we try to achieve:

  1. Assume Kibana is running with this feature ON
  2. Restart Kibana with the feature OFF
  3. We want to be sure that we don't run the task when the feature is OFF

Currently, this is not happening in master.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

return;
}
const internalRepo = coreStart.savedObjects.createInternalRepository([SEARCH_SESSION_TYPE]);
const internalSavedObjectsClient = new SavedObjectsClient(internalRepo);
await checkRunningSessions(
Expand All @@ -50,7 +59,10 @@ function searchSessionRunner(core: CoreSetup, { logger, config }: SearchSessionT
};
}

export function registerSearchSessionsTask(core: CoreSetup, deps: SearchSessionTaskDeps) {
export function registerSearchSessionsTask(
core: CoreSetup<DataEnhancedStartDependencies>,
deps: SearchSessionTaskDeps
) {
deps.taskManager.registerTaskDefinitions({
[SEARCH_SESSIONS_TASK_TYPE]: {
title: 'Search Sessions Monitor',
Expand Down
Loading