-
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
[Discover] Add DiscoverSavedSearchContainer #153528
Conversation
- A container to centralize functionality around the usage of saved searches in Discover
* Can also be used to track changes to the saved search | ||
* It centralizes functionality that was spread across the Discover main codebase | ||
*/ | ||
export interface DiscoverSavedSearchContainer { |
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.
Dear reviewer, my I introduce the new (sub) state container to you ... most of the code in this PR is about using this ... so this is the ❤️ of the PR
Pinging @elastic/kibana-data-discovery (Team:DataDiscovery) |
const stateContainer = useSingleton<DiscoverStateContainer>(() => | ||
getDiscoverStateContainer({ | ||
history, | ||
services, | ||
}) | ||
); |
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.
Apart from adding a new state container, this is the biggest change, before this, DiscoverStateContainer
was initialized 2x on every change to different saved search
*/ | ||
useEffect(() => { |
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've heard rumors that useEffect isn't popular anymore (right @mattkime ), happy to say, that I removed some, and there might be potential for more ... oh yes, there a big hook gone use_discover_state
, 👋 goodbye and thx for all the fish.
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.
ok, this effect is a bad example because it was just moved to L90 😄
@@ -17,10 +18,12 @@ export const FieldStatisticsTab: React.FC<Omit<FieldStatisticsTableProps, 'query | |||
const querySubscriberResult = useQuerySubscriber({ | |||
data: services.data, | |||
}); | |||
const savedSearch = useSavedSearch(); |
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.
why not using the state container here ... it's not available in the saved search embeddable
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.
Wow! So many changes! Leaving some questions to clarify 💫
src/plugins/discover/public/application/main/components/field_stats_table/field_stats_table.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/application/main/components/top_nav/on_save_search.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/application/main/components/top_nav/on_save_search.tsx
Outdated
Show resolved
Hide resolved
src/plugins/discover/public/application/main/components/top_nav/open_alerts_popover.tsx
Show resolved
Hide resolved
src/plugins/discover/public/application/main/components/layout/discover_layout.tsx
Show resolved
Hide resolved
DiscoverLayoutProps, | ||
'navigateTo' | 'savedSearch' | 'searchSource' | ||
> & { | ||
export type DiscoverTopNavProps = Pick<DiscoverLayoutProps, 'navigateTo'> & { |
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.
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.
this is odd, I couldn't reproduce ... were there any error message in console? can you reproduce? many thx!
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.
yeah, also can't reproduce, sorry!
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.
that's the hardest one, I suspect it broke for the same reason alerting broke, causing a silent exception here
src/plugins/discover/public/application/main/components/top_nav/open_alerts_popover.tsx
Show resolved
Hide resolved
const initialFetchStatus = shouldSearchOnPageLoad | ||
? FetchStatus.LOADING | ||
: FetchStatus.UNINITIALIZED; | ||
const getInitialFetchStatus = () => { |
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.
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.
Working on a fix, moving the listener for the /
path, that creates a new saved search or resets an existing one to discover_main_route.tsx
The way it was implemented before caused a duplicate fetch command before ... will follow up, first CI needs to reply
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.
The issue is still present
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.
hmm, for me it seems fixed, something to follow up after 🥚 & 🇳🇱
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.
@davismcphee I can't reproduce it, but this needs definitely another pair of 👀
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.
Well, well, found and fixed it (needed a query to reproduce) 98efa69
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.
thx!
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.
Looks like this issue with ignored discover:searchOnPageLoad
is happening again
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.
We chatted "offline" and it was because the refresh interval was "On" ... So it works like before. However me might consider, switching the refresh interval to "Off" when triggering New
... When I start a new search I think I first want to apply data, query, filter, before I want to have the data refreshed? ... not part of this PR of course
Yes, sorry for that, also seems I rushed to the review state. Apologies for that. Flagging it to draft again, until fixes are applied, 🙇 thx a lot for this first review! |
@elasticmachine merge upstream |
@@ -141,21 +145,45 @@ export function resolveDataView( | |||
}); | |||
return ownDataView; | |||
} | |||
if (!Boolean(isTextBasedQuery)) { |
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.
Was this condition redundant?
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.
No, it wasn't but I guess it was a change that got lost since it was added during the long period of time of this PR ... This message doesn't make sense when loading a new saved search with SQL in the state URL. I reverted this change to consider text based queries , also added a test for it: 53ec89d
const initialFetchStatus = shouldSearchOnPageLoad | ||
? FetchStatus.LOADING | ||
: FetchStatus.UNINITIALIZED; | ||
const getInitialFetchStatus = () => { |
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.
Looks like this issue with ignored discover:searchOnPageLoad
is happening again
@elasticmachine merge upstream |
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.
Did another test round: looking good! 👍
Thanks for improving the state management!
@jughosta thx a lot for taking care of this 🐰 🕳️ in terms of a review, for catching so much stuff 👍 |
@kertal I was planning on doing a final review/round of testing on this today but didn't get the chance -- will definitely take a look at it tomorrow though! |
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.
Just finished a final round of reviewing and testing, and I'm happy to say it LGTM 🥳 That was a long time coming, but this was much needed work and I'm excited to finally see it merged to main
. It's also good that it's early in the minor so we have time to catch and fix anything we might have missed. Great work, and I'd say we're good to push the button whenever you're ready!
@elasticmachine merge upstream |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Page load bundle
Unknown metric groupsAPI count
ESLint disabled line counts
References to deprecated APIs
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @kertal |
Thx so much @jughosta @davismcphee for taking care of the reviews of this 👹 🙇 |
Summary
This PR adds the new
DiscoverSavedSearchContainer
. It fundamentally changes the way Discover treats the handling of the actual saved search.DiscoverSavedSearchContainer
we store the initial and the updated version. This allows us diff between both versions and can be used to provide an indication in the UI that there have been changes to the saved search, removing a common pain point in Discover (One it's implemented)DiscoverState
has been initialized 2 times before this PR, now it's a singleton high up in the component tree, so when Discover Main is displayed it's just initialized 1x.Context
This is the final PR refactoring Discover's state management, that was scattered around the code base (saved search, appState, dataState via observables), it's cleaning, and centralizing stuff. Its focus is to get a clearer structure, better separation from UI, removing of hook dependencies making it hard to debug the code. So refactoring without regressions, which makes it easier to further migrate to a future state management solution if needed. One of the biggest advantage is that we can now write unit test what kind of URL creates which kind of state and triggers which behavior.
Generally, the state container consists of several other state containers
kibana/src/plugins/discover/public/application/main/services/discover_state.ts
Lines 70 to 98 in 2d8a2b5
Those can be directly accessed by the UI. For complex state manipulation which need more sub states,
actions
can be used from the UIkibana/src/plugins/discover/public/application/main/services/discover_state.ts
Lines 98 to 175 in 2d8a2b5
With this state container we can write unit tests like e.g. for the
loadSavedSearch
actionkibana/src/plugins/discover/public/application/main/services/discover_state.test.ts
Lines 392 to 403 in 2d8a2b5
Reviewing
First dear reviewer, apologies, this is a big PR ... but also, if I would have opened the initial draft PR fro review ... it would have been much worse. So, the best way to start reviewing is the first commit, which is introducing the new state container:
071fa37
And since the creator of this PR has been working on that for a while, he might not see the fires for tree.
There's certainly potential for follow up improvements, but it was time to bring the scope over the finishing line. Ideally we could use this state container also in the saved search embeddable, but this is Zukunfsmusik (🚀 🎼 )
Testing
The new version needs to be tested for regressions. Functional tests caught lots of stuff during development, not only Discover tests, also ML & alerting test were helpful. Nothing should have changed but since it's also a fundamental change, more testing is better than less. What should be tested
New
menu bar entry to create new saved searches (ideally after loading persisted ones)New
, loading the saved searchResolves #142131
Fixes #155193
Checklist