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] Extend URL state by selected data source #181963

Closed
Tracked by #181728
kertal opened this issue Apr 29, 2024 · 1 comment · Fixed by #183108
Closed
Tracked by #181728

[Discover] Extend URL state by selected data source #181963

kertal opened this issue Apr 29, 2024 · 1 comment · Fixed by #183108
Assignees
Labels
enhancement New value added to drive a business result Project:OneDiscover Enrich Discover with contextual awareness / Merge with Logs Explorer Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Data, DataViews)

Comments

@kertal
Copy link
Member

kertal commented Apr 29, 2024

📓 Summary

Today Discover supports two different data sources:

  • ES|QL query
  • Data view (persisted and ad hoc)
  • Saved search (contains an ES|QL query or data view reference) Saved search is not a data source itself, but contains a data source (ES|QL query or data view)

For One Discover, this list will need to extend to an unknown number of data sources, which the current URL structure and application state are not well suited for. To support this requirement, we will introduce an abstraction called a “data source” which represents some target for data exploration in Discover.

Supported data sources will be modelled as a union type, and passed to Discover on navigation through the URL. This is an example of what this type might look like:

type DiscoverDataSource =
  | { type: 'esql'; }
  | { type: 'dataView'; dataViewId: string };

The data source abstraction will later be used as part of the context resolution process, but the first step is to update Discover's URL handling and state management to support this concept.

✔ Acceptance Criteria

  • The new data source abstraction should support both current data sources (ES|QL query and data view)
  • URLs using the legacy Discover format should continue working and redirect to the new URL structure
  • The Discover application state should work with the new structure and continue syncing with the URL
@botelastic botelastic bot added the needs-team Issues missing a team label label Apr 29, 2024
@kertal kertal added Project:OneDiscover Enrich Discover with contextual awareness / Merge with Logs Explorer Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Data, DataViews) labels Apr 29, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@botelastic botelastic bot removed the needs-team Issues missing a team label label Apr 29, 2024
@kertal kertal added the enhancement New value added to drive a business result label Apr 29, 2024
davismcphee added a commit that referenced this issue May 3, 2024
#182063)

## Summary

These commits are split out from my working branch for #181963 to make
reviewing easier.

Our state management code is currently split between a folder called
`services` and the `utils` folder. This makes working with the code more
difficult, especially where One Discover will soon introduce additional
complexity. To make the state management code easier to work with, I
moved all of the related files into a single folder called
`state_management` with a `utils` subfolder for its utility functions.

Similarly, all of our data fetching code was spread between files in the
`utils` folder. For the same reason, I moved these files into a new
folder called `data_fetching`, since they will also undergo a lot of
changes for One Discover.

Part of #181963.

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
davismcphee added a commit that referenced this issue May 15, 2024
## Summary

This PR replaces the `index` property of Discover's app state with a new
`dataSource` property, containing two initial types: `dataView` and
`esql`. The new data source abstraction will be used in the One Discover
data source context resolution process, as well as preparing Discover to
support additional data sources as needed in the future. Additionally,
it creates a clearer division in the Discover state between "data view
mode" and "ES|QL mode", where previously this was implicit based on
whether the `query` property was an ES|QL query vs KQL or Lucene.

The goal of this PR is to add initial support for the `dataSource`
property without introducing broader changes to the Discover state
management. However, these changes open up opportunities for future
improvements to simplify the state management, such as checking the data
source type to determine which mode Discover is in instead of always
checking the query directly. These types of enhancements can be built on
this foundation in followup PRs.

Part of #181963.

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
davismcphee added a commit that referenced this issue May 18, 2024
## Summary

This PR updates Discover to rely on `dataSource` for determining if
ES|QL mode is enabled instead of relying on `query` directly. This
creates a clearer separation between the modes and moves Discover toward
supporting multiple data source types. It also includes a number of
cleanups around ES|QL mode that make the intent of the code clearer and
removes tech debt / code duplication.

Changes included in the PR:
- Add a new `useIsEsqlMode` hook that relies on `dataSource` for
checking if Discover is in ES|QL mode.
- Remove "raw record type" concept in Discover and replace it with ES|QL
`dataSource` checks.
- Remove references to `isPlainRecord` and replace them with ES|QL
`dataSource` checks.
- Remove `isTextBasedQuery` utility function and replace it with ES|QL
`dataSource` checks or `isOfAggregateQueryType` where casting is
necessary.
- Replace references to `isOfAggregateQueryType` with ES|QL `dataSource`
checks except where casting is necessary.
- Replace other references to "text based" with "ES|QL" for clarity and
consistency.

Closes #181963.

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [ ] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)

### For maintainers

- [ ] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Project:OneDiscover Enrich Discover with contextual awareness / Merge with Logs Explorer Team:DataDiscovery Discover App Team (Document Explorer, Saved Search, Surrounding documents, Data, DataViews)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants