elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.76k stars 8.16k forks source link

Kibana doesn't warn the user about panels losing their drilldowns when users edit viz in lens and do save and return #147646

Open bhavyarm opened 1 year ago

bhavyarm commented 1 year ago

Kibana version: 8.6.0 BC7

Elasticsearch version: 8.6.0 BC7

Server OS version: darwin_x86_64

Browser version: chrome latest

Browser OS version: OS X

Original install method (e.g. download page, yum, from source, etc.): from staging

Describe the bug: Lens doesn't display any message to user about their panels losing their drill-downs once user navigates to edit in lens from a panel and clicks save and return to dashboard.

Steps to reproduce:

  1. Make sure to have a panel with either classic aggregation or TSVB viz with a drilldown in dashboard
  2. Click edit on dashboard -> visualize->edit visualization in lens
  3. Click save and return to dashboard
  4. That panel has lost the drilldown

Expected behavior: Kibana should tell the user about them losing drilldowns.

Panel with drilldowns:

Screen Shot 2022-12-15 at 3 25 22 PM

Same panel in lens:

Screen Shot 2022-12-15 at 3 25 50 PM

Panel back in dashboard with drilldowns missing:

Screen Shot 2022-12-15 at 3 25 55 PM
elasticmachine commented 1 year ago

Pinging @elastic/kibana-visualizations @elastic/kibana-visualizations-external (Team:Visualizations)

stratoula commented 1 year ago

I am changing this to enhancement because it needs some discussion.

@elastic/kibana-presentation as you know, we allow the users to translate their legacy visualizations to Lens. These are replaced in the dashboard as by value visualizations. Can we somehow keep the drilldowns set?

I don't want to add a warning because I want this to be an appealing path for the users. So I would like to explore first if there is a way to do the replacement but keep the drilldowns.

ThomThomson commented 1 year ago

I think it should be completely possible to retain drilldowns on panel type change. The only complexity I can think of with this is the potential for the new type of panel to not support all of the same triggers - but as far as I can tell that shouldn't happen, due to Lens and TSVB being mostly 1 to 1 in their functionality. @stratoula is that the case?

If so, I think it's relatively low risk for us to just spread the enhancements on the embeddable even when changing types.

stratoula commented 1 year ago

Yes @ThomThomson this is correct, Lens supports more drilldowns than the legacy visualizations (the same + discover drilldown) so it is safe to also transfer them to the replaced embeddable.

stratoula commented 1 year ago

@ThomThomson the same applies for custom timerange, right? Does it require changes from your side too?

ThomThomson commented 1 year ago

TimeRange is a strange one. When I edit a lens panel with a custom time range in Lens, we do pass it to Lens in the valueInput key of the state transfer service.

You can see this in embeddableEditorIncomingState in x-pack/plugins/lens/public/app_plugin/mounter.tsx. But by the time we get the Lens embeddable back the timeRange is missing. You can see after save and return in incomingEmbeddable in src/plugins/dashboard/public/dashboard_container/embeddable/dashboard_container.tsx ~line 272. The only reason the time range is retained in this case is because for same type embeddables, the older input gets spread (line 286).

Ideally, all input that gets passed into the editor should be returned on save and return. That way it can be updated properly.

stratoula commented 1 year ago

@ThomThomson I could make the custom timerange to work for all cases expect from one

When I have a saved TSVB visualization --> Edit in TSVB --> Edit in Lens --> Save and return

At this point I dont have on the visualize editor the input for some reason. Here is the useEffect that I retrieve the embeddable input for the by ref visualizations https://github.com/elastic/kibana/blob/main/src/plugins/visualizations/public/visualize_app/components/visualize_editor.tsx#L76

Do you have a clue why I dont have this at this point?

Here is my PR https://github.com/elastic/kibana/pull/155113

ThomThomson commented 1 year ago

Looks like we don't provide the valueInput for a by reference TSVB / visualization because on the embeddable side there is an explicit check for saved object id, and if the panel is by reference we don't add any input to the state transfer package at all.

Wondering out loud - would it mess anything up on the Lens or TSVB side if we included the valueInput all the time, even when editing the panel by reference? I did some preliminary testing with that condition removed and everything seemed to work fine with Visualize by reference editing, but I haven't covered every case.

stratoula commented 1 year ago

Do you have a PR that enables that Devon? If yes I can also do some tests. I dont think it will cause any problem tbh.

ThomThomson commented 1 year ago

I can open one that does that, sure! Edit - here's a draft PR

dej611 commented 1 year ago

Just noticed also tags are lost on Edit viz in Lens, when testing #155113