From cca50f708cecf064be88b8901c4b11728b806852 Mon Sep 17 00:00:00 2001 From: Nicolas Chaulet Date: Tue, 29 Mar 2022 10:53:22 -0400 Subject: [PATCH 1/4] Update status when summary is updated --- src/core/server/status/plugins_status.test.ts | 26 +++++++++++++++++++ src/core/server/status/plugins_status.ts | 6 ++++- 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/src/core/server/status/plugins_status.test.ts b/src/core/server/status/plugins_status.test.ts index c07624826ff830..8130698379eda7 100644 --- a/src/core/server/status/plugins_status.test.ts +++ b/src/core/server/status/plugins_status.test.ts @@ -285,6 +285,32 @@ describe('PluginStatusService', () => { ]); }); + it('updates when a plugin status observable emits with the same level but a different summary', async () => { + const service = new PluginsStatusService({ + core$: coreAllAvailable$, + pluginDependencies: new Map([['a', []]]), + }); + const statusUpdates: Array> = []; + const subscription = service + .getAll$() + // the first emission happens right after core services emit (see explanation above) + .pipe(skip(1)) + .subscribe((pluginStatuses) => statusUpdates.push(pluginStatuses)); + + const aStatus$ = new BehaviorSubject({ + level: ServiceStatusLevels.available, + summary: 'summary initial', + }); + service.set('a', aStatus$); + aStatus$.next({ level: ServiceStatusLevels.available, summary: 'summary updated' }); + subscription.unsubscribe(); + + expect(statusUpdates).toEqual([ + { a: { level: ServiceStatusLevels.available, summary: 'summary initial' } }, + { a: { level: ServiceStatusLevels.available, summary: 'summary updated' } }, + ]); + }); + it('emits an unavailable status if first emission times out, then continues future emissions', async () => { const service = new PluginsStatusService( { diff --git a/src/core/server/status/plugins_status.ts b/src/core/server/status/plugins_status.ts index 8d042d4cba3f9f..bcb2adb5dade5c 100644 --- a/src/core/server/status/plugins_status.ts +++ b/src/core/server/status/plugins_status.ts @@ -331,11 +331,15 @@ export class PluginsStatusService { */ private updatePluginReportedStatus(plugin: PluginName, reportedStatus: ServiceStatus): void { const previousReportedLevel = this.pluginData[plugin].reportedStatus?.level; + const previousReportedSummary = this.pluginData[plugin].reportedStatus?.summary; this.pluginData[plugin].reportedStatus = reportedStatus; this.pluginStatus[plugin] = reportedStatus; - if (reportedStatus.level !== previousReportedLevel) { + if ( + reportedStatus.level !== previousReportedLevel || + reportedStatus.summary !== previousReportedSummary + ) { this.updatePluginsStatuses([plugin]); } } From e3b852777c158cb4fab19b7208b5ff90b1da4538 Mon Sep 17 00:00:00 2001 From: Nicolas Chaulet Date: Tue, 29 Mar 2022 15:51:34 -0400 Subject: [PATCH 2/4] Compare whole status instead of level and summary --- src/core/server/status/plugins_status.ts | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/core/server/status/plugins_status.ts b/src/core/server/status/plugins_status.ts index bcb2adb5dade5c..b3010d031c31b3 100644 --- a/src/core/server/status/plugins_status.ts +++ b/src/core/server/status/plugins_status.ts @@ -14,7 +14,7 @@ import { timeoutWith, startWith, } from 'rxjs/operators'; -import { sortBy } from 'lodash'; +import { sortBy, isEqual } from 'lodash'; import { isDeepStrictEqual } from 'util'; import { type PluginName } from '../plugins'; @@ -330,16 +330,12 @@ export class PluginsStatusService { * @param {ServiceStatus} reportedStatus The newly reported status for that plugin */ private updatePluginReportedStatus(plugin: PluginName, reportedStatus: ServiceStatus): void { - const previousReportedLevel = this.pluginData[plugin].reportedStatus?.level; - const previousReportedSummary = this.pluginData[plugin].reportedStatus?.summary; + const previousReportedStatus = this.pluginData[plugin].reportedStatus; this.pluginData[plugin].reportedStatus = reportedStatus; this.pluginStatus[plugin] = reportedStatus; - if ( - reportedStatus.level !== previousReportedLevel || - reportedStatus.summary !== previousReportedSummary - ) { + if (!isEqual(previousReportedStatus, reportedStatus)) { this.updatePluginsStatuses([plugin]); } } From 43743259e844b1743a16399161ee050e7acb1ecf Mon Sep 17 00:00:00 2001 From: Gerard Soldevila Date: Wed, 30 Mar 2022 17:46:10 +0200 Subject: [PATCH 3/4] Misc enhancements --- src/core/server/status/plugins_status.ts | 63 +++++++++++++++--------- 1 file changed, 41 insertions(+), 22 deletions(-) diff --git a/src/core/server/status/plugins_status.ts b/src/core/server/status/plugins_status.ts index b3010d031c31b3..8d8a262bf862ed 100644 --- a/src/core/server/status/plugins_status.ts +++ b/src/core/server/status/plugins_status.ts @@ -14,7 +14,7 @@ import { timeoutWith, startWith, } from 'rxjs/operators'; -import { sortBy, isEqual } from 'lodash'; +import { sortBy } from 'lodash'; import { isDeepStrictEqual } from 'util'; import { type PluginName } from '../plugins'; @@ -71,7 +71,12 @@ export class PluginsStatusService { this.coreSubscription = deps.core$ .pipe(debounceTime(10)) - .subscribe((coreStatus: CoreStatus) => this.updateCoreAndPluginStatuses(coreStatus)); + .subscribe((coreStatus: CoreStatus) => { + this.coreStatus = coreStatus; + this.updateRootPluginsStatuses(); + this.updateDependenciesStatuses(this.rootPlugins); + this.reportNewStatusToObservers(); + }); } /** @@ -96,8 +101,19 @@ export class PluginsStatusService { this.reportedStatusSubscriptions[plugin] = status$ // Set a timeout for externally-defined status Observables - .pipe(timeoutWith(this.statusTimeoutMs, status$.pipe(startWith(defaultStatus)))) - .subscribe((status) => this.updatePluginReportedStatus(plugin, status)); + .pipe( + timeoutWith(this.statusTimeoutMs, status$.pipe(startWith(defaultStatus))), + distinctUntilChanged() + ) + .subscribe((status) => { + const levelChanged = this.updatePluginReportedStatus(plugin, status); + + if (levelChanged) { + this.updateDependenciesStatuses([plugin]); + } + + this.reportNewStatusToObservers(); + }); } /** @@ -233,16 +249,14 @@ export class PluginsStatusService { } /** - * Updates the core services statuses and plugins' statuses - * according to the latest status reported by core services. - * @param {CoreStatus} coreStatus the latest status of core services + * Updates the root plugins statuses according to the current core services status */ - private updateCoreAndPluginStatuses(coreStatus: CoreStatus): void { - this.coreStatus = coreStatus!; + private updateRootPluginsStatuses(): void { const derivedStatus = getSummaryStatus(Object.entries(this.coreStatus), { allAvailableSummary: `All dependencies are available`, }); + // note that the derived status is the same for all root plugins this.rootPlugins.forEach((plugin) => { this.pluginData[plugin].derivedStatus = derivedStatus; if (!this.isReportingStatus[plugin]) { @@ -250,18 +264,19 @@ export class PluginsStatusService { this.pluginStatus[plugin] = derivedStatus; } }); - - this.updatePluginsStatuses(this.rootPlugins); } /** - * Determine the derived statuses of the specified plugins and their dependencies, + * Determine the derived statuses of the specified plugins' dependencies, * updating them on the pluginData structure * Optionally, if the plugins have not registered a custom status Observable, update their "current" status as well. - * @param {PluginName[]} plugins The names of the plugins to be updated + * @param {PluginName[]} plugins The names of the plugins to be updated, along with their subtrees */ - private updatePluginsStatuses(plugins: PluginName[]): void { - const toCheck = new Set(plugins); + private updateDependenciesStatuses(plugins: PluginName[]): void { + const toCheck = new Set(); + plugins.forEach((plugin) => + this.pluginData[plugin].reverseDependencies.forEach((revDep) => toCheck.add(revDep)) + ); // Note that we are updating the plugins in an ordered fashion. // This way, when updating plugin X (at depth = N), @@ -276,9 +291,6 @@ export class PluginsStatusService { this.pluginData[current].reverseDependencies.forEach((revDep) => toCheck.add(revDep)); } } - - this.pluginData$.next(this.pluginData); - this.pluginStatus$.next({ ...this.pluginStatus }); } /** @@ -328,15 +340,22 @@ export class PluginsStatusService { * Updates the reported status for the given plugin, along with the status of its dependencies tree. * @param {PluginName} plugin The name of the plugin whose reported status must be updated * @param {ServiceStatus} reportedStatus The newly reported status for that plugin + * @return {boolean} true if the level of the reported status changed */ - private updatePluginReportedStatus(plugin: PluginName, reportedStatus: ServiceStatus): void { + private updatePluginReportedStatus(plugin: PluginName, reportedStatus: ServiceStatus): boolean { const previousReportedStatus = this.pluginData[plugin].reportedStatus; this.pluginData[plugin].reportedStatus = reportedStatus; this.pluginStatus[plugin] = reportedStatus; - if (!isEqual(previousReportedStatus, reportedStatus)) { - this.updatePluginsStatuses([plugin]); - } + return previousReportedStatus?.level !== reportedStatus.level; + } + + /** + * Emit new status to internal Subjects, propagating it to the output Observables. + */ + private reportNewStatusToObservers(): void { + this.pluginData$.next(this.pluginData); + this.pluginStatus$.next({ ...this.pluginStatus }); } } From c0d5f34b3866465c4a2b5508dd57108ae8b84457 Mon Sep 17 00:00:00 2001 From: Gerard Soldevila Date: Wed, 30 Mar 2022 18:13:25 +0200 Subject: [PATCH 4/4] Improve naming and JSDoc --- src/core/server/status/plugins_status.ts | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/src/core/server/status/plugins_status.ts b/src/core/server/status/plugins_status.ts index 8d8a262bf862ed..d77529f06ddec5 100644 --- a/src/core/server/status/plugins_status.ts +++ b/src/core/server/status/plugins_status.ts @@ -74,8 +74,8 @@ export class PluginsStatusService { .subscribe((coreStatus: CoreStatus) => { this.coreStatus = coreStatus; this.updateRootPluginsStatuses(); - this.updateDependenciesStatuses(this.rootPlugins); - this.reportNewStatusToObservers(); + this.updateDependantStatuses(this.rootPlugins); + this.emitCurrentStatus(); }); } @@ -109,10 +109,10 @@ export class PluginsStatusService { const levelChanged = this.updatePluginReportedStatus(plugin, status); if (levelChanged) { - this.updateDependenciesStatuses([plugin]); + this.updateDependantStatuses([plugin]); } - this.reportNewStatusToObservers(); + this.emitCurrentStatus(); }); } @@ -267,12 +267,11 @@ export class PluginsStatusService { } /** - * Determine the derived statuses of the specified plugins' dependencies, - * updating them on the pluginData structure - * Optionally, if the plugins have not registered a custom status Observable, update their "current" status as well. - * @param {PluginName[]} plugins The names of the plugins to be updated, along with their subtrees + * Update the derived statuses of the specified plugins' dependant plugins, + * If impacted plugins have not registered a custom status Observable, update their "current" status as well. + * @param {PluginName[]} plugins The names of the plugins whose dependant plugins must be updated */ - private updateDependenciesStatuses(plugins: PluginName[]): void { + private updateDependantStatuses(plugins: PluginName[]): void { const toCheck = new Set(); plugins.forEach((plugin) => this.pluginData[plugin].reverseDependencies.forEach((revDep) => toCheck.add(revDep)) @@ -352,10 +351,11 @@ export class PluginsStatusService { } /** - * Emit new status to internal Subjects, propagating it to the output Observables. + * Emit the current status to internal Subjects, effectively propagating it to observers. */ - private reportNewStatusToObservers(): void { + private emitCurrentStatus(): void { this.pluginData$.next(this.pluginData); + // we must clone the plugin status to prevent future modifications from updating current emission this.pluginStatus$.next({ ...this.pluginStatus }); } }