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

Unneccessary search source creation in vis courier request handler #16928

Closed
stacey-gammon opened this issue Feb 27, 2018 · 4 comments
Closed
Labels
chore Feature:Visualizations Generic visualization features (in case no more specific feature label is available)

Comments

@stacey-gammon
Copy link
Contributor

I'm trying to track down the flow of search source creation and inheritance and ran into the code here:

            for (const agg of vis.getAggConfig()) {
              const nestedSearchSource = new SearchSource().inherits(searchSource);
              resp = await agg.type.postFlightRequest(resp, vis.aggs, agg, nestedSearchSource);
            }

It appears this creates a new search source for every aggregation, even though each search source looks exactly the same. I did a quick test of replacing nestedSearchSource with just searchSource and everything seems to be working fine, but I wasn't overly thorough (part of the reason I suggested a test be added for the issue #16643 fixed).

cc @elastic/kibana-visualizations

related to #16641

@stacey-gammon stacey-gammon added chore Feature:Visualizations Generic visualization features (in case no more specific feature label is available) triage_needed labels Feb 27, 2018
@ppisljar
Copy link
Member

ppisljar commented Mar 9, 2018

new searchsource is created as postFlightRequest will modify the searchsource to issue a new request. We still need the original one as spy panel will inspect it to show request/response information.

Possible perf. improvement would be to send the original searchsource in and rely on postFlightRequest implementation to create a new one in case it will need it. However that could lead to problems if postFlightRequest does not implement this correctly (modifies the passed in searchsource).

This will no longer be required once @timroes finishes with migration from spy panels to inspector

@timroes
Copy link
Contributor

timroes commented Mar 9, 2018

I mark this as blocked, and we will refactor this as soon as #16185 is ready.

@cjcenizal
Copy link
Contributor

@timroes Are you planning on tackling this now that the inspector has been merged?

@timroes
Copy link
Contributor

timroes commented Jun 30, 2018

Will look at this next week. I think we can change it to have the postFlightRequestHandler actually create the child search source when it needs it. I'll discuss this with Peter on Monday.

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

No branches or pull requests

4 participants