[ML] Adds cache for data recognizer module configs to reduce number of privilege checks by jgowdyelastic · Pull Request #126338 · elastic/kibana

jgowdyelastic · 2022-02-24T12:27:31Z

Adding a cache for modules loaded from disk and from the saved objects index.
The modules on disk and in the saved objects will not change in the time frame that each DataRecognizer instance exist.

By retrieving the modules from a cache for subsequent calls to _loadConfigs, the total number privileges checks caused by the saved object loading is drastically reduced.

vs

Fixes #125913

Unit or functional tests were updated or added to match the most common scenarios
This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2022-02-24T12:27:34Z

Pinging @elastic/ml-ui (:ml)

peteharverson

LGTM

darnautov · 2022-03-03T15:33:55Z

x-pack/plugins/ml/server/models/data_recognizer/data_recognizer.ts

  private _resultsService: ReturnType<typeof resultsServiceProvider>;
  private _calculateModelMemoryLimit: ReturnType<typeof calculateModelMemoryLimitProvider>;

+  private _configCache: Config[] | null = null;


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

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.

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.

Added in 561d3ad

jgowdyelastic · 2022-03-07T12:00:17Z

@elasticmachine merge upstream

walterra

Code LGTM, did a local test and verified the improvement in APM compared to main . Just added a comment about potentially adding some comments to the code.

…dyelastic/kibana into adding-data-recognizer-config-cache

kibana-ci · 2022-03-07T16:48:15Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #27966 succeeded 0d87ac1
💚 Build #26165 succeeded 8789b5f

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

cc @jgowdyelastic

