Kibana should allow ES request cancellation when browser disconnects

[joshdover]

When a browser request disconnects, we should cancel any pending requests to Elasticsearch that the browser request started. This could be done via the ScopedClient class that has access to the raw server request.

This would eliminate wasted Elasticsearch CPU cycles for results that will never be used.

Doing this at the global ElasticsearchService level would allows us to have this behavior by default for all Elasticsearch requests, but we should also provide an escape hatch to opt-out of this behavior for a given request.

Example

router.get(
  { path: 'my-route/', validation },
  async (context, req, res) => {
    const { dataClient } = context.core.elasticsearch;
    
    // This request would automatically cancel if the browser
    // disconnects from Kibana early
    const results = await dataClient.callForCurrentUser('search', query);
    
    // This request would not automatically cancel
    const results = await dataClient.callForCurrentUser('search', query, {
      cancelOnDisconnect: false
    });
  }
);

Related: #42764

[elasticmachine]

Pinging @elastic/kibana-platform

[stacey-gammon]

fwiw, I think this will block the new data access services from being in NP and replacing courier (#29215).

[joshdover]

We discussed this today during the Platform Team's sync and now we think that, yes, we should build this feature into the core ElasticsearchService, but we'd like make it an opt-in behavior.

The example above would now be:

router.get(
  { path: 'my-route/', validation },
  async (context, req, res) => {
    const { dataClient } = context.core.elasticsearch;
    
    // This request would not automatically cancel
    const results = await dataClient.callForCurrentUser('search', query);
    
    // This request would automatically cancel if the browser
    // disconnects from Kibana early
    const results = await dataClient.callForCurrentUser('search', query, {
      cancelOnDisconnect: true
    });
  }
);

By doing this opt-in instead of opt-out, we can introduce this behavior in a lower-risk way and adopt it in pieces across the different query services.

The reason we still want to do this in the core service itself rather than in plugin code, is because we don't want to expose request events to plugins at this time. This is the only known usage of request events, so it does not seem appropriate to expose the request event interface outside of core.

@stacey-gammon Do you have any thoughts or objections?

[stacey-gammon]

that sgtm, I think we can work with that and pass the setting on to users of the search service if they wish to override, while we default to setting it on.

[mshustov]

The reason we still want to do this in the core service itself rather than in plugin code, is because we don't want to expose request events to plugins at this time. This is the only known usage of request events, so it does not seem appropriate to expose the request event interface outside of core.

after thinking more on the proposed solution:

// This request would not automatically cancel
const results = await dataClient.callForCurrentUser('search', query, {
  cancelOnDisconnect: false
});

I see several problems with it:

• API inconsistency between request handler context and elasticsearch service contract
• necessity to handle several abort signals

const results = await dataClient.callForCurrentUser('search', query, {
  signal: abortController.signal, <--- user still can pass signal manually
  cancelOnDisconnect: false
});

Probably we should consider adding nodejs request events to KibanaRequest? There is abort event as well https://nodejs.org/api/http.html#http_event_abort

// KibanaRequest
import { IncomingMessage } from 'http';
class KibanaRequest {
  public readonly on: IncomingMessage['on'];
}
// Plugin code
router.get({ path: '/', validate: false }, async (context, req, res) => {
  const abortController = new AbortController();
  const { signal } = abortController;

  req.on('aborted', abortController.abort);

  const data = await context.core.elasticsearch.dataClient.callAsCurrentUser('ping', {}, { signal });

  return res.ok({ body: data });
});

[joshdover]

Probably we should consider adding nodejs request events to KibanaRequest? There is abort event as well nodejs.org/api/http.html#http_event_abort

I'm ok with this, but I do think that we should expose this as an Observable to be consistent with other Platform APIs. AFAIK, we don't have any event listener-style APIs currently.

[lukasolson]

@joshdover Are you thinking there would be a separate observable for each type of event? Or would there be a single observable for all events?

[joshdover]

Separate might be more ergonomic, though honestly it'd be trivial to expose both.

[pgayvallet]

Probably we should consider adding nodejs request events to KibanaRequest? There is abort event as well https://nodejs.org/api/http.html#http_event_abort

@joshdover Are you thinking there would be a separate observable for each type of event? Or would there be a single observable for all events?

Is there any need to expose any other event/observable that abort ATM? For consistency, I also think it should be expose either as a observable or a a promise. Using a promise for ES calls is probably more elegant/logic though, as this should/can only emit once.

WDYT of

// KibanaRequest
class KibanaRequest {
  public readonly aborted$: Observable<void>
}
// Plugin code
router.get({ path: '/', validate: false }, async (context, req, res) => {

  const data = await context.core.elasticsearch.dataClient.callAsCurrentUser(
    'ping', 
    {}, 
    { abort: req.aborted$.toPromise() }
  );

  return res.ok({ body: data });
});

We could even add some sugar by just passing up the request and doing the machinery internally.

// KibanaRequest
class KibanaRequest {
  public readonly aborted$: Observable<void>
}
// Plugin code
router.get({ path: '/', validate: false }, async (context, req, res) => {

  const data = await context.core.elasticsearch.dataClient.callAsCurrentUser(
    'ping', 
    {}, 
    { request: req }
  );

  return res.ok({ body: data });
});

[lukasolson]

Would it look something like this? https://github.com/elastic/kibana/compare/master...lukasolson:abort?expand=1

If platform team doesn't have the resources to implement this, I'm sure either @lizozom or I would be willing to tackle this as it seems like it might be low fruit.

[joshdover]

That looks pretty close, though I would suggest using the `fromEvent` API rather than a `Subject`: https://rxjs.dev/api/index/function/fromEvent Mikhail was planning on picking this up soon, but if you want to move forward, we can review it for you.

…

On Tue, Jan 14, 2020 at 2:25 PM Lukas Olson ***@***.***> wrote: Would it look something like this? https://github.com/elastic/kibana/compare/master...lukasolson:abort?expand=1 If platform team doesn't have the resources to implement this, I'm sure either @lizozom <https://github.com/lizozom> or I would be willing to tackle this as it seems like it *might* be low fruit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#44302?email_source=notifications&email_token=AAN2UEERCZ3FR4KTHHROCATQ5YUWNA5CNFSM4IRN5MQKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEI6F2QQ#issuecomment-574381378>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAN2UEG4MDEHYR4RIDIQ6LLQ5YUWNANCNFSM4IRN5MQA> .

[mshustov]

started working on the issue.

We could even add some sugar by just passing up the request and doing the machinery internally.

This is similar to proposed solution #44302 (comment) with drawbacks mentioned in #44302 (comment)

[pgayvallet]

This is similar to proposed solution #44302 (comment) with drawbacks mentioned in #44302 (comment)

You are right, forget the dataClient was coming from the handler context and therefor could already be aware of the request implicitly.