[Investigations][DataViews] - Remove non-performant toSpec usage

[michaelolo24]

Summary

This PR looks to improve the performance of the application by using DataView in place of DataViewSpec. The currently used DataViewSpec turns out to not be performant at scale because it lacks caching, and performs quite a few nested iterations in the DataView.toSpec call as seen here, here, and finally here. While this may not be a significant issue at a few thousand fields, this does not scale well as the number of fields reaches the tens/hundreds of thousands.

This PR makes the change to rely on the DataView instance directly, which is cached in memory. These performance impacts aren't seen currently as the new framework is currently disabled behind the below feature flag.

** Relevant configurations **
xpack.securitySolution.enableExperimental: ['newDataViewPickerEnabled']

[elasticmachine]

Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations)

[maximpn]

Shouldn't it check newDataViewPickerEnabled flag and pick the corresponding implementation?

Suggested change

	experimentalDataView?.fields?.getByName(EXECUTION_UUID_FIELD_NAME) ||
	oldSourcererDataView.fields?.[EXECUTION_UUID_FIELD_NAME],
	newDataViewPickerEnabled ? experimentalDataView.fields?.getByName(EXECUTION_UUID_FIELD_NAME) : oldSourcererDataView.fields?.[EXECUTION_UUID_FIELD_NAME],

[michaelolo24]

Yep, you're right. I relied on the presence of the DataView at first, but preferred the more explicit approach of using the flag, and just missed this one. Thanks!

[maximpn]

@michaelolo24 Thanks for replacing DataViewSpec with DataView 👍

I tested D&R side and haven't spotted any issues. The changes mostly trivial. I left a comment on possible improvement.

---

Besides that I'm curious that Data View Manager's useDataView hook only accepts dataViewManagerScope. The name sounds generic but the hook handles only sourcerer data views. Should it be renamed to useSourcererDataView?

[PhilippeOberti]

I'd like to offer an alternative here. I'm not a fan of the idea of adding a prop that will be removed (hopefully) soon. Doing it this way will mean that we'll have to modify this file again when we're removing that code.

We could change the existing dataViewSpec prop to dataViewand have all the components handle the conversion from the DataViewSpec to DataView in those instead? That way the code in this combineQueries would be final (or as close as possible to).

If this is not feasible or too complex, then I'd suggest to rename the newly added newDataViewPickerEnabledDataView prop to just dataView and keep it alongside the already existing dataViewSpec one. That way when we're ready to clean all this up we just have to delete the dataViewSpec prop.

I'd also add some comments and maybe a deprecated sign to the dataViewSpec prop in the CombineQueries interface line 30 above in this file?

export interface CombineQueries {
  config: EsQueryConfig;
  dataProviders: DataProvider[];
  /**
   * blah
   */ 
  dataView?: DataView;
  /**
   * @deprecated
   */
  dataViewSpec?: DataViewSpec;
  browserFields: BrowserFields;
  filters: Filter[];
  kqlQuery: Query;
  kqlMode: string;
}

And similar comments within the convertToBuildEsQuery function.

[michaelolo24]

This could be worse from a performance perspective because it would introduce additional logic to those components where they are either requesting the DataView based on the spec or creating it, to match this new API. It shouldn't be a performance hit if those DataViews are cached, but it could be. I can test it, but I actually prefer the second option you propose by deprecating the dataViewSpec. Regarding the name, I actually named it that so it didn't get forgotten or skipped since there would be no way for someone who is unaware of this PR to identify that it should be removed/modified during cleanup? I can also just create a ticket specifically for this 😅 . Anyways, lmk what you think? Will definitely add the deprecation message though 👍🏾

[PhilippeOberti]

Regarding the name, I actually named it that so it didn't get forgotten or skipped since there would be no way for someone who is unaware of this PR to identify that it should be removed/modified during cleanup?

We will eventually want to change that newDataViewPickerEnabledDataView prop into dataView correct? If so I'd name it the correct name right now and be done with it. When we do the cleanup, we only have to remove the dataViewSpec... I feel like if we add documentation in the interfaced we have right now it should be clear to devs what they should use and what will have to be removed, no?

[michaelolo24]

Yep, that works for me!

[PhilippeOberti]

What do you think about making dataView NOT optional here? This way it mimics more how the previous dataViewSpec prop was working...

I made the changes on this PR to your branch, type_check passes locally but have not ran any of the unit tests nor have I tested the UI locally yet.

wdyt?

[michaelolo24]

Done! Thanks for the PR 😄

[michaelolo24]

Besides that I'm curious that Data View Manager's useDataView hook only accepts dataViewManagerScope. The name sounds generic but the hook handles only sourcerer data views. Should it be renamed to useSourcererDataView?

@maximpn The scopes are actually the exact same. When all this work is done, sourcerer is going away in favor of the new DataViewManager. We need to replace the SourcererScopeName with the DataViewManagerScopeName. Will do that in a follow up PR.

[kibanamachine]

Starting backport for target branches: 8.19, 9.1

https://github.com/elastic/kibana/actions/runs/16281944962

[kibanamachine]

💔 All backports failed

Status	Branch	Result
❌	8.19	Backport failed because of merge conflicts You might need to backport the following PRs to 8.19: - [Security Solution][Sourcerer] Add dedicated scope for explore pages (#226545)
❌	9.1	Backport failed because of merge conflicts You might need to backport the following PRs to 9.1: - [Security Solution][Sourcerer] Add dedicated scope for explore pages (#226545) - [Security Solution] Fix initial data view flash (#225675) - [Security Solution] [EUI Refresh] [Explore page] Migrate styled-components (#207318)

Manual backport

To create the backport manually run:

node scripts/backport --pr 225726

Questions ?

Please refer to the Backport tool documentation

[kibanamachine]

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 225726 locally
cc: @michaelolo24

[michaelolo24]

💚 All backports created successfully

Status	Branch	Result
✅	9.1
✅	8.19

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

[kibanamachine]

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.
cc: @michaelolo24