-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Logs+] Restore Dataset selection from page URL #161144
[Logs+] Restore Dataset selection from page URL #161144
Conversation
…lections and be controlled
…iani/kibana into 160146-all-entry-dataset-selector
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
…-ref HEAD~1..HEAD --fix'
…ani/kibana into 160425-restore-dataset-selection
…ani/kibana into 160425-restore-dataset-selection
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate the effort to make the initialization steps more explicit, but to be honest I think I preferred your original approach (although maybe moving the isCustomizationServiceInitialized
check to the initialization useEffect
rather than within loadSavedSearch
itself to make the initialization steps clearer).
The idea behind useDiscoverCustomizationService
was to encapsulate the lifecycle of the customization framework behind a single hook rather than having the main Discover code manage it. This way the framework lifecycle can be tested as a single hook, it has a limited footprint within the Discover codebase, and it's portable enough to be moved/removed later if needed.
While the changes here are limited, I worry that starting to mix the customization framework with the main Discover code could create dependencies that become harder to remove over time. Ideally discover_main_route
's only interaction with the customization framework would be to delay loading Discover until the service exists, at which time it can be guaranteed that the customization framework is initialized and Discover is safe to load.
src/plugins/discover/public/application/main/discover_main_route.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/application/main/discover_main_route.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/customizations/customization_provider.ts
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/customizations/customization_provider.ts
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/customizations/customization_provider.ts
Outdated
Show resolved
Hide resolved
Hey @davismcphee, thanks for the in-depth review! I appreciate you explaining the downside of the approach I proposed for initializing the flow, and I can see all of them and agree with you. Switching back to the original approach should resolve all the other suggestions you left. Thanks again for the review! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for understanding my concerns, and with the latest updates this looks good to go on my end. I also tested and confirmed Discover is working as expected and Logs Explorer dataset selections are preserved on refresh. Thanks for fixing the Discover initialization bug too! LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed and Tested the infra changes! LGTM overall
Thanks for adding the tests, let us discuss afterward how to use the stateContainer and merge our work together ! 🚀
|
||
export type DefaultLogExplorerProfileState = WithDatasetSelection; | ||
|
||
export type LogExplorerProfileTypestate = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: TypeState
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsasync chunk count
ESLint disabled in files
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
## 📓 Summary Closes elastic#160425 After the [first implementation of the log-explorer profile](elastic#159907), we wanted to restore the selection of the dataset for a user when landing on the Discover log-explorer profile. Since we create an ad-hoc data view for Discover starting from the dataset details, we needed to develop a system for intercepting the `index` query parameter (which is used by Discover as the source of truth for restoring a data view), create our ad-hoc data view and store in the URL an encoded ID with the required details to restore the selection. The following video shows the user journey for: - Landing on the log-explorer profile with no index param, nothing to restore and fallback to All log datasets. - Landing on the log-explorer profile invalid index param, notify about failure and fallback to All log datasets. - Select a different dataset, applies the new data view and update the URL. When the URL is accessed directly, restore and initialize the data view for the selection. - Navigate back and forth in the browser history, restoring the selection and data view on `index` param changes. https://github.com/elastic/kibana/assets/34506779/37a212ee-08e4-4e54-8e42-1d739c38f164 ## 💡 Reviewer hints To have better control over the page selection and the restore process, we prepared the DatasetSelector component for [being controlled by the parent component](elastic#160971). Having that ready, we now implemented a new top-level state machine with the following responsibilities: - Re-initialize (decompress/decode) the dataset selection from the `index` query params. - Derive and set into Discover state a new ad-hoc data view. - Keep track of new dataset selection changes and update the URL state and the current data view. <img width="1224" alt="log-explorer-machine" src="https://github.com/elastic/kibana/assets/34506779/67e3ff17-dc3f-4dcf-b6c0-f40dbbea2d44"> We found a race condition between the Discover URL initialization + data view initialization against the log-explorer profile customizations being applied. To guarantee we correctly initialize the state machine and restore the selection before Discover goes through its initialization steps, we need to wait for the customization service to exist in Discover so that also the customization callbacks are successfully invoked. --------- Co-authored-by: Marco Antonio Ghiani <[email protected]> Co-authored-by: kibanamachine <[email protected]>
📓 Summary
Closes #160425
After the first implementation of the log-explorer profile, we wanted to restore the selection of the dataset for a user when landing on the Discover log-explorer profile.
Since we create an ad-hoc data view for Discover starting from the dataset details, we needed to develop a system for intercepting the
index
query parameter (which is used by Discover as the source of truth for restoring a data view), create our ad-hoc data view and store in the URL an encoded ID with the required details to restore the selection.The following video shows the user journey for:
index
param changes.restore-dataset.mov
💡 Reviewer hints
To have better control over the page selection and the restore process, we prepared the DatasetSelector component for being controlled by the parent component.
Having that ready, we now implemented a new top-level state machine with the following responsibilities:
index
query params.We found a race condition between the Discover URL initialization + data view initialization against the log-explorer profile customizations being applied.
To guarantee we correctly initialize the state machine and restore the selection before Discover goes through its initialization steps, we need to wait for the customization service to exist in Discover so that also the customization callbacks are successfully invoked.