-
Notifications
You must be signed in to change notification settings - Fork 8k
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+] Add All entry for DatasetSelector #160971
[Logs+] Add All entry for DatasetSelector #160971
Conversation
…lections and be controlled
Pinging @elastic/infra-monitoring-ui (Team:Infra Monitoring UI) |
Pinging @elastic/fleet (Team:Fleet) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
public static createAllLogsDataset() { | ||
return new Dataset({ | ||
name: 'logs-*-*' as IndexPattern, | ||
title: 'All log datasets', |
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.
question: shall we translate this title or can it be a static value as the other dataset names?
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 would suggest translating it since you are already translating the selector header Select dataset
|
||
export function DatasetSelector({ | ||
datasets, | ||
datasetsError, | ||
initialSelected, | ||
datasetSelection, |
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 component relies on this controlled property to present what the current selection is, while the state machine will notify with a new selection instance anytime the user clicks on an entry.
storeAllSelection: assign((_context) => ({ | ||
selection: AllDatasetSelection.create(), | ||
})), | ||
storeSingleSelection: assign((_context, event) => | ||
'dataset' in event ? { selection: SingleDatasetSelection.create(event.dataset) } : {} |
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 want to store the correct DatasetSelection instance depending on the selection mode the user can select.
These actions are triggered uniquely when a new selection is applied and work as a setter for the dataset selection strategy.
…iani/kibana into 160146-all-entry-dataset-selector
* until we handle the restore/initialization of the dataview with https://github.com/elastic/kibana/issues/160425, | ||
* where this value will be used to control the DatasetSelector selection with a top level state machine. | ||
*/ | ||
const [datasetSelection, setDatasetSelection] = useState<DatasetSelection>(() => |
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.
note: as mentioned above, this state is only used to control the selection state, in the next task we'll move this logic and its initialization to a state machine that restores from the URL instead of hardcoding the AllDatasetSelection selection type.
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 encoding basically does these steps in order (reversed for decoding):
- Validate that the object selection input is in a valid format.
- Rison encodes the selection.
- compressToBase64 the rison-encoded value
Although lz-string
provides methods with a higher compression rate, the resulting string would be a non-ASCII string. If there is any better option to update these encoding/decoding I'll be happy to explore the options!
import { SingleDatasetSelection } from './single_dataset_selection'; | ||
import { DatasetSelectionPlain } from './types'; | ||
|
||
export const hydrateDatasetSelection = (datasetSelection: DatasetSelectionPlain) => { |
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 added in preparation for the following task, where we'll need to transform it into a DatasetSelection instance once the URL state is correctly restored.
export type SingleDatasetSelectionPayload = rt.TypeOf<typeof singleDatasetSelectionPayloadRT>; | ||
export type DatasetSelectionPlain = rt.TypeOf<typeof datasetSelectionPlainRT>; | ||
|
||
export interface DatasetSelectionStrategy { |
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 the contract definition that any existing or future selection mode should respect.
|
||
public static createAllLogsDataset() { | ||
return new Dataset({ | ||
name: 'logs-*-*' as IndexPattern, |
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.
is there a way we can set remote_cluster:logs-*-*
?
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.
At the current stage, we won't allow customizing the datasets also to access remote clusters but is something we'll maybe allow the user to customize in future.
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 set the index pattern to [remote_cluster:logs-*-*, logs-*-*]
? without any customizations
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.
As it is for the current Logs UI implementation, it is possible that a remote cluster is not configured or reachable.
I'm not sure what is the plan in long term, but currently we are not allowing to access the remote clusters from this dataset selector on serverless enviroment.
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.
LGTM! Clean work
Tested it and works perfectly 👍 Do we plan on adding tests for the whole component?
import { DataViewSpec } from '@kbn/data-views-plugin/common'; | ||
import { IndexPattern } from '@kbn/io-ts-utils'; |
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.
would adding type
here make sense ?
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.
It is not necessary as IndexPattern as purely a type with no runtime code, it is exported using the export type
syntax, which guarantees it gets removed from the bundle.
Here is the detail on when is useful to use the import type
syntax.
Absolutely, I created an issue to track what tests we need as soon as we have are done with this initial MVP set of features 👌 |
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsESLint disabled in files
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
## 📓 Summary Closes elastic#160146 This PR adds the entry to allow users to select a dataset that creates a `logs-*-*` dataview. Although the presentational and UX changes are minimal for the user, this work lays the foundation for [restoring the dataset selection from the URL](elastic#160425) and for the [dataset multi-selection feature](https://github.com/elastic/observability-dev/issues/2744). The core changes for this implementation consist of: - Update DatasetSelector state machine to manage two parallel states: a `popover` one to manage the navigation on the selector and a `selection` state that handles the selection modes (currently only `all` and `single`, but ready to also implement `multi`) <img width="1522" alt="state-machine" src="https://github.com/elastic/kibana/assets/34506779/c240e5d5-6a38-4d08-b893-117132477896"> - DatasetSelector is now a controlled component regarding the selection value: it will react to the injected `datasetSelection` property, and notify the parent component of any change with the `onSelectionChange` event handler. This will allow us to always reinitialize the DatasetSelector state machine from the URL state and fall back to the chosen selection in case there is no one to restore. https://github.com/elastic/kibana/assets/34506779/4887b1d4-63ba-476b-a74f-5b4a9504f939 ## Architectural choices - The state machine will handle the two states in parallel such that we can switch to available selection modes depending on the interactions without clashing with the independent selector navigation. - The `DatasetSelection` data structure is now what represents the state selection in different modes. The three available modes (counting also `multi`, but not implemented yet), differs in mostly any aspect: - Internal data structure shape - DataViewSpecs composition - Payload to encode into a URL state - Extraction of the presentational values (title, icons, and with `multi` also the datasets which are currently selected) With all these differences but the same final purposes of creating an ad-hoc DataView and storing encoding a URL state to store, applying the concepts for the Strategy pattern seemed the most sensed option to me. This allows us to scale and add new selection modes (`multi`) respecting the existing strategies contract and encapsulating all the concerned transformations and parsing. Encoding and decoding of the Dataview id that will be used for restoring from the URL are already created and will be handy for the upcoming tasks. --------- Co-authored-by: Marco Antonio Ghiani <marcoantonio.ghiani@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
## 📓 Summary Closes #160425 After the [first implementation of the log-explorer profile](#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](#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 <marcoantonio.ghiani@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
## 📓 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 <marcoantonio.ghiani@elastic.co> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
📓 Summary
Closes #160146
This PR adds the entry to allow users to select a dataset that creates a
logs-*-*
dataview.Although the presentational and UX changes are minimal for the user, this work lays the foundation for restoring the dataset selection from the URL and for the dataset multi-selection feature.
The core changes for this implementation consist of:
popover
one to manage the navigation on the selector and aselection
state that handles the selection modes (currently onlyall
andsingle
, but ready to also implementmulti
)datasetSelection
property, and notify the parent component of any change with theonSelectionChange
event handler.This will allow us to always reinitialize the DatasetSelector state machine from the URL state and fall back to the chosen selection in case there is no one to restore.
all-datasets.mov
Architectural choices
DatasetSelection
data structure is now what represents the state selection in different modes.The three available modes (counting also
multi
, but not implemented yet), differs in mostly any aspect:multi
also the datasets which are currently selected)With all these differences but the same final purposes of creating an ad-hoc DataView and storing encoding a URL state to store, applying the concepts for the Strategy pattern seemed the most sensed option to me. This allows us to scale and add new selection modes (
multi
) respecting the existing strategies contract and encapsulating all the concerned transformations and parsing.Encoding and decoding of the Dataview id that will be used for restoring from the URL are already created and will be handy for the upcoming tasks.