[Discover] Add container for data state (migrated from useSavedSearch hook) #148614

kertal · 2023-01-10T09:29:58Z

Summary

This PR migrates the hook responsible for data fetching in Discover main (use_saved_search.ts) to be part of DiscoverStateContainer as DataStateContainer.

Testing

There shall be no regression, everything should work like before

Reviewing

The majority of files were changed because import paths changed. Sadly GitHub doesn't diff the main change:

The logic of

kibana/src/plugins/discover/public/application/main/hooks/use_saved_search.ts

Line 95 in 08d8555

export const useSavedSearch = ({

was migrated to

kibana/src/plugins/discover/public/application/main/services/discover_data_state_container.ts

Line 118 in 2202d1d

export function getDataStateContainer({

This is part of #142131 and code was extracted of the POC #140765. There will be follow up cleanups once the next part of the POC are migrated.

Checklist

Unit or functional tests were updated or added to match the most common scenarios

…fix'

kertal · 2023-01-16T13:46:38Z

@elasticmachine merge upstream

kertal · 2023-01-16T16:09:45Z

src/plugins/discover/public/application/main/utils/get_fetch_observable.ts

-   * Skip initial fetch when discover:searchOnPageLoad is disabled.
-   */
-  if (initialFetchStatus === FetchStatus.UNINITIALIZED) {
-    fetch$ = fetch$.pipe(skip(1));
-  }


Removed and made more implicit, not to skip a fetch in the given condition, but to fetch when it's needed and not to initially fetch when discover:searchOnPageLoad is disabled

kertal · 2023-01-16T16:17:10Z

src/plugins/discover/public/utils/get_sharing_data.ts

+          ? columns.map((field) => ({ field, include_unmapped: 'true' }))
+          : [{ field: '*', include_unmapped: 'true' }];


Without this change, the CSV export broke.
Why? [{ field: '*', include_unmapped: 'true' }] is necessary to export all the fields given useFieldsApi is true (Which is the default)
Bevor the refactoring of this PR this was added by the code executed in the hook. So it was added before , and at a different part of the code, which made it hard to track & debug

This is now slightly better, more decoupled, independent from previous searchSource mutations

elasticmachine · 2023-01-17T08:23:24Z

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

kertal · 2023-01-17T10:25:37Z

@elasticmachine merge upstream

…-fix'

mattkime

A few minor requests and a couple of items to discuss before I give this my 👍

src/plugins/discover/public/application/main/components/field_stats_table/field_stats_table.tsx

src/plugins/discover/public/application/main/services/discover_data_state_container.test.ts

mattkime · 2023-01-19T03:39:01Z

src/plugins/discover/public/application/main/services/discover_data_state_container.ts

+  /**
+   * Observable triggering fetching data from ES
+   */
+  refetch$: DataRefetch$;


Why is a Subject being used? Is something happening that couldn't be expressed in terms of a function?

We have several observables that can trigger a fetch, and this is one of them:

kibana/src/plugins/discover/public/application/main/utils/get_fetch_observable.ts

Line 40 in 2202d1d

refetch$,

don't think we can change this so easily, but there's already a fetch function exported that's using the observable internally:

kibana/src/plugins/discover/public/application/main/services/discover_data_state_container.ts

Line 89 in 08325ee

fetch: () => void;

I think just this function could be used in the UI. so the Subject would be a internal implementation detail

mattkime · 2023-01-19T03:41:49Z

src/plugins/discover/public/application/main/services/discover_data_state_container.ts

+  /**
+   * Start subscribing to other observables that trigger data fetches
+   */
+  subscribe: () => () => void;


Why is this called externally? Is there a reason to instantiate this object without subscribing?

yes, we currently initialize the discover state container twice, also legacy behavior I'll change in a follow up, it was part of the POC:

first:

kibana/src/plugins/discover/public/application/main/discover_main_route.tsx

Line 110 in 503b466

const { appState } = getDiscoverStateContainer({

second:

kibana/src/plugins/discover/public/application/main/hooks/use_discover_state.ts

Lines 50 to 54 in 2b3a5d2

const stateContainer = useMemo(() => {

const container = getDiscoverStateContainer({

history,

savedSearch,

services,

furthermore it makes sense, since we are subscribing to more store / services / container to orchestrate subscribing.

src/plugins/discover/public/application/main/services/discover_data_state_container.ts

mattkime · 2023-01-19T03:48:30Z

src/plugins/discover/public/application/main/services/discover_data_state_container.ts

+  /**
+   * Observable emitting when a next fetch is triggered
+   */
+  fetch$: DataFetch$;


Its difficult to tell the difference between fetch$ and refetch$ based on name, type, and description. As best I can tell refetch$ is called by external consumers but not fetch$, and external consumers might subscribe to fetch$ but probably not refetch$

I agree, fetch$ was just recently exposed for the histogram, so not exposing refetch$ (but just a fetch function which is more implicit) might make this clearer

update: can't do this, because we need refetch$ here:

kibana/src/plugins/discover/public/application/main/components/field_stats_table/field_stats_table.tsx

Line 147 in 2b3a5d2

const refetch = stateContainer?.dataState.refetch$.subscribe(() => {

)

So I think improving the description would be the way to go ... or naming
refetch$ -> startFetch$ , triggerFetch$
open for suggestions 🙇

mattkime

Changes look good to me and work well.

kertal · 2023-01-22T20:55:33Z

@elasticmachine merge upstream

kibana-ci · 2023-01-22T21:58:28Z

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	443	441	-2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`discover`	395.8KB	392.2KB	-3.6KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`discover`	28.1KB	28.1KB	+14.0B

History

💚 Build #101599 succeeded c808e7e
💚 Build #101298 succeeded 12dbf3a
💚 Build #101210 succeeded 010f5d4
💚 Build #101187 succeeded 47e8f27
💚 Build #100647 succeeded ff88dfa
💚 Build #100571 succeeded 2202d1d

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @kertal

kertal and others added 2 commits January 10, 2023 10:29

Add discover data store

241be2c

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

dbf6ec8

…fix'

kertal changed the title ~~[Discover] Add discover data store~~ [Discover] Add data store Jan 10, 2023

kertal self-assigned this Jan 10, 2023

kertal added Feature:Discover Discover Application Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Jan 10, 2023

kertal added 3 commits January 10, 2023 23:59

improve use_discover_state.ts

ea1f881

fixing tests

d695659

fix sharing

52a68f2

kertal mentioned this pull request Jan 13, 2023

[Unified Histogram] [Discover] Unified Histogram solutions cleanup #146352

Merged

9 tasks

kertal added 2 commits January 16, 2023 11:59

Fix search session

ae290d8

Fix type error

96e32dd

Merge branch 'main' into discover-data-store

19cdbd7

kertal commented Jan 16, 2023

View reviewed changes

Fix tests and add comments

b5f6b59

kertal commented Jan 16, 2023

View reviewed changes

kertal added 3 commits January 16, 2023 18:06

Add documentation

6c6b055

Remove use_saved_search.ts

1e4f2ae

Fix types

2202d1d

kertal mentioned this pull request Jan 17, 2023

[Discover] Refactor and simplify state container #142131

Closed

5 tasks

kertal changed the title ~~[Discover] Add data store~~ [Discover] Add container for data state (migrated from useSavedSearch hook) Jan 17, 2023

kertal added v8.7.0 chore release_note:skip Skip the PR/issue when compiling release notes labels Jan 17, 2023

kertal marked this pull request as ready for review January 17, 2023 08:23

kertal requested a review from a team as a code owner January 17, 2023 08:23

Merge branch 'main' into discover-data-store

ff88dfa

kertal and others added 4 commits January 18, 2023 19:21

Merge main, fix conflicts

08325ee

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

47e8f27

…-fix'

Cleanup code

2b3a5d2

fix

010f5d4

mattkime requested changes Jan 19, 2023

View reviewed changes

kertal added 2 commits January 19, 2023 07:28

Address review comments

30950d5

Improve test

12dbf3a

mattkime approved these changes Jan 19, 2023

View reviewed changes

Merge branch 'main' into discover-data-store

c808e7e

Merge branch 'main' into discover-data-store

0a7c958

kertal merged commit 0755f6d into elastic:main Jan 23, 2023

kibanamachine added the backport:skip This commit does not require backporting label Jan 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover] Add container for data state (migrated from useSavedSearch hook) #148614

[Discover] Add container for data state (migrated from useSavedSearch hook) #148614

kertal commented Jan 10, 2023 •

edited

Loading

kertal commented Jan 16, 2023

kertal Jan 16, 2023

kertal Jan 16, 2023

elasticmachine commented Jan 17, 2023

kertal commented Jan 17, 2023

mattkime left a comment

mattkime Jan 19, 2023

kertal Jan 19, 2023

mattkime Jan 19, 2023

kertal Jan 19, 2023

mattkime Jan 19, 2023

kertal Jan 19, 2023

mattkime left a comment

kertal commented Jan 22, 2023

kibana-ci commented Jan 22, 2023

		? columns.map((field) => ({ field, include_unmapped: 'true' }))
		: [{ field: '*', include_unmapped: 'true' }];

	const stateContainer = useMemo(() => {
	const container = getDiscoverStateContainer({
	history,
	savedSearch,
	services,

[Discover] Add container for data state (migrated from useSavedSearch hook) #148614

[Discover] Add container for data state (migrated from useSavedSearch hook) #148614

Conversation

kertal commented Jan 10, 2023 • edited Loading

Summary

Testing

Reviewing

Checklist

kertal commented Jan 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jan 17, 2023

kertal commented Jan 17, 2023

mattkime left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattkime left a comment

Choose a reason for hiding this comment

kertal commented Jan 22, 2023

kibana-ci commented Jan 22, 2023

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

History

kertal commented Jan 10, 2023 •

edited

Loading