Skip to content
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.

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

[Watcher] New Platform (NP) Migration #50908

Merged
merged 38 commits into from
Dec 11, 2019

Conversation

jloleysens
Copy link
Contributor

@jloleysens jloleysens commented Nov 18, 2019

Summary

NP migration for the watcher application.

Public

Public is a large number of dependencies and should be properly regression tested (TODO). Most tests expect React ^16.9.

Server

Not many changes here at the moment, mostly consumes server.route and server.plugins.elasticsearch from legacy.

How to Reiview

Public

  • All plugin dependencies or state derived from plugin dependencies have been moved into a new app wide context that was created.
  • Cleaned up use of React hooks to be in line with Eslint rules. In two points it was explicitly opted out of due to wrong behaviour:
    • x-pack/legacy/plugins/watcher/public/np_ready/application/sections/watch_edit/components/threshold_watch_edit/watch_visualization.tsx
    • x-pack/legacy/plugins/watcher/public/np_ready/application/sections/watch_edit/components/threshold_watch_edit/threshold_watch_edit.tsx
  • All watcher application files now live in x-pack/legacy/plugins/watcher/public/np_ready/application (so lots of path changes)
  • Everything inside models directory has been left as is (still JS)
  • Client tests are still disabled (they have been for some time, this just makes refactors like this more risky!). See __jest__ folder.
  • Minor changes to a shared dependency src/plugins/es_ui_shared/public/request/np_ready_request.ts to also support query in network requests.

Server

  • Migrated to NP router and from JS -> TS. This introduced a large number of changes inside of x-pack/legacy/plugins/watcher/server/np_ready/routes/api. Only manual testing performed via the UI, no pre-existing tests! There are some functional tests that help here, but still needed to do a large amount of manual testing
  • models has been left untouched
  • Migrated to new featureCatalogue APIs
  • Migrated all of lib to TS.
  • Removed wrap_es_errors from lib - we are just using new kibana response directly.

Deferred

  • looks like there is an issue where when a watch fires and you ack it, then switch to history and back to status, the stale unacked version shows up. Will create an issue after this PR is merged.

Still need to switch to np ready version of use_request
…her [skip ci]

* upstream/master: (54 commits)
  allows plugins to define validation schema for "enabled" flag (elastic#50286)
  Add retry to find.existsByDisplayedByCssSelector (elastic#48734)
  [i18n] integrate latest translations (elastic#50864)
  ui/resize_checker 👉 src/plugins/kibana_utils (elastic#44750)
  Fix @reach/router types (elastic#50863)
  [ML] Adding ML node warning to overview and analytics pages (elastic#50766)
  Bump storybook dependencies (elastic#50752)
  [APM Replace usage of idx with optional chaining (elastic#50849)
  [SIEM] Fix eslint errors (elastic#49713)
  Improve "Browser client is out of date" error message (elastic#50296)
  [SIEM][Detection Engine] REST API improvements and changes from UI/UX feedback (elastic#50797)
  Move @kbn/es-query into data plugin - es-query folder (elastic#50182)
  Index Management new platform migration (elastic#49359)
  Increase retry for cloud snapshot to finish (elastic#50781)
  Removing EuiCode from inside EuiPanel (elastic#50683)
  [SIEM] Tests for search_after and bulk index (elastic#50129)
  Make babel understand TypeScript 3.7 syntax (elastic#50772)
  Fixing mocha tests and broken password change status codes (elastic#50704)
  [Canvas] Use compressed forms in sidebar (elastic#49419)
  Add labels to shell scripts in Jenkins (elastic#49657)
  ...
- Some updates after API changes
@jloleysens jloleysens added release_note:roadmap Feature:Watcher v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.6.0 labels Nov 18, 2019
@jloleysens jloleysens requested a review from rudolf November 18, 2019 14:31
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💔 Build Failed

@jloleysens
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

@jloleysens nice job with this!

I had a couple comments/questions:

  • I think it would be great to remove the skip on the component integration tests (now that react has been upgraded) and confirm everything is passing. Not sure how much effort this would be.
  • What benefit do we get from creating the np_ready directory? I only ask because it looks like almost all of the code lives under it. It seems like it would require more overhead on the final migration, as I assume we would remove it and then have to update file paths everywhere. I know we're doing this elsewhere, so there might be something I'm missing.
  • (nit) I noticed there's a lot of @ts-ignore scattered throughout the code. I'm not sure how much effort it would be to create xyz.d.ts files, but it seems like that would be easier to maintain in the long term.

Overall, testing looks good! I found a couple bugs. I did not test all of the action types.

  • I think something is wrong with the Indices to query input on the threshold watch. When I type, I would expect it to filter the results (e.g., "kibana_sample_"), but it does not.

Screen Shot 2019-12-09 at 8 47 06 PM

  • I selected an index (kibana_sample_data_logs) on the threshold alert, but when I make changes to the condition and select a field, I see fields from other indices (looks like kibana_sample_data_flights), which is not expected.

Screen Shot 2019-12-09 at 8 47 51 PM

Uncaught (in promise) TypeError: Cannot read property 'watchHistoryItems' of null
    at deserializer (api.ts:91)
    at _callee2$ (np_ready_request.ts:150)
    at tryCatch (runtime.js:45)
    at Generator.invoke [as _invoke] (runtime.js:271)
    at Generator.prototype.<computed> [as next] (runtime.js:97)
    at asyncGeneratorStep (np_ready_request.ts:18)
    at _next (np_ready_request.ts:20)

@@ -209,13 +209,6 @@ module.exports = {
'react-hooks/rules-of-hooks': 'off',
},
},
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you close #49566 once this PR is merged?

import routes from 'ui/routes';
import { management, MANAGEMENT_BREADCRUMB } from 'ui/management';
// @ts-ignore
import { TimeBuckets } from 'ui/time_buckets';
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed since you created src/legacy/ui/public/time_buckets/index.d.ts?

@jloleysens
Copy link
Contributor Author

jloleysens commented Dec 10, 2019

@cjcenizal Thanks for the review! :D

@alisonelizabeth Thanks for the review too! :D

In response to your comments/questions:

I think it would be great to remove the skip on the component integration tests...

I think this can be considered a separate piece of work as it's more related to the recent react 16.12 upgrade. I created this issue #52620 for tracking this work - I am happy to tackle this straight after :). I get that getting test coverage after a refactor like this is really valuable! Fwiw, after unskipping thewatch_create_json.test.tsx I was seeing the following kind of errors:

    Expected: "{\"executeDetails\":{\"triggerData\":{\"triggeredTime\":\"now+5s\",\"scheduledTime\":\"now+5s\"},\"ignoreCondition\":true,\"actionModes\":{\"my-logging-action\":\"force_execute\"}},\"watch\":{\"id\":\"my-test-watch\",\"type\":\"json\",\"isNew\":true,\"actions\":[],\"watch\":{\"trigger\":{\"schedule\":{\"interval\":\"30m\"}},\"input\":{\"search\":{\"request\":{\"body\":{\"size\":0,\"query\":{\"match_all\":{}}},\"indices\":[\"*\"]}}},\"condition\":{\"compare\":{\"ctx.payload.hits.total\":{\"gte\":10}}},\"actions\":{\"my-logging-action\":{\"logging\":{\"text\":\"There are {{ctx.payload.hits.total}} documents in your index. Threshold is 10.\"}}}}}}"
    Received: "{\"body\":\"{\\\"executeDetails\\\":{\\\"triggerData\\\":{\\\"triggeredTime\\\":\\\"now+5s\\\",\\\"scheduledTime\\\":\\\"now+5s\\\"},\\\"ignoreCondition\\\":true,\\\"actionModes\\\":{\\\"my-logging-action\\\":\\\"force_execute\\\"}},\\\"watch\\\":{\\\"id\\\":\\\"my-test-watch\\\",\\\"type\\\":\\\"json\\\",\\\"isNew\\\":true,\\\"actions\\\":[],\\\"watch\\\":{\\\"trigger\\\":{\\\"schedule\\\":{\\\"interval\\\":\\\"30m\\\"}},\\\"input\\\":{\\\"search\\\":{\\\"request\\\":{\\\"body\\\":{\\\"size\\\":0,\\\"query\\\":{\\\"match_all\\\":{}}},\\\"indices\\\":[\\\"*\\\"]}}},\\\"condition\\\":{\\\"compare\\\":{\\\"ctx.payload.hits.total\\\":{\\\"gte\\\":10}}},\\\"actions\\\":{\\\"my-logging-action\\\":{\\\"logging\\\":{\\\"text\\\":\\\"There are {{ctx.payload.hits.total}} documents in your index. Threshold is 10.\\\"}}}}}}\"}"

Not sure they are related to the NP migration?

[Update]
Since the issues were caused by the NP migration to http we decided to unskip the tests here

What benefit do we get from creating the np_ready directory?

The primary benefit is outlined here in the migration guide. Basically linting for ensuring that we have moved direct use of all legacy ui/ deps out of the application logic. Also, ideally, the paths in application should mostly remain unchanged as we want to only move the stuff inside np_ready once we migrate :)

I noticed there's a lot of @ts-ignore scattered throughout the code.

Most of the remaining untyped JS modules are inside of models dirs. public has them any'ed through an index.d.ts file which uses the public/.. path convention for public code. So no matter from where they're used the module declaration requires only one entry in the index.d.ts file. I'm not sure the same exists for server side :( ? So everytime we use a model we need to append the unique path for the module to the index.d.ts file which is not that much easier than adding // @ts-ignore at this point. Overall I think the very best would be to convert them to TS which is quite a big job that I'd prefer we tackle as a separate PR 😆 . Wdyt? (there may be another way of solving this that I'm unaware of)

@alisonelizabeth
Copy link
Contributor

alisonelizabeth commented Dec 10, 2019

Thanks for the explanations, @jloleysens!

Fwiw, after unskipping thewatch_create_json.test.tsx I was seeing the following kind of errors:

Looks like it might be related. The "received" value is wrapped in body and is a JSON string. Otherwise, the content itself looks the same. I think this is a result of moving to the NP http service.

Most of the remaining untyped JS modules are inside of models dirs. public has them any'ed through an index.d.ts file which uses the public/.. path convention for public code. So no matter from where they're used the module declaration requires only one entry in the index.d.ts file. I'm not sure the same exists for server side :( ? So everytime we use a model we need to append the unique path for the module to the index.d.ts file which is not that much easier than adding // @ts-ignore at this point. Overall I think the very best would be to convert them to TS which is quite a big job that I'd prefer we tackle as a separate PR 😆 . Wdyt? (there may be another way of solving this that I'm unaware of)

Yeah, it will be quite a lot of work to convert all the models to TS. I think it's ok to leave as is for now - thanks!

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

Tested with the latest changes. LGTM. Thanks!

@cuff-links
Copy link
Contributor

cuff-links commented Dec 11, 2019 via email

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

History

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

@jloleysens jloleysens merged commit aa31b53 into elastic:master Dec 11, 2019
@jloleysens jloleysens deleted the np-migration/watcher branch December 11, 2019 08:54
@jloleysens
Copy link
Contributor Author

I want to run through the test suite before merging this. I will test it locally.

On Tue, Dec 10, 2019, 6:48 PM Elastic Machine @.***> wrote: 💚 Build Succeeded - continuous-integration/kibana-ci/pull-request https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/14677/ - Commit: 471c801 <471c801> History - 💚 Build #14585 https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/14585/ succeeded 6e90b77 <6e90b77> - 💚 Build #14515 https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/14515/ succeeded 61078aa <61078aa> - 💚 Build #14280 https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/14280/ succeeded cbf30e1 <cbf30e1> - 💔 Build #14262 https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/14262/ failed f65a526 <f65a526> - 💚 Build #14194 https://kibana-ci.elastic.co/job/elastic+kibana+pipeline-pull-request/14194/ succeeded 0ed3d4b <0ed3d4b> To update your PR or re-run it, just comment with: @elasticmachine merge upstream — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#50908?email_source=notifications&email_token=AAK5UDXUNXZ4QU5777EJCW3QYATGTA5CNFSM4JOUL47KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEGRLPRY#issuecomment-564312007>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAK5UDWN4XDQPNOOHC5BWULQYATGTANCNFSM4JOUL47A .

Hey @cuff-links , sorry for merging - I missed this message. For any issues you've found I'll open up a fix PR 👍

jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 11, 2019
* First iteration of watch public -> new platform
Still need to switch to np ready version of use_request

* - Switched to using np ready request
- Some updates after API changes

* First attempt at server shim

* Rename file and re-enable react hooks linting

* Fix some public types and react hooks lint rules

* Fix types

* More ES lint react hooks fixes

* Migrated server lib -> ts. Part way done with migrating routes to NP router and TS

* Big subset of routes to TS and NP router - almost there

* Delete legacy error wrappers and moved last set of routes to TS and NP router

* Remove @ts-ignore's and update route registration to use shim with http router

* Added routes validations, fixes for hooks and fixes for types

* Fix more types and finish testing API routes

* Fix usage of feature catalogue and fix time buckets types

* Fix error message shape [skip ci]

* Split legacy from new platform dependencies server-side

* Refactor: Seperate client legacy and NP dependencies

* Add file: added types file

* Fix UISettings client type import

* Update license pre-routing factory spec

* Update variable names, use of I18nContext (use NP) and docs

* Use NP elasticsearchclient

* Simplify is_es_error_factory

* Fix types

* Improve code legibility and remove second use of `useAppContext`

* Use @kbn/config-schema (not validate: false) on routes!

* Fix watch create JSON spec

* Create threshold test working

* Unskip watch_edit.test.ts

* Unskip watch_list.test.ts

* Done re-enabling component integration tests

* TimeBuckets typo + remove unnecessary // @ts-ignore
jloleysens added a commit to jloleysens/kibana that referenced this pull request Dec 11, 2019
…le-sql-highlighting

* 'master' of github.com:elastic/kibana: (56 commits)
  Migrate url shortener service (elastic#50896)
  Re-enable datemath in from/to canvas timelion args (elastic#52159)
  [Logs + Metrics UI] Remove eslint exceptions (elastic#50979)
  [Logs + Metrics UI] Add missing headers in Logs & metrics (elastic#52405)
  [ML] API integration tests - initial tests for bucket span estimator (elastic#52636)
  [Watcher] New Platform (NP) Migration (elastic#50908)
  Decouple Authorization subsystem from Legacy API. (elastic#52638)
  [APM] Fix some warnings logged in APM tests (elastic#52487)
  [ui/public/utils] Delete unused base_object & find_by_param (elastic#52500)
  [ui/public/utils] Move items into ui/vis (elastic#52615)
  fix newlines in kbn-analytics build script
  Add top level examples folder and command to run, `--run-examples`. (elastic#52027)
  feat(NA): add trap for SIGINT in the git precommit hook (elastic#52662)
  [DOCS] Updtes description of elasticsearch.requestHeadersWhitelist (elastic#52675)
  [Telemetry/Pulse] Updates advanced settings text for usage data (elastic#52657)
  [SIEM][Detection Engine] Adds the default name space to the end of the signals index
  [Logs UI] Generalize ML module management (elastic#50662)
  Removing stateful saved object finder (elastic#52166)
  Shim oss telemetry (elastic#51168)
  [Reporting/Screenshots] Do not fail the report if request is aborted (elastic#52344)
  ...

# Conflicts:
#	src/legacy/core_plugins/console/public/legacy.ts
#	src/legacy/core_plugins/console/public/np_ready/application/models/legacy_core_editor/mode/elasticsearch_sql_highlight_rules.ts
#	src/legacy/core_plugins/console/public/np_ready/lib/autocomplete/components/full_request_component.ts
#	src/legacy/core_plugins/console/public/quarantined/src/sense_editor/row_parser.js
jloleysens added a commit that referenced this pull request Dec 11, 2019
* [Watcher] New Platform (NP) Migration (#50908)

* First iteration of watch public -> new platform
Still need to switch to np ready version of use_request

* - Switched to using np ready request
- Some updates after API changes

* First attempt at server shim

* Rename file and re-enable react hooks linting

* Fix some public types and react hooks lint rules

* Fix types

* More ES lint react hooks fixes

* Migrated server lib -> ts. Part way done with migrating routes to NP router and TS

* Big subset of routes to TS and NP router - almost there

* Delete legacy error wrappers and moved last set of routes to TS and NP router

* Remove @ts-ignore's and update route registration to use shim with http router

* Added routes validations, fixes for hooks and fixes for types

* Fix more types and finish testing API routes

* Fix usage of feature catalogue and fix time buckets types

* Fix error message shape [skip ci]

* Split legacy from new platform dependencies server-side

* Refactor: Seperate client legacy and NP dependencies

* Add file: added types file

* Fix UISettings client type import

* Update license pre-routing factory spec

* Update variable names, use of I18nContext (use NP) and docs

* Use NP elasticsearchclient

* Simplify is_es_error_factory

* Fix types

* Improve code legibility and remove second use of `useAppContext`

* Use @kbn/config-schema (not validate: false) on routes!

* Fix watch create JSON spec

* Create threshold test working

* Unskip watch_edit.test.ts

* Unskip watch_list.test.ts

* Done re-enabling component integration tests

* TimeBuckets typo + remove unnecessary // @ts-ignore

* Backport for "xPack:defaultAdminEmail" 7.x specific behaviour
@jloleysens jloleysens mentioned this pull request Jan 14, 2020
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:NP Migration Feature:Watcher Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more v7.6.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants