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

Rewrite plugin_status logic limiting usage of Observables (reducing heap size) #128324

Merged
merged 11 commits into from
Mar 25, 2022

Conversation

gsoldevila
Copy link
Contributor

@gsoldevila gsoldevila commented Mar 22, 2022

Attempt to solve #128061

The idea is to rewrite the src/core/server/status/plugins_status.ts logic in order to simplify the RxJS pipeline. We take into account "input" observables and "output" observables, but we no longer use RxJS for the internal update of plugin statuses.

image

  1. We receive core status updates, and plugin reported status updates through input observables. We debounce these events to avoid unnecessary calculations.
  2. The PluginsStatusService class maintains an updated copy of the state, in a sort of Redux store pattern, so to speak. The state includes convenient structures to make updates more efficient. These structures include a tree of reverse dependencies, and a list of plugin names sorted by depth. The root plugins (depth = 1) are those that do not depend on any other plugin. A plugin that depends on a root plugin will have depth = 2, and so on so forth.
  3. The state is updated iteratively, going through the ordered list of plugins. This causes less recursive calls and less heap consumption.
  4. Once the state has been updated, PluginsStatusService then signals the internal Subjects that are connected to the output observables.

Key changes to reduce heap size and improve performance

  • Previous implementation used Observables to recalculate and propagate the status of each plugin through the tree of dependencies. The switchMap and concatMap operators create new Observables with each higher order emission.
  • The recursive approach is not optimal in this scenario, because in order to make sure all statuses are updated, we might need to visit each node more than once. For instance, if plugin C depends on plugins A and B we might have to calculate C's status twice (once when we process A's dependencies and another time when we process B's dependencies).

Memory consumption impact

Heap size after Kibana started reduced from 800Mb to 280Mb.
Heap snapshot is taken right after Kibana started.
main branch
2022-03-21_18-14-56
the current branch.
2022-03-23_17-03-43

@gsoldevila gsoldevila added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.2.0 labels Mar 22, 2022
@gsoldevila gsoldevila requested a review from mshustov March 22, 2022 22:27
@mshustov
Copy link
Contributor

buildkite build this

@mshustov
Copy link
Contributor

buildkite test this

@jbudz jbudz self-requested a review March 23, 2022 16:44
@mshustov
Copy link
Contributor

@gsoldevila, if we confirm the positive impact of the PR on memory consumption, I'd consider porting it back to 8.1 and 7.x.

Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

This is huge, memory consumption looks great:
image

#128061 (comment) for comparison.

src/core/server/status/plugins_status.ts Outdated Show resolved Hide resolved
src/core/server/status/plugins_status.ts Outdated Show resolved Hide resolved
this.coreStatus = coreStatus!;
// console.log('⚡ CORE STATUS! elastic: ', coreStatus.elasticsearch.level.toString(), '; savedObjects: ', coreStatus.savedObjects.level.toString());
const derivedStatus = getSummaryStatus(Object.entries(this.coreStatus), {
allAvailableSummary: `All dependencies are available`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we shouldn't pass the default values?

Copy link
Contributor Author

@gsoldevila gsoldevila Mar 24, 2022

Choose a reason for hiding this comment

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

This was like that in the previous implementation.
The getSummaryStatus is used in a couple of places:

  • status_service uses the default "All services are available"
  • plugins_status (this class) uses the slightly different "All dependencies are available".

The problem is that getSummaryStatus method does not currently have the knowledge of whether it's being called with plugins statuses only, or with plugins statuses + core services statuses.

But if we agree to use the exact same message for both cases, then I can probably remove that default value.

src/core/server/status/plugins_status.ts Outdated Show resolved Hide resolved
src/core/server/status/status_service.ts Outdated Show resolved Hide resolved
src/core/server/status/plugins_status.ts Outdated Show resolved Hide resolved

private updatePluginsStatuses(plugins: PluginName[]): void {
const toCheck = new Set<PluginName>(plugins);
for (let i = 0; i < this.orderedPluginNames.length && toCheck.size > 0; ++i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

question: maybe we should iterate through toCheck set?

Copy link
Contributor Author

@gsoldevila gsoldevila Mar 24, 2022

Choose a reason for hiding this comment

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

Actually, we need to iterate in "depth" order:

  • first updating root plugins' statuses, then updating depth=2, and so on so forth.

Otherwise we might try to update a plugin status before updating its dependencies.

src/core/server/status/plugins_status.ts Show resolved Hide resolved
src/core/server/status/plugins_status.ts Outdated Show resolved Hide resolved
@gsoldevila gsoldevila marked this pull request as ready for review March 24, 2022 18:43
@gsoldevila gsoldevila requested a review from a team as a code owner March 24, 2022 18:43
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@TinaHeiligers TinaHeiligers added loe:medium Medium Level of Effort impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. labels Mar 24, 2022
@gsoldevila gsoldevila changed the title WIP Rewrite plugin_status logic limiting usage of Observables (reducing heap size) Mar 25, 2022
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

@gsoldevila gsoldevila merged commit a645545 into elastic:main Mar 25, 2022
gsoldevila added a commit to gsoldevila/kibana that referenced this pull request Apr 5, 2022
…eap size) (elastic#128324)

* WIP

* Fix behavior when no plugins are defined

* Remove unused import, reduce debounce times

* Fix startup behavior

* Misc improvements following PR comments

* Fix plugin_status UTs

* Code cleanup + enhancements

* Remove fixed FIXME

(cherry picked from commit a645545)
gsoldevila added a commit that referenced this pull request Apr 6, 2022
…ucing heap size) (#128324) (#129507)

* Rewrite plugin_status logic limiting usage of Observables (reducing heap size) (#128324)

* WIP

* Fix behavior when no plugins are defined

* Remove unused import, reduce debounce times

* Fix startup behavior

* Misc improvements following PR comments

* Fix plugin_status UTs

* Code cleanup + enhancements

* Remove fixed FIXME

(cherry picked from commit a645545)

* Add extra tests from #128769

* Fix linting issues (older typescript version?)

* Fix TS version issues
@tylersmalley tylersmalley added ci:cloud-deploy Create or update a Cloud deployment and removed ci:deploy-cloud labels Aug 17, 2022
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 ci:cloud-deploy Create or update a Cloud deployment impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:medium Medium Level of Effort release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants