[SearchProfiler] Move out of legacy #55331

jloleysens · 2020-01-20T14:46:52Z

Summary

Continuation of #48795

This is the final step which moves Searchprofiler on to New Platform (NP) proper.

How to review

I would recommend looking mainly at public/plugin.ts and server/plugin.ts. Most important changes are there - how the plugin starts up and the services it uses.
Could be helpful to have a second local copy of the kibana repo so that you can use git-diff like git diff --no-index ../current-master/x-pack/watcher/public/np_ready/application ./x-pack/plugins/searchprofiler/public/application for a cleaner summary of application specific changes
Migrated server side to NP as part of this move out of legacy folder
Style (SASS) files are still in legacy for now.

Basic Test plan

With basic license (or higher)
1. With working query
  1. Run search profile (default). Check that expansion works and that flyout displays correct informtion.
  2. Run aggregation profile
2. With broken query (e.g., misspell match_all -> matchall). Should report error and location of error in toast
3. With bad JSON, should prevent request from being submitted
If UI already loaded and license changes
1. UI should load, but be disabled (i.e., prevent user input)
2. Server should respond with 403 and message relating to license
If license lower than basic at startup: UI should not load.

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
~~Documentation was added for features that require explanation or tutorials~~
~~Unit or functional tests were updated or added to match the most common scenarios~~
This was checked for keyboard-only and screenreader accessibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

…hings need testing

