Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Discover] Deangularize discover controller #96766

Conversation

kertal
Copy link
Member

@kertal kertal commented Apr 12, 2021

Summary

This PR reduces Discover's Angular controller to a minimum (mainly router functions).

Changes

  • Most of discover.js logic is migrated into hooks and a wrapper component, only URL logic is left behind (Future PR)
  • Clicking on "Reset search" when loading a saved search no longer reloads the whole URL
  • Switching index patterns no longer reloads the whole URL
  • URL reload only happens when a saved search is saved, loaded or New is selected (will remove that once we migrate the while plugin to react router)
  • Lots of files were moved for a better directory structure, so files just relevant to the main part of Discover can be found here: src/plugins/discover/public/application/apps/main/
  • Due to the usage of Observables the number initial number of re-renderings of the layout when loading the component was reduced to 3 (originally it was 6+), when clicking on 'Reload', only 2 re-renderings take place

Note that splitting the single request into 3 requests for number of hits, aggregation data for the histogram and documents was part of this PR, but for a better scope this was removed and there will be the follow up PR #100326.

Reviewing

Best to start with the new controller like component DiscoverMainApp

https://github.com/elastic/kibana/blob/b78f2b13404b56873e0b428d883354a55970028c/src/plugins/discover/public/application/apps/main/discover_main_app.tsx

It contains the old controller logic splitted into 4 hooks , useDiscoverState, useUrl, useSearchSession,useSavedSearchData, and some inline code

The majority of other changes are just different paths, so I'd recommend using Github's viewed button, and then to review the relevant stuff

Testing

There shall be no regressions!

Checklist

@kertal kertal self-assigned this Apr 12, 2021
@kertal kertal added the Feature:Discover Discover Application label Apr 12, 2021
@kertal kertal mentioned this pull request Apr 12, 2021
9 tasks
@kertal kertal changed the title [Discover] Refactor discover controller and search source [Discover] Deangularize discover controller and split queries Apr 27, 2021
Copy link
Contributor

@flash1293 flash1293 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested around and didn't manage to break it, LGTM

This definitely needs at least one other pair of eyes due to the large complexity involved.

@kertal
Copy link
Member Author

kertal commented Jun 7, 2021

It looks like restoring a search session with a relative time range doesn't work.
The problem is that the time range inside the original search is different from the ones saved inside a search session object. So that when the session is restored time ranges don't match.

@Dosant thx for the hint, I've adapted the code and added a test for this

Also noticed that when you successfully restored a session and then navigate away from it by doing a state change (e.g time change), then you get two network requests. I didn't check master for this, so not sure if this is a regression, but it looks like an indication that there are further possible state mgmt optimizations that would reduce re-renderings. Might be something worth debugging:

also fixed this, could you have another look? many thx!

@kertal kertal requested a review from Dosant June 7, 2021 06:54
@kertal kertal requested a review from timroes June 7, 2021 07:05
Copy link
Contributor

@Dosant Dosant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#96766 (comment)

Retested those pieces. LGTM 👍

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Despite my minor comments left, code LGTM. I'd see some optimizations especially around how we could optimize state handling in the saved search and state hook, but I am fine having those done in another PR, since we should get this PR through to unblock other work.

Tested it on Chrome Linux with playing around in Discover a bit and couldn't find anything breaking so far.

@kertal kertal linked an issue Jun 8, 2021 that may be closed by this pull request
12 tasks
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
discover 369 377 +8

Async chunks

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

id before after diff
discover 420.9KB 426.4KB +5.5KB

Page load bundle

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

id before after diff
data 839.6KB 839.7KB +70.0B
discover 74.8KB 74.5KB -279.0B
total -209.0B

History

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

cc @kertal

@kertal kertal merged commit 6525105 into elastic:master Jun 8, 2021
kertal added a commit to kertal/kibana that referenced this pull request Jun 8, 2021
kertal added a commit that referenced this pull request Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v7.14.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Discover] Deangularize Discover App
9 participants