[Lens][Visualize][Embeddable] Optimize Lens embeddables on error #144015

dej611 · 2022-10-26T10:53:53Z

Summary

In this PR there are some embeddable rendering optimizations for the error case.
Lens handles error states internally without passing to default EmbeddablePanel flows - this has been made explicit in the Lens Embeddable component. The embeddable was triggering multiple time the render method in both successful and failure case: while the expression component renderer had some debouncing/memoize optimization to reduce the effective rerenders, the error route had none leading to a total of 3 rerenderings (loading + some changes detected).

In this PR I've managed to reduce the rerenders to 2 (loading + final state) with some gatekeeping on the internal state reporting.
As for the data-rendering-complete the logic has changed to set it to true also in Error state, but I would collect some feedback from @elastic/kibana-app-services on this side to better understand the impact. Before this change the data-error attribute could be checked to detect the final state of the panel, but that introduces some problems for Lens panels for the error reporting, hence the need to have the complete flag changed.

Checklist

Delete any items that are not applicable to this PR.

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
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)
If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)
This was checked for cross-browser compatibility

Risk Matrix

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:

Risk	Probability	Severity	Mitigation/Notes
Multiple Spaces—unexpected behavior in non-default Kibana Space.	Low	High	Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces.
Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks.	High	Low	Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure.
Code should gracefully handle cases when feature X or plugin Y are disabled.	Medium	High	Unit tests will verify that any feature flag or plugin combination still results in our service operational.
See more potential risk examples

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2022-10-27T10:05:58Z

Pinging @elastic/kibana-vis-editors @elastic/kibana-vis-editors-external (Team:VisEditors)

alexwizp · 2022-10-28T09:20:18Z

src/plugins/kibana_utils/public/render_complete/render_complete_dispatcher.ts

