[discuss] Courier-replacement proposal

[cjcenizal]

Prior art: #16641

I've been working on refactoring Courier, and I think a lot of incidental complexity exists due to the way Courier has control over requests and search sources, as opposed to the current app. If we were to reverse this, and put the app in control of composing search sources and dispatching requests, this I think we can make the system simpler and give us the flexibility to do things like setting custom filters per dashboard panel.

It's important to note that in this proposal, a SearchSource is just a data structure that represents part of an eventual request to Elasticsearch.

Here's some pseudo-code with comments to explain how I think such a system could work in Dashboards:

import { autoRefresh } from 'autoRefresh';
import { SearchContext, SearchRequestFactory } from 'search'; // formerly known as courier
import { timepicker } from 'timepicker';
import { queryBar } from 'queryBar';
import { filterBar } from 'filterBar';

class DashboardApp {
 constructor(embeddables) {
    this.embeddables = embeddables;

    // Add existing embeddables' searchSources. If embeddables are added
    // or removed, the corresponding searchSource will also need
    // to be added or removed (not shown here).
    this.embeddables.forEach(embeddable => {
      const embeddableSearchSource = embeddable.getSearchSource();
      embeddable.onChange(this.onSearchSourceChange);
    });

    // Listen for changes from any other part of the system which
    // affects searches.
    timepicker.onChange(this.onSearchSourceChange);
    queryBar.onChange(this.onSearchSourceChange);
    filterBar.onChange(this.onSearchSourceChange);
  }

  // If there's a change in any search source, then we'll need to
  // re-run the search. We could also debounce this or add
  // additional logic to optimize the UX.
  onSearchSourceChange =() => {
    this.search();
  }

  // autoRefresh will a) automatically call this callback on an
  // interval, and b) call it as soon as it returns results if
  // the interval is faster than the response time. Note that
  // there's still some edge cases to consider that aren't
  // addressed in this example, e.g. overwriting this.searchRequest
  // when calling search() in quick succession.
  search = autoRefresh.setSearchCallback(() => {
    const searchContexts = this.embeddables.map(embeddable => (
      // This data structure defines the relationship among the
      // searchSources when they're "flattened" to generate
      // request parameters.
      new SearchContext(embeddable.id, [
        timepicker.getSearchSource(),
        filterBar.getSearchSource(),
        queryBar.getSearchSource(),
        embeddable.getSearchSource(),
      ]);
    ));

    // The searchContext and SearchRequests are tightly coupled
    // due to how complex the logic is for compiling searchSources
    // and generating suitable request parameters.
    this.searchRequest = SearchRequestFactory.createFromSearchContexts(searchContexts);
    return this.searchRequest.sendRequest().then(responsesAndErrors => {
      // We'll get back all of the responses and errors for all of 
      // our searchContexts, so we'll associate each one to the
      // originating embeddable.
      responsesAndErrors.forEach(responseOrError => {
        const { id, isError } = responseOrError;
        const embeddable = this.getEmbeddableWithId(id);
        
        if (embeddable) {
          if (isError) {
            embeddable.setError(responseOrError);
          } else {
            embeddable.update(responseOrError);
          }
        }
      });
    }).catch(error => {
      // If there was a fundamental problem with executing the
      // search, we can tell the user about it here.
      this.setSearchError(error);
    });
  });

  destroy = () => {
    // Clean up after ourselves.
    timepicker.removeChangeCallback(this.onSearchSourceChange);
    queryBar.removeChangeCallback(this.onSearchSourceChange);
    filterBar.removeChangeCallback(this.onSearchSourceChange);
    autoRefresh.clearSearchCallback();

    if (this.searchRequest) {
      this.searchRequest.abort();
    }
  };
}

[timroes]

I think that approach would actually cause us quite some issues for the direction we are trying to go to.

The reasoning behind #16641 was to get rid of the application needing to know anything about search sources at all, and rather just pass down a "context" (filters/queries/timerange) to the actual panel/embeddable. The embeddable within that panel can use it for whatever it wants.

The issues I see with the above approach:

Assumption about using search source at all
It seems like we are making assumptions about a panel always using search source to require. That's not true for today's visualizations (e.g. timelion, TSVB and Vega are not using courier/search source at all) and it will be even less true in the future, when we are using the Canvas pipeline to render visualizations. A panel could have no data loading at all, it could have a search source, it could use something else, it could have 5 search source requests. The embeddable within the panel even doesn't (and imho) shouldn't know about that. It only knows which pipeline expression makes up the visualization and passes that to the pipeline loader. This will in the end then parse it, and would potentially have knowledge about how many functions within the expression actually required a search source.

