[Discover][ES]QL] Ensure the same time range is being used for documents and histogram #203215

kertal · 2024-12-06T09:05:01Z

Summary

Resolves #201128

This PR fixes the case a relative time range is set (for example "Last 15 minutes") and the time range goes out of sync for table and histogram requests on Discover in ES|QL mode.

The main change is here:

export interface DataMainMsg extends DataMsg {
   foundDocuments?: boolean;
   timeRange?: TimeRange;
   timeRangeRelative?: TimeRange;
 }

By extending the DataMainMsg with timeRange, timeRangeRelative, and consuming these values in subsequent code relevant for data fetching, we make sure, no relative timeRange is being converted with a delay causing a slightly different absolute timeRange. These values are set when the data fetching starts, and the way the code was rewritten also will lead to less redundant requests caused by the Histogram in Discover, but it doesn't fully resolve the issue #165192 (which I thought it does initially, built on the given approach to reduce the sources of truth for all relevant state for data fetching, I'm confident this can be resolved longer down the road)

Testing

Set a relative time range in Discover in ES|QL mode
Observe via Inspect that time range filter is the same for Table and Visualization calls.

Checklist

Unit or functional tests were updated or added to match the most common scenarios
Flaky Test Runner was used on any tests changed

kertal · 2024-12-06T09:18:58Z

/ci

kertal · 2024-12-06T10:50:20Z

/ci

…-fix'

kertal · 2024-12-06T11:37:56Z

/ci

…na into discover-histogram-debugging

kertal · 2024-12-06T13:02:58Z

/ci

kertal · 2024-12-06T15:44:15Z

/ci

davismcphee · 2024-12-06T16:46:00Z

Thanks for investigating! This approach seems really similar to what I was thinking in #201128 (comment), and I wonder if it addresses that issue too? I know this investigation is mainly about the extra fetches, but maybe it's a two birds (or flies 🙂) at once kinda thing.

kertal · 2024-12-06T18:05:22Z

Thanks for investigating! This approach seems really similar to what I was thinking in #201128 (comment), and I wonder if it addresses that issue too? I know this investigation is mainly about the extra fetches, but maybe it's a two birds (or flies 🙂) at once kinda thing.

Yes, it seems to fix #201128 and #165192 , and CI is green?

In any case I think it should be a relatively easy issue to resolve (famous last words) @davismcphee

You're right, and it wasn't even my intention to fix this (if this is true), I just wanted to see if #201128 was still valid with all the changes we applied recently.

So I did use the main$ observable to store timerange, which is basically what you suggested with internalState, but it's the same concept ... and we can do more to simplify, I think

src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts

src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts

…debugging

kertal · 2024-12-10T11:11:53Z

/ci

…debugging

kertal · 2024-12-11T13:39:20Z

/ci

kertal · 2024-12-11T16:20:30Z

/ci

kibanamachine · 2024-12-11T17:53:03Z

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7580

[✅] test/functional/apps/discover/group3/config.ts: 25/25 tests passed.
[✅] test/functional/apps/discover/esql/config.ts: 25/25 tests passed.

see run history

elasticmachine · 2024-12-11T18:26:16Z

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

jughosta · 2024-12-12T10:24:25Z

src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts

+    timeRange,
+    relativeTimeRange: timeRangeRelative,


Can we have just one time range which is in absolute format? The relative one was probably necessary in the histogram for the previous approach but now it might be redundant.

I had a look at it, but I don't think we can currently do this because we are using the relative time range for e.g. the edit visualization

kibana/src/plugins/unified_histogram/public/layout/layout.tsx

Lines 84 to 87 in 61d0320

/**

* The relative time range, used when timeRange is an absolute range (e.g. for edit visualization button)

*/

relativeTimeRange?: TimeRange;

I think that's used e.g. for the time range when clicking on the Lens button in DataView mode

jughosta · 2024-12-12T10:30:59Z

src/plugins/discover/public/application/main/data_fetching/update_search_source.ts

