[ML] New Platform server shim: update annotation routes to use new platform router #57067

alvarezmelissa87 · 2020-02-07T01:14:07Z

Summary

Updates all annotation routes to use new platform router.

Checklist

Delete any items that are not applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11

elasticmachine · 2020-02-07T01:14:10Z

Pinging @elastic/ml-ui (:ml)

elasticmachine · 2020-02-07T02:34:02Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 5a4e95420ca23301a4d8f89a1b0fe6e34f346eaa

Test Failures

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/legacy/plugins/ml/server/models/annotation_service.annotation_service deleteAnnotation() should delete annotation

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

Stack Trace

TypeError: Cannot read property 'mlClient' of undefined
    at annotationProvider (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/ml/server/models/annotation_service/annotation.ts:72:41)
    at annotationServiceProvider (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/ml/server/models/annotation_service/index.ts:12:8)
    at Object.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/ml/server/models/annotation_service/annotation.test.ts:38:36)
    at resolve (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:43:12)
    at new Promise (<anonymous>)
    at mapper (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
    at promise.then (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:73:41)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/legacy/plugins/ml/server/models/annotation_service.annotation_service getAnnotation() should get annotations for specific job

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

Stack Trace

TypeError: Cannot read property 'mlClient' of undefined
    at annotationProvider (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/ml/server/models/annotation_service/annotation.ts:72:41)
    at annotationServiceProvider (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/ml/server/models/annotation_service/index.ts:12:8)
    at Object.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/ml/server/models/annotation_service/annotation.test.ts:58:34)
    at resolve (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:43:12)
    at new Promise (<anonymous>)
    at mapper (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
    at promise.then (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:73:41)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Kibana Pipeline / x-pack-intake-agent / X-Pack Jest Tests.x-pack/legacy/plugins/ml/server/models/annotation_service.annotation_service getAnnotation() should throw and catch an error

Link to Jenkins

Standard Out

Failed Tests Reporter:
  - Test has not failed recently on tracked branches

Stack Trace

TypeError: Cannot read property 'mlClient' of undefined
    at annotationProvider (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/ml/server/models/annotation_service/annotation.ts:72:41)
    at annotationServiceProvider (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/ml/server/models/annotation_service/index.ts:12:8)
    at Object.it (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/x-pack/legacy/plugins/ml/server/models/annotation_service/annotation.test.ts:88:34)
    at Object.asyncJestTest (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/jasmineAsyncInstall.js:102:37)
    at resolve (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:43:12)
    at new Promise (<anonymous>)
    at mapper (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:26:19)
    at promise.then (/var/lib/jenkins/workspace/elastic+kibana+pipeline-pull-request/kibana/node_modules/jest-jasmine2/build/queueRunner.js:73:41)
    at process._tickCallback (internal/process/next_tick.js:68:7)

and 3 more failures, only showing the first 3.

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

walterra

Looks good overall, just added some questions/suggestions.

walterra · 2020-02-07T10:21:26Z

x-pack/legacy/plugins/ml/server/models/annotation_service/annotation.ts

@@ -67,7 +68,8 @@ export type callWithRequestType = (
  params: annotationProviderParams
 ) => Promise<any>;

-export function annotationProvider(callWithRequest: callWithRequestType) {
+export function annotationProvider(context: RequestHandlerContext) {
+  const callAsCurrentUser = context.ml!.mlClient.callAsCurrentUser;


Can we work around this in another way? The styleguide says to avoid non-null assertions https://github.com/elastic/kibana/blob/master/STYLEGUIDE.md#avoid-non-null-assertions

I agree, this has been bothering me too in the recent NP work.
Does ml need to be optional in our RequestHandlerContext interface? especially as we're asserting that it isn't optional in every route.

Adding ml to the RequestHandlerContext interface was initially made optional to prevent issues with other plugins using RequestHandlerContext. This was the suggested definition from kibana-platform.
Had a chat with Josh about this and looks like they'll be improving this by adding a generic type arg to core.http.createRouter so you can specify your own type for RequestHandlerContext. The PR should be up soon. Then we'll be able to get rid of the non-null assertions. 🙌
So we're stuck with this for now but once that PR is in we can add the change in the next routes PR 👌

walterra · 2020-02-07T10:25:19Z

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

+    {
+      path: '/api/ml/annotations',
+      validate: {
+        body: schema.object({


What do you think of moving all schema definitions to new_platform/annotations_schema.ts for consistency even if they are not as complex as the indexAnnotationSchema one?

walterra · 2020-02-07T10:25:55Z

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

+    {
+      path: '/api/ml/annotations/delete/{annotationId}',
+      validate: {
+        params: schema.object({ annotationId: schema.string() }),


What do you think of moving all schema definitions to new_platform/annotations_schema.ts for consistency even if they are not as complex as the indexAnnotationSchema one?

peteharverson

Tested these changes, and all works fine, apart from the annotations tab in the Jobs list where I am seeing this error:

peteharverson · 2020-02-10T13:59:29Z

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

+        }
+
+        const { indexAnnotation } = annotationServiceProvider(context);
+        const username = _.get(request, 'auth.credentials.username', ANNOTATION_USER_UNKNOWN);


There doesn't seem to be an auth object as part of the request with this NP switch, so this is always getting set to 'unknown'. We must have to get the logged in username some other way.

Looks like this is no longer available in the NP request object. It has to be accessed via the NP security plugin api - added in a4ad5a5039fd1c809bbb121a9bc327a63ce8a53b

cc @peteharverson

peteharverson

Tested latest edits and LGTM

alvarezmelissa87 · 2020-02-11T15:40:09Z

Thanks, everyone, for taking a look 😄 This is ready for a final check when you get a chance! cc @jgowdyelastic, @walterra

jgowdyelastic

LGTM

kibanamachine · 2020-02-11T18:03:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 8a9892a

History

💚 Build #25693 succeeded a4ad5a5039fd1c809bbb121a9bc327a63ce8a53b
💚 Build #25420 succeeded a3b489de78dadca0370771c6e3048831caa4178f
💔 Build #25384 failed f059e07c31a6128b4363d8bb281edeb97612dad0
💔 Build #25138 failed 5a4e95420ca23301a4d8f89a1b0fe6e34f346eaa

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

…atform router (elastic#57067) * update annotation routes to use NP router * create ts file for feature check * update schema to allow null value for earliest/latestMs * update annotations model test * use NP security plugin to access user info

* 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) ...

…atform router (#57067) (#57361) * update annotation routes to use NP router * create ts file for feature check * update schema to allow null value for earliest/latestMs * update annotations model test * use NP security plugin to access user info

alvarezmelissa87 added :ml v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:NP Migration v7.7.0 labels Feb 7, 2020

alvarezmelissa87 requested review from walterra, peteharverson and jgowdyelastic February 7, 2020 01:14

alvarezmelissa87 requested a review from a team as a code owner February 7, 2020 01:14

alvarezmelissa87 self-assigned this Feb 7, 2020

peteharverson mentioned this pull request Feb 7, 2020

[ML] New platform migration #49743

Closed

78 tasks

walterra reviewed Feb 7, 2020

View reviewed changes

peteharverson reviewed Feb 7, 2020

View reviewed changes

alvarezmelissa87 force-pushed the ml-new-platform-annotation-routes branch from f059e07 to a3b489d Compare February 8, 2020 17:53

peteharverson reviewed Feb 10, 2020

View reviewed changes

peteharverson approved these changes Feb 11, 2020

View reviewed changes

jgowdyelastic approved these changes Feb 11, 2020

View reviewed changes

alvarezmelissa87 added 5 commits February 11, 2020 11:27

update annotation routes to use NP router

da77d5f

create ts file for feature check

b44ced4

update schema to allow null value for earliest/latestMs

f65d53f

update annotations model test

8ec7007

use NP security plugin to access user info

8a9892a

alvarezmelissa87 force-pushed the ml-new-platform-annotation-routes branch from a4ad5a5 to 8a9892a Compare February 11, 2020 16:36

alvarezmelissa87 merged commit 9fd75db into elastic:master Feb 11, 2020

alvarezmelissa87 mentioned this pull request Feb 11, 2020

[7.x] [ML] New Platform server shim: update annotation routes to use new platform router (#57067) #57361

Merged

alvarezmelissa87 deleted the ml-new-platform-annotation-routes branch February 11, 2020 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] New Platform server shim: update annotation routes to use new platform router #57067

[ML] New Platform server shim: update annotation routes to use new platform router #57067

alvarezmelissa87 commented Feb 7, 2020

elasticmachine commented Feb 7, 2020

elasticmachine commented Feb 7, 2020

Standard Out

Stack Trace

Standard Out

Stack Trace

Standard Out

Stack Trace

walterra left a comment

walterra Feb 7, 2020

jgowdyelastic Feb 7, 2020 •

edited

Loading

alvarezmelissa87 Feb 7, 2020

walterra Feb 7, 2020

walterra Feb 7, 2020

peteharverson left a comment

peteharverson Feb 10, 2020

alvarezmelissa87 Feb 10, 2020 •

edited

Loading

peteharverson left a comment

alvarezmelissa87 commented Feb 11, 2020

jgowdyelastic left a comment

kibanamachine commented Feb 11, 2020

[ML] New Platform server shim: update annotation routes to use new platform router #57067

[ML] New Platform server shim: update annotation routes to use new platform router #57067

Conversation

alvarezmelissa87 commented Feb 7, 2020

Summary

Checklist

elasticmachine commented Feb 7, 2020

elasticmachine commented Feb 7, 2020

💔 Build Failed

Test Failures

Standard Out

Stack Trace

Standard Out

Stack Trace

Standard Out

Stack Trace

walterra left a comment

Choose a reason for hiding this comment

walterra Feb 7, 2020

Choose a reason for hiding this comment

jgowdyelastic Feb 7, 2020 • edited Loading

Choose a reason for hiding this comment

alvarezmelissa87 Feb 7, 2020

Choose a reason for hiding this comment

walterra Feb 7, 2020

Choose a reason for hiding this comment

walterra Feb 7, 2020

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

peteharverson Feb 10, 2020

Choose a reason for hiding this comment

alvarezmelissa87 Feb 10, 2020 • edited Loading

Choose a reason for hiding this comment

peteharverson left a comment

Choose a reason for hiding this comment

alvarezmelissa87 commented Feb 11, 2020

jgowdyelastic left a comment

Choose a reason for hiding this comment

kibanamachine commented Feb 11, 2020

💚 Build Succeeded

History

jgowdyelastic Feb 7, 2020 •

edited

Loading

alvarezmelissa87 Feb 10, 2020 •

edited

Loading