Assumption about the dashboard knowing anything about the search source parameters
It seems like the dashboard would then directly be in control of the filters/queries/timerange of the actual searchsource. This won't work with the new pipeline. The actual filters/timerange/queries are just part of the context that can be passed into that functions. If they are actually coming from the panel context or not will be completely depend on the pipeline expression. E.g. the following pipeline (simplified example):
kibanaContext | esaggs (some params) | pie_chart
kibanaContext will now read the filters/timerange/queries that it got from the embeddable (i.e. from dashboard) and produces a context output of these (pretty much just containing the same information) and that will be used by the .esaggs function to configure it's internal search source. A user could now easily modify the above pipeline expression to the following:
context timeRange=now-7d,now query='foobar' | esaggs (some params) | pie_chart
The context passed in by the embeddable from the dashboard would be completely ignored in that case, we just use a manually specified context. This could of course also be more complex, and you just use the kibanaContext function but pass that output to an offset function that shifts the timerange.

Long story short: the dashboard itself has no idea anymore, what the search source, that esaggs is creating needs for parameters, since they are purely determined inside the pipeline expression.

Nevertheless I think that dashboard can (and I think actually needs to) trigger the refresh for all panels. Since a refresh with the pipeline, doesn't mean just re-execute a search source, but means that the pipeline will need to be re-rendered completely (whatever it uses internally). So the implementation for auto refresh should change, maybe giving it a reload method, that dashboard automatically triggers. @stacey-gammon any ideas about that?

[cjcenizal]

@timroes Thanks for this explanation! This is very helpful. So it sounds like the key architectural feature we need is that embeddables should be responsible for generating (or at least defining) their own search requests. It may use SearchSource(s), it may use a custom API endpoint, it may use something entirely different.

So, what if the Dashboard was required for aggregating the different requests from each embeddable, and for dispatching the requests at the appropriate times? An appropriate time would be a user clicking a "Refresh" button or changing a filter, or when the auto-refresh interval elapses. This way the Dashboard is still responsible for sending requests, but the embeddable is responsible for defining those requests. Here's some updated pseudocode:

import { autoRefresh } from 'autoRefresh';
import { SearchContext, SearchRequestFactory } from 'search'; // formerly known as courier
import { timepicker } from 'timepicker';
import { queryBar } from 'queryBar';
import { filterBar } from 'filterBar';

class DashboardApp {
 constructor(embeddables) {
    this.embeddables = embeddables;

    // Listen for changes to embeddables which require a new search.
    this.embeddables.forEach(embeddable => {
      embeddable.onChange(this.onSearchContextChange);
    });

    // Listen for changes from any other part of the system which
    // affects searches.
    timepicker.onChange(this.onSearchContextChange);
    queryBar.onChange(this.onSearchContextChange);
    filterBar.onChange(this.onSearchContextChange);
  }

  // If there's a change in any search source, then we'll need to
  // re-run the search. We could also debounce this or add
  // additional logic to optimize the UX.
  onSearchContextChange =() => {
    this.search();
  }

  // autoRefresh will a) automatically call this callback on an
  // interval, and b) call it as soon as it returns results if
  // the interval is faster than the response time. Note that
  // there's still some edge cases to consider that aren't
  // addressed in this example, e.g. overwriting this.optimizedSearcher
  // when calling search() in quick succession.
  search = autoRefresh.setSearchCallback(() => {
    // SearchContext is only responsible for providing contextual
    // data which may or may not affect the search requests.
    const searchContext = new SearchContext({
      time: timepicker.getTime(),
      query: queryBar.getQuery(),
      filters: filterBar.getFilters(),
    });

    // OptimizedSearcher is responsible for batching requests using
    // registered batching strategies.
    this.optimizedSearcher = new OptimizedSearcher();
    this.embeddables.forEach(embeddable => {
      const { id, type } = embeddable;
      // The embeddable defines the request to be used to retrieve
      // the data it needs.
      const searchRequest = embeddable.getSearchRequest(searchContext);
      optimizedSearcher.addSearchRequest({ id, type, searchRequest, searchContext });
    });

    this.optimizedSearcher.executeSearch().then(responsesAndErrors => {
      // We'll get back all of the responses and errors for all of 
      // our requests, so we'll associate each one to the
      // originating embeddable.
      responsesAndErrors.forEach(responseOrError => {
        const { id, isError } = responseOrError;
        const embeddable = this.getEmbeddableWithId(id);
        
        if (embeddable) {
          if (isError) {
            embeddable.setError(responseOrError);
          } else {
            embeddable.update(responseOrError);
          }
        }
      });
    }).catch(error => {
      // If there was a fundamental problem with executing the
      // search, we can tell the user about it here.
      this.setSearchError(error);
    });
  });

  destroy = () => {
    // Clean up after ourselves.
    timepicker.removeChangeCallback(this.onSearchContextChange);
    queryBar.removeChangeCallback(this.onSearchContextChange);
    filterBar.removeChangeCallback(this.onSearchContextChange);
    autoRefresh.clearSearchCallback();

    if (this.optimizedSearcher) {
      this.optimizedSearcher.abortSearch();
    }
  };
}

[cjcenizal]

I talked with @nreese and he suggested letting the embeddable be responsible for making the request. I think this makes sense; the only challenge would be to allow the embeddables to optimize the search request, similar to the way courier currently works. Here's how this could look:

