Skip to content

[Discover][Embeddable] Search embeddable to keep all SO fields #204651

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
kowalczyk-krzysztof wants to merge 1 commit into from
Closed

[Discover][Embeddable] Search embeddable to keep all SO fields #204651

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/plugins/discover/public/embeddable/initialize_search_embeddable_api.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,7 @@ export const initializeSearchEmbeddableApi = async (
const density$ = new BehaviorSubject<DataGridDensity | undefined>(initialState.density);
const sort$ = new BehaviorSubject<SortOrder[] | undefined>(initialState.sort);
const savedSearchViewMode$ = new BehaviorSubject<VIEW_MODE | undefined>(initialState.viewMode);
const breakdownField$ = new BehaviorSubject<string | undefined>(initialState.breakdownField);

/**
Comment on lines +100 to 102
Copy link
Copy Markdown
Member

@kertal kertal Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm , I'm not familar with the embeddable code here, but if we want to keep those fields, do we need to introduce observables for all of those, even if they don't change? @jughosta , do you know ? thx

Copy link
Copy Markdown
Member Author

@kowalczyk-krzysztof kowalczyk-krzysztof Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I asked @Heenawter about this, and apparently this is the way to go and this is where the question about whether we really want ALL of the SO properties comes from.

Copy link
Copy Markdown
Member

@kertal kertal Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's a good question, because if this is the way to go, we would need to change this part of the code every time new properties are introduced. Which doesn not happen too often, but still ... I'd prefer a way that would take care of this automatically, @Heenawter is there an alternative to his ? thx!

Copy link
Copy Markdown
Contributor

@Heenawter Heenawter Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kertal I guess I'm a bit confused what the goal is here. When you unlink a saved search, if that saved object has state that doesn't need to be used for the embeddable, why would we want to keep track of it in the embeddable? If you want to track a piece of state, the comparators for an embeddable require an observable because it's assuming that most state is "editable" - if it's state that doesn't change and isn't required for the embeddable to function, do you really need to track it? Why keep state around that you don't need? 🤷

cc @nreese @ThomThomson

Copy link
Copy Markdown
Contributor

@ThomThomson ThomThomson Dec 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly sure why you'd want to track all of the saved object state when the panel is no longer linked. In #203662 I also added a rawSavedObjectAttributes which tracks the attributes taken directly from the saved object. So if you really need to retain that information I'd recommend storing it there.

On the other hand, if there isn't a user-facing reason for this change is it really worth doing? I'd let the data discovery team weigh in on that.

Copy link
Copy Markdown
Member

@kertal kertal Dec 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's pause until @davismcphee is back from PTO in the new year, who originally requested this change

Copy link
Copy Markdown
Contributor

@davismcphee davismcphee Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey all, I'm back from PTO and catching up on this now. I originally raised this in the refactor review partly because it was a change from what existed in main, and because there are scenarios where it may be unexpected.

A Discover session (saved search) contains more state than what we actually use in the embeddable panel, e.g. breakdown field and Lens vis config. If I create a Discover session with a custom Lens vis and add it as a dashboard panel, clicking "Open in Discover" will include my custom vis. This could be useful for users who want to kick off an investigation from a dashboard to a fully configured Discover view (especially as Discover sessions evolve for One Discover). But after the refactor, unlinking a Discover session panel removes all but the panel state, so the custom vis is gone when navigating to Discover. We don't have save and return support in Discover currently, but the same situation would apply there too.

So it's mainly a question of expectations. As a user, I would personally expect all of my Discover state to be preserved when unlinking or save-and-returning to support these cases (especially if rawSavedObjectAttributes makes this easy), but I don't feel super strongly about it. If others disagree, we could leave it as is and revisit if we receive feedback or it impacts later work.

Copy link
Copy Markdown
Member Author

@kowalczyk-krzysztof kowalczyk-krzysztof Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have anything of value to add to this discussion but I would appreciate if some kind of decision could be made on how to proceed with this issue. This started as a papercut but I'm not sure if that still would be the case now and if I should be involved in this.

Copy link
Copy Markdown
Contributor

@davismcphee davismcphee Jan 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this and bearing with us @kowalczyk-krzysztof. The team is all back from holidays now so I've added an agenda item for us to discuss this in our team sync next Monday, and we'll followup with the decision. If we decide we need something more complex than what you have here, we can take over the remainder of the work.

Copy link
Copy Markdown
Contributor

@davismcphee davismcphee Jan 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kowalczyk-krzysztof Looks like you noticed and closed already, but for future reference we discussed this in our team sync today and decided to leave this as a known issue for now unless a user raises it again or it blocks later work. Apologies that you worked on this only to have us close it, but we appreciate the effort regardless!

* This is the state that comes from the search source that needs individual publishing subjects for the API
Expand Down Expand Up @@ -136,6 +137,7 @@ export const initializeSearchEmbeddableApi = async (
viewMode: savedSearchViewMode$,
density: density$,
inspectorAdapters: inspectorAdapters$,
breakdownField: breakdownField$,
};

/** The saved search should be the source of truth for all state */
Expand Down Expand Up @@ -228,6 +230,7 @@ export const initializeSearchEmbeddableApi = async (
],
viewMode: [savedSearchViewMode$, (value) => savedSearchViewMode$.next(value)],
density: [density$, (value) => density$.next(value)],
breakdownField: [breakdownField$, (value) => breakdownField$.next(value)],
},
};
};
1 change: 1 addition & 0 deletions src/plugins/discover/public/embeddable/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ export type SearchEmbeddableState = Pick<
| 'viewMode'
| 'grid'
| 'density'
| 'breakdownField'
> & {
rows: DataTableRecord[];
columnsMeta: DataTableColumnsMeta | undefined;
Expand Down
1 change: 1 addition & 0 deletions src/plugins/discover/public/embeddable/utils/get_discover_locator_params.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ describe('getDiscoverLocatorParams', () => {
sort: savedSearchMock.sort,
viewMode: savedSearchMock.viewMode,
hideAggregatedPreview: savedSearchMock.hideAggregatedPreview,
breakdownField: savedSearchMock.breakdownField,
});
});
});
1 change: 1 addition & 0 deletions src/plugins/discover/public/embeddable/utils/get_discover_locator_params.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ export const getDiscoverLocatorParams = (
sort: savedSearch?.sort,
viewMode: savedSearch?.viewMode,
hideAggregatedPreview: savedSearch?.hideAggregatedPreview,
breakdownField: savedSearch?.breakdownField,
};

return locatorParams;
Expand Down