Skip to content

[ML] Adds cache for data recognizer module configs to reduce number of privilege checks #126338

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

Merged
jgowdyelastic merged 4 commits into from
Mar 7, 2022
Merged

[ML] Adds cache for data recognizer module configs to reduce number of privilege checks #126338

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8789b5f
[ML] Adding data recognizer module config cache
jgowdyelastic Feb 24, 2022
0d87ac1
Merge branch 'main' into adding-data-recognizer-config-cache
kibanamachine Mar 7, 2022
561d3ad
adding comment about cache
jgowdyelastic Mar 7, 2022
3d21669
Merge branch 'adding-data-recognizer-config-cache' of github.com:jgow…
jgowdyelastic Mar 7, 2022
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
19 changes: 18 additions & 1 deletion x-pack/plugins/ml/server/models/data_recognizer/data_recognizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,17 @@ export class DataRecognizer {
private _resultsService: ReturnType<typeof resultsServiceProvider>;
private _calculateModelMemoryLimit: ReturnType<typeof calculateModelMemoryLimitProvider>;

/**
* A temporary cache of configs loaded from disk and from save object service.
* The configs from disk will not change while kibana is running.
* The configs from saved objects could potentially change while an instance of
* DataRecognizer exists, if a fleet package containing modules is installed.
* However the chance of this happening is very low and so the benefit of using
* this cache outweighs the risk of the cache being out of date during the short
* existence of a DataRecognizer instance.
*/
private _configCache: Config[] | null = null;
Copy link
Copy Markdown
Contributor

@darnautov darnautov Mar 3, 2022

Choose a reason for hiding this comment

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

Should we invalidate this cache if the saved object configs have been changed?

Copy link
Copy Markdown
Member Author

@jgowdyelastic jgowdyelastic Mar 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

As I mentioned in the description

The modules on disk and in the saved objects will not change in the time frame that each DataRecognizer instance exist.

Each instance of the DataRecognizer exists for one endpoint call. The modules on disk will not change in that time frame. The modules in the saved object could theoretically change if someone installs a fleet module at the exact same time, but IMO it is very unlikely.

The _has_privileges calls were all happening from inside the saved object client and so I imagine adding a check to see if the saved objects have changed each loop would reintroduce them.

Copy link
Copy Markdown
Contributor

@walterra walterra Mar 7, 2022

Choose a reason for hiding this comment

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

Suggestion: It would be great if we could add some of the PR description and this explanation as comments to the code so the reasoning is also documented there.

Copy link
Copy Markdown
Member Author

@jgowdyelastic jgowdyelastic Mar 7, 2022

Choose a reason for hiding this comment

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

Added in 561d3ad


/**
* List of the module jobs that require model memory estimation
*/
Expand Down Expand Up @@ -181,6 +192,10 @@ export class DataRecognizer {
}

private async _loadConfigs(): Promise<Config[]> {
if (this._configCache !== null) {
return this._configCache;
}

const configs: Config[] = [];
const dirs = await this._listDirs(this._modulesDir);
await Promise.all(
Expand Down Expand Up @@ -211,7 +226,9 @@ export class DataRecognizer {
isSavedObject: true,
}));

return [...configs, ...savedObjectConfigs];
this._configCache = [...configs, ...savedObjectConfigs];

return this._configCache;
}

private async _loadSavedObjectModules() {
Expand Down