@@ -61,7 +63,7 @@ export class RenderCompleteDispatcher {
  public dispatchError() {
    if (!this.el) return;
    this.count++;
-    this.el.setAttribute('data-render-complete', 'false');
+    this.el.setAttribute('data-render-complete', 'true');


@vadimkibana for followup work, we could accept an optional Error parameter, and use it to update the contents of a data-render-error attribute with a message. WDYT?

alexwizp · 2022-10-28T09:49:38Z

x-pack/plugins/lens/public/embeddable/expression_wrapper.tsx

+  searchSessionId,
+}: VisualizationErrorProps) {
+  // use a combination of sessionid + error messages to decide whether to trigger a rerender
+  const rerenderKey = `${searchSessionId || ''}-${errors


not sure that I fully understand why we need in include searchSessionId into key. The session id is the same for all panels inside e.g. dashboard.

What if we exclude searchSessionId from the key, and do:

if (rerenderKey && keyRef.current !== rerenderKey) { keyRef.current = rerenderKey; onExpressionError(); }

Using the searchSessionId as part of the key will give the user the ability to try again to load the panel by clicking the Refresh button. Otherwise the panel state will be not be updated.

alexwizp · 2022-10-28T09:51:21Z

x-pack/plugins/lens/public/embeddable/expression_wrapper.tsx

+    .join('-')}`;
+
+  const keyRef = useRef(rerenderKey);
+  // Skip error logging when no search session id is passed


I don't think this comment is relevant.

// Skip error logging when no search session id is passed

alexwizp

Added some questions but all works fine. Thank you

drewdaemon · 2022-10-28T19:09:21Z

@dej611 I just wanted to make sure you're aware of #143367 which changes the way the Lens embeddable reports errors.

dej611 · 2022-11-02T11:54:19Z

@dej611 I just wanted to make sure you're aware of #143367 which changes the way the Lens embeddable reports errors.

Good spot. There are definitely some overlaps between the two, which needs to be coordinated. Will check that PR and see whether it makes sense to wait for it, in case the new error rendering approach solves part of the issue here.

flash1293 · 2022-11-03T13:52:39Z

Is it expected that data-loading is still set to true? Not sure whether it makes any difference:

VladLasitsa · 2022-11-15T13:49:53Z

@elastic/kibana-visualizations , @elastic/kibana-app-services, could you please review?

VladLasitsa · 2022-11-17T11:17:10Z

@elasticmachine merge upstream

kibana-ci · 2022-11-17T12:20:29Z

💚 Build Succeeded

Buildkite Build
Commit: 735cea5

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`lens`	1.3MB	1.3MB	-6.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`kibanaUtils`	68.8KB	68.9KB	+59.0B

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	59	65	+6
`osquery`	108	113	+5
`securitySolution`	441	447	+6
total			+19

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	67	73	+6
`osquery`	109	115	+6
`securitySolution`	518	524	+6
total			+20

History

💚 Build #87989 succeeded cad6f55
💚 Build #83105 succeeded 30b1e9f
💚 Build #82768 succeeded cae7625

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

mbondyra

LGTM 🆗

tsullivan · 2022-11-17T18:27:01Z

As for the data-rendering-complete the logic has changed to set it to true also in Error state, but I would collect some feedback from @elastic/kibana-app-services on this side to better understand the impact. Before this change the data-error attribute could be checked to detect the final state of the panel, but that introduces some problems for Lens panels for the error reporting, hence the need to have the complete flag changed.

I am ++ to setting the "render complete" attribute value to true in the error case. This will help it not block Reporting. I wonder if there could be an unwanted impact to performance monitoring. cc @lizozom

Also, as I was reviewing this, I noticed that the Render Complete Dispatcher doesn't handle the placement of a data-render-error attribute in the error case, which is expected by Reporting. I'm filing a separate issue for that: the render complete dispatcher should automatically handle things that favor Reporting and performance monitoring. cc @vadimkibana

tsullivan

LGTM

…stic#144015) ## Summary Fixes elastic#143552 In this PR there are some embeddable rendering optimizations for the error case. Lens handles error states internally without passing to default `EmbeddablePanel` flows - this has been made explicit in the Lens `Embeddable` component. The embeddable was triggering multiple time the `render` method in both successful and failure case: while the expression component renderer had some debouncing/memoize optimization to reduce the effective rerenders, the error route had none leading to a total of 3 rerenderings (loading + some changes detected). In this PR I've managed to reduce the rerenders to 2 (loading + final state) with some gatekeeping on the internal state reporting. As for the `data-rendering-complete` the logic has changed to set it to `true` also in Error state, but I would collect some feedback from @elastic/kibana-app-services on this side to better understand the impact. Before this change the `data-error` attribute could be checked to detect the final state of the panel, but that introduces some problems for Lens panels for the error reporting, hence the need to have the `complete` flag changed. ### Checklist Delete any items that are not applicable to this PR. - [ ] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [ ] If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker) - [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) ### Risk Matrix Delete this section if it is not applicable to this PR. Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release. When forming the risk matrix, consider some of the following examples and how they may potentially impact the change: | Risk | Probability | Severity | Mitigation/Notes | |---------------------------|-------------|----------|-------------------------| | Multiple Spaces—unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. | | Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. | | Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. | | [See more potential risk examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx) | ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) Co-authored-by: Uladzislau Lasitsa <[email protected]> Co-authored-by: Kibana Machine <[email protected]> (cherry picked from commit 87c7b39)

kibanamachine · 2022-11-18T09:00:56Z

💚 All backports created successfully

Status	Branch	Result
✅	8.6

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

#144015) (#145695) # Backport This will backport the following commits from `main` to `8.6`: - [[Lens][Visualize][Embeddable] Optimize Lens embeddables on error (#144015)](#144015)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Marco Liberati <[email protected]>

dej611 added 2 commits October 26, 2022 12:43

🐛 Reduce rerendering on expression error

512859c

🔧 Set complete also on error

cae7625

dej611 added Team:Visualizations Visualization editors, elastic-charts and infrastructure Team:AppServicesSv release_note:skip Skip the PR/issue when compiling release notes Feature:Lens labels Oct 26, 2022

dej611 requested a review from alexwizp October 26, 2022 10:54

dej611 marked this pull request as ready for review October 27, 2022 10:05

dej611 requested review from a team as code owners October 27, 2022 10:05

Merge branch 'main' into fix/143552

30b1e9f

alexwizp reviewed Oct 28, 2022

View reviewed changes

alexwizp approved these changes Oct 28, 2022

View reviewed changes

VladLasitsa added 2 commits November 15, 2022 13:23

Merge remote-tracking branch 'upstream/main' into fix/143552

2463d6a

Some updates

cad6f55

Merge branch 'main' into fix/143552

735cea5

mbondyra approved these changes Nov 17, 2022

View reviewed changes

stratoula added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) v8.6.0 v8.7.0 labels Nov 17, 2022

