-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Cutover aggs to the new platform #59605
Conversation
Pinging @elastic/kibana-app-arch (Team:AppArch) |
59d7a0d
to
79495f9
Compare
ba448d7
to
c2e811e
Compare
5b69e10
to
c776aba
Compare
*/ | ||
function noWhiteSpace(html: string) { | ||
const TAGS_WITH_WS = />\s+</g; | ||
return html.replace(TAGS_WITH_WS, '><'); |
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.
This is a one-line function so I inlined it here rather than importing from legacy kibana
plugin as we had done previously.
export interface DataSetup { | ||
search: SearchSetup; | ||
} | ||
export interface DataSetup {} // eslint-disable-line @typescript-eslint/no-empty-interface |
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.
Kept these empty interfaces in place because there were a few areas in Kibana still using them. (trying to keep things as small as possible).
setInjectedMetadata(core.injectedMetadata); | ||
|
||
export class DataPlugin implements Plugin<DataSetup, DataStart> { | ||
public setup(core: CoreSetup) { | ||
// This is to be deprecated once we switch to the new search service fully | ||
addSearchStrategy(defaultSearchStrategy); |
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.
@lizozom said this will be going away shortly
export * from './index_patterns'; | ||
export * from './es_query'; | ||
export * from './utils'; |
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.
Removed this export because now the only thing in utils
is internal to other items in the common
directory.
@@ -3,6 +3,9 @@ | |||
"version": "kibana", | |||
"server": true, | |||
"ui": true, | |||
"requiredPlugins": ["uiActions"], | |||
"requiredPlugins": [ | |||
"expressions", |
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.
expressions is now required to register esaggs
function
actions: { | ||
createFiltersFromEvent, | ||
}, |
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.
createFiltersFromEvent
is technically stateful as it relies on index patterns behind the scenes. Open to suggestions on where this should live, but in its current form it cannot be exported statically @ppisljar
geoTileBucketAgg, | ||
], | ||
}; | ||
export function getAggTypes(deps: { uiSettings: IUiSettingsClient }) { |
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.
Now that this has been wrapped with a provider pattern, we can (as a follow up step) get rid of the getters/setters that are currently being used to retrieve stateful services in each agg type, and instead explicitly pass those deps down.
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.
might make sense to pass down something that can be same on client and server (like getConfig function)
@@ -41,11 +40,10 @@ import { | |||
export function mockDataServices() { |
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.
This still lives as a test helper inside search.aggs
, but might be more generically useful elsewhere in the data plugin. Perhaps we consider eventually moving it up to a different directory
search: newSearchMock, | ||
__LEGACY: { | ||
esClient: { | ||
search: searchMock, | ||
msearch: msearchMock, | ||
}, | ||
}, | ||
}, | ||
} as unknown) as jest.Mocked<ISearchStart>, |
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.
Changed this to casting because there was a circular dependency going back to require the search.aggs mocks, and it felt unnecessary to duplicate the entire search service contract here
@@ -18,18 +18,19 @@ | |||
*/ | |||
|
|||
export { | |||
calculateObjectHash, |
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.
calculateObjectHash wasn't exported at the top level so I added it here, and alphabetized
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
c776aba
to
399c193
Compare
cc19af4
to
3094c26
Compare
3094c26
to
36d078d
Compare
aa00d90
to
fcfa468
Compare
💚 Build SucceededHistory
To update your PR or re-run it, just comment with: |
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.
Migration guide changes LGTM. Thanks for making the plugins.
prefix more clear 😄
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.
code LGTM
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.
LGTM, mostly checked the default editor changes, great work!
I will review this PR today. |
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.
Tested and couldn't find any problems, LGTM 👍
Summary
Moves
agg_types
into the new platform under thedata
plugin'ssearch
service. Once this PR is merged, it should unblock anybody waiting on the rest of the search service to move to the new platform.Some specific items moved include:
agg_types
esaggs
tabify
At this point there is nothing left in
src/legacy/core_plugins/data
that anybody should need to rely on... anything there is getting left behind for BWC and can be completely deleted when downstream apps complete migration.For reviewers
If you got pinged for a codeowners review, don't be intimidated by the size of this PR 😬
Most of the significant changes here affect code owned by app arch, but since nearly all of the code has been removed from
src/legacy/core_plugins/data
at this point, imports needed to be updated across the project. In most cases you should only have a few changed lines of code to review.For those reviewing large portions of the PR, I recommend going commit-by-commit instead of attempting to peruse the whole thing. I've tried to keep a coherent commit history so it should make things somewhat easier to follow.
For QA
This PR should introduce no functional changes to Kibana; it is 100% internal refactoring as we migrate our search aggregation infrastructure to the new platform.
Additional info
data
plugin, a ton of markdown files were createdui/agg_types
were preserved for BWC and to minimize the changes that need to go into this (already large) PR.core_plugins/data
has been preserveddata/common
. This includesparseEsInterval
,dateHistogramInverval
,isValidEsInterval
, etc.ui/new_platform
to ensure functionality that was necessary for the legacy platform still works without the legacy data pluginDev docs
I am skipping docs on this PR, as the documentation is being added to the 7.7 ui/public cleanup documentation issue.