[Lens] Integrate searchSessionId into Lens app by dej611 · Pull Request #86297 · elastic/kibana

dej611 · 2020-12-17T14:31:44Z

Summary

the aim of the PR is to integrate the searchSessionId parameter in the Lens EditorFrame component.
During the integration it was noticed that there were duplicate state updates on filter updates: that led to a refactor of the state update process to remove the duplications.

✨ Add searchSessionId on Editor open/landing
🔥 Clear the session id on app leave
🔥 remove duplicate state update
✅ Add unit tests for new integration
- this required a small refactor of the services mocks
- some tangential tests have been updated with a searchSessionId mock to work

Manual tests have been performed to test various app leave flows. They should all be covered now, but any feedback on this side is welcome.

Checklist

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

elasticmachine · 2020-12-17T14:31:46Z

Pinging @elastic/kibana-app (Team:KibanaApp)

flash1293

Didn't test in depth yet, but two things:

Is it resetting the search session id on refresh?
Seems like it's not using the search session id for the suggestion panels

dej611 · 2020-12-17T14:57:39Z

Is it resetting the search session id on refresh?

Yes.

Seems like it's not using the search session id for the suggestion panels

You are quite right 😓

dej611 · 2020-12-17T17:20:05Z

Added the session id for suggestions too: not sure what to test in there.

flash1293 · 2020-12-18T08:45:26Z

@elasticmachine merge upstream

flash1293

Looks pretty good already, I just noticed one problem - if auto-refresh is used, the session id is not reset.

You can subscribe to the observable provided by the timefilter service in the data plugin to get notified and change the session id based on it (timefilter.getAutoRefreshFetch$()).

If you do this, you can probably remove the subscriptions in the workspace and suggestion panels as they would get refetch automatically because of the incoming session id change - simplifying the code there as a little bonus :)

kibana/x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx

Line 365 in daa92c3

    
           const autoRefreshFetch$ = useMemo(() => timefilter.getAutoRefreshFetch$(), [timefilter]);

and

kibana/x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx

Line 274 in 928687b

    
           const autoRefreshFetch$ = plugins.data.query.timefilter.timefilter.getAutoRefreshFetch$();

wylieconlon

I ran into exactly the same problem as you are, which is that the API of onAppLeave is unintuitive. We will need to find another solution to clearing searchSessionId.

wylieconlon · 2020-12-18T20:45:19Z

x-pack/plugins/lens/public/app_plugin/app.tsx

  useEffect(() => {
    onAppLeave((actions) => {
+      // Clear the session when leaving Lens
+      data.search.session.clear();


We definitely should not be adding side effects to the onAppLeave handler because of how it's called. The onAppLeave function is executed immediately, and is not executed when the user tries to leave. Basically it sets up a state that says "if the user were to leave in the future, prevent them".

What this means is that I don't think we have a reliable way to make a side effect happen as the user is leaving, and I don't think we should be adding any side effects to the onAppLeave handlers.

Good point, I guess we can put it in an unmount handler (e.g. useUnmount() )

Did some investigation on this area:

during the saving process, no problem with that, as the clear can be explicitly set

otherwise if the user clicks on another plugin/app there's no other way to intercept it within Lens and clear the session. The session check happens before the component get unmounted leading to an error page. Note that this error page happens only on the dev environment, while in production the session gets cleared.

Discussed with @flash1293 and found a simpler solution to that

wylieconlon · 2020-12-18T20:46:44Z

x-pack/plugins/lens/public/app_plugin/app.tsx

-                dateRange: {
-                  fromDate: dateRange.from,
-                  toDate: dateRange.to,
-                },


So we're using the timeFilter manager instead now?

The code was already using the timeFilter, but this code here was covering the scenario where the data didn't change but we want the state to trigger a new render ("Refresh" click).
Previously when the dateRange values changed the state was update twice with the same content, triggering a double rendering. Now this scenario is covered by the timeFilter subscription.
In case of no dateRange change ("Refresh" button), the session id is regenerated triggering a re-render now.

We still need the dateRange state, but we can reduce the number of time we update it.

wylieconlon · 2020-12-18T20:48:06Z

x-pack/plugins/lens/public/app_plugin/app.tsx

      isSaveModalVisible: false,
      indicateNoData: false,
      isSaveable: false,
+      searchSessionId: data.search.session.start(),


Can you remove dateRange from the LensAppState now?

Moved the dateRange out of state in the last commit. An object reference needs to be still hold for the frame component, but there's a single source of truth now for it.

flash1293 · 2020-12-22T09:41:33Z

x-pack/plugins/lens/public/editor_frame_service/editor_frame/suggestion_panel.tsx

      />
    );
-  }, [plugins.data.query.timefilter.timefilter, ExpressionRendererComponent]);
+  }, [frame.searchSessionId, ExpressionRendererComponent]);