tsullivan approved these changes Nov 17, 2022

View reviewed changes

tsullivan mentioned this pull request Nov 18, 2022

[Reporting/Maps] reporting does not read errors from maps embeddable. #140780

Open

VladLasitsa merged commit 87c7b39 into elastic:main Nov 18, 2022

kibanamachine mentioned this pull request Nov 18, 2022

[8.6] [Lens][Visualize][Embeddable] Optimize Lens embeddables on error (#144015) #145695

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Lens][Visualize][Embeddable] Optimize Lens embeddables on error #144015

[Lens][Visualize][Embeddable] Optimize Lens embeddables on error #144015

dej611 commented Oct 26, 2022 •

edited by kibanamachine

Loading

elasticmachine commented Oct 27, 2022

alexwizp Oct 28, 2022

tsullivan Nov 17, 2022

alexwizp Oct 28, 2022

alexwizp Oct 28, 2022

dej611 Nov 2, 2022

alexwizp Oct 28, 2022

alexwizp left a comment

drewdaemon commented Oct 28, 2022

dej611 commented Nov 2, 2022

flash1293 commented Nov 3, 2022

VladLasitsa commented Nov 15, 2022

VladLasitsa commented Nov 17, 2022

kibana-ci commented Nov 17, 2022

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

mbondyra left a comment

tsullivan commented Nov 17, 2022 •

edited

Loading

tsullivan left a comment

kibanamachine commented Nov 18, 2022

[Lens][Visualize][Embeddable] Optimize Lens embeddables on error #144015

[Lens][Visualize][Embeddable] Optimize Lens embeddables on error #144015

Conversation

dej611 commented Oct 26, 2022 • edited by kibanamachine Loading

Summary

Checklist

Risk Matrix

For maintainers

elasticmachine commented Oct 27, 2022

alexwizp Oct 28, 2022

Choose a reason for hiding this comment

tsullivan Nov 17, 2022

Choose a reason for hiding this comment

alexwizp Oct 28, 2022

Choose a reason for hiding this comment

alexwizp Oct 28, 2022

Choose a reason for hiding this comment

dej611 Nov 2, 2022

Choose a reason for hiding this comment

alexwizp Oct 28, 2022

Choose a reason for hiding this comment

alexwizp left a comment

Choose a reason for hiding this comment

drewdaemon commented Oct 28, 2022

dej611 commented Nov 2, 2022

flash1293 commented Nov 3, 2022

VladLasitsa commented Nov 15, 2022

VladLasitsa commented Nov 17, 2022

kibana-ci commented Nov 17, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

Page load bundle

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

mbondyra left a comment

Choose a reason for hiding this comment

tsullivan commented Nov 17, 2022 • edited Loading

tsullivan left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 18, 2022

💚 All backports created successfully

Questions ?

dej611 commented Oct 26, 2022 •

edited by kibanamachine

Loading

tsullivan commented Nov 17, 2022 •

edited

Loading