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

Remove data_enhanced plugin #122075

Merged
merged 29 commits into from
Apr 29, 2022
Merged

Conversation

Dosant
Copy link
Contributor

@Dosant Dosant commented Dec 28, 2021

Summary

Close #119321

This pr removes the data_enhanced plugin and moves its code to the data plugin
The _enhanced plugins were created as a license workaround which is no longer needed and we can clean it up.

Most of the moved code is search-sessions code. That code moved to data plugin and data plugin now has to depend on security and task_manager (src plugins can depend on x-pack plugins as an optional dep). In some places this dependency created circular dependencies which were fixed by fixing the dependence to be on dataViews plugin instead of data plugin.

Risk Matrix

Risk Probability Severity Mitigation/Notes
Check that search-session work. Including the configs and task manager tasks Mid High Most of the moved code is search-session related

For maintainers

Release notes

xpack.data_enhanced.* kibana.yml config params are deprecated and data.* counterparts should be used instead

@Dosant
Copy link
Contributor Author

Dosant commented Dec 30, 2021

@elasticmachine merge upstream

@Dosant Dosant force-pushed the d/2021-12-22-remove-data-enhanced branch from 1d61349 to 83640bc Compare April 6, 2022 13:27
@Dosant
Copy link
Contributor Author

Dosant commented Apr 14, 2022

@elasticmachine merge upstream

@Dosant Dosant changed the title D/2021 12 22 remove data enhanced Remove data_enhanced plugin Apr 20, 2022
@@ -154,6 +154,19 @@
}
}
},
"search-session": {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@elastic/telemetry this config was moved from x-pack to oss. Is it safe to do? Is there anything else to be aware of?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll let @Bamieh or @afharo confirm, but I think it's fine

Copy link
Member

Choose a reason for hiding this comment

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

yup! all good! I don't think it changes the resulting output :)

},
"include": [
"src/core/server/index.ts",
"src/core/public/index.ts",
"src/plugins/data/server/index.ts",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

After moving the code from data_enhanced to data this type generation for docs stopped working because data now references code from x-pack (task_manager, security) and this changed the output folder structure of this types.

Since we don't use these types for app services plugins generation anymore, I just removed them. I added the rootDir to preserve the previous file tree output structure.

@@ -19,4 +19,7 @@ export const autocompleteConfigDeprecationProvider: ConfigDeprecationProvider =
renameFromRoot('kibana.autocompleteTimeout', 'data.autocomplete.valueSuggestions.timeout', {
level: 'warning',
}),
renameFromRoot('xpack.data_enhanced.search.sessions', 'data.search.sessions', {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

session config has to be changed

@Dosant Dosant requested a review from lukasolson April 20, 2022 14:57
@Dosant
Copy link
Contributor Author

Dosant commented Apr 21, 2022

@elasticmachine merge upstream

Copy link
Member

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

Code changes LGTM and things seem to be working properly (even having sessions from master & then checking out this PR).

checkPersistedCompletedSessionExpiration
);

this.setupCompleted = true;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand why this is now necessary. Could you explain?

Copy link
Contributor Author

@Dosant Dosant Apr 25, 2022

Choose a reason for hiding this comment

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

Mostly a precaution, because task_manager plugin is optional, we have to handle a code branch where search_service doesn't have access to task_manager and it can't initialize session_service:

if (taskManager) {
this.sessionService.setup(core, { taskManager, security });
} else {
// this should never happen in real world, but
// taskManager and security are optional deps because they are in x-pack
this.logger.debug('Skipping sessionService setup because taskManager is not available');
}

This makes it possible so that setup is never called and I wanted a clear error in case start is called by mistake in this case

data: DataPublicPluginStart;
}
// eslint-disable-next-line @typescript-eslint/no-empty-interface
export interface StartPlugins {}
Copy link
Member

Choose a reason for hiding this comment

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

Was this unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

Response Ops changes LGTM

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

Security Solution changes LGTM

@Kerry350 Kerry350 self-requested a review April 26, 2022 12:48
Copy link
Contributor

@Kerry350 Kerry350 left a comment

Choose a reason for hiding this comment

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

Infra changes LGTM.

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@afharo afharo left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -154,6 +154,19 @@
}
}
},
"search-session": {
Copy link
Member

Choose a reason for hiding this comment

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

yup! all good! I don't think it changes the resulting output :)

@Dosant
Copy link
Contributor Author

Dosant commented Apr 28, 2022

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-services (Team:AppServicesSv)

@Dosant Dosant added Feature:Search Querying infrastructure in Kibana v8.3.0 release_note:deprecation labels Apr 28, 2022
@Dosant
Copy link
Contributor Author

Dosant commented Apr 29, 2022

@elasticmachine merge upstream

@Dosant
Copy link
Contributor Author

Dosant commented Apr 29, 2022

@elasticmachine merge upstream

@patrykkopycinski
Copy link
Contributor

@elasticmachine merge upstream

@patrykkopycinski
Copy link
Contributor

@elasticmachine merge upstream

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
data 456 506 +50
dataEnhanced 57 - -57
total -7

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
data 2802 2851 +49
dataEnhanced 16 - -16
total +33

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
data 15.1KB 52.4KB +37.3KB
dataEnhanced 45.4KB - -45.4KB
total -8.1KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
data 18 19 +1
dataEnhanced 2 - -2
total -1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
data 410.9KB 416.3KB +5.4KB
dataEnhanced 9.3KB - -9.3KB
esUiShared 130.3KB 130.3KB -21.0B
total -3.9KB
Unknown metric groups

API count

id before after diff
data 3414 3463 +49
dataEnhanced 16 - -16
total +33

async chunk count

id before after diff
data 2 4 +2
dataEnhanced 2 - -2
total -0

ESLint disabled line counts

id before after diff
data 39 57 +18
dataEnhanced 18 - -18
runtimeFields 2 3 +1
total +1

References to deprecated APIs

id before after diff
data 375 382 +7
dataEnhanced 49 - -49
total -42

Total ESLint disabled count

id before after diff
data 43 61 +18
dataEnhanced 18 - -18
runtimeFields 2 3 +1
total +1

Unreferenced deprecated APIs

id before after diff
data 56 57 +1

History

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

@Dosant Dosant merged commit e603d92 into elastic:main Apr 29, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Apr 29, 2022
kertal pushed a commit to kertal/kibana that referenced this pull request May 24, 2022
Code moved into `data` plugin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Search Querying infrastructure in Kibana release_note:deprecation v8.3.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[data.search] remove data_enhanced