…ed-unexpectedly-error * 'main' of github.com:elastic/kibana: (46 commits) Fix copy and pasted renderer user_name test (elastic#126663) [Gauge] Vis editors gauge legacy percent mode. (elastic#126318) Remove all cases related code from timelines (elastic#127003) Hide Enterprise search panel when no nodes are present (elastic#127100) [Lens] Fixed flakiness on runtime fields' appearance on the list (elastic#126945) [Security Solution][Lists] - Add missing privileges callout to exception lists page (elastic#126874) [Security Solution][Lists] - Updates exception flyout edit error messages (elastic#126875) [Security Solution][Rules] - Remove rule selection for read only users (elastic#126827) Fix session cleanup test (elastic#126966) [ftr] implement support for accessing ES through CCS (elastic#126547) [type-summarizer] always use normalized paths, fix windows compat (elastic#127055) Revert "[ci] Configure hourly pipeline for a small spot instance trial (elastic#126824)" Revert "[CI] Expand spot instance trial a bit (elastic#126928)" [Alerting] Adding functional tests for alerting and actions telemetry (elastic#126528) [Telemetry] Check permissions when requesting telemetry (elastic#126238) Don't submit empty seed_urls or sitemap_urls when making a partial crawl request (elastic#126972) Remove License Requirement for Enterprise Search App Search Meta Engines (elastic#127046) [ML] Adding data recognizer module config cache (elastic#126338) skip flaky suite (elastic#126027) [Reporting] Improve error logging for rescheduled jobs (elastic#126737) ... # Conflicts: # x-pack/plugins/reporting/server/core.ts # x-pack/plugins/reporting/server/lib/tasks/execute_report.ts

…re-browser-errors * 'main' of github.com:elastic/kibana: (46 commits) Fix copy and pasted renderer user_name test (elastic#126663) [Gauge] Vis editors gauge legacy percent mode. (elastic#126318) Remove all cases related code from timelines (elastic#127003) Hide Enterprise search panel when no nodes are present (elastic#127100) [Lens] Fixed flakiness on runtime fields' appearance on the list (elastic#126945) [Security Solution][Lists] - Add missing privileges callout to exception lists page (elastic#126874) [Security Solution][Lists] - Updates exception flyout edit error messages (elastic#126875) [Security Solution][Rules] - Remove rule selection for read only users (elastic#126827) Fix session cleanup test (elastic#126966) [ftr] implement support for accessing ES through CCS (elastic#126547) [type-summarizer] always use normalized paths, fix windows compat (elastic#127055) Revert "[ci] Configure hourly pipeline for a small spot instance trial (elastic#126824)" Revert "[CI] Expand spot instance trial a bit (elastic#126928)" [Alerting] Adding functional tests for alerting and actions telemetry (elastic#126528) [Telemetry] Check permissions when requesting telemetry (elastic#126238) Don't submit empty seed_urls or sitemap_urls when making a partial crawl request (elastic#126972) Remove License Requirement for Enterprise Search App Search Meta Engines (elastic#127046) [ML] Adding data recognizer module config cache (elastic#126338) skip flaky suite (elastic#126027) [Reporting] Improve error logging for rescheduled jobs (elastic#126737) ... # Conflicts: # x-pack/plugins/reporting/server/lib/tasks/execute_report.ts

kibanamachine · 2022-03-09T18:36:18Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 126338 or prevent reminders by adding the backport:skip label.

[ML] Adding data recognizer module config cache

8789b5f

jgowdyelastic added bug Fixes for quality problems that affect the customer experience release_note:fix :ml Feature:Anomaly Detection ML anomaly detection v8.2.0 labels Feb 24, 2022

jgowdyelastic requested review from darnautov and peteharverson February 24, 2022 12:27

jgowdyelastic self-assigned this Feb 24, 2022

jgowdyelastic requested a review from a team as a code owner February 24, 2022 12:27

peteharverson requested a review from walterra March 2, 2022 15:11

peteharverson approved these changes Mar 2, 2022

View reviewed changes

darnautov reviewed Mar 3, 2022

View reviewed changes

darnautov approved these changes Mar 7, 2022

View reviewed changes

Merge branch 'main' into adding-data-recognizer-config-cache

0d87ac1

walterra approved these changes Mar 7, 2022

View reviewed changes

jgowdyelastic added 2 commits March 7, 2022 15:17

adding comment about cache

561d3ad

Merge branch 'adding-data-recognizer-config-cache' of github.com:jgow…

3d21669

…dyelastic/kibana into adding-data-recognizer-config-cache

jgowdyelastic merged commit 0640782 into elastic:main Mar 7, 2022

jgowdyelastic deleted the adding-data-recognizer-config-cache branch March 7, 2022 18:33

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 9, 2022

jgowdyelastic added backport:skip This PR does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. labels Mar 10, 2022

lcawl changed the title ~~[ML] Adding data recognizer module config cache~~ [ML] Adds cache for data recognizer module configs to reduce number of privilege checks Apr 19, 2022

lcawl added release_note:enhancement and removed release_note:fix labels Apr 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

[ML] Adds cache for data recognizer module configs to reduce number of privilege checks#126338
jgowdyelastic merged 4 commits intoelastic:mainfrom
jgowdyelastic:adding-data-recognizer-config-cache

jgowdyelastic commented Feb 24, 2022 •

edited

Loading

Uh oh!

elasticmachine commented Feb 24, 2022

Uh oh!

peteharverson left a comment

Uh oh!

darnautov Mar 3, 2022

Uh oh!

jgowdyelastic Mar 7, 2022 •

edited

Loading

Uh oh!

walterra Mar 7, 2022

Uh oh!

jgowdyelastic Mar 7, 2022

Uh oh!

jgowdyelastic commented Mar 7, 2022

Uh oh!

walterra left a comment

Uh oh!

kibana-ci commented Mar 7, 2022

Uh oh!

kibanamachine commented Mar 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

jgowdyelastic commented Feb 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Feb 24, 2022

Uh oh!

peteharverson left a comment

Choose a reason for hiding this comment

Uh oh!

darnautov Mar 3, 2022

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic Mar 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

walterra Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic Mar 7, 2022

Choose a reason for hiding this comment

Uh oh!

jgowdyelastic commented Mar 7, 2022

Uh oh!

walterra left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Mar 7, 2022

💚 Build Succeeded

Metrics [docs]

History

Uh oh!

kibanamachine commented Mar 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

jgowdyelastic commented Feb 24, 2022 •

edited

Loading

jgowdyelastic Mar 7, 2022 •

edited

Loading