This causes the component to get renewed each time the session id changes, which causes the loading bug again that got fixed with #86092

Please either put the search session id in a ref as well to avoid that or remove the AutoRefreshExpressionRenderer completely and just pass session id and search context down to the place where the component is actually used. With both of these approaches we can make sure the expression loader is not destroyed and re-created for each change.

I'm also seeing duplicated requests for the suggestions, but those might be caused by the same problem

flash1293

Tested and works as expected. The issue with double fetch is caused by another problem, I will create a separate PR to fix

flash1293 · 2020-12-23T16:36:20Z

@elasticmachine merge upstream

dej611 · 2020-12-28T09:52:11Z

@elasticmachine merge upstream

kibanamachine · 2020-12-28T12:05:27Z

💚 Build Succeeded

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

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.0MB	1.0MB	+348.0B

Distributable file count

id	before	after	diff
`default`	47267	48027	+760

Page load bundle

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

id	before	after	diff
`lens`	54.2KB	54.3KB	+64.0B

History

💚 Build #96190 succeeded 1614dc1
💛 Build #95955 was flaky 19a3bac
💚 Build #95753 succeeded 42e15d1
💚 Build #95340 succeeded daa92c3
💚 Build #95191 succeeded 928687b

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

wylieconlon

Changes LGTM!

