Skip to content
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.

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

unify the way global context is passed down to visualize #16641

Closed
ppisljar opened this issue Feb 9, 2018 · 7 comments · Fixed by #19172
Closed

unify the way global context is passed down to visualize #16641

ppisljar opened this issue Feb 9, 2018 · 7 comments · Fixed by #19172
Assignees
Labels
chore Feature:Dashboard Dashboard related features Feature:Visualizations Generic visualization features (in case no more specific feature label is available)

Comments

@ppisljar
Copy link
Member

ppisljar commented Feb 9, 2018

we should come up with a way to handle global context in all our apps

right now we have two scenarios when it comes to visualize:

  • dashboard sets the rootsearchsource which will include the dashboard filters and query and time
  • visualize editor passes appState down to visualize from where visualize will get the query and filters. rootsearch source is used only for time filter

the problems with current approaches:

  • we cannot pass different filter to different visualizations (on dashboard for example)
  • request handlers or visualizations need to be aware of this global context and tightly coupled to searchsource or appstate

proposed solution:

  • pass all the global context into embeddables directly
  • appState approach already works in a similar way, as app state can be a plain js object (with filters and query properties), however we could simplify this

work to be done:

  • refactor visualizeLoader to accept query and filter parameters and pass those down to visualize. if they are not provided we could still fall back to appState
  • refactor current request handlers (courier, timelion, tsvb) to access those passed in properties instead of hacking their way into obtaining global context directly
  • refactor dashboard and visualizeApp to pass query and filter parameter to visualization loader
  • refactor discovery embeddable to receive query, filter and time range parameter directly
  • refactor discovery app to pass this parameters down to discovery embeddable
  • refactor dashboard to pass this parameters down to discovery embeddable

issues that we might encounter:

  • how do we inform the app doing the embedding about changes to query/filters/timerange in a unified way, as the app doing the embedding will need to update appState with new values (in some scenarios)
@ppisljar ppisljar added Feature:Dashboard Dashboard related features :Discovery Feature:Visualizations Generic visualization features (in case no more specific feature label is available) labels Feb 9, 2018
@epixa
Copy link
Contributor

epixa commented Feb 9, 2018

how do we inform the app doing the embedding about changes to query/filters/timerange in a unified way, as the app doing the embedding will need to update appState with new values (in some scenarios)

I don't know what you mean by "as the app doing the embedding will need to update appState with new values", but it sounds like you'd benefit from passing observables to embeddables instead of the raw values. This gives you the mechanism to retrieve values and react to changes in those values in a single solution. It also gives you the ability to compose new observables if you needed to modify the values in some way without having to bake that knowledge into the embeddable itself.

So basically, an embeddable would have no knowledge of "global" context or anything like that, it would simply be given an observable for search context and react accordingly.

There would be a global search context system that exposes an observable for search context.

The app that is doing the embedding would either pass the global search context observable directly into the embeddable (the histogram on discover would potentially be an example of this), or it would compose a new observable from the global one that applies some customizations or even an entirely new search context for that specific embeddable and pass that along (dashboard would probably do this).

Apps that don't deal with a global search context at all but still want to utilize embeddables can skip the global search context part entirely and just create an observable with some sort of "local" search context. The embeddable wouldn't know or care.

@stacey-gammon
Copy link
Contributor

how do we inform the app doing the embedding about changes to query/filters/timerange in a unified way, as the app doing the embedding will need to update appState with new values (in some scenarios)

This will be important for drill down links. The embeddable needs to tell dashboard to apply a filter when, for instance, a line on a chart is clicked on. Then dashboard can then open up a context menu at the current mouse location with some options for the user to choose from: apply the filter to the dashboard, or go to some other pre configured link (using the filter supplied by the embeddable).

For implementation specifics, observables sounds like a good idea.

@stacey-gammon
Copy link
Contributor

stacey-gammon commented Feb 27, 2018

Thanks for fleshing this out @ppisljar! I spent some time trying to walk through the search source flow and I think your proposed solution will work.

This is what I think the current state of affairs looks like:

screen shot 2018-02-27 at 1 08 19 pm

So it does seem like if dashboard simply stops setting it's own source as app source and instead passes it's search source down to the embeddables (or a different search source, based on per panel configurations), and the visualizations inherit from the passed in search source (making sure not to override a possibly inherited from linked search source), it would work. We might need to make use of that onlyHardLinked flag so that the root time filter isn't always applied, if we want to allow for panel configurations. Or perhaps just completely get rid of automatically inheriting from root source, and requiring it to be set explicitly.

Caveat - I only followed the flow for regular visualizations, not TSVB or saved searches. Those will also need to be looked at as well, to make sure they abide by the new pattern instead of inheriting from root source automatically.

Has anyone started work on this? I can dig in this week while you all are at Elasticon if not. It's a pretty big pre-req for a lot of other stuff in the works for dashboard.

@stacey-gammon
Copy link
Contributor

stacey-gammon commented Mar 5, 2018

One other thing we may want to think about is how courier is used, maybe ripping it out, replacing it, or at least trying to unify how requests are made to ES from embeddables. Right now it's pretty random.

screen shot 2018-03-05 at 2 03 12 pm

Discover uses a totally different mechanism than visualizations. Visualizations make use of broadcasting to create and fetch a new request, while discover relies on the search looper to continuously fetch a single request that gets put on the queue and never removed.

One thing I have been thinking about is if the requests went through the dashboard layer, the dashboard may be able to be more intelligent about fetching, caching and making performance enhancements (prioritizing certain panels so they get fetched first?). It also has the benefit of consolidating fetch logic for all embeddables if they so choose to take advantage of it, rather than forcing them to use our courier system.

Something like this:
screen shot 2018-03-05 at 3 01 21 pm

If we did something like that instead of the current mechanisms, I think we could really simplify the code. Even if we didn't go that far, but simply hooked up the listener for the filter changing, and made dashboard handle the auto refresh, I think we could get rid of the search looper and a lot of the broadcasting.

@ppisljar
Copy link
Member Author

ppisljar commented Mar 9, 2018

i think we should go further and not pass down the searchsource but just filters, query and time. Visualizations like TSVB or timelion will not use searchsource at all.

@cjcenizal
Copy link
Contributor

cjcenizal commented Jun 30, 2018

If we did something like that instead of the current mechanisms, I think we could really simplify the code. Even if we didn't go that far, but simply hooked up the listener for the filter changing, and made dashboard handle the auto refresh, I think we could get rid of the search looper and a lot of the broadcasting.

As I've progressed with my Courier refactor I've settled on the same idea. The app should be responsible for explicitly sending requests in response to state changes, which would reduce a lot of complexity.

@cjcenizal
Copy link
Contributor

I recorded my initial thoughts here if anyone cares to give feedback: #20364

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Feature:Dashboard Dashboard related features Feature:Visualizations Generic visualization features (in case no more specific feature label is available)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants