[ML] New Platform server shim: update recognize modules routes to use new platform router #57206

darnautov · 2020-02-10T15:23:39Z

Summary

Part of #49743. Updates recognize module routes to use the new platform router.

Checklist

Documentation was added for features that require explanation or tutorials

elasticmachine · 2020-02-10T15:23:42Z

Pinging @elastic/ml-ui (:ml)

alvarezmelissa87 · 2020-02-10T20:09:18Z

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

+  ) => Promise<any>;
+
+  constructor(context: RequestHandlerContext) {
+    this.callWithRequest = context.ml!.mlClient.callAsCurrentUser;


I think it would be a good idea to name this this.callAsCurrentUser to be consistent with the other updated route models

alvarezmelissa87 · 2020-02-10T20:16:44Z

x-pack/legacy/plugins/ml/server/routes/modules.ts

+        params: schema.object({
+          moduleId: schema.string(),
+        }),
+        body: schema.object({


What do you think about moving this to a schema file to be consistent with the other converted routes?
There are also enough schema files in the new_platform directory that I think it warrants creating a schemas dir in there. I'm happy to do that part but would be good to get these body schemas in a separate file.

Moved to a dedicated file in 63ea75b

alvarezmelissa87 · 2020-02-10T20:33:21Z

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

+  id: any;
+  title: any;
+  query: any;
+  description: any;


Wouldn't some of these types be string?

Added types in 63ea75b

alvarezmelissa87 · 2020-02-10T20:47:10Z

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

@@ -445,11 +536,11 @@ export class DataRecognizer {
  }

  async loadIndexPatterns() {
-    return await this.savedObjectsClient.find({ type: 'index-pattern', perPage: 1000 });
+    return await this.savedObjectsClient!.find({ type: 'index-pattern', perPage: 1000 });


It actually might be worth checking that this.savedObjectsClient exists here instead of using non-null assertion here. Even though right now loadIndexPatterns is only used after savedObjectsClient has been set this check would guard against any future updates/additions to this class that might try to use this method differently.

I believe it should be passed to the constructor, not the method. In that case, no check is required. I just wanted to avoid any code changes in this PR, but probably it worth a small refactoring.

Refactored in 63ea75b

alvarezmelissa87 · 2020-02-10T20:53:26Z

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

-  loadExistingSavedObjects(type) {
-    return this.savedObjectsClient.find({ type, perPage: 1000 });
+  loadExistingSavedObjects(type: string) {
+    return this.savedObjectsClient!.find({ type, perPage: 1000 });


As mentioned above, might be worth checking that savedObjectsClient has been set.

alvarezmelissa87 · 2020-02-10T20:58:01Z

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

    const reducedConfig = {
      jobs: moduleConfig.jobs,
      datafeeds: moduleConfig.datafeeds,
      kibana: moduleConfig.kibana,
-    };
+    } as const;


I'm not familiar with this - why is it needed?

https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-4.html#const-assertions
But I added const assertion here by mistake, will remove

alvarezmelissa87 · 2020-02-10T21:01:26Z

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

    const { limits } = await this.callWithRequest('ml.info');
    const maxMml = limits.max_model_memory_limit;
    if (maxMml !== undefined) {
-      const maxBytes = numeral(maxMml.toUpperCase()).value();
+      // @ts-ignore
+      const maxBytes: number = numeral(maxMml.toUpperCase()).value();


Would be good to know what is being ignored here - to be consistent

For some reason, the issue is numeral from @elastic/numeral. It complains that value prop does not exist.

alvarezmelissa87 · 2020-02-10T21:01:48Z

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

          if (mml !== undefined) {
-            const mmlBytes = numeral(mml.toUpperCase()).value();
+            // @ts-ignore
+            const mmlBytes: number = numeral(mml.toUpperCase()).value();


jgowdyelastic · 2020-02-10T21:02:42Z

x-pack/legacy/plugins/ml/server/routes/modules.ts

+          end,
+          jobOverrides,
+          datafeedOverrides,
+          getSavedObjectsClient(elasticsearchPlugin, savedObjects!)


i'm unsure about using getSavedObjectsClient here as it uses the elevated internal user to create the saved object repository which could be a security risk.
The original version used the getSavedObjectsClient from the request which I imagine had the current user's level of privileges.
I'm unsure about this and I think is worth investigating.
Also, I didn't realise that the savedObjects being passed to the router are of the type SavedObjectsLegacyService.
I wonder if it's possible to use the non-legacy version of the saved objects client in our plugin right now, before we move it to the new platform proper.

Agreed 🤔 I think we should be to use NP's context.core.savedObjects.client which is meant to replace request.getSavedObjectsClient (https://github.com/elastic/kibana/blob/master/src/core/MIGRATION.md#server-side)

Since context is scoped like the previously used legacy request this should give us the same behavior, I'd think.

Fixed in 63ea75b

jgowdyelastic · 2020-02-11T09:14:25Z

x-pack/legacy/plugins/ml/server/routes/modules.ts

+          moduleId: schema.string(),
+        }),
+        body: schema.object({
+          prefix: schema.string(),


prefix should be optional.
See documentation here:
https://github.com/elastic/ml-team/blob/master/ui/rest_api/modules/modules_api_setup.md

jgowdyelastic

One small change needed, but otherwise LGTM

jgowdyelastic · 2020-02-11T13:19:46Z

x-pack/legacy/plugins/ml/server/new_platform/modules.ts

+export const setupModuleBodySchema = schema.object({
+  prefix: schema.maybe(schema.string()),
+  groups: schema.maybe(schema.arrayOf(schema.string())),
+  indexPatternName: schema.string(),


indexPatternName needs to be optional

alvarezmelissa87

LGTM ⚡️

kibanamachine · 2020-02-11T16:05:17Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 6d23884

History

💚 Build #25833 succeeded 9edbdec
💔 Build #25814 failed 63ea75b
💔 Build #25646 failed 1f975d4355655e07727daf75aab2e7abffbf0d59
💔 Build #25596 failed 3f6488a75c97e5d77a021843d2175bdc4d4d55d9

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

… new platform router (elastic#57206) * [ML] modules with NP router * [ML] use extendedRouteInitializationDeps * [ML] add apidoc annotations * [ML] address PR comments * [ML] convert data_recognizer test to jest * [ML] optional indexPatternName

… new platform router (#57206) (#57335) * [ML] modules with NP router * [ML] use extendedRouteInitializationDeps * [ML] add apidoc annotations * [ML] address PR comments * [ML] convert data_recognizer test to jest * [ML] optional indexPatternName

* master: (27 commits) Include actions new platform plugin for codeowners (elastic#57252) [APM][docs] 7.6 documentation updates (elastic#57124) Expressions refactor (elastic#54342) [ML] New Platform server shim: update annotation routes to use new platform router (elastic#57067) Remove injected ui app vars from Canvas (elastic#56190) update max_anomaly_score route schema to handle possible undefined values (elastic#57339) [Add panel flyout] Moving create new to the top of SavedObjectFinder (elastic#56428) Add mock of a legacy ui api to re-enable Canvas storybook (elastic#56673) [monitoring] Removes noisy event received log (elastic#57275) Remove use of copied MANAGEMENT_BREADCRUMBS and use `setBreadcrumbs` from management section's mount (elastic#57324) Advanced Settings management app to kibana platform plugin (elastic#56931) [ML] New Platform server shim: update recognize modules routes to use new platform router (elastic#57206) [ML] Fix overall stats for saved search on the Data Visualizer page (elastic#57312) [ML] [NP] Removing ui imports (elastic#56358) [SIEM] Fixes failing Cypress tests (elastic#57202) Create observability CODEOWNERS reference (elastic#57109) fix results service schema (elastic#57217) don't register a wrapper if browser side function exists. (elastic#57196) Ui Actions explorer example (elastic#57006) Fix update alert API to still work when AAD is out of sync (elastic#57039) ...

darnautov added :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Feb 10, 2020

darnautov requested review from alvarezmelissa87 and peteharverson February 10, 2020 15:23

darnautov requested a review from a team as a code owner February 10, 2020 15:23

darnautov self-assigned this Feb 10, 2020

alvarezmelissa87 reviewed Feb 10, 2020

View reviewed changes

jgowdyelastic reviewed Feb 10, 2020

View reviewed changes

jgowdyelastic reviewed Feb 11, 2020

View reviewed changes

darnautov added 4 commits February 11, 2020 10:54

[ML] modules with NP router

c730ba8

[ML] use extendedRouteInitializationDeps

fb7b555

[ML] add apidoc annotations

9ca99ad

[ML] address PR comments

63ea75b

darnautov force-pushed the ML-49743-modules branch from 1f975d4 to 63ea75b Compare February 11, 2020 10:00

darnautov requested review from jgowdyelastic and alvarezmelissa87 February 11, 2020 10:05

[ML] convert data_recognizer test to jest

9edbdec

jgowdyelastic approved these changes Feb 11, 2020

View reviewed changes

peteharverson mentioned this pull request Feb 11, 2020

[ML] New platform migration #49743

Closed

78 tasks

alvarezmelissa87 approved these changes Feb 11, 2020

View reviewed changes

[ML] optional indexPatternName

6d23884

darnautov merged commit b133357 into elastic:master Feb 11, 2020

darnautov deleted the ML-49743-modules branch February 11, 2020 16:11

darnautov mentioned this pull request Feb 11, 2020

[7.x] [ML] New Platform server shim: update recognize modules routes to use new platform router (#57206) #57335

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] New Platform server shim: update recognize modules routes to use new platform router #57206

[ML] New Platform server shim: update recognize modules routes to use new platform router #57206

darnautov commented Feb 10, 2020 •

edited

Loading

elasticmachine commented Feb 10, 2020

alvarezmelissa87 Feb 10, 2020

alvarezmelissa87 Feb 10, 2020

darnautov Feb 11, 2020

alvarezmelissa87 Feb 10, 2020

darnautov Feb 11, 2020

alvarezmelissa87 Feb 10, 2020

darnautov Feb 11, 2020

darnautov Feb 11, 2020

alvarezmelissa87 Feb 10, 2020

alvarezmelissa87 Feb 10, 2020 •

edited

Loading

darnautov Feb 11, 2020

alvarezmelissa87 Feb 10, 2020

darnautov Feb 11, 2020

alvarezmelissa87 Feb 10, 2020

jgowdyelastic Feb 10, 2020

alvarezmelissa87 Feb 10, 2020 •

edited

Loading

darnautov Feb 11, 2020

jgowdyelastic Feb 11, 2020

darnautov Feb 11, 2020

jgowdyelastic left a comment

jgowdyelastic Feb 11, 2020

alvarezmelissa87 left a comment

kibanamachine commented Feb 11, 2020

[ML] New Platform server shim: update recognize modules routes to use new platform router #57206

[ML] New Platform server shim: update recognize modules routes to use new platform router #57206

Conversation

darnautov commented Feb 10, 2020 • edited Loading

Summary

Checklist

elasticmachine commented Feb 10, 2020

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

alvarezmelissa87 Feb 10, 2020 • 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

alvarezmelissa87 Feb 10, 2020 • 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

jgowdyelastic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 11, 2020

💚 Build Succeeded

History

darnautov commented Feb 10, 2020 •

edited

Loading

alvarezmelissa87 Feb 10, 2020 •

edited

Loading

alvarezmelissa87 Feb 10, 2020 •

edited

Loading