-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[SearchProfiler] Move out of legacy #55331
[SearchProfiler] Move out of legacy #55331
Conversation
…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
Pinging @elastic/es-ui (Team:Elasticsearch UI) |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
… (wait for first license object before rendering)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
* 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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 👍
@@ -19,8 +19,22 @@ interface ReturnValue { | |||
error?: string; | |||
} | |||
|
|||
const extractProfilerErrorMessage = async (e: any): Promise<string | undefined> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it need to be async?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Initially I was doing something async in there but then simplified and forgot to update the async bit 👍
*/ | ||
|
||
import { IRouter, Logger } from 'kibana/server'; | ||
import { ElasticsearchPlugin } from 'src/legacy/core_plugins/elasticsearch'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it OK to import from the src
alias?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update!
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
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
* 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>
Summary
Continuation of #48795
This is the final step which moves Searchprofiler on to New Platform (NP) proper.
How to review
public/plugin.ts
andserver/plugin.ts
. Most important changes are there - how the plugin starts up and the services it uses.git-diff
likegit 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 changesBasic Test plan
1. Run search profile (default). Check that expansion works and that flyout displays correct informtion.
2. Run aggregation profile
match_all
->matchall
). Should report error and location of error in toastChecklist
Use
strikethroughsto remove checklist items you don't feel are applicable to this PR.Documentation was added for features that require explanation or tutorialsUnit or functional tests were updated or added to match the most common scenariosFor maintainers