[Search] Fix dashboard embeddables don't refetch on searchSessionId change #84261

Dosant · 2020-11-24T17:53:48Z

Summary

Fixes a bug on a dashboard with session indicator, that, for example, when switching from view to edit, session indicator disappears:

Now in master, on any dashboard container state change new search session is started, but inner embeddables "dirty" state checking don't check for sessionId changes, so embeddable don't refetch the data.
Because there is new session and no searches are fired, session indicator gets into "empty" state and is not shown.

This pr makes search, visualize and lens embeddables (only those support searchSessionId for now) to check if searchSessionId is changed and if it is, then do refetch.

To not trigger data refetch on any dashboard container state change, there is also a new optimization inside dashboard to not create a new session on every state change: https://github.com/elastic/kibana/pull/84261/files#diff-d4d5bed484b46a4b7df6cd2d09b106bd761c956c53df96f8da6e683901475156R636

In an original pr which added searchSessionId I initially triggered refetch on searchSessionId change, but reverted because of this edge case: #81297 (comment)
Basically, when refetch is triggered on searchSesssionId hitting refresh caused double fetches for each embeddable:
When refresh is hit lastReloadRequestTime and searchSesssionId were updated, then onResetInput updated embeddable input and then called force reload: https://github.com/elastic/kibana/blob/master/src/plugins/embeddable/public/lib/embeddables/embeddable.tsx#L206
It caused embeddable to fetch twice: once because of updated input and searchSesssionId change and then because of force reload
To workaround this (not very happy with the workaround, open for better ideas) I introduced getUpdated$ observable https://github.com/elastic/kibana/pull/84261/files#diff-07c6d43a257e24b0d89b752078b298591b28d74047ddc44a38f35d130958a25dR126 which is different from merge(getInput$(), getOutput$()) that is

debounces input/output updates allowing to react only once if input/output changes synchronously (not really relevant for this workaround, but this is a nice nit optimization)
delays emit to next macrotask.
This observable allows for embeddables to first react to force reload with the latest input/output and later when getUpdated$() is emitted embeddable dirty state checking will prevent running fetch again.

How to test

xpack.data_enhanced.search.sendToBackground.enabled: true
data.search.aggs.shardDelay.enabled: true # helpful to create "slow" visualizations

Background session indicator shouldn't disappear when switching from dashboard view modes or doing other non-search result relevant change
Embeddable shouldn't try to refetch data twice on state changes or when hitting refresh

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

# Conflicts: # src/plugins/discover/public/application/embeddable/search_embeddable.ts

Dosant · 2020-12-02T11:46:08Z

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

