[Rollups] Server NP migration by alisonelizabeth · Pull Request #55606 · elastic/kibana

alisonelizabeth · 2020-01-22T20:09:26Z

This PR starts the NP migration of the rollup jobs server code. It also converts the server code to TS.

Things to note:

In legacy world, there’s not a real initializer context, so I'm currently creating a NP plugin that calls this and then exposes the results to the legacy plugin
The index_patterns route was using server.inject(). I removed this and pulled in IndexPatternsFetcher directly from the index_patterns plugin.

What to test

Test all API endpoints for the crud UI (fetch, create, start, stop, delete rollup jobs)
Test integration with index patterns
Test integration with TSVB
Test usage collector is working properly

What's left in order to migrate out of legacy

Dependent on the Index Management plugin, which has not been migrated yet (planned for 7.7)
Adopt NP license checker

elasticmachine · 2020-01-22T20:09:28Z

Pinging @elastic/es-ui (Team:Elasticsearch UI)

flash1293

TSVB and Visualize integrations still working fine 👍

flash1293 · 2020-02-05T09:18:18Z

x-pack/legacy/plugins/rollup/index.ts

+        usageCollection,
+        metrics,
+        __LEGACY: {
+          route: server.route.bind(server),


Is this used anywhere? couldn't find a reference.

Good catch. You're right, it's not needed anymore.

flash1293 · 2020-02-05T09:20:28Z

x-pack/legacy/plugins/rollup/server/routes/api/search.ts

+      const requests = request.body.map(({ index, query }: { index: string; query: any }) =>
+        callWithRequest('rollup.search', {
+          index,
+          rest_total_hits_as_int: true,


Just as a side note - AFAIK rest_total_hits_as_int will get deprecated with 8.0, so this has to be handled differently in future.

Thanks for the heads up! Do you know if there is an issue open somewhere to track this?

This seems to be the main one: #26356

alisonelizabeth · 2020-02-06T01:49:33Z

@elasticmachine merge upstream

sebelga · 2020-02-06T02:41:05Z

x-pack/legacy/plugins/rollup/server/lib/call_with_request_factory/call_with_request_factory.ts

+  return elasticsearchService.createClient('rollup', config);
+});
+
+export const callWithRequestFactory = (


Do we need this factory? In my PR, I've used it directly from the router context

https://github.com/elastic/kibana/pull/56829/files#diff-178c0726cd4b1e7299f257e5069b4a42R28

Thanks for sharing! I'm going to leave as is for now, but will consider changing this in my next PR.

sebelga · 2020-02-06T02:45:55Z

.../legacy/plugins/rollup/server/lib/license_pre_routing_factory/license_pre_routing_factory.ts

+import { LICENSE_STATUS_VALID } from '../../../../../common/constants/license_status';
+import { ServerShim } from '../../types';
+
+export const licensePreRoutingFactory = (


I wonder if we should first agree on the solution for this or if we should merge this PR and change it later.
I would think it would be good to first agree on a common pattern (probably a class exposed) to avoid copy/pasting this code + the tests in all our apps.

WDYT @alisonelizabeth @jloleysens @cjcenizal ?

I think that is a fair point. The logic for isEsError could probably be moved to a shared space too.

I don't think we need to block this PR though. This work will need to be updated again when the licensing plugin is used and then it can just use the new pattern we have adopted.

I agree with @jloleysens. I'd prefer not to block this PR and address when I finish the migration for rollups and adopt the NP licensing plugin.

jloleysens

Code

Left non-blocker comments.

UX/UI

I tested CRUD of rollups and that seemed to work. Not sure how to test integration with Index Patterns?

Overall everything worked as expected!

jloleysens · 2020-02-06T11:30:02Z

.../legacy/plugins/rollup/server/lib/license_pre_routing_factory/license_pre_routing_factory.ts

+import { LICENSE_STATUS_VALID } from '../../../../../common/constants/license_status';
+import { ServerShim } from '../../types';
+
+export const licensePreRoutingFactory = (


I think that is a fair point. The logic for isEsError could probably be moved to a shared space too.

I don't think we need to block this PR though. This work will need to be updated again when the licensing plugin is used and then it can just use the new pattern we have adopted.

jloleysens · 2020-02-06T11:36:39Z

x-pack/legacy/plugins/rollup/server/lib/search_strategies/rollup_search_strategy.ts

    name = 'rollup';

-    constructor(server) {
+    constructor(server: ElasticsearchServiceSetup) {


Nit, we should consider renaming server to something else. Perhaps elasticsearchService, just to clean up what looks like a legacy reference.

jloleysens · 2020-02-06T11:48:13Z

x-pack/legacy/plugins/rollup/server/routes/api/index_patterns.ts

+  deps.router.get(
+    {
+      path: '/api/index_patterns/rollup/_fields_for_wildcard',
+      validate: {


Nit; If you would like to re-use the type information of the query validations you can move this object to outside of the register function:

import { TypeOf } from '@kbn/config-schema'; const queryValidation = schema.object({...}); type RouteQuery = TypeOf<typeof queryValidation>; ... const handler: RequestHandler<unknown, RouteQuery> = ... ... validate: { query: queryValidation }, ...

jloleysens · 2020-02-06T11:49:15Z

x-pack/legacy/plugins/rollup/server/routes/api/index_patterns.ts

+  try {
+    parsedFields = parseMetaFields(metaFields);
+  } catch (error) {
+    return response.badRequest();


Does the above error have useful information we can send back in the response body?

jloleysens · 2020-02-06T11:51:11Z

x-pack/legacy/plugins/rollup/server/plugin.ts

+    this.log = initializerContext.logger.get();
+  }
+
+  async setup(


@rudolf informed me that async setup is going to be deprecated, so everything in setup phase for plugins should be synchronous. Not sure it is worth making the changes here though, just wanted to flag this.

Leaving as is for now.

alisonelizabeth · 2020-02-06T14:22:30Z

@elasticmachine merge upstream

alisonelizabeth · 2020-02-07T13:25:26Z

@elasticsearchmachine merge upstream

elasticmachine · 2020-02-07T14:57:21Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 941ccd7

History

💚 Build #24914 succeeded 792946f
💚 Build #24816 succeeded 95a5562
💛 Build #24095 was flaky 9dfe043
💔 Build #24078 failed 6d1a0db
💔 Build #24037 failed d0ba3bf

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

…t-state * upstream/master: (96 commits) top nav ts arg support (elastic#56984) [SIEM][detection engine] Limit network rules to filebeat source semantics (elastic#57130) Add docs for alerting and action settings (elastic#57035) Add Test to Verify Endpoint App Landing Page (elastic#57129) Update `markdown-to-jsx` (`6.9.3` → `6.11.0`) and `url-parse` (`1.4.4` → `1.4.7`) dependencies. (elastic#57126) chore(NA): removes use of parallel option in the terser minimizer (elastic#57077) [ML] New Platform server shim: update file data visualizer routes to use new platform router (elastic#56972) Specifying valid licenses for the Graph feature (elastic#55911) [APM][docs] Add troubleshooting for non-indexed fields (elastic#54948) [ML] DF Analytics creation: update schema definition for create route (elastic#56979) Remove Kibana a11y guide in favor of EUI (elastic#57021) [Logs UI] Set streamLive false in URL state when arriving from link-to (elastic#56329) [docs] Fix spaces api example json (elastic#50411) Add new config for filebeat index name (elastic#56920) [Metrics-UI] Fix toolbar popover for metrics table row (elastic#56796) Saved Objects testing (elastic#56965) Disabled categorization stats validation (elastic#57087) [Rollups] Server NP migration (elastic#55606) [Metrics UI] Limit group by selector to only 2 fields (elastic#56800) fix auto closing new vis modal when navigating to lens or when navigating away with browser history (elastic#56998) ...

* master: (96 commits) top nav ts arg support (elastic#56984) [SIEM][detection engine] Limit network rules to filebeat source semantics (elastic#57130) Add docs for alerting and action settings (elastic#57035) Add Test to Verify Endpoint App Landing Page (elastic#57129) Update `markdown-to-jsx` (`6.9.3` → `6.11.0`) and `url-parse` (`1.4.4` → `1.4.7`) dependencies. (elastic#57126) chore(NA): removes use of parallel option in the terser minimizer (elastic#57077) [ML] New Platform server shim: update file data visualizer routes to use new platform router (elastic#56972) Specifying valid licenses for the Graph feature (elastic#55911) [APM][docs] Add troubleshooting for non-indexed fields (elastic#54948) [ML] DF Analytics creation: update schema definition for create route (elastic#56979) Remove Kibana a11y guide in favor of EUI (elastic#57021) [Logs UI] Set streamLive false in URL state when arriving from link-to (elastic#56329) [docs] Fix spaces api example json (elastic#50411) Add new config for filebeat index name (elastic#56920) [Metrics-UI] Fix toolbar popover for metrics table row (elastic#56796) Saved Objects testing (elastic#56965) Disabled categorization stats validation (elastic#57087) [Rollups] Server NP migration (elastic#55606) [Metrics UI] Limit group by selector to only 2 fields (elastic#56800) fix auto closing new vis modal when navigating to lens or when navigating away with browser history (elastic#56998) ...

alisonelizabeth added v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// release_note:skip Skip the PR/issue when compiling release notes Feature:Rollups v7.7.0 labels Jan 22, 2020

alisonelizabeth added 3 commits January 24, 2020 11:12

init commit: convert server code to TS + create shim

7be7447

added back support for rollup usage collector

8524ce5

added back support for index_patterns route

ed8e4e9

alisonelizabeth force-pushed the np/rollups branch from aa7356c to ed8e4e9 Compare January 24, 2020 17:03

alisonelizabeth added 3 commits January 29, 2020 15:02

add back support for index management data enricher

323927a

rework integration with index_patterns

186c901

Merge branch 'master' into np/rollups

41e3fdb

alisonelizabeth added the Feature:NP Migration label Jan 31, 2020

alisonelizabeth added 9 commits January 31, 2020 15:15

add back support for search_strategy

57a3a8f

fix TS

b2432e1

Merge branch 'master' of github.com:elastic/kibana into np/rollups

760d75c

update tests + index pattern route validation

7cb95c3

fix i18n

d0ba3bf

Merge branch 'master' of github.com:elastic/kibana into np/rollups

906ab1d

fix logger

2c99beb

Merge branch 'master' of github.com:elastic/kibana into np/rollups

6d1a0db

fix license_pre_routing_factory tests

9dfe043

alisonelizabeth marked this pull request as ready for review February 3, 2020 17:52

alisonelizabeth requested review from flash1293, jloleysens and joshdover February 3, 2020 17:53

alisonelizabeth changed the title ~~[Rollups] [WIP] Server NP migration~~ [Rollups] Server NP migration Feb 3, 2020

flash1293 approved these changes Feb 5, 2020

View reviewed changes

Merge branch 'master' into np/rollups

95a5562

sebelga reviewed Feb 6, 2020

View reviewed changes

jloleysens approved these changes Feb 6, 2020

View reviewed changes

address review feedback

b90d0a9

elasticmachine and others added 2 commits February 6, 2020 09:22

Merge branch 'master' into np/rollups

792946f

address feedback

941ccd7

elastic deleted a comment from kibanamachine Feb 7, 2020

alisonelizabeth merged commit 86e186c into elastic:master Feb 7, 2020

alisonelizabeth deleted the np/rollups branch February 7, 2020 15:01

alisonelizabeth mentioned this pull request Feb 7, 2020

[7.x] [Rollups] Server NP migration (#55606) #57116

Merged

alisonelizabeth added a commit that referenced this pull request Feb 7, 2020

[Rollups] Server NP migration (#55606) (#57116)

84bc980

lukeelmers mentioned this pull request Mar 26, 2020

Move src/legacy/server/index_patterns to data plugin (server) #44190

Closed

5 tasks

Comments

Conversation

alisonelizabeth commented Jan 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What to test

What's left in order to migrate out of legacy

Uh oh!

elasticmachine commented Jan 22, 2020

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alisonelizabeth commented Feb 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jloleysens left a comment

Choose a reason for hiding this comment

Code

UX/UI

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alisonelizabeth commented Feb 6, 2020

Uh oh!

alisonelizabeth commented Feb 7, 2020

Uh oh!

elasticmachine commented Feb 7, 2020

💚 Build Succeeded

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alisonelizabeth commented Jan 22, 2020 •

edited

Loading