[new-platform] migrate ui/chrome/loading_count API to new platform #21967

spalger · 2018-08-14T19:08:11Z

Part of #20696, required for #20697

This migrates the chrome.loadingCount API to the new platform, which was not planned to happen before #20697 but is required as the UiSettingsClient uses the loading count to activate the global loading indicator in Kibana. This service is pretty simple, it allows adding an observable with core.loadingCount.add(observable) that will be subscribed to in order to contribute to the current "loading count", which can be retrieved with core.loadingCount.get$().

The ui/chrome/api/loading_count module is taking the start contract from the service and re-exposing it via the existing chrome.loadingCount api that we have today, including increment(), decrement(), subscribe(), and the automatic watching of the loading count exposed by the angular $http service.

elasticmachine · 2018-08-14T21:26:09Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-08-15T08:32:59Z

ACK: will try to take a look today or tomorrow evening (CEST) at the latest.

azasypkin

Code looks good to me, just a couple of things. I haven't noticed anything bad during testing except for one weird warning in the browser console that seems to be related to this PR and that we should take care of before we merge this:

azasypkin · 2018-08-15T12:49:28Z

src/ui/public/chrome/api/loading_count.js

-    }
+export function __newPlatformInit__(instance) {
+  if (newPlatformLoadingCount) {
+    throw new Error('ui/chrome/loading_count already initialized with new platform apis');


nit: ui/chrome/loading_count ---> ui/chrome/api/loading_count?

azasypkin · 2018-08-15T13:08:23Z

src/core/public/loading_count/loading_count_service.ts

+      },
+
+      getCount$: () => {
+        return this.count$.asObservable();


question: is there any reason why we don't want to use something like this this.count$.pipe(distinct()) instead? It looks like this will always fire even if delta above is 0.

nope, I think it makes the most sense for this API to only emit the loading count when it changes.

azasypkin · 2018-08-15T13:17:46Z

src/core/public/loading_count/loading_count_service.ts

+            .subscribe({
+              next: ([prev, next]) => {
+                const delta = next - prev;
+                this.count$.next(this.count$.getValue() + delta);


issue: it can easily go to negative numbers, what should do we in this case? Call fatalErrors.add or just log this weird behavior and keep 0 as the minimum count?

UPDATE: just put console.log here and indeed I've seen -1 for this.count$.getValue() + delta a couple of times (with x-pack plugins). Looks like old code didn't have this issue.

Hmm, pretty sure we currently only have two things that are contributing to the loading count: the length of the $http.pendingRequest array (can't ever go below zero) and calls to chrome.loadingCount.increment() and decrement() from here:

kibana/src/ui/ui_settings/public/send_request.js

Line 24 in f0cc432

chrome.loadingCount.increment();

Based on that I can't imagine how we are getting negative loading counts...

Can you try running in the debugger and tracing back up the call stack to see what is causing the loading count to go negative?

Can't reproduce it with the latest changes, so it's all good now! :)

azasypkin · 2018-08-15T13:24:22Z

src/core/public/loading_count/loading_count_service.ts

+      subscription.unsubscribe();
+    }
+
+    this.subscriptions.length = 0;


question: should we this.count$.next(0) and this.count$.complete() here and restart in start (and simple test for this if you agree)? Otherwise this.count$ and its subscribers will stay in some "undefined" state.

azasypkin · 2018-08-15T13:27:38Z

src/core/public/loading_count/loading_count_service.test.ts

+
+  const startContract = service.start({
+    fatalErrors: {
+      add: jest.fn(),


optional nit: maybe add a simple test for error case when we call fatalErrors.add?

…latform/loading-count

…nd cleanup better

spalger · 2018-08-16T01:03:11Z

@azasypkin tracked down that error, it was caused by digest cycles that get triggered when react components use methods like getBasePath() which are passed down to react components as props. ngReact automatically wraps these functions in calls to $scope.$apply() to make sure that angular sees any updates, which was causing our watcher to emit the number of active angular http requests (almost always zero). This is now prevented by using distinctUntilChanged(), but will still be triggered if someone executes a web request in a render for some reason 👍

elasticmachine · 2018-08-16T02:43:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

azasypkin · 2018-08-16T07:23:12Z

@azasypkin tracked down that error, it was caused by digest cycles that get triggered when react components use methods like getBasePath() which are passed down to react components as props. ngReact automatically wraps these functions in calls to $scope.$apply() to make sure that angular sees any updates, which was causing our watcher to emit the number of active angular http requests (almost always zero). This is now prevented by using distinctUntilChanged(), but will still be triggered if someone executes a web request in a render for some reason 👍

Huh, nice find!

azasypkin

LGTM! Tested locally one more time and haven't noticed anything suspicious.

azasypkin · 2018-08-16T07:01:59Z

src/core/public/loading_count/loading_count_service.ts

+            tap(count => {
+              if (count < 0) {
+                throw new Error(
+                  'Observables passed to loadingCount.add() must only emit possitive numbers'


typo: possitive ---> positive.

Spelling is not my strong suit. Thank you 🙏

…latform/loading-count

elasticmachine · 2018-08-16T17:37:16Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

spalger · 2018-08-17T16:19:23Z

Retest

elasticmachine · 2018-08-17T17:59:40Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…lastic#21967) Part of elastic#20696, required for elastic#20697 This migrates the `chrome.loadingCount` API to the new platform, which was not planned to happen before elastic#20697 but is required as the UiSettingsClient uses the loading count to activate the global loading indicator in Kibana. This service is pretty simple, it allows adding an observable with `core.loadingCount.add(observable)` that will be subscribed to in order to contribute to the current "loading count", which can be retrieved with `core.loadingCount.get$()`. The `ui/chrome/api/loading_count` module is taking the start contract from the service and re-exposing it via the existing `chrome.loadingCount` api that we have today, including `increment()`, `decrement()`, `subscribe()`, and the automatic watching of the loading count exposed by the angular `$http` service.

…orm (#21967) (#22156) Backports the following commits to 6.x: - [new-platform] migrate ui/chrome/loading_count API to new platform (#21967)

spalger · 2018-08-18T05:01:08Z

6.x/6.5: fad921d

[new-platform] migrate ui/chrome/loading_count API to new platform

57b9aa8

spalger added review Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Feature:New Platform v7.0.0 v6.5.0 labels Aug 14, 2018

spalger requested a review from azasypkin August 14, 2018 19:08

azasypkin requested changes Aug 15, 2018

View reviewed changes

spalger added 3 commits August 15, 2018 11:46

Merge branch 'master' of github.com:elastic/kibana into migrate-new-p…

e673c5c

…latform/loading-count

fix typo

b4d5426

[core/loadingCount] tweak count handling to dedupe, zero-out count, a…

237f497

…nd cleanup better

spalger force-pushed the migrate-new-platform/loading-count branch from 51faa92 to 237f497 Compare August 16, 2018 01:02

spalger requested a review from azasypkin August 16, 2018 01:45

azasypkin approved these changes Aug 16, 2018

View reviewed changes

spalger added 2 commits August 16, 2018 09:20

Merge branch 'master' of github.com:elastic/kibana into migrate-new-p…

0645aa4

…latform/loading-count

fix typo

a39cd1e

spalger merged commit 191ea1f into elastic:master Aug 17, 2018

spalger mentioned this pull request Aug 17, 2018

[6.x] [new-platform] migrate ui/chrome/loading_count API to new platform (#21967) #22156

Merged

spalger deleted the migrate-new-platform/loading-count branch August 18, 2018 05:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[new-platform] migrate ui/chrome/loading_count API to new platform #21967

[new-platform] migrate ui/chrome/loading_count API to new platform #21967

spalger commented Aug 14, 2018

elasticmachine commented Aug 14, 2018

azasypkin commented Aug 15, 2018

azasypkin left a comment

azasypkin Aug 15, 2018

azasypkin Aug 15, 2018

spalger Aug 15, 2018

azasypkin Aug 15, 2018

azasypkin Aug 15, 2018 •

edited

Loading

spalger Aug 15, 2018 •

edited

Loading

spalger Aug 15, 2018

azasypkin Aug 16, 2018

azasypkin Aug 15, 2018

azasypkin Aug 15, 2018

spalger commented Aug 16, 2018

elasticmachine commented Aug 16, 2018

azasypkin commented Aug 16, 2018

azasypkin left a comment

azasypkin Aug 16, 2018

spalger Aug 16, 2018

elasticmachine commented Aug 16, 2018

spalger commented Aug 17, 2018

elasticmachine commented Aug 17, 2018

spalger commented Aug 18, 2018

[new-platform] migrate ui/chrome/loading_count API to new platform #21967

[new-platform] migrate ui/chrome/loading_count API to new platform #21967

Conversation

spalger commented Aug 14, 2018

elasticmachine commented Aug 14, 2018

💚 Build Succeeded

azasypkin commented Aug 15, 2018

azasypkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

spalger Aug 15, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spalger commented Aug 16, 2018

elasticmachine commented Aug 16, 2018

💚 Build Succeeded

azasypkin commented Aug 16, 2018

azasypkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Aug 16, 2018

💔 Build Failed

spalger commented Aug 17, 2018

elasticmachine commented Aug 17, 2018

💚 Build Succeeded

spalger commented Aug 18, 2018

azasypkin Aug 15, 2018 •

edited

Loading

spalger Aug 15, 2018 •

edited

Loading