@@ -245,16 +253,9 @@ export class Embeddable
  };

  async reload() {
-    const currentTime = Date.now();
-    if (this.externalSearchContext.lastReloadRequestTime !== currentTime) {


I am either missing something or I think this check didn't make sense, because I'd expect Date.now() on line above to always return later timestamp that is saved inside lastReloadRequestTime.

I decided to clean this up, pls let me know if this was actually handling some edge case

I think it was put in place to protect from multiple synchronous reload calls, but I didn't find a case where that's a problem. It's fine from my side.

…shboard-start-session-fix

elasticmachine · 2020-12-02T12:37:35Z

Pinging @elastic/kibana-app-services (Team:AppServices)

Dosant · 2020-12-03T10:02:07Z

@elasticmachine merge upstream

…shboard-start-session-fix

…osant/kibana into dev/search/dashboard-start-session-fix

ThomThomson

I personally don't have a problem at all with the workaround of adding a getUpdated$ observable. Anything that helps standardize how embeddables respond to changes is good in my books.

When testing locally, I verified embeddables only fire one network request on refresh, filter change & time range change. The background sessions indicator is also always present now.

One thing I did notice is that the dashboard is reloaded now on 'cancel' when you choose to discard changes. This wasn't the case before, as nothing is changing that would require a re-fetch. Is this intended?

flash1293

Tested the Lens part and seems to work fine

flash1293 · 2020-12-08T10:35:22Z

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

@@ -245,16 +253,9 @@ export class Embeddable
  };

  async reload() {
-    const currentTime = Date.now();
-    if (this.externalSearchContext.lastReloadRequestTime !== currentTime) {


I think it was put in place to protect from multiple synchronous reload calls, but I didn't find a case where that's a problem. It's fine from my side.

flash1293 · 2020-12-08T10:48:10Z

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

@@ -110,7 +110,9 @@ export class Embeddable

    this.expressionRenderer = deps.expressionRenderer;
    this.initializeSavedVis(initialInput).then(() => this.onContainerStateChanged(initialInput));
-    this.subscription = this.getInput$().subscribe((input) => this.onContainerStateChanged(input));
+    this.subscription = this.getUpdated$().subscribe(() =>
+      this.onContainerStateChanged(this.input)


nit: Should this.input be used? There's also this.getInput() which is used in other places. Not sure whether important, just wanted to point it out.

this.getInput() is public and this.input is protected, so I guess inside the embeddable it is fine to use both

Dosant · 2020-12-08T11:44:30Z

@ThomThomson,

One thing I did notice is that the dashboard is reloaded now on 'cancel' when you choose to discard changes. This wasn't the case before, as nothing is changing that would require a re-fetch. Is this intended?

Good catch.

If you "discard changes" and there is no change in state keys that cause starting a new session, then no refetch would happen. e.g. when just switching between view -> edit -> (discard) -> view mode.

But in general with this change we are triggering a refetch more often then we had before.
For example, we would refetch in case any change to panels happens (because we are starting a new session).

This fixes following bug @lukasolson pointed out:

Looks like there’s a bit of a bug when you wait until all the results have loaded, then add a new panel (which will only cause the new panel to fetch data), then send to background. If you restore, only the newest panel will actually restore properly

But in general with this change we are triggering a refetch more often then we had before.

I think we have to accept this drawback for now with current session management design :(
@lukasolson, probably we could avoid this excessive refetches if we introduce a way to "extend" a session (like clone and add more async searches to it)

lizozom · 2020-12-08T12:31:21Z

@Dosant there shouldn't be a problem with adding a new request to the current session, in this new panel scenario.
We could "re-open" it, and I think everything should still work.

lizozom

Tested the following

Switching to edit mode - doesn't clear session
Cancel back to view mode - doesn't clear session
Fullscreen - doesn't clear session
Delete a panel from a dashboard - doesn't clear session
Moving a panel and saving - doesn't clear session
Adding a panel and saving - doesn't clear session

When a sesison is not cleared, it means items are added to the existing session, which I find reasonable.

LGTM

Dosant · 2020-12-08T15:06:01Z

@Dosant there shouldn't be a problem with adding a new request to the current session, in this new panel scenario.
We could "re-open" it, and I think everything should still work.

@ThomThomson, would you be OK if I create an issue for this excessive fetches we gonna track and look into improving if/when we add this re-open API @lizozom mentions?

---- edit

created: #85293

ThomThomson

With an issue for awareness around the excessive fetches I am good to approve this. Hopefully there will be a fix, but if this is just the way the session service works it's not too bad a change anyway.

Dosant · 2020-12-09T09:07:16Z

@elasticmachine merge upstream

Dosant · 2020-12-09T14:53:59Z

@kertal, although this got all code owners review, search embeddable still needs a 👀 🙏

kertal · 2020-12-10T07:40:50Z

@Dosant ACK, yes, thx, will have a look today

Dosant · 2020-12-10T08:16:03Z

@elasticmachine merge upstream

kibanamachine · 2020-12-10T09:57:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0d4a68d

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
`dashboard`	197.0KB	197.2KB	+248.0B
`discover`	450.3KB	450.4KB	+187.0B
`lens`	1011.8KB	1011.8KB	+4.0B
total			+439.0B

Distributable file count

id	before	after	diff
`default`	46990	47750	+760

Page load bundle

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

id	before	after	diff
`embeddable`	225.7KB	226.1KB	+394.0B
`visualizations`	169.0KB	168.9KB	-148.0B
total			+246.0B

History

💚 Build #93015 succeeded 2295478
💚 Build #92412 succeeded 105b1f5
💚 Build #91657 succeeded 4a0f114
💚 Build #91340 succeeded 5fe756a
💚 Build #91314 succeeded c398df9

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

kertal

Code LGTM, mainly took a look at the search embeddable and it's fetching behavior, recently fixed a double fetch there, so the good news after testing locally: it fetches when it should, no less, no more 👍

…hange (elastic#84261)

…hange (#84261) (#85562)

Dosant mentioned this pull request Dec 1, 2020

[Search] Send to background #83640

Closed

38 tasks

Dosant added 2 commits December 2, 2020 11:21

improve dashboard embeddable fetches

043b1d9

# Conflicts: # src/plugins/discover/public/application/embeddable/search_embeddable.ts

post merge fix

b268407

Dosant force-pushed the dev/search/dashboard-start-session-fix branch from 242fcc2 to b268407 Compare December 2, 2020 10:39

wip

c398df9

Dosant changed the title ~~[wip] Dev/search/dashboard start session fix~~ [Search] Fix dashboard embeddable refetches on sessionId changes Dec 2, 2020

Dosant commented Dec 2, 2020

View reviewed changes

Dosant added 2 commits December 2, 2020 13:06

Merge branch 'master' of github.com:elastic/kibana into dev/search/da…

eb44f38

…shboard-start-session-fix

docs

533c3bc

Dosant changed the title ~~[Search] Fix dashboard embeddable refetches on sessionId changes~~ [Search] Fix dashboard embeddables don't refetch on sessionId change Dec 2, 2020

Dosant changed the title ~~[Search] Fix dashboard embeddables don't refetch on sessionId change~~ [Search] Fix dashboard embeddables don't refetch on searchSessionId change Dec 2, 2020

Dosant marked this pull request as ready for review December 2, 2020 12:36

Dosant requested a review from a team December 2, 2020 12:36

Dosant requested review from a team as code owners December 2, 2020 12:36

Dosant requested review from ThomThomson, kertal and flash1293 December 2, 2020 12:36

Dosant added bug Fixes for quality problems that affect the customer experience Feature:Embedding Embedding content via iFrame Feature:Search Querying infrastructure in Kibana release_note:skip Skip the PR/issue when compiling release notes Team:AppServices v7.11.0 v8.0.0 labels Dec 2, 2020

Dosant added the Feature:Dashboard Dashboard related features label Dec 2, 2020

add test

5fe756a

Merge branch 'master' into dev/search/dashboard-start-session-fix

4a0f114

Dosant added 2 commits December 7, 2020 17:22

Merge branch 'master' of github.com:elastic/kibana into dev/search/da…

0180ab3

…shboard-start-session-fix

Merge branch 'dev/search/dashboard-start-session-fix' of github.com:D…

105b1f5

…osant/kibana into dev/search/dashboard-start-session-fix

ThomThomson reviewed Dec 7, 2020

View reviewed changes

flash1293 approved these changes Dec 8, 2020

View reviewed changes

lizozom approved these changes Dec 8, 2020

View reviewed changes

ThomThomson approved these changes Dec 8, 2020

View reviewed changes

Dosant mentioned this pull request Dec 8, 2020

[Search][Dashboard] Don't create search session on "panels" change #85293

Closed

Merge branch 'master' into dev/search/dashboard-start-session-fix

2295478

Merge branch 'master' into dev/search/dashboard-start-session-fix

0d4a68d

kertal approved these changes Dec 10, 2020

View reviewed changes

Dosant merged commit cb29438 into elastic:master Dec 10, 2020

Dosant mentioned this pull request Dec 10, 2020

[7.x] [Search] Fix dashboard embeddables don't refetch on searchSessionId change (#84261) #85562

Merged

Dosant added a commit to Dosant/kibana that referenced this pull request Dec 10, 2020

[Search] Fix dashboard embeddables don't refetch on searchSessionId c…

1cc7f53

…hange (elastic#84261)

Dosant added a commit that referenced this pull request Dec 10, 2020

[Search] Fix dashboard embeddables don't refetch on searchSessionId c…

174a349

…hange (#84261) (#85562)

ThomThomson added a commit to ThomThomson/kibana that referenced this pull request Dec 10, 2020

worked in changes from elastic#84261

7fda631

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Search] Fix dashboard embeddables don't refetch on searchSessionId change #84261

[Search] Fix dashboard embeddables don't refetch on searchSessionId change #84261

Dosant commented Nov 24, 2020 •

edited

Loading

Dosant Dec 2, 2020

flash1293 Dec 8, 2020

elasticmachine commented Dec 2, 2020

Dosant commented Dec 3, 2020

ThomThomson left a comment

flash1293 left a comment

flash1293 Dec 8, 2020

flash1293 Dec 8, 2020

Dosant Dec 8, 2020 •

edited

Loading

Dosant commented Dec 8, 2020

lizozom commented Dec 8, 2020

lizozom left a comment

Dosant commented Dec 8, 2020 •

edited

Loading

ThomThomson left a comment

Dosant commented Dec 9, 2020

Dosant commented Dec 9, 2020

kertal commented Dec 10, 2020

Dosant commented Dec 10, 2020

kibanamachine commented Dec 10, 2020

kertal left a comment

[Search] Fix dashboard embeddables don't refetch on searchSessionId change #84261

[Search] Fix dashboard embeddables don't refetch on searchSessionId change #84261

Conversation

Dosant commented Nov 24, 2020 • edited Loading

Summary

How to test

Checklist

For maintainers

Dosant Dec 2, 2020

Choose a reason for hiding this comment

flash1293 Dec 8, 2020

Choose a reason for hiding this comment

elasticmachine commented Dec 2, 2020

Dosant commented Dec 3, 2020

ThomThomson left a comment

Choose a reason for hiding this comment

flash1293 left a comment

Choose a reason for hiding this comment

flash1293 Dec 8, 2020

Choose a reason for hiding this comment

flash1293 Dec 8, 2020

Choose a reason for hiding this comment

Dosant Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Dosant commented Dec 8, 2020

lizozom commented Dec 8, 2020

lizozom left a comment

Choose a reason for hiding this comment

Dosant commented Dec 8, 2020 • edited Loading

ThomThomson left a comment

Choose a reason for hiding this comment

Dosant commented Dec 9, 2020

Dosant commented Dec 9, 2020

kertal commented Dec 10, 2020

Dosant commented Dec 10, 2020

kibanamachine commented Dec 10, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

Distributable file count

Page load bundle

History

kertal left a comment

Choose a reason for hiding this comment

Dosant commented Nov 24, 2020 •

edited

Loading

Dosant Dec 8, 2020 •

edited

Loading

Dosant commented Dec 8, 2020 •

edited

Loading