-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Kibana should allow ES request cancellation when browser disconnects #44302
Comments
Pinging @elastic/kibana-platform |
fwiw, I think this will block the new data access services from being in NP and replacing courier (#29215). |
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? |
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. |
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:
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 // 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 });
}); |
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. |
@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? |
Separate might be more ergonomic, though honestly it'd be trivial to expose both. |
Is there any need to expose any other event/observable that 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 });
}); |
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. |
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>
.
|
started working on the issue.
This is similar to proposed solution #44302 (comment) with drawbacks mentioned in #44302 (comment) |
* deprecate msearch * Missing export * adjust tests, revert loading method of esaggs/boot * getInjectedMetadata * Fix jest tests * update default strategy abort test * notice update * Allow running discover errors test independently * Remove batchSearches * Detect painless script error * don't show notifications for aborted requests * Fix jest tests * Restore loader indicator * Decreace loading count on error * update search test * Trigger digest after fetching fresh index patterns * Revert isEqual * accurate revert * Return full error details to client from search endpoint * Re-throw AbortError from http when user aborts request. * fix typo * typo * Adjust routes jest test * Restore msearch using a separate es connection * typescript fixes * set http service mock * Move es client to dat aplugin, for follow up PR * Add karma mock * krma mock * fix tests * ts * Pass in version dynamically * add headers to esClient host * Restored fetch soon test Use tap for loadingCount side effects * Cleanup search params * Cleanup search params test * Revert "Cleanup search params" This reverts commit ca9dea0. * Revert "Cleanup search params test" This reverts commit 30b9478. * Revert code to use old es client until #44302 is resolved * Revert changes to getPainlessError * Fix jest test * Refactor esClient to trigger loadingIndicator * fixing tests * use esClient from searchService * git remove comment * fix jest Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* deprecate msearch * Missing export * adjust tests, revert loading method of esaggs/boot * getInjectedMetadata * Fix jest tests * update default strategy abort test * notice update * Allow running discover errors test independently * Remove batchSearches * Detect painless script error * don't show notifications for aborted requests * Fix jest tests * Restore loader indicator * Decreace loading count on error * update search test * Trigger digest after fetching fresh index patterns * Revert isEqual * accurate revert * Return full error details to client from search endpoint * Re-throw AbortError from http when user aborts request. * fix typo * typo * Adjust routes jest test * Restore msearch using a separate es connection * typescript fixes * set http service mock * Move es client to dat aplugin, for follow up PR * Add karma mock * krma mock * fix tests * ts * Pass in version dynamically * add headers to esClient host * Restored fetch soon test Use tap for loadingCount side effects * Cleanup search params * Cleanup search params test * Revert "Cleanup search params" This reverts commit ca9dea0. * Revert "Cleanup search params test" This reverts commit 30b9478. * Revert code to use old es client until elastic#44302 is resolved * Revert changes to getPainlessError * Fix jest test * Refactor esClient to trigger loadingIndicator * fixing tests * use esClient from searchService * git remove comment * fix jest Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
* deprecate msearch * Missing export * adjust tests, revert loading method of esaggs/boot * getInjectedMetadata * Fix jest tests * update default strategy abort test * notice update * Allow running discover errors test independently * Remove batchSearches * Detect painless script error * don't show notifications for aborted requests * Fix jest tests * Restore loader indicator * Decreace loading count on error * update search test * Trigger digest after fetching fresh index patterns * Revert isEqual * accurate revert * Return full error details to client from search endpoint * Re-throw AbortError from http when user aborts request. * fix typo * typo * Adjust routes jest test * Restore msearch using a separate es connection * typescript fixes * set http service mock * Move es client to dat aplugin, for follow up PR * Add karma mock * krma mock * fix tests * ts * Pass in version dynamically * add headers to esClient host * Restored fetch soon test Use tap for loadingCount side effects * Cleanup search params * Cleanup search params test * Revert "Cleanup search params" This reverts commit ca9dea0. * Revert "Cleanup search params test" This reverts commit 30b9478. * Revert code to use old es client until #44302 is resolved * Revert changes to getPainlessError * Fix jest test * Refactor esClient to trigger loadingIndicator * fixing tests * use esClient from searchService * git remove comment * fix jest Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
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
Related: #42764
The text was updated successfully, but these errors were encountered: