Expressions service fixes: better error and loading states handling #51183

Dosant · 2019-11-20T16:23:50Z

Summary

1. This pr fixes regression in v7.6 - #51153.

Visualisation which are using ExpressionLoader directly are swallowing render errors in 7.6, because generic error handling is happening only on expression_renderer.tsx level (react component wrapper around renderer).

Now react agnostic render code render.ts, loader.ts:

has it's own default error handler which is toast from notification service.
It also allows to pass custom error handler instead of default one

React expression renderer expression_renderer.tsx:

if consumer wants to render error as react component inline, then the component uses custom error handler from render.ts to wire up react component.

The pr also adds unsubscribes from ExpressionLoader observable in expression_renderer.tsx

2. This pr fixes issue of `loader.ts` where initial loading$ was not emitted

Had to change loadingSubject from Subject to BehaviorSubject to remember the last value

3. This pr fixes dependencies in effects of `expression_renderer.tsx`

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

~~[ ] This was checked for cross-browser compatibility, including a check against IE11~~
~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
~~[ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately
This includes a feature addition or change that requires a release note and was labeled appropriately

elasticmachine · 2019-11-20T18:31:17Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: f7730c39e62ee6c99ecacf190a732c99a0bc915b

elasticmachine · 2019-11-21T10:20:10Z

Pinging @elastic/kibana-app-arch (Team:AppArch)

elasticmachine · 2019-11-21T11:46:45Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 88a753a

elasticmachine · 2019-11-21T13:08:26Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 4cf0fa9

elasticmachine · 2019-11-21T15:23:06Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: c15646e

elasticmachine · 2019-11-21T17:11:55Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: cb88913

prettier remove error$ and errorRenderer, use onRenderError callback instead for both use custom error handler only if it is provided by consumer, otherwise fallback to default handler add comment fix linter issues fix

move utilities to kibana_react/util

Dosant · 2019-11-22T14:07:45Z

src/plugins/expressions/public/expression_renderer.tsx

-        renderError ? (
-          renderError(state.error.message)
-        ) : (
-          <div data-test-subj="expression-renderer-error">{state.error.message}</div>


Default error state now fallbacks to default of render.ts which is toast from notification service

If user of this react component provided renderErrorfunction, then the component itself will handle the error and will render the error component returned from renderError()

elasticmachine · 2019-11-22T15:22:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 21d8c9c

Dosant · 2019-11-25T07:48:41Z

@elasticmachine merge upstream

…errors

ppisljar · 2019-11-25T07:57:40Z

src/plugins/expressions/public/loader.ts

@@ -64,9 +72,14 @@ export class ExpressionLoader {
      this.render(data);
    });

+    this.render$.subscribe(() => {
+      this.loadingSubject.next(false);


are we emitting false now after loading has completed ?

No, I left the public observable the same,
This is done due to:

this.loading$ = this.loadingSubject.asObservable().pipe( filter(_ => _ === true), map(() => void 0) );

ppisljar · 2019-11-25T07:59:48Z

src/plugins/expressions/public/loader.ts

+    // as loading$ could emit straight away in the constructor
+    // and we want to notify subscribers about it, but all subscriptions will happen later
+    this.loading$ = this.loadingSubject.asObservable().pipe(
+      filter(_ => _ === true),


can you please explain why we need boolean observable, where we then filter out all the falses and map trues to void ?

The intention was to leave public api (loading$) unchanged in this pr and just fix the bug where expression_renderer.tsx was missing initial loading state. This was fixed by changing Subject to BehaviourSubject to remember last emitted value

ppisljar

LGTM

elasticmachine · 2019-11-25T09:11:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ef46757

wylieconlon

This looks like it's changing the default behavior of the error handler when the expression fails to render. Try going to Lens and configuring only a Y axis.

wylieconlon · 2019-11-25T19:59:34Z

src/plugins/expressions/public/expression_renderer.test.tsx

-    expect(instance.find(EuiProgress)).toHaveLength(0);
-  });
-
-  it('should display an error message when the expression fails', () => {


You've changed the behavior this test was testing- why?

Default error handling of expression_renderer.tsx now fallbacks to default error handling of render.ts. This was done primarily to reduce number of code paths to support.

Pls see: https://github.com/elastic/kibana/pull/51183/files#r349611820

And that is why test was deleted as we have less code paths in this component.

This looks like it's changing the default behavior of the error handler when the expression fails to render. Try going to Lens and configuring only a Y axis.

I guess I indeed missing the change, where I had to explicitly override error handling, where previous default of expression_renderer.tsx was expected.
I found one missing place here: x-pack/legacy/plugins/lens/public/editor_frame_plugin/embeddable/expression_wrapper.tsx and added error override, but as I understand this is for dashboard page?
9a3282f
Any other places I am missing?

Try going to Lens and configuring only a Y axis.

I am likely doing it wrong, but I think I am getting the same behaviour as in master. Blank screen. Any suggestions?

…sion-service-render-swallows-errors

…thub.com:Dosant/kibana into bugfix/expression-service-render-swallows-errors

elasticmachine · 2019-11-26T12:45:49Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4351781

…sion-service-render-swallows-errors # Conflicts: # src/plugins/expressions/public/render.ts

elasticmachine · 2019-11-26T14:45:47Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: 1a76b6f

elasticmachine · 2019-11-26T15:52:07Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request
Commit: cca211e

wylieconlon

I was wrong about the state of master, this is not changing the behavior. LGTM

elasticmachine · 2019-11-26T16:37:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 4b730df

Dosant · 2019-11-27T08:05:48Z

@elasticmachine merge upstream

…errors

elasticmachine · 2019-11-27T09:20:20Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: b37a36f

…lastic#51183) 1. This pr fixes regression in v7.6 - elastic#51153. Visualisation which are using ExpressionLoader directly are swallowing render errors in 7.6, because generic error handling is happening only on expression_renderer.tsx level (react component wrapper around renderer). Now react agnostic render code render.ts, loader.ts: has it's own default error handler which is toast from notification service. It also allows to pass custom error handler instead of default one React expression renderer expression_renderer.tsx: if consumer wants to render error as react component inline, then the component uses custom error handler from render.ts to wire up react component. 2. This pr fixes issue of loader.ts where initial loading$ was not emitted Had to change loadingSubject from Subject to BehaviorSubject to remember the last value 3. This pr fixes dependencies in effects of expression_renderer.tsx # Conflicts: # src/plugins/expressions/public/loader.test.ts

…51183) (#51800) 1. This pr fixes regression in v7.6 - #51153. Visualisation which are using ExpressionLoader directly are swallowing render errors in 7.6, because generic error handling is happening only on expression_renderer.tsx level (react component wrapper around renderer). Now react agnostic render code render.ts, loader.ts: has it's own default error handler which is toast from notification service. It also allows to pass custom error handler instead of default one React expression renderer expression_renderer.tsx: if consumer wants to render error as react component inline, then the component uses custom error handler from render.ts to wire up react component. 2. This pr fixes issue of loader.ts where initial loading$ was not emitted Had to change loadingSubject from Subject to BehaviorSubject to remember the last value 3. This pr fixes dependencies in effects of expression_renderer.tsx # Conflicts: # src/plugins/expressions/public/loader.test.ts

* upstream/7.x: Fix infinite redirect loop when multiple cookies are sent (elastic#50452) (elastic#51821) [Console] Proxy fallback (elastic#50185) (elastic#51814) Added endgame-* index and new heading 3 Elastic Endpoint SMP. (elastic#51071) (elastic#51828) [Maps] Added options to disable zoom, hide tool tips, widgets/overlays in embeddable maps (elastic#50663) (elastic#51811) Move errors and validate index pattern ⇒ NP (elastic#51805) (elastic#51831) [SIEM][Detection Engine] Adds ecs threat properties to rules (elastic#51782) (elastic#51827) [Lens] Remove client-side reference to server source code (elastic#51763) (elastic#51825) fixes drag and drop in tests (elastic#51806) (elastic#51813) [Uptime] Redesign/44541 new monitor list expanded row (elastic#46567) (elastic#51809) [7.x] [Telemetry] collector set to np (elastic#51618) (elastic#51787) [Uptime] added test for chart wrapper (elastic#50399) (elastic#51808) Expressions service fixes: better error and loading states handling (elastic#51183) (elastic#51800) Query String(Bar) Input - cleanup (elastic#51598) (elastic#51804) [ML] Adjust and re-enable categorization advanced wizard test (elastic#51005) (elastic#51017) fixes url state tests (elastic#51746) (elastic#51798) fixes browser field tests (elastic#51738) (elastic#51799) [Task Manager] Tests for the ability to run tasks of varying durations in parallel (elastic#51572) (elastic#51701) [ML] Fix anomaly detection test suite (elastic#51712) (elastic#51795) [SIEM] Fix Timeline drag and drop behavior (elastic#51558) (elastic#51793)

…ra/kibana into IS-46410_remove-@kbn/ui-framework * 'IS-46410_remove-@kbn/ui-framework' of github.com:mbondyra/kibana: (49 commits) [ML] Re-activate after method in transform test (elastic#51815) [SIEM] [Detection Engine] Add edit on rule creation (elastic#51670) De-angularize visLegend (elastic#50613) [SIEM][Detection Engine] Change security model to use SIEM permissions [Monitoring] Sass cleanup (elastic#51100) Move errors and validate index pattern ⇒ NP (elastic#51805) fixes pagination tests (elastic#51822) Split legacy plugin discovery, expose SavedObjects scopedClient, wrappers, repository (elastic#48882) [SIEM][Detection Engine] Adds ecs threat properties to rules (elastic#51782) [Lens] Remove client-side reference to server source code (elastic#51763) Fix infinite redirect loop when multiple cookies are sent (elastic#50452) fixes drag and drop in tests (elastic#51806) [Console] Proxy fallback (elastic#50185) Query String(Bar) Input - cleanup (elastic#51598) shim visualizations plugin (elastic#50624) Expressions service fixes: better error and loading states handling (elastic#51183) fixes url state tests (elastic#51746) fixes browser field tests (elastic#51738) [ML] Fix anomaly detection test suite (elastic#51712) [SIEM] Fix Timeline drag and drop behavior (elastic#51558) ...

Dosant force-pushed the bugfix/expression-service-render-swallows-errors branch from f7730c3 to 88a753a Compare November 21, 2019 10:18

Dosant added bug Fixes for quality problems that affect the customer experience Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:AppArch v7.6.0 v8.0.0 labels Nov 21, 2019

Dosant marked this pull request as ready for review November 21, 2019 10:22

Dosant requested a review from a team as a code owner November 21, 2019 10:22

Dosant added the review label Nov 21, 2019

Dosant requested review from wylieconlon and ppisljar November 21, 2019 10:22

Dosant added the release_note:skip Skip the PR/issue when compiling release notes label Nov 21, 2019

Dosant added 2 commits November 22, 2019 15:02

fix expression service missing initial loading

21d8c9c

move utilities to kibana_react/util

Dosant force-pushed the bugfix/expression-service-render-swallows-errors branch from d814bd0 to 21d8c9c Compare November 22, 2019 14:03

Dosant commented Nov 22, 2019

View reviewed changes

Dosant changed the title ~~fix expressions service render swallows rendering errors #51153~~ Expressions service fixes: better error and loading states handling Nov 22, 2019

Merge branch 'master' into bugfix/expression-service-render-swallows-…

ef46757

…errors

ppisljar reviewed Nov 25, 2019

View reviewed changes

ppisljar approved these changes Nov 25, 2019

View reviewed changes

wylieconlon suggested changes Nov 25, 2019

View reviewed changes

Dosant added 3 commits November 26, 2019 10:10

Merge branch 'master' of github.com:elastic/kibana into bugfix/expres…

33f16d4

…sion-service-render-swallows-errors

Merge branch 'bugfix/expression-service-render-swallows-errors' of gi…

a9003a3

…thub.com:Dosant/kibana into bugfix/expression-service-render-swallows-errors

Render error inline for lens emedbale as it was previosly

9a3282f

Dosant requested a review from a team November 26, 2019 11:19

Add data-test-subj

4351781

Dosant added 2 commits November 26, 2019 14:35

Merge branch 'master' of github.com:elastic/kibana into bugfix/expres…

1a76b6f

…sion-service-render-swallows-errors # Conflicts: # src/plugins/expressions/public/render.ts

fix ts errors - change RenderResult to RenderId

cca211e

fix types

4b730df

wylieconlon approved these changes Nov 26, 2019

View reviewed changes

Merge branch 'master' into bugfix/expression-service-render-swallows-…

b37a36f

…errors

Dosant added the v7.7.0 label Nov 27, 2019

Dosant merged commit 0fd63ee into elastic:master Nov 27, 2019

Dosant deleted the bugfix/expression-service-render-swallows-errors branch November 27, 2019 09:55

Dosant mentioned this pull request Nov 27, 2019

[7.x] Expressions service fixes: better error and loading states handling (#51183) #51800

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expressions service fixes: better error and loading states handling #51183

Expressions service fixes: better error and loading states handling #51183

Dosant commented Nov 20, 2019 •

edited

Loading

elasticmachine commented Nov 20, 2019

elasticmachine commented Nov 21, 2019

elasticmachine commented Nov 21, 2019

elasticmachine commented Nov 21, 2019

elasticmachine commented Nov 21, 2019

elasticmachine commented Nov 21, 2019

Dosant Nov 22, 2019 •

edited

Loading

elasticmachine commented Nov 22, 2019

Dosant commented Nov 25, 2019

ppisljar Nov 25, 2019

Dosant Nov 25, 2019

ppisljar Nov 25, 2019

Dosant Nov 25, 2019

ppisljar left a comment

elasticmachine commented Nov 25, 2019

wylieconlon left a comment

wylieconlon Nov 25, 2019

Dosant Nov 26, 2019 •

edited

Loading

elasticmachine commented Nov 26, 2019

elasticmachine commented Nov 26, 2019

elasticmachine commented Nov 26, 2019

wylieconlon left a comment

elasticmachine commented Nov 26, 2019

Dosant commented Nov 27, 2019

elasticmachine commented Nov 27, 2019

Expressions service fixes: better error and loading states handling #51183

Expressions service fixes: better error and loading states handling #51183

Conversation

Dosant commented Nov 20, 2019 • edited Loading

Summary

1. This pr fixes regression in v7.6 - #51153.

2. This pr fixes issue of loader.ts where initial loading$ was not emitted

3. This pr fixes dependencies in effects of expression_renderer.tsx

Checklist

For maintainers

elasticmachine commented Nov 20, 2019

💔 Build Failed

elasticmachine commented Nov 21, 2019

elasticmachine commented Nov 21, 2019

💔 Build Failed

elasticmachine commented Nov 21, 2019

💔 Build Failed

elasticmachine commented Nov 21, 2019

💔 Build Failed

elasticmachine commented Nov 21, 2019

💚 Build Succeeded

Dosant Nov 22, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Nov 22, 2019

💚 Build Succeeded

Dosant commented Nov 25, 2019

ppisljar Nov 25, 2019

Choose a reason for hiding this comment

Dosant Nov 25, 2019

Choose a reason for hiding this comment

ppisljar Nov 25, 2019

Choose a reason for hiding this comment

Dosant Nov 25, 2019

Choose a reason for hiding this comment

ppisljar left a comment

Choose a reason for hiding this comment

elasticmachine commented Nov 25, 2019

💚 Build Succeeded

wylieconlon left a comment

Choose a reason for hiding this comment

wylieconlon Nov 25, 2019

Choose a reason for hiding this comment

Dosant Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Nov 26, 2019

💚 Build Succeeded

elasticmachine commented Nov 26, 2019

💔 Build Failed

elasticmachine commented Nov 26, 2019

💔 Build Failed

wylieconlon left a comment

Choose a reason for hiding this comment

elasticmachine commented Nov 26, 2019

💚 Build Succeeded

Dosant commented Nov 27, 2019

elasticmachine commented Nov 27, 2019

💚 Build Succeeded

Dosant commented Nov 20, 2019 •

edited

Loading

2. This pr fixes issue of `loader.ts` where initial loading$ was not emitted

3. This pr fixes dependencies in effects of `expression_renderer.tsx`

Dosant Nov 22, 2019 •

edited

Loading

Dosant Nov 26, 2019 •

edited

Loading