Batch mget requests on dashboard load and cache field_stat results

[stacey-gammon]

Took @nreese's PR (#10050) and added some additional logic to avoid multiple field_stat requests.

Here is my finding after playing around with loading times of a dashboard with 30 visualizations (note: all are from the same index, requiring only a single field_stats lookup).

Total # of runs: 10
Total # of visualizations: 30

Variation	total requests	Avg page load	lowest	highest
Current	87	5.928s	5.6s	6.65s
one mget	58	5.794s	5.37s	6.29s
one mget and one field stat	29	4.95s	4.8s	5.3s

So it appears that this implementation will shave off about a second of page load time. I suspect that number would grow for slow connection speeds.

[trevan]

Every now and then, I'll have one of the field stats request take a long time (>60 seconds). Only sending one field stats request per dashboard has been very beneficial in speed for me.

[stacey-gammon]

Only sending one field stats request per dashboard has been very beneficial in speed for me.

@trevan You tested this PR and found it helped? Or do you mean you have already implemented logic to send only one field_stats api request and found it helped?

[trevan]

@stacey-gammon, I already had extremely similar logic to what you have and found it very helpful.

[spalger]

Sorry I let this sit for so long, I've been heads down on #9853 for a while

[spalger]

A few observations/questions:

• _.once()'d functions should almost never accept arguments
• What does isDefered mean? What is/should be defered? The Saved Object?
• Calling the docSource._createRequest method from here feels like a violation of the intended public api for the docSource. Why not just change how docSource.fetch() functions?

[nreese]

@spalger isDefered tells init how to load the saved object. When false, the saved object is fetched immediately. When true, the saved object fetch request is placed on the request queue ... it is deferred.

The PR was just trying to keeping things simple. I did not want to re-engineer anything. Kibana already had the concept of a request queue and I figured this was a good use for it.

[spalger]

Yeah, I think that's a good idea. If we want to keep the current code I would probably change the variable to deferDocSourceRequest (my second comment was more about naming).

That said, I think all calls to docSource.fetch() should probably be using the request queue. I don't think there is any reason to require requesters to opt-into this style of fetching. One wrinkle in that is the interval at which the request queue is cleared. We might need to have some debounced request clearing trigger that gets triggered each time docSource.fetch() is called on any source...

[spalger]

Maybe something like this (pseudo code):

// DocSource class
fetch() {
  const request = new Request({ urgent: true });
  requestQueue.addNew(request)
  return request.defer.promise
}

// in RequestQueue class
addNew(req) {
  this._all.push(req);
  this._scheduleCheck()
}

_scheduleCheck() {
  if (this._pendingCheck) {
    clearTimeout(this._pendingCheck)
  }

  this._pendingCheck = setTimeout(
    () => this._checkForReadyRequest(),
    CLEAR_REQUEST_QUEUE_DEBOUNCE_MS
  )
}

_checkForReadyRequests() {
  return this._all.filter(req => {
    return req.isUrgent() || req.isNaturallyReady()
  })
}

[stacey-gammon]

That said, I think all calls to docSource.fetch() should probably be using the request queue.

What about when just retrieving a single object by id? E.g. this gets called when you save a dashboard because it reloads the dashboard you just saved by id. In that case, maybe it's not worth the overhead? It looks like the looper checks for requests every 1.5 seconds, so that's adding an average .75s delay when just getting a single object.

That said, I agree about the naming, but what about batchRequest? I like batch better than defer, as it gives more reasoning behind it.

I feel like the entire batching logic could be improved to circumvent the looper delay entirely. Send in an array of savedObjects, or ids, and grab them all at once in a single batch (or split batches up to avoid any one batch from being too large if that was a potential problem). But that would be too large of a code overhaul at this point, so IMO, just doing the rename might be the best way to go at this point.

[spalger]

👍

Just pushed updates, not really qualified to review anymore

[spalger]

@nreese, @stacey-gammon and I worked on integrating the batching into the courier itself, I just pushed the changes we worked on that make all calls to DataSource#fetch() batch by a tiny amount, rather than waiting for the next tick of the looper (applies to data and search sources). Mind taking a look?

@pickypg another review from you would be helpful too

[stacey-gammon]

Did this end up not being a problem with requests being fetched more than once?

[spalger]

Oops, forgot to update this to the new req.isFetchRequestedAndPending() method that takes requests' started state into account.

[stacey-gammon]

One minor comment, otherwise looks good! Though some of this is my code, so should have another reviewer too,

[stacey-gammon]

Why the need for the double negative? Is it to avoid the undefined case? It'll still all work as expected right? Maybe just initialize this._fetchRequested to false in that case?

Just seems a little strange when the value is essentially a boolean, and the double negative requires an extra extra brain cycle or two to make the conversion. :)

[pickypg]

Forces boolean true out of anything. I agree that it's unusual since it's part of a larger boolean statement and now I wonder if there's more to the trick than I remember or just a force of habit.

[spalger]

Yeah, when it was just returning the value of _fetchRequested I was a little more concerned about it always being a boolean value, but now that it's returning the && calculation it's totally unnecessary.

[pickypg]

LGTM