-
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
[Explorer] Extract URL state persistence from the component #165255
Comments
Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI) |
Whilst this issue is focussed around URL state extraction, after discussions with Felix and Mohamed we considered whether this should reach a bit broader to state syncing in general. Here I've tried to provide an overview of how things work currently, and potentially some ideas for how we can move forward. Here we'll be referring to 3 layers: Observability Log Explorer (top level consumer) > Log Explorer (reusable log explorer component that registers profile customisations etc) > Discover Container (essentially an embeddable wrapper around Discover's main route with an injectable scoped history etc). These are mounted left to right. Broadly speaking we want a way to:
We can apply some forward thinking here and try to ensure that the Log Explorer layer communicates (with top level consumers) in a manner that will suit our planned persistence layer: https://github.com/elastic/logs-dev/issues/26 The issue we have here is that the Explorers and Discover were developed at very different times, so we are trying to retrofit this state syncing on top of something that's existed for a very long time. Discover's state containerDiscover provides a "state container", this is also provided to the customisation callbacks as a parameter. This state container wraps several child portions of state. Not everything here is relevant to us, but we would be interested in How are we interacting with this state now?Right now our only top level consumer is Observability, and this plugin has no knowledge of Discover state internals. As we move down a layer to the Log Explorer there is knowledge of, and interaction with, Discover's state container. Most of this interaction is isolated within the
const filtersSubscription = context.controlGroupAPI.onFiltersPublished$.subscribe(
(newFilters) => {
stateContainer.internalState.transitions.setCustomFilters(newFilters);
stateContainer.actions.fetchData();
}
);
stateContainer.stateStorage.change$<ControlPanels>(CONTROL_PANELS_URL_KEY)
.subscribe((controlPanels) => {
if (!deepEqual(controlPanels, context.controlPanels)) {
send({ type: 'UPDATE_CONTROL_PANELS', controlPanels });
}
});
stateContainer.stateStorage.set(
CONTROL_PANELS_URL_KEY,
cleanControlPanels(controlsPanelsWithId),
{ replace: true }
); Outside of the
We can see that the Log Explorer currently has a lot of knowledge of the Discover state container. This is in no way meant to criticise the current implementation and interaction with the Discover state container, this was the only way to facilitate the functionality that the Log Explorer needed based on the architecture that existed at the time (and still exists now). Whilst we are somewhat protected by types, there is no "formal" contract here. How can we improve this?Here are some initial ideas. It's important that we try to create a good solution, but one that is only as complicated as we need for now (with the long term goal being all of the Discover components being extracted). Consumer <-> Log Explorer interaction isn't as complicated, here we can likely create an actor / state machine that can handle "talking" our future persistence layer language. When the consumer receives state updates they are completely in control of how these are persisted in the URL, as long as they can be translated back to the Log Explorer interface. The Log Explorer <-> Discover container interaction is more complicated.
This would allow us to move forward in a way that doesn't require big changes to Discover. |
@Kerry350 great analysis 👏 The plan looks good, since we moved the Log Explorer into our own app it makes sense to get more control over the state as explained in your proposal. A quick note on the following:
The |
Ah, thank you for the heads up 🙏 |
Felix and I had some good discussions last week about an architecture we can move forward with, taking into account the following: #165255 (comment) and using the original URL issue as the start point: #165255 (comment). Also taking into account we don't want to make any large changes to Discover, as the future would be using the separate Discover components. This will almost certainly change a bit once implementation begins.
For the following examples we've tried to consider that consumers might not be using state machines, so there should be an agnostic method for accessing state / actions. Here Observables would be the agnostic communication channel (as these can easily be hooked up to state machines if wanted). We'll introduce a controller concept (this is just pseudocode to try and show some concepts): const { state$, event$, actions, controller } = useObservabilityLogExplorerController({
initialState: {
filters: serviceFilters,
timeRange: stateFromUrl.timeRange,
},
options: {
tableRows: {
rowActions: (previousRowActions) => [firstAction, ...previousRowAction],
}
},
}); The controller would essentially take care of instantiating the relevant Log Explorer state machine, observables etc, and can therefore be passed on to the Log Explorer component. For Observability we'll use a specific Obs version. The
Rendering then could loosely look like this:
( A consumer that isn't using state machines is now more or less ready to go, they've passed some initial state (their job to instantiate this), they have actions to call, and they can observe the two observables in the best way they see fit (React hooks, hooking up to Redux etc). This explains the changes for the Log Explorer layer, and lastly we'll need the Observability Log Explorer layer. In the Observability Log Explorer we are using state machines. During the Observability Log Explorer initialisation process we will also create a controller, we'll then store these Observables etc into context (this means we can essentially call the same functions that the hook would call for other consumers). We can then use For this issue specifically we'll focus on adding these changes to facilitate URL state extraction. As such, in the Observability Log Explorer, we'll have a new state machine that handles initialising from the URL, creating a controller, and passing the controller the |
@Kerry350 thanks for the in-depth explanation! A quick question:
const { state$, event$, actions, controller } = useObservabilityLogExplorerController(...);
// `isValidAction(actions.reload)` is meant to control whether the action is already available?
<button disabled={!isValidAction(actions.reload)} onClick={() => actions.reload()}>Reload Logs</button>
// Here actions and state updates are not yet available.
<ObservabilityLogExplorer
controller={controller}
/>
// Here it rerenders because the controller got access to the internal stateContainer and initialized actions, state$ etc. Sorry for the primitive and not structured flow example, it my understanding of how it work correct? |
So, this might end up looking a little different but the idea is that we'd just have a way to know when an action was available / in a valid state to perform. I did a similar thing here in the custom integrations package whereby the "dispatchable event" would be available or
Roughly (again, this is just hypothetical and may look different after code changes) the actions and the state should be immediately available (using "hot" observables), but there just might not be any actions that can be performed yet, and the state may only contain the initial state. However, the
I think the hierarchy will change slightly here. You are right, we need to go through the customisation callback flow, state container setup, etc before we're fully initialised. I need to play around a bit to see how this will look with the new "extended" state container. It's possible this may be instantiated a little earlier. Basically, I don't have a 100% answer on this yet 😅 This should become clearer in implementation (which I've started on today) 🤞 We can always have some additional UI here too, whilst the |
Thanks for clarifying those questions! |
## 📓 Summary Closes #169506 This PR introduces a mechanism to apply customizations on the LogExplorer component. The first necessary customization which is implemented is for the flyout detail, allowing the consumer to display additional content on top of what is already displayed. This is a temporary solution which will be updated and embedded in a more structured customization system as a result of the work done for #165255. The current solution creates already a context to allow granular consumption of the customizations only for those subtrees where a specific customization should apply. The LogAIAssistant is used to customize the current LogExplorer as the first usage of this customization. https://github.com/elastic/kibana/assets/34506779/c9e6b40e-e636-456a-9e19-1778c26142db --------- Co-authored-by: Marco Antonio Ghiani <[email protected]> Co-authored-by: kibanamachine <[email protected]>
Pinging @elastic/obs-ux-logs-team (Team:obs-ux-logs) |
📓 Summary
The
LogExplorer
component that thelog_explorer
plugin exports currently inherits the URL state synchronization from Discover. In order to make it reusable in different UIs we want to prevent it from modifying the URL so the consumer has control over that.✔️ Acceptance criteria
LogExplorer
component doesn't interact with the URL in any way.observability_log_explorer
synchronizes the necessary parts of the state to the URL, such that...Please see #165255 (comment) for more specific implementation details.
💡 Implementation hints
The text was updated successfully, but these errors were encountered: