Add debounce to the status observers to reduce unnecessary CPU loops #108952

afharo · 2021-08-17T17:23:12Z

Summary

During plugin registration, plugins can specify their own custom logic to declare the status. The status service was triggering an update of its observers for each registration, using unnecessary extra CPU loops.

This PR adds debounceTime to swallow any requests happening too close in time.

For maintainers

This was checked for breaking API changes and was labeled appropriately

lukeelmers

Overall makes sense to me, but to avoid regressions it would be nice if we could update the unit test to catch the edge case that wasn't covered by the existing debounce:

kibana/src/core/server/status/plugins_status.test.ts

Line 365 in 5244019

it('debounces events in quick succession', async () => {

src/core/server/status/plugins_status.ts

TinaHeiligers

With this change the setup to start time (locally) is less than a second:

[2021-08-17T10:54:48.282-07:00][INFO ][plugins-system.standard][]---Setting up [112] plugins: ...

//...setup logs

[2021-08-17T10:54:51.084-07:00][INFO ][plugins-system.standard][]---Starting [112] plugins: ...

One small suggestion is to use the same const for the debounce time in getPluginStatuses and getDerivedStatus$ but other than waiting for green CI, LGTM.
Note: My initial LGTM is pending other ✅

TinaHeiligers · 2021-08-17T17:50:58Z

src/core/server/status/plugins_status.ts

@@ -76,6 +76,7 @@ export class PluginsStatusService {

  public getDerivedStatus$(plugin: PluginName): Observable<ServiceStatus> {
    return this.update$.pipe(
+      debounceTime(25),


We have the same value for debounceTime in getDerivedStatus$ as in getPluginStatuses$ now, which makes sense. Maybe we should consider adding the value as a const and using that in both?

I removed it from getPluginStatuses$. I noticed it's not actually needed there.

joshdover

Seems to work for me. +1 on improving the test coverage for this case.

TinaHeiligers

LGTM pending other ✅

joshdover · 2021-08-17T18:22:32Z

@cachedout this may fix #107300 as well. Let's test it once it's been merged and available in a snapshot.

afharo · 2021-08-17T18:34:24Z

Thank you all for your comments!

Added the test in 1aa2639 (#108952).
I also had to update some previous tests since we now don't return the initial default All dependencies are available anymore thanks to the initial debounce
I also noticed that the debounceTime is only needed in one this.update.pipe. Updated the code for that and added a comment explaining why it's needed there.

lukeelmers

🚀

afharo · 2021-08-17T18:41:11Z

src/core/server/status/plugins_status.test.ts

@@ -247,7 +247,6 @@ describe('PluginStatusService', () => {
      subscription.unsubscribe();

      expect(statusUpdates).toEqual([
-        { a: { level: ServiceStatusLevels.available, summary: 'All dependencies are available' } },


No longer needed since we don't emit the initial transient status anymore.

elasticmachine · 2021-08-17T19:02:04Z

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

cachedout · 2021-08-17T19:07:52Z

this may fix #107300 as well. Let's test it once it's been merged and available in a snapshot.

@joshdover Thanks! FWIW, I tried all day to replicate the slow-start Kibana problem by hand and I still can't get it to happen, but for whatever reason in the CI, we see it regularly. Hopefully this is the fix!

…lastic#108952)

kibanamachine · 2021-08-17T20:50:07Z

💚 Backport successful

Status	Branch	Result
✅	7.x

This backport PR will be merged automatically after passing CI.

kibanamachine · 2021-08-17T21:25:08Z

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

💚 Build #146316 succeeded 1aa2639
💔 Build #146274 failed 89243dc

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

…108952) (#108998) Co-authored-by: Alejandro Fernández Haro <[email protected]>

afharo · 2021-08-18T08:11:49Z

Backporting to 7.14.1 as well: #109052

…108952) (#109052)

kobelb · 2021-09-01T03:38:57Z

Great diagnosis @afharo <3

Add debounce to the status observers to reduce unnecessary CPU loops

89243dc

afharo added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes auto-backport Deprecated - use backport:version if exact versions are needed v7.15.0 v7.14.1 labels Aug 17, 2021

afharo requested a review from a team as a code owner August 17, 2021 17:23

afharo requested a review from joshdover August 17, 2021 17:23

lukeelmers reviewed Aug 17, 2021

View reviewed changes

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

TinaHeiligers approved these changes Aug 17, 2021

View reviewed changes

TinaHeiligers self-requested a review August 17, 2021 18:19

joshdover approved these changes Aug 17, 2021

View reviewed changes

TinaHeiligers reviewed Aug 17, 2021

View reviewed changes

Add tests + debounce only when necessary

1aa2639

lukeelmers approved these changes Aug 17, 2021

View reviewed changes

afharo commented Aug 17, 2021

View reviewed changes

afharo removed the v7.14.1 label Aug 17, 2021

afharo enabled auto-merge (squash) August 17, 2021 18:49

lukeelmers added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Aug 17, 2021

lukeelmers added the v8.0.0 label Aug 17, 2021

afharo merged commit 335393e into elastic:master Aug 17, 2021

afharo deleted the status-observers/add-debounce branch August 17, 2021 20:46

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Aug 17, 2021

Add debounce to the status observers to reduce unnecessary CPU loops (e…

42fbfd7

…lastic#108952)

kibanamachine mentioned this pull request Aug 17, 2021

[7.x] Add debounce to the status observers to reduce unnecessary CPU loops (#108952) #108998

Merged

afharo added the v7.14.1 label Aug 17, 2021

kibanamachine added a commit that referenced this pull request Aug 17, 2021

Add debounce to the status observers to reduce unnecessary CPU loops (#…

62cad9b

…108952) (#108998) Co-authored-by: Alejandro Fernández Haro <[email protected]>

afharo mentioned this pull request Aug 18, 2021

[7.14] Add debounce to the status observers to reduce unnecessary CPU loops (#108952) #109052

Merged

mshustov mentioned this pull request Aug 18, 2021

Add alerting on Core APM metrics #109055

Open

This was referenced Aug 18, 2021

Hitting /api/status successfully takes a very long time after start #107300

Closed

Error while running the ALL stage on CI builds elastic/apm-integration-testing#1188

Closed

afharo added a commit that referenced this pull request Aug 18, 2021

Add debounce to the status observers to reduce unnecessary CPU loops (#…

a07d688

…108952) (#109052)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add debounce to the status observers to reduce unnecessary CPU loops #108952

Add debounce to the status observers to reduce unnecessary CPU loops #108952

afharo commented Aug 17, 2021

lukeelmers left a comment

TinaHeiligers left a comment •

edited

Loading

TinaHeiligers Aug 17, 2021

afharo Aug 17, 2021

joshdover left a comment

TinaHeiligers left a comment

joshdover commented Aug 17, 2021

afharo commented Aug 17, 2021

lukeelmers left a comment

afharo Aug 17, 2021

elasticmachine commented Aug 17, 2021

cachedout commented Aug 17, 2021

kibanamachine commented Aug 17, 2021

kibanamachine commented Aug 17, 2021

afharo commented Aug 18, 2021

kobelb commented Sep 1, 2021

Add debounce to the status observers to reduce unnecessary CPU loops #108952

Add debounce to the status observers to reduce unnecessary CPU loops #108952

Conversation

afharo commented Aug 17, 2021

Summary

For maintainers

lukeelmers left a comment

Choose a reason for hiding this comment

TinaHeiligers left a comment • edited Loading

Choose a reason for hiding this comment

TinaHeiligers Aug 17, 2021

Choose a reason for hiding this comment

afharo Aug 17, 2021

Choose a reason for hiding this comment

joshdover left a comment

Choose a reason for hiding this comment

TinaHeiligers left a comment

Choose a reason for hiding this comment

joshdover commented Aug 17, 2021

afharo commented Aug 17, 2021

lukeelmers left a comment

Choose a reason for hiding this comment

afharo Aug 17, 2021

Choose a reason for hiding this comment

elasticmachine commented Aug 17, 2021

cachedout commented Aug 17, 2021

kibanamachine commented Aug 17, 2021

💚 Backport successful

kibanamachine commented Aug 17, 2021

💚 Build Succeeded

Metrics [docs]

History

afharo commented Aug 18, 2021

kobelb commented Sep 1, 2021

TinaHeiligers left a comment •

edited

Loading