[Visualizations] Opening and closing legend causes data re-fetch

[stacey-gammon]

This behavior is different from 6.3. Not sure if it's a blocker or not. I think it's a bug because the user can't be sure what time range the data is showing, nor will it be consistent with the other visualizations on a dashboard. Note that we already have some inconsistencies (that exist in 6.3 as well) when you add a new panel - it will display newer data than the panels already on a dashboard.

Below are gifs, but they are a bit difficult to see what's going on, so, to repro manually:

1. Add flight sample data
2. find a relative time range that is on the border of no data to data, like last 100 seconds or something.
3. add the airline carrier pie chart to a dashboard with that time range
4. wait some seconds to make sure there now is no data in the range
5. open or close the legend. View the data disappears.

I'll file a separate bug for adding panels causing inconsistent time ranges among the panels (why below you will see the same visualization with different data). That exists in 6.3.

6.3:

master [and thus I assume 6.4 as well]:

cc @elastic/kibana-visualizations

[timroes]

That's indeed expected behavior right now, but I am open to discuss this again. Lemme shortly explain what's happening:

Closing/opening a legend will change the uiState of that chart, since the legend state is something that should be saved. Since refactoring the loader, the visualize embeddable actually listenes on the uiState and fetch and render in case something in the uiState change. The reasoning behind this is, that we don't know what visualization implementations actually want to store in their uiState and maybe that will actually require a new fetch for that chart, since something within the uiState is used to actually determine what data to fetch. Where this actually happens is in the maps, who store their zoom level and position in the uiState and that actually determines the bounding box for the geo query, so only data within the viewport is rendered.

We had a similar mechanism in beforehand, that mechanism was just a bit broken, why it actually didn't trigger for everything in uiState properly.

In case we don't want that refetch when a legend toggles, we should discuss again if we want to refetch on uiState change? In case we need some state that changes data fetch, maybe pass in two separate states, and merge them both at the embeddable level into one container state, but so we only need to refetch if something changes in the uiStateThatRequiresFetch?

@ppisljar any ideas on that one?

[LeeDr]

Just noting this is still an issue in 6.6.0. I don't think it's a blocker for 6.7.0.

[timroes]

Updates: This is still the desired and expected behavior, so it's still present in 6.6.0 and will also be in 6.7.0 (and most likely beyond). With the switch to the expressions for rendering, that will happen at some point, we're planning to take this into a different direction and refactor the existing visualizations. uiState should really only be a state of your UI, and you cannot have access to it, for fetching data. Meaning we also won't need to retrigger a fetch once it's updated. Instead visualizations that want to change the query based on their uiState (like maps as stated above), will - besides updating their uiState - also manipulate the expression used to fetch it's data. So basically a map would when zooming update its uiState AND the expression to contain the geo filter it needs.

[JacobBrandt]

I would be careful to call out of sync panels on dashboards desirable. #11233 doesn't want this behavior either. It might be expected for the time being because of the workaround for the maps like you described but not really desirable.

The uiState has changed and in this case there is no reason for it to require a fetch. I found that the code already has some state checking to determine if fetch is required here:

https://github.com/elastic/kibana/blob/6.6/src/ui/public/visualize/loader/embedded_visualize_handler.ts#L190

Unfortunately it is incorrectly setting fetchRequired to true because of these object checks in the file below that add things to updatedParams when their values have not changed.

https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/kibana/public/visualize/embeddable/visualize_embeddable.ts#L109

So technically two fetchAndRender calls are being made for this one event. It seems overkill that all uiState changes are causing fetches because of some map workaround. The checking done in embedded_visualize_handler.ts is what we want. Perhaps we could give visualizations a way to tap into this routine so that they can add to this logic? As it stands right now it doesn't even matter if it properly set the fetchRequired flag because the uiState change is going to call fetchAndRender anyways.

[JacobBrandt]

How long does one have to wait for regressions to get worked? It seems as if there is no point to even issue bugs. This regression when found was happening on master....and it never got prioritized.

[ppisljar]

Sorry Jacob, we are hoping to address this issue with a big redesign of our rendering infrastructure we are working on. Unfortunately this is taking us a long time, but i can guarantee we are not ignoring the issued bug reports.

[JacobBrandt]

IMO a regression about data being out of sync on a dashboard should never have needed to wait for a major redesign to be fixed. What’s funny about you saying that is a redesign caused this issue in the first place. This is something that worked until the team broke it with version 6 redesign and now you are saying it won’t get fixed until some other redesign is ready. Do you see how frustrating that logic sounds?

[stratoula]

This has been fixed on master. I am closing it