class DashboardApp {
 constructor(embeddables) {
    this.embeddables = embeddables;

    timepicker.onChange(this.onSearchContextChange);
    queryBar.onChange(this.onSearchContextChange);
    filterBar.onChange(this.onSearchContextChange);
  }

  onSearchContextChange =() => {
    this.update();
  }

  update = autoRefresh.setSearchCallback(() => {
    const searchContext = new SearchContext({
      time: timepicker.getTime(),
      query: queryBar.getQuery(),
      filters: filterBar.getFilters(),
    });


    const embeddablesByType = this.groupEmbeddablesByType();
    const uniqueEmbeddables = this.getUniqueEmbeddables();

    // Allow embeddables that are the same type to perform optimizations,
    // like batching their search requests.
    uniqueEmbeddables.forEach(embeddable => {
      const { type, update } = embeddable;
      const embeddables = embeddablesByType[type];
      embeddable.update(searchContext, embeddables)
    });
  });

  destroy = () => {
    // Clean up after ourselves.
    timepicker.removeChangeCallback(this.onSearchContextChange);
    queryBar.removeChangeCallback(this.onSearchContextChange);
    filterBar.removeChangeCallback(this.onSearchContextChange);
    autoRefresh.clearSearchCallback();
  };
}

[cjcenizal]

I have a working POC of how this idea can be applied to Visualize in #20395.

[timroes]

That looks already way better. As far as I understand the latest draft in your comment, the only thing that dashboard will now do is calling an update method on the Embeddable and let that do whatever it needs to do with the given context? Everything else would stay the same? If so, that's a way that would work, and pretty much what I meant above with (these slightly more confusing words):

So the implementation for auto refresh should change, maybe giving it a reload method, that dashboard automatically triggers.

Just to give you a little bit of more context, the new visualization embeddable that uses the pipeline would look around that (simplified), but to make clear there is no search source or vis anymore.

class VisualizeEmbeddable {

  constructor(panel) {
    this.panel = panel;
  }

  render(domNode, containerState) {
    // this.panel.savedObject.pipeline will just be the pipeline string as shown above, like
    // `context timeRange=now-7d,now query='foobar' | esaggs (some params) | pie_chart`
    this.handler = pipelineLoader.render(domNode, this.panel.savedObject.pipeline, {
      timeRange: containerState.timeRange,
      filters: containerState.filters,
      query: containerState.query,
    });
  }

  // This method already exists, so I assume the "update" method you were creating above,
  // would pretty much do exactly the same as this method, just let the pipeline rerender,
  // with possible new "search context".
  onContainerStateChanged(dashboardContext) {
    this.handler.update({
      timeRange: containerState.timeRange,
      filters: containerState.filters,
      query: containerState.query,
    });
  }
}

Also maybe just for further clarification, there won't be any search source attached to the actual savedObject anymore. Each function inside the pipeline would create it itself. In the above example also noone would actually use the passed in "search context", since no function in the pipeline will consume it. Rather the context function just creates it's own filters/queries/timeRange context from manually specified values and will pass that onto the esaggs function. That itself will use that context (and absolutely not care were it came from) and create a SearchSource that it will then fetch. Potentially there could also be 3 more functions in there, that would also create a new SearchSource.

[stacey-gammon]

I'm still on vacation so haven't read in depth through all of this, but I've had the same thoughts you are having now @cjcenizal, but I think I might have changed my mind.

One issue with putting this logic in Dashboard is that there are going to be multiple apps that use embeddables and a performant request mechanism would want to be used by all, so we shouldn't make it container specific. I think we need to come up with a generalized approach.

I also think a server side component would be really helpful. Send requests to the Kibana server, which can then be intelligent about how to batch/stream back requests gathered from elasticsearch. Do this through a client side component which can also be somewhat intelligent.

Gotta run, so these are my quick thoughts for now!

[cjcenizal]

@timroes That sounds great, I think we have a shared understanding.

One issue with putting this logic in Dashboard is that there are going to be multiple apps that use embeddables and a performant request mechanism would want to be used by all, so we shouldn't make it container specific. I think we need to come up with a generalized approach.

@stacey-gammon When you get back let's talk about this some more! I think I need clarification on what you mean by "container-specific" and "generalized approach".

[stacey-gammon]

Other issues related to courier we could possibly fix/implement (or at least should be considered) in the event of a rewrite:

• Canceling ability:
  • Cancel underlying aggregation query if aggregation is modified #17846
  • Cancel underlying query when switching context in Kibana or old query is replaced #17845
  • Cancel query / filter automatically if new query entered or running query is removed #17844
  • Feature in Kibana GUI that allows you to cancel a query (task management).  #17265
• Improve dashboard load performance: Improve dashboard load performance #14750
• Code clean up chores:
  • Dashboard avoid reliance on broadcast emits #14789

[lukasolson]

Closing this out as we've refactored courier and the querying infrastructure in the new data.search service, which essentially works in the same way this discussion led to.