[Embeddables] Decouple fetching from search sessions

[Heenawter]

Closes #239610

Summary

This PR decouples fetching from search session IDs - i.e. we no longer need the search session ID to change in order to trigger a refetch. This unblocks #233124, since making a selection in a section-scoped control should not trigger a new search session but it should cause a refetch for the targeted panels. This is also just a cleaner implementation overall, since search sessions should not drive fetching.

Note that, in order to ensure that fetching still happens after the search session ID changes (so that embeddables receive the up-to-date ID and no double fetch happens), I had to add an async callback that allows the Dashboard to let the fetch$ observable know when the search session ID is stable.

Checklist

• Unit or functional tests were updated or added to match the most common scenarios
• The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines
• Review the backport guidelines and apply applicable backport:* labels.

[Heenawter]

@jughosta This is the problem that @ThomThomson was messaging you about.

Basically, as a summary of the problem: in this PR, we are making it so that a change to the search session ID does not trigger the fetch$ observable for embeddables. Unfortunately, the LensRenderer component that is used for the Discover histogram was relying on this behaviour when hitting refresh if Discover had an absolute time range.

In the Lens app, they accounted for this by adding a lastReloadRequestTime prop to the LensRenderer component that triggers a reload:

kibana/x-pack/platform/plugins/shared/lens/public/react_embeddable/renderer/lens_custom_renderer_component.tsx

Lines 93 to 95 in 38ada44

	useEffect(() => {
	reload$.next();
	}, [reload$, lastReloadRequestTime]);

However, Discover wasn't using this. So this is my attempt to plug it in to that system. Let me know if you can think of a better approach :)

[jughosta]

Hi @Heenawter,

Thanks for reaching out!

Trying to understand the changes now. So, introducing lastReloadRequestTime$ in the Discover state management helps to prevent fetches when only the search session id changes on Dashboard page, right? Can you please point to the lines of code which control this?

What is the reason for also modifying the Unified Histogram code in this PR? If it's not blocking I would rather consider applying these changes in a separate PR as more testing (and possibly more changes) would be needed. The Discover main request is not the only reason why we would want the histogram to refetch. There are other triggers which would not update lastReloadRequestTime$ in the same way.

[Heenawter]

Previously, the embeddable fetch$ observable required that the search session ID changed in order to trigger a refetch. This PR removes this necessity - instead, we rely on filter, query, and time range changes in order to reload the data (plus we have a manual reload that can be triggered via the embeddable API). So consider the following scenario:

1. You open Discover and set an absolute time range
2. You press the refresh button
3. Before this PR, pressing the refresh button generated a new search session ID - since everything else is equal (no time range changes due to the absolute time range, and filters and queries also didn't change since this is a manual refresh) this is what caused the histogram to refresh

If I included the fetch$ changes without the Discover changes in this PR, the behaviour was as follows:

1. You open Discover and set an absolute time range
2. You press the refresh button
3. Filters, queries, and time range are all equal, so fetch$ does not emit.
4. The Discover histogram does not refetch, because nothing told it to 🔥

Now, consider the behaviour with the Discover + histogram changes:

1. You open Discover and set an absolute time range
2. You press the refresh button
3. This still generates a new session ID in Discover, but that is no longer enough to trigger a refresh. Instead, pressing the refresh button causes Discover to send in a new lastReloadRequestTime to the histogram component
4. This causes the following useEffect to run, and calling reload$.next() causes fetch$ to emit - so, the histogram reloads as you would expect 🎉

kibana/x-pack/platform/plugins/shared/lens/public/react_embeddable/renderer/lens_custom_renderer_component.tsx

Lines 93 to 95 in 38ada44

	useEffect(() => {
	reload$.next();
	}, [reload$, lastReloadRequestTime]);

If it's not blocking I would rather consider applying these changes in a separate PR

These changes are necessary in order to ensure that the Discover histogram gets properly refreshed when the "refresh" button is clicked. Without them, Discover is broken due to my changes to the fetch$ observable. So we cannot separate them out.

There are other triggers which would not update lastReloadRequestTime$ in the same way.

Can you give examples of when the histogram needs to refresh from other than filter + time range + query changes and/or a manual refresh? All of the listed scenarios should still work. And this PR only impacts fetches triggered from unified search - breakdown field, interval change, etc. are not driven by the fetch$ observable, so they are not impacted by this at all.

[jughosta]

I see, thanks for explaining!

In addition to the breakdown field and interval changes (which ideally should not trigger the main Discover request, unfortunately they do now), we have the following triggers:

• opening the histogram panel after it was collapsed
• triggering a histogram update only after the main Discover ES|QL request is finished (in KQL mode both the main request and the histogram requests run in parallel, in ES|QL mode the histogram request is deferred).

And restoring a search session should not trigger a new histogram request even if lastReloadRequestTime is different.

I will continue testing and reviewing the PR.

[Heenawter]

opening the histogram panel after it was collapsed

I checked main and this does not currently trigger a refetch. So the behaviour on this PR is the same from what I can tell.

triggering a histogram update only after the main Discover ES|QL request is finished

I am seeing two esql_async requests on both main and this PR when I update the ES|QL query. From what I can tell, we aren't seeing any duplicated requests.

And restoring a search session should not trigger a new histogram request even if lastReloadRequestTime is different.

I am seeing the same behaviour on this PR and main here. When I restore a session that is different than my current one, I see a refetch - and when I try to restore the same session I am already running, I see no network requests.

[davismcphee]

(plus we have a manual reload that can be triggered via the embeddable API)

I like the sound of this better, presumably a much bigger change though? In an ideal situation, we'd be able to disable auto fetching on prop changes and imperatively control when fetching occurs.

About the lastReloadRequestTime changes specifically, do we need to manage this state in Discover and pass it all the way down? Would it work to just add lastReloadRequestTime: Date.now() to buildLensProps? We already memoize these props to control Lens fetches and avoid over-fetching:

kibana/src/platform/packages/shared/kbn-unified-histogram/components/chart/hooks/use_lens_props.ts

Lines 51 to 68 in 4a3776f

	const buildLensProps = useCallback(() => {
	if (!visContext) {
	return;
	}
	const { attributes, requestData } = visContext;
	return {
	requestData: JSON.stringify(requestData),
	lensProps: getLensProps({
	searchSessionId: request?.searchSessionId,
	getTimeRange,
	attributes,
	esqlVariables,
	onLoad,
	}),
	};
	}, [visContext, request?.searchSessionId, getTimeRange, esqlVariables, onLoad]);

[Heenawter]

I like the sound of this better, presumably a much bigger change though? In an ideal situation, we'd be able to disable auto fetching on prop changes and imperatively control when fetching occurs.

This would be a lot more work, yeah. We would have to surface the Lens API all the way from LensRenderer -> kbn-unified-histogram -> Discover. Considering this PR is blocking some very important Controls Anywhere work, I would rather that be done as a follow up 😅

Would it work to just add lastReloadRequestTime: Date.now() to buildLensProps

This felt less technically correct to me but if ya'll prefer that as a solution, we can go with it :) Done in dae4f42

[Heenawter]

@davismcphee I tried the solution above, but it resulted in a legitimate error in src/platform/packages/shared/kbn-unified-histogram/components/chart/histogram.test.tsx where lastReloadRequestTime was triggering the Lens props to change without an explicit fetch$

I have reverted the changes and restored the original approach. 7cfa8c3

[davismcphee]

Thanks for giving it a shot anyway! I must have been overlooking something. While I'm not a huge fan of the lastReloadRequestTime thing, I tested a bit locally and I'm not noticing any changes from main.

I'll leave the final review to @jughosta though since she's working in this area atm and will have a better sense of things.

[elasticmachine]

Pinging @elastic/kibana-presentation (Team:Presentation)

[ThomThomson]

Changes LGTM! I focused mostly on the fetch reorganization, which is looking great! Not approving because it seems like Nathan is also looking at this PR at the same time, so will let him finish his review and approve!

[nreese]

kibana-presentation changes LGTM
code review only

[jughosta]

LGTM 👍
I have not noticed any issues when testing on Discover page.