[FilterEditor] Load DataView in case it's not provided by the consuming plugin #173017

kertal · 2023-12-11T05:58:05Z

Summary

In Discover you can create filters for the current selected data view, switch data view, create another filter.
When you try to edit the first filter being created with the previous data view, it can no longer be edited, since the data view is no longer available in this context. Here is a screen cap by @drewdaemon showing this situation:

Screen.Recording.2023-04-11.at.4.38.34.PM.mov

With this PR this is resolved by actually loading the missing data view, so users can edit this Filter. Note that it doesn't resolve this issue when the filter was created by an ad-hoc data view, and it was not persisted, and the browser window was refreshed. This case we can't fix, unless we start to add e.g. the index pattern to the created filter. But this is out of scope of this PR

Checklist

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

kertal · 2023-12-11T08:27:31Z

/ci

…fix'

…er-editor-changes

kertal · 2023-12-12T15:08:26Z

/ci

…fix'

kertal · 2023-12-12T17:18:56Z

@elasticmachine merge upstream

kertal · 2023-12-12T17:19:05Z

/ci

…er-editor-changes # Conflicts: # src/plugins/unified_search/tsconfig.json

kertal · 2023-12-12T21:28:16Z

/ci

kertal · 2023-12-13T08:51:37Z

/ci

elasticmachine · 2023-12-13T15:26:25Z

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

src/plugins/unified_search/public/filter_bar/filter_editor/filter_editor.tsx

kertal · 2023-12-13T16:20:24Z

@elasticmachine merge upstream

kertal · 2024-01-08T19:35:03Z

@elasticmachine merge upstream

kertal · 2024-01-09T08:35:40Z

src/plugins/unified_search/public/filter_bar/filter_item/filter_items.tsx

@@ -84,6 +84,7 @@ const FilterItemsUI = React.memo(function FilterItemsUI(props: FilterItemsProps)
          readOnly={readOnly}
          suggestionsAbstraction={props.suggestionsAbstraction}
          filtersCount={props.filters.length}
+          dataViews={data?.dataViews}


depending on the context relying on the data service cause functional test errors, same issue I encountered using the dataViews service directly.

kertal · 2024-01-09T09:54:44Z

@elasticmachine merge upstream

kertal · 2024-01-09T14:38:57Z

@elasticmachine merge upstream

smith

Exploratory view changes look good. 👍🏻

drewdaemon

This definitely resolves this issue. I initially expected you to load all the data views and pass them to the component as is done in Lens.

However, I read your comment and I see where you're coming from.

I'm wondering if giving these mostly-presentational components a reference to the entire data views service is overkill. Have you considered passing a simple callback instead (e.g. getDataViewById), or at least constraining the call signature to just the get method using Pick?

Another thought—have we considered making the interface either/or?

Right now it looks like the components will accept both an array of already-loaded data views and a way to load new ones on-demand. Will consumers of these components understand what the function of the two are? Does it make sense to choose one or the other to support?

cc @stratoula

stratoula · 2024-01-10T07:32:35Z

I am so sorry for coming late in the party. I am trying to understand why this behavior has impact high 🤔 The solution is nice but is any customer unhappy with how it worked? @kertal @drewdaemon

@drewdaemon

Right now it looks like the components will accept both an array of already-loaded data views and a way to load new ones on-demand. Will consumers of these components understand what the function of the two are? Does it make sense to choose one or the other to support?

I see your point but is it possible? The consumer (Discover, Lens ...) has knowledge of the current dataview, doesnt have knowledge of the filters dataview. So this would add a lot of complexity in the consumers' code. Only on-demand on the other hand I cant think of how it would work. A dashboard can have many charts with different indices but no filters. The consumers communicate their indices to the dashboard and the dashboard to the unified search. I am not sure how else the component can know the dataviews of these charts 🤔 But maybe you have something else in mind. Can you elaborate more?

Last but not least, I think this behavior happens only for the applications which have enabled the dataview picker (a unified search component, so it makes sense to solve this inside the unified search component)

kertal · 2024-01-10T09:34:56Z

This definitely resolves this issue. I initially expected you to load all the data views and pass them to the component as is done in Lens.

However, I read your comment and I see where you're coming from.

I originally thought it's an easy fix, we need to load and provide more data views in Discover. But it turned out, we would need to load all data views, which we shouldn't do, and on top of that, we would just fix the problem in Discover, not in Lens & Dashboards. That's why I closed this approach, but then I had another idea ...

I'm wondering if giving these mostly-presentational components a reference to the entire data views service is overkill. Have you considered passing a simple callback instead (e.g. getDataViewById), or at least constraining the call signature to just the get method using Pick?

This would be doable, however, I think we could need more of the data view service in this area, and maybe it's time to refactor this component, to have a better split between presentation and business logic. Why I think the whole service could be useful in the future? Mainly because of this discussion, #168131. If we e.g. would support DataViews and deduplicated Indexpatterns provided by ES|QL in an advanced filter editor, we would need to DataView service to create Ad-Hoc data views to provide the field list. Maybe it's not the time to think so far ahead, and to optimize for the given problem. I'd be fine with moving the a simple callback solution.

Another thought—have we considered making the interface either/or?

Right now it looks like the components will accept both an array of already-loaded data views and a way to load new ones on-demand. Will consumers of these components understand what the function of the two are? Does it make sense to choose one or the other to support?

The current solution is providing a fallback in case the given filter id is not part of the provided data views list. It's additional functionality, making sure it works. Just so support this fallback functionality, would mean, this list of selectable data views would no be provided, so it's complementary functionality. We could add this context in Component type description. If the data views service is not provided, this fallback doesn't work. There's some usage of this UnifiedSearch component, where the data service is not provided, although using just the dataView service of useKibana, caused other functionals to fail.

kertal · 2024-01-10T09:43:40Z

I am so sorry for coming late in the party.

Late party guest shouldn't be sorry, they are always welcome, and the buffet + fridge still has plenty of opportunies!

I am trying to understand why this behavior has impact high 🤔 The solution is nice but is any customer unhappy with how it worked? @kertal @drewdaemon

Maybe medium would be more accurate. This issue landed on the @elastic/kibana-data-discovery table, we thought it's very simple to run into it, it leads to bad UX when running into it and high likely simple to fix (change our consumption of the component), but that wasn't the case.

@drewdaemon

Right now it looks like the components will accept both an array of already-loaded data views and a way to load new ones on-demand. Will consumers of these components understand what the function of the two are? Does it make sense to choose one or the other to support?

I see your point but is it possible? The consumer (Discover, Lens ...) has knowledge of the current dataview, doesnt have knowledge of the filters dataview. So this would add a lot of complexity in the consumers' code. Only on-demand on the other hand I cant think of how it would work. A dashboard can have many charts with different indices but no filters. The consumers communicate their indices to the dashboard and the dashboard to the unified search. I am not sure how else the component can know the dataviews of these charts 🤔 But maybe you have something else in mind. Can you elaborate more?

As I mentioned, it's a fallback solution for these cases, it mainly fixes UX in Discover, where it's pretty easy to run into that. But opposed to my first attempt focusing on Discover, it's now fixed for all consumers (given they provide the data service)

drewdaemon

I'm not as familiar with the unified search components as I should be. I now understand that the filter_editor_wrapper is not used on its own, but only as part of unified search. This makes my architecture suggestions mostly irrelevant.

This PR fixes the issue, so no problem from my end.

drewdaemon · 2024-01-10T17:59:26Z

we would need to load all data views, which we shouldn't do

FWIW, I believe this is exactly what we do in Lens

kertal · 2024-01-10T18:06:37Z

@elasticmachine merge upstream

kertal · 2024-01-10T18:13:32Z

we would need to load all data views, which we shouldn't do

FWIW, I believe this is exactly what we do in Lens

Thx for the review @drewdaemon! I don't think you're loading really all data views, but missing ones, which I assume is a subset

const allIndexPatterns = await Promise.allSettled(missingIds.map((id) => dataViews.get(id)));

if you would load all, the given issue wouldn't surface in Lens, but it does/did. However, it might be the case, that you won't need to load the missingIds, if they are just being used in the FilterEditor. Then this PR takes care of loading them on demand.

kibana-ci · 2024-01-10T19:13:31Z

💚 Build Succeeded

Buildkite Build
Commit: 1acaf24

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
`exploratoryView`	203.3KB	203.3KB	+48.0B
`unifiedSearch`	213.7KB	214.6KB	+892.0B
total			+940.0B

History

💚 Build #186590 succeeded aefb1f3
💔 Build #186528 failed bb6cdea
💔 Build #186517 failed 2f6fb9c
💔 Build #186469 failed e681aec
💔 Build #185251 failed 060997e
💔 Build #184906 failed 678e712

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

cc @kertal

drewdaemon · 2024-01-10T21:59:23Z

@kertal that method is called with an empty cache so missingIds will be all the data views. But, when I say all the data views, I mean all the data views belonging to the current vis so I think we're okay after all :)

I can see why this approach would be untenable for Discover, where you are displaying a comprehensive list.

…ng plugin (elastic#173017) Fix issue with missing data view by loading it, allowing users to edit the filter in the FilterEditor.

MakoWish · 2024-06-19T15:28:13Z

This is unfortunately still not fixed in 8.14.1.

Initial changes

2971a60

kibanamachine and others added 3 commits December 11, 2023 08:33

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

9d57639

…fix'

Improve testing

d6f7956

Merge remote-tracking branch 'origin/filter-editor-changes' into filt…

d9631ed

…er-editor-changes

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

5db9dad

…fix'

kertal changed the title ~~[FilterEditor] Load DataView if missing~~ [FilterEditor] Load DataView if its missing in the list of available DataViews Dec 12, 2023

Merge branch 'main' into filter-editor-changes

01e0a6a

kertal added 2 commits December 12, 2023 21:53

Fix circular dependency

715f99e

Merge remote-tracking branch 'origin/filter-editor-changes' into filt…

67e1311

…er-editor-changes # Conflicts: # src/plugins/unified_search/tsconfig.json

kertal added Feature:Filters 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 12, 2023

Fix missing dataViews service

42d0700

Rename dataViewService to dataViews for naming consistency

2b64b37

kertal changed the title ~~[FilterEditor] Load DataView if its missing in the list of available DataViews~~ [FilterEditor] Load DataView in case it's not provided by the consuming plugin Dec 13, 2023

kertal marked this pull request as ready for review December 13, 2023 15:26

kertal requested review from a team as code owners December 13, 2023 15:26

kertal commented Dec 13, 2023

View reviewed changes

src/plugins/unified_search/public/filter_bar/filter_editor/filter_editor.tsx Outdated Show resolved Hide resolved

Merge branch 'main' into filter-editor-changes

baf82f1

kertal self-assigned this Dec 14, 2023

Merge branch 'main' into filter-editor-changes

e681aec

kertal requested review from lukasolson, jughosta and davismcphee January 8, 2024 20:14

Fix functional tests

2f6fb9c

kertal commented Jan 9, 2024

View reviewed changes

Merge branch 'main' into filter-editor-changes

bb6cdea

Merge branch 'main' into filter-editor-changes

aefb1f3

smith approved these changes Jan 9, 2024

View reviewed changes

drewdaemon reviewed Jan 9, 2024

View reviewed changes

drewdaemon approved these changes Jan 10, 2024

View reviewed changes

Merge branch 'main' into filter-editor-changes

1acaf24

kertal enabled auto-merge (squash) January 10, 2024 18:09

kertal merged commit 797b29a into elastic:main Jan 10, 2024
24 checks passed

kibanamachine added v8.13.0 backport:skip This commit does not require backporting labels Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[FilterEditor] Load DataView in case it's not provided by the consuming plugin #173017

[FilterEditor] Load DataView in case it's not provided by the consuming plugin #173017

kertal commented Dec 11, 2023 •

edited

Loading

kertal commented Dec 11, 2023

kertal commented Dec 12, 2023

kertal commented Dec 12, 2023

kertal commented Dec 12, 2023

kertal commented Dec 12, 2023

kertal commented Dec 13, 2023

elasticmachine commented Dec 13, 2023

kertal commented Dec 13, 2023

kertal commented Jan 8, 2024

kertal Jan 9, 2024

kertal commented Jan 9, 2024

kertal commented Jan 9, 2024

smith left a comment

drewdaemon left a comment •

edited

Loading

stratoula commented Jan 10, 2024 •

edited

Loading

kertal commented Jan 10, 2024

kertal commented Jan 10, 2024

drewdaemon left a comment

drewdaemon commented Jan 10, 2024

kertal commented Jan 10, 2024

kertal commented Jan 10, 2024

kibana-ci commented Jan 10, 2024

drewdaemon commented Jan 10, 2024

MakoWish commented Jun 19, 2024

[FilterEditor] Load DataView in case it's not provided by the consuming plugin #173017

[FilterEditor] Load DataView in case it's not provided by the consuming plugin #173017

Conversation

kertal commented Dec 11, 2023 • edited Loading

Summary

Checklist

kertal commented Dec 11, 2023

kertal commented Dec 12, 2023

kertal commented Dec 12, 2023

kertal commented Dec 12, 2023

kertal commented Dec 12, 2023

kertal commented Dec 13, 2023

elasticmachine commented Dec 13, 2023

kertal commented Dec 13, 2023

kertal commented Jan 8, 2024

kertal Jan 9, 2024

Choose a reason for hiding this comment

kertal commented Jan 9, 2024

kertal commented Jan 9, 2024

smith left a comment

Choose a reason for hiding this comment

drewdaemon left a comment • edited Loading

Choose a reason for hiding this comment

stratoula commented Jan 10, 2024 • edited Loading

kertal commented Jan 10, 2024

kertal commented Jan 10, 2024

drewdaemon left a comment

Choose a reason for hiding this comment

drewdaemon commented Jan 10, 2024

kertal commented Jan 10, 2024

kertal commented Jan 10, 2024

kibana-ci commented Jan 10, 2024

💚 Build Succeeded

Metrics [docs]

Async chunks

History

drewdaemon commented Jan 10, 2024

MakoWish commented Jun 19, 2024

kertal commented Dec 11, 2023 •

edited

Loading

drewdaemon left a comment •

edited

Loading

stratoula commented Jan 10, 2024 •

edited

Loading