* master: (108 commits) [DOCS] Refreshes Data Visualizer screenshot (elastic#87017) [Security Solution] Change 'anti-virus' text to 'antivirus' (elastic#87001) [securitySolution/cypress] temporarily limit to PRs [AppServices/Examples] Add the example for Reporting integration (elastic#82091) [Build Chromium] Improve git checkout (elastic#83225) Deprecate `services.callCluster` in alerts and actions executors (elastic#86474) [Security Solution] Use system node version for Cypress and increase exec command timeout (elastic#86985) [Lens] Add more chart editor tests based on the debug state (elastic#86750) [Lens] Integrate searchSessionId into Lens app (elastic#86297) skip "pagination updates results and page number" elastic#86975 skip "Custom detection rules" elastic#83772 [logging/json] use merge from kbn/std (elastic#86330) skip network and timeline inspection. elastic#85677, elastic#85678 skip "adds correctly a filter to the global search bar" elastic#86552 [ftr/flags] improve help text (elastic#86971) skip "Fields Browser rendering. elastic#60209 skip "Closes and opens alerts" elastic#83773 [Security Solution] Skip failing Cypress tests (elastic#86967) Removes archives (elastic#86537) [ML] Fix charts grid on the Anomaly Explorer page (elastic#86904) ...

* master: (72 commits) [DOCS] Refreshes Data Visualizer screenshot (elastic#87017) [Security Solution] Change 'anti-virus' text to 'antivirus' (elastic#87001) [securitySolution/cypress] temporarily limit to PRs [AppServices/Examples] Add the example for Reporting integration (elastic#82091) [Build Chromium] Improve git checkout (elastic#83225) Deprecate `services.callCluster` in alerts and actions executors (elastic#86474) [Security Solution] Use system node version for Cypress and increase exec command timeout (elastic#86985) [Lens] Add more chart editor tests based on the debug state (elastic#86750) [Lens] Integrate searchSessionId into Lens app (elastic#86297) skip "pagination updates results and page number" elastic#86975 skip "Custom detection rules" elastic#83772 [logging/json] use merge from kbn/std (elastic#86330) skip network and timeline inspection. elastic#85677, elastic#85678 skip "adds correctly a filter to the global search bar" elastic#86552 [ftr/flags] improve help text (elastic#86971) skip "Fields Browser rendering. elastic#60209 skip "Closes and opens alerts" elastic#83773 [Security Solution] Skip failing Cypress tests (elastic#86967) Removes archives (elastic#86537) [ML] Fix charts grid on the Anomaly Explorer page (elastic#86904) ...

kibanamachine · 2020-12-31T13:24:39Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine · 2021-01-01T13:34:39Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

kibanamachine · 2021-01-04T13:44:40Z

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

dej611 added 2 commits December 17, 2020 14:49

✨ Add searchSessionId to Lens app + refactor filters state updates

596ebe4

✅ Fix tests with new searchSessionId prop

0d389aa

dej611 added Team:Visualizations Team label for Lens, elastic-charts, Graph, legacy editors (TSVB, Visualize, Timelion) t// v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Feature:Lens v7.12.0 labels Dec 17, 2020

dej611 requested a review from a team December 17, 2020 14:31

flash1293 reviewed Dec 17, 2020

View reviewed changes

✨ use session id for suggestions

928687b

dej611 mentioned this pull request Dec 17, 2020

[Lens] Suggestions don't refresh along with the workspace panel #86276

Closed

Merge branch 'master' into feature/85517

daa92c3

flash1293 reviewed Dec 18, 2020

View reviewed changes

wylieconlon suggested changes Dec 18, 2020

View reviewed changes

dej611 added 4 commits December 21, 2020 14:28

👌 Fix the autoRefresh and onAppLeave flows

bcb83c9

Merge remote-tracking branch 'upstream/master' into feature/85517

a1a4296

🔥 Remove session clear test as it happens at higher level

1e4f18a

🚚 Move out of state the dateRange

42e15d1

flash1293 requested changes Dec 22, 2020

View reviewed changes

⚡ Use ref for memoized renderer in suggestion panel

19a3bac

flash1293 approved these changes Dec 22, 2020

View reviewed changes

Merge branch 'master' into feature/85517

1614dc1

Merge branch 'master' into feature/85517

c35e4c0

wylieconlon approved these changes Dec 28, 2020

View reviewed changes

dej611 merged commit bd908c6 into elastic:master Dec 29, 2020

dej611 deleted the feature/85517 branch December 29, 2020 13:18

dej611 mentioned this pull request Dec 30, 2020

[7.x] [Lens] Integrate searchSessionId into Lens app (#86297) #87065

Merged

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Dec 31, 2020

kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Jan 5, 2021

flash1293 pushed a commit that referenced this pull request Jan 5, 2021

[Lens] Integrate searchSessionId into Lens app (#86297) (#87065)

d41cfa0

This was referenced Jan 15, 2021

[Search Sessions][Lens] Explicitly opt-in to "send to background UI" #88442

Closed

[Search Sessions] Make search session indicator UI opt-in #88699

Merged

Conversation

dej611 commented Dec 17, 2020

Summary

Checklist

Uh oh!

elasticmachine commented Dec 17, 2020

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

dej611 commented Dec 17, 2020

Uh oh!

dej611 commented Dec 17, 2020

Uh oh!

flash1293 commented Dec 18, 2020

Uh oh!

flash1293 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wylieconlon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flash1293 left a comment

Choose a reason for hiding this comment

Uh oh!

flash1293 commented Dec 23, 2020

Uh oh!

dej611 commented Dec 28, 2020

Uh oh!

kibanamachine commented Dec 28, 2020

💚 Build Succeeded

Metrics [docs]

Async chunks

Distributable file count

Page load bundle

History

Uh oh!

wylieconlon left a comment

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Dec 31, 2020

Uh oh!

kibanamachine commented Jan 1, 2021

Uh oh!

kibanamachine commented Jan 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

flash1293 left a comment •

edited

Loading