@@ -48,7 +50,7 @@ export function updateVolatileSearchSource(

  if (dataView.type !== DataViewType.ROLLUP) {
    // Set the date range filter fields from timeFilter using the absolute format. Search sessions requires that it be converted from a relative range
-    const timeFilter = data.query.timefilter.timefilter.createFilter(dataView);
+    const timeFilter = data.query.timefilter.timefilter.createFilter(dataView, inputTimeRange);


I think we need to be passing a time range to the histogram which is a result of createFilter instead of the input time range because the bounds can be different due to its internal logic.

maybe the naming is misleading, because the input here is

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

Line 242 in 1023a8e

timefilter.getAbsoluteTime(),

And getAbsoluteTime takes care of the bounds and considering now

elasticmachine · 2024-12-17T08:16:12Z

💚 Build Succeeded

Buildkite Build
Commit: 1a22e75

Metrics [docs]

Async chunks

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

id	before	after	diff
`discover`	786.1KB	786.4KB	+306.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`discover`	18	17	-1

Total ESLint disabled count

id	before	after	diff
`discover`	20	19	-1

History

💚 Build #260720 succeeded b8261ec
💚 Build #259405 succeeded 7d1430d
💔 Build #259310 failed 7fa23e3
💔 Build #258656 failed 19bdbff
💚 Build #257862 succeeded 51fbf2e
💔 Build #257816 failed 7d0d206

cc @kertal

jughosta · 2024-12-17T10:24:00Z

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

+  timeRange?: TimeRange;
+  timeRangeRelative?: TimeRange;


I would rather call it differently:

Suggested change

timeRange?: TimeRange;

timeRangeRelative?: TimeRange;

timeRangeAbsolute?: TimeRange;

timeRangeOriginal?: TimeRange;

Btw is it correct that they are optional?

jughosta · 2024-12-17T13:33:56Z

src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts

@@ -57,7 +80,7 @@ export function sendLoadingMsg<T extends DataMsg>(
  props?: Omit<T, 'fetchStatus'>
 ) {
  if (data$.getValue().fetchStatus !== FetchStatus.LOADING) {
-    data$.next({ ...props, fetchStatus: FetchStatus.LOADING } as T);
+    data$.next({ ...data$.getValue(), ...props, fetchStatus: FetchStatus.LOADING } as T);


Might be better to spread only the new props. I am not sure how it would affect the app otherwise. Don't want surprises 😀

jughosta · 2024-12-17T13:35:56Z

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

@@ -235,6 +237,12 @@ export function getDataStateContainer({

          abortController?.abort();
          abortControllerFetchMore?.abort();
+          sendFetchStartMsg(


I think it should be after if (options.fetchMore) { block as fetching more should be based on the original time range.

jughosta · 2024-12-17T13:42:11Z

src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts

+    ...main$.getValue(),
+    timeRange,
+    timeRangeRelative,
+    query,


documents$ already has query but it's being set after documents are loaded. I anticipate it becoming confusing if we introduce query for main$ too but it would be set before the loading even starts. What are we trying to solve by adding query with a different logic here?

jughosta · 2024-12-17T13:47:30Z

src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts

+  timeRangeRelative: TimeRange,
+  query: AggregateQuery | Query | undefined
+) {
+  main$.next({


I think we break "state machine" concept by editing props without changing fetchStatus. We could either introduce a new state or incorporate this new data into LOADING event.

Or create another observable like searchRequestParams$ instead of modifying main$ or documents$.

jughosta · 2024-12-17T14:13:43Z

src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts

-    timefilter.getTimeUpdate$().pipe(map(() => timefilter.getTime())),
-    timefilter.getTime()
-  );
+  const { timeRangeRelative, timeRange, query } = useObservable(main$, main$.getValue());


I am seeing a delay in screen updates. Like it briefly shows "Searching" message and then pauses with a white screen. And only after some moments it shows loading indicators in the histogram and table sections. Can it be related to these changes?

jughosta · 2024-12-17T14:17:04Z

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

+            timefilter.getAbsoluteTime(),
+            timefilter.getTime(),
+            appStateContainer.getState().query
+          );


Would not sendResetMsg erase it?

kertal · 2024-12-19T10:53:41Z

closing in favor of #204694

Initial histogram debugging

a472a2a

Fixing tests and lining

3e0ffc7

kertal mentioned this pull request Dec 6, 2024

[Unified histogram][ES|QL] Extra fetches in specific scenarios #165192

Closed

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

e5b1b4c

…-fix'

kertal added 2 commits December 6, 2024 14:01

Fix test

0204ac5

Merge branch 'discover-histogram-debugging' of github.com:kertal/kiba…

7d0d206

…na into discover-histogram-debugging

Fix functional test

51fbf2e

kertal commented Dec 6, 2024

View reviewed changes

src/plugins/discover/public/application/main/components/layout/use_discover_histogram.ts Outdated Show resolved Hide resolved

kertal commented Dec 6, 2024

View reviewed changes

src/plugins/discover/public/application/main/hooks/use_saved_search_messages.ts Outdated Show resolved Hide resolved

kertal added 4 commits December 6, 2024 23:23

Merge remote-tracking branch 'upstream/main' into discover-histogram-…

2c39b21

…debugging

Rename to sendFetchStartMsg

afe95ca

Update sendResetMsg

81af64a

Update use_discover_histogram.ts

19bdbff

kertal added 3 commits December 10, 2024 15:50

iterate

1023a8e

Add tests

87a3fc4

Merge remote-tracking branch 'upstream/main' into discover-histogram-…

044ad48

…debugging

kertal self-assigned this Dec 11, 2024

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. release_note:fix labels Dec 11, 2024

kertal changed the title ~~Initial histogram debugging~~ [Discover][ES]QL] Ensure the same time range is being used for documents and histogram Dec 11, 2024

Add tests

7fa23e3

kertal added 2 commits December 11, 2024 17:01

Merge main, fix conflict

529222a

Fix tests

7d1430d

kertal marked this pull request as ready for review December 11, 2024 18:26

kertal requested a review from a team as a code owner December 11, 2024 18:26

kertal added the backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) label Dec 12, 2024

jughosta reviewed Dec 12, 2024

View reviewed changes

kertal requested a review from jughosta December 13, 2024 08:18

kertal added 2 commits December 16, 2024 11:16

Merge branch 'main' into discover-histogram-debugging

b8261ec

Merge branch 'main' into discover-histogram-debugging

1a22e75

jughosta reviewed Dec 17, 2024

View reviewed changes

kertal closed this Dec 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Discover][ES]QL] Ensure the same time range is being used for documents and histogram #203215

[Discover][ES]QL] Ensure the same time range is being used for documents and histogram #203215

kertal commented Dec 6, 2024 •

edited

Loading

kertal commented Dec 6, 2024

kertal commented Dec 6, 2024

kertal commented Dec 6, 2024

kertal commented Dec 6, 2024

kertal commented Dec 6, 2024

davismcphee commented Dec 6, 2024

kertal commented Dec 6, 2024

kertal commented Dec 10, 2024

kertal commented Dec 11, 2024

kertal commented Dec 11, 2024

kibanamachine commented Dec 11, 2024

elasticmachine commented Dec 11, 2024

jughosta Dec 12, 2024

kertal Dec 12, 2024

jughosta Dec 12, 2024

kertal Dec 12, 2024

elasticmachine commented Dec 17, 2024

ESLint disabled line counts

Total ESLint disabled count

jughosta Dec 17, 2024

jughosta Dec 17, 2024

jughosta Dec 17, 2024

jughosta Dec 17, 2024

jughosta Dec 17, 2024

jughosta Dec 17, 2024

jughosta Dec 17, 2024

jughosta Dec 17, 2024

kertal commented Dec 19, 2024

	/**
	* The relative time range, used when timeRange is an absolute range (e.g. for edit visualization button)
	*/
	relativeTimeRange?: TimeRange;

[Discover][ES]QL] Ensure the same time range is being used for documents and histogram #203215

[Discover][ES]QL] Ensure the same time range is being used for documents and histogram #203215

Conversation

kertal commented Dec 6, 2024 • edited Loading

Summary

Checklist

kertal commented Dec 6, 2024

kertal commented Dec 6, 2024

kertal commented Dec 6, 2024

kertal commented Dec 6, 2024

kertal commented Dec 6, 2024

davismcphee commented Dec 6, 2024

kertal commented Dec 6, 2024

kertal commented Dec 10, 2024

kertal commented Dec 11, 2024

kertal commented Dec 11, 2024

kibanamachine commented Dec 11, 2024

Flaky Test Runner Stats

🎉 All tests passed! - kibana-flaky-test-suite-runner#7580

elasticmachine commented Dec 11, 2024

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

elasticmachine commented Dec 17, 2024

💚 Build Succeeded

Metrics [docs]

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

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

Choose a reason for hiding this comment

kertal commented Dec 19, 2024

kertal commented Dec 6, 2024 •

edited

Loading