- Updated license check to only check for presence of basic license (not search profiler as a feature - Updated the payload: removed types from validation - Also added README in public regarding the location of styles

…r reporting from profiler endpoint

elasticmachine · 2020-01-20T14:46:56Z

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

jloleysens · 2020-01-20T14:47:13Z

@elasticmachine merge upstream

jloleysens · 2020-01-20T16:36:13Z

@elasticmachine merge upstream

… (wait for first license object before rendering)

sebelga

Awesome work @jloleysens ! I tested locally and everything works as expected.

With lower than basic license
UI should load, but be disabled (i.e., prevent user input)
Server should respond with 403 and message relating to license

When I started in OSS, I actually did not have the searchprofiler at all. How do we test the above scenario?

sebelga · 2020-01-22T08:02:52Z

x-pack/plugins/searchprofiler/common/constants.ts

+ * or more contributor license agreements. Licensed under the Elastic License;
+ * you may not use this file except in compliance with the Elastic License.
+ */
+import { LicenseType } from '../../licensing/common/types';


I think it would be good to export from a barrel index.ts at the root of "licensing" pluggin what can be imported elsewhere, in this case the types. WDYT?

[EDIT] From the platform doc "The New Platform only considers values exported from my_plugin/public and my_plugin/server to be stable.". So, in this case, the barrel should be in public and export from the common folder?

Right, so that value is being pulled in from the common folder in the licensing plugin. They have marked that type public which is way I am reaching in to that plugin to pull it out.

Perhaps worth opening a question about it to either have licensing explain the reasoning or update the platform doc to cater for values that are not public or server specific.

I will add an index.ts file to common inside search profiler marking these values similarly as internal 👍

sebelga · 2020-01-22T08:42:44Z

x-pack/plugins/searchprofiler/public/application/hooks/use_request_profile.ts

@@ -19,8 +19,22 @@ interface ReturnValue {
  error?: string;
 }

+const extractProfilerErrorMessage = async (e: any): Promise<string | undefined> => {


Does it need to be async?

Good catch! Initially I was doing something async in there but then simplified and forgot to update the async bit 👍

sebelga · 2020-01-22T09:14:12Z

x-pack/plugins/searchprofiler/server/types.ts

+ */
+
+import { IRouter, Logger } from 'kibana/server';
+import { ElasticsearchPlugin } from 'src/legacy/core_plugins/elasticsearch';


Is it OK to import from the src alias?

Will update!

jloleysens · 2020-01-22T11:30:40Z

@sebelga

I was testing in a situation where the license changes while the user is interacting with the UI, will clarify, thanks for testing that scenario!

Marked types and values as internal Removed unnecessary "async" from function Update import to not use "src" alias

kibanamachine · 2020-01-22T12:53:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: dab0135

History

💚 Build #21516 succeeded c14047c
💔 Build #21442 failed 5994f61
💔 Build #21414 failed 75d1302

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

* Initial move of searchprofiler into new platform directory, lots of things need testing * Whitespace, clean up types and remove unused files * First iteration of end-to-end plugin working - Updated license check to only check for presence of basic license (not search profiler as a feature - Updated the payload: removed types from validation - Also added README in public regarding the location of styles * Added extractProfilerErrorMessage function to interface with new error reporting from profiler endpoint * Fix paths to test_utils * Update I18n for search profiler * Fix react hooks ordering bug with license status updates and fix test (wait for first license object before rendering) * Added index.ts file to common in searchprofiler route Marked types and values as internal Removed unnecessary "async" from function Update import to not use "src" alias Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

…t-of-legacy * 'master' of github.com:elastic/kibana: [Watcher] Add support for additional watch action statuses (elastic#55092) [ML] Fix entity controls update. (elastic#55524) [ML] Fixing ML's watcher integration (elastic#55439) [ML] Fixes permissions checks for data visualizer create job links (elastic#55431) [SearchProfiler] Move out of legacy (elastic#55331) # Conflicts: # x-pack/plugins/watcher/server/models/action_status/action_status.js

* Initial move of searchprofiler into new platform directory, lots of things need testing * Whitespace, clean up types and remove unused files * First iteration of end-to-end plugin working - Updated license check to only check for presence of basic license (not search profiler as a feature - Updated the payload: removed types from validation - Also added README in public regarding the location of styles * Added extractProfilerErrorMessage function to interface with new error reporting from profiler endpoint * Fix paths to test_utils * Update I18n for search profiler * Fix react hooks ordering bug with license status updates and fix test (wait for first license object before rendering) * Added index.ts file to common in searchprofiler route Marked types and values as internal Removed unnecessary "async" from function Update import to not use "src" alias Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

jloleysens added 6 commits January 20, 2020 12:49

Initial move of searchprofiler into new platform directory, lots of t…

5bb4a15

…hings need testing

Whitespace, clean up types and remove unused files

ea827b4

First iteration of end-to-end plugin working

a17d790

- Updated license check to only check for presence of basic license (not search profiler as a feature - Updated the payload: removed types from validation - Also added README in public regarding the location of styles

Added extractProfilerErrorMessage function to interface with new erro…

58b618d

…r reporting from profiler endpoint

Fix paths to test_utils

28b4707

Update I18n for search profiler

80957cc

jloleysens added Feature:Dev Tools release_note:roadmap v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more Feature:Search Profiler v7.7.0 labels Jan 20, 2020

jloleysens requested a review from sebelga January 20, 2020 14:46

Merge branch 'master' into np/searchprofiler/move-out-of-legacy

75d1302

elasticmachine and others added 2 commits January 20, 2020 11:36

Merge branch 'master' into np/searchprofiler/move-out-of-legacy

5994f61

Fix react hooks ordering bug with license status updates and fix test…

c14047c

… (wait for first license object before rendering)

sebelga approved these changes Jan 22, 2020

View reviewed changes

Added index.ts file to common in searchprofiler route

dab0135

Marked types and values as internal Removed unnecessary "async" from function Update import to not use "src" alias

jloleysens merged commit b8d3510 into elastic:master Jan 22, 2020

jloleysens deleted the np/searchprofiler/move-out-of-legacy branch January 22, 2020 12:58

jloleysens mentioned this pull request Jan 22, 2020

[7.x] [SearchProfiler] Move out of legacy (#55331) #55543

Merged

cjcenizal added this to Done in Elasticsearch UI Mar 2, 2020

cjcenizal moved this from Done to Done - NP migration in Elasticsearch UI Mar 2, 2020

cjcenizal added the Feature:NP Migration label Apr 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SearchProfiler] Move out of legacy #55331

[SearchProfiler] Move out of legacy #55331

jloleysens commented Jan 20, 2020 •

edited

Loading

elasticmachine commented Jan 20, 2020

jloleysens commented Jan 20, 2020

jloleysens commented Jan 20, 2020

sebelga left a comment

sebelga Jan 22, 2020

jloleysens Jan 22, 2020

sebelga Jan 22, 2020

jloleysens Jan 22, 2020

sebelga Jan 22, 2020

jloleysens Jan 22, 2020

jloleysens commented Jan 22, 2020

kibanamachine commented Jan 22, 2020

[SearchProfiler] Move out of legacy #55331

[SearchProfiler] Move out of legacy #55331

Conversation

jloleysens commented Jan 20, 2020 • edited Loading

Summary

How to review

Basic Test plan

Checklist

For maintainers

elasticmachine commented Jan 20, 2020

jloleysens commented Jan 20, 2020

jloleysens commented Jan 20, 2020

sebelga left a comment

Choose a reason for hiding this comment

sebelga Jan 22, 2020

Choose a reason for hiding this comment

jloleysens Jan 22, 2020

Choose a reason for hiding this comment

sebelga Jan 22, 2020

Choose a reason for hiding this comment

jloleysens Jan 22, 2020

Choose a reason for hiding this comment

sebelga Jan 22, 2020

Choose a reason for hiding this comment

jloleysens Jan 22, 2020

Choose a reason for hiding this comment

jloleysens commented Jan 22, 2020

kibanamachine commented Jan 22, 2020

💚 Build Succeeded

History

jloleysens commented Jan 20, 2020 •

edited

Loading