[Discover][Embeddable] Search embeddable to keep all SO fields#204651
[Discover][Embeddable] Search embeddable to keep all SO fields#204651kowalczyk-krzysztof wants to merge 1 commit intoelastic:mainfrom
Conversation
|
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
|
@davismcphee |
|
@kertal can you answer this while davis is out? |
|
sure, so @davismcphee wrote When I unlink a search panel from the library in main, it keeps all of the SO properties and passes them to Discover when clicking the edit dropdown link. In this PR it looks like some of the properties are dropped when unlinking, so clicking edit doesn't carry over some state like the breakdown field. I feel like it might be best to keep all of the state if possible in case we decide to use it in the embeddable in the future. Yes, I think we should keep them all |
| const breakdownField$ = new BehaviorSubject<string | undefined>(initialState.breakdownField); | ||
|
|
||
| /** |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
@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? 🤷
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
let's pause until @davismcphee is back from PTO in the new year, who originally requested this change
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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!
7 participants
Summary
This PR makes it so unlinking a search panel from the library retains all SO attributes.
Closes: #196732