From 07ab8d4c4b65ee54be92f1e9ea598d209c416f86 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Mon, 16 Sep 2019 18:15:56 +0200 Subject: [PATCH] Draft RFC for unblocking kibana startup --- rfcs/text/0006_lifecycle_unblocked.md | 206 +++++++++++++------------- 1 file changed, 105 insertions(+), 101 deletions(-) diff --git a/rfcs/text/0006_lifecycle_unblocked.md b/rfcs/text/0006_lifecycle_unblocked.md index e1e5d1f90c618e0..dc68a8f72587a02 100644 --- a/rfcs/text/0006_lifecycle_unblocked.md +++ b/rfcs/text/0006_lifecycle_unblocked.md @@ -22,20 +22,21 @@ resolve before calling the next. This allows plugins to depend on the API's returned from other plugins. # Detailed design -1. Lifecycle functions are synchronous functions, they can perform asynchronous - operations but Core doesn't wait for these to complete. - -2. Context provider functions are synchronous functions. - +1. Synchronous lifecycle functions +2. Synchronous Context provider functions 3. A new Plugin Lifecycle Context is introduced for exposing API's to plugins. - 4. Core only exposes it's API's through the Plugin Lifecycle Context. - 5. Plugins register their API's with the Plugin Lifecycle Context in order to expose functionality to other plugins. ### 1. Synchronous lifecycle functions +Lifecycle functions are synchronous functions, they can perform asynchronous +operations but Core doesn't wait for these to complete. This guarantees that +no plugin lifecycle function can block other plugins or core from starting up. + ### 2. Synchronous Context provider functions +Synchronous Context provider functions guarantee that a context handler will +never be blocked by downstream context providers. ```ts export type IContextProvider< @@ -48,7 +49,32 @@ export type IContextProvider< ) => TContext[TContextName]; ``` -*(2.1): exposing elasticsearch through the route handler context:* +### 3. Core should not expose API's as Observables +All Core API's should be reactive, when internal state changes their behaviour +should change accordingly. But, exposing these internal state changes as part +of the API contract leaks internal implementation details consumers can't do +anything useful with and don't care about. + +For example: Core currently exposes `core.elasticsearch.adminClient$`, an +Observable which emits a pre-configured elasticsearch client everytime there's +a configuration change. This includes changes such as the elasticsearch +cluster `hosts` that alter the behaviour of the elasticsearch client. As a +plugin author who wants to make search requests against elasticsearch I +shouldn't have to care about, react to, or keep track of, how many times the +underlying configuration has changed. I want to use the `callAsInternalUser` +method and I expect Core to use the most up to date configuration to send this +request to the correct `hosts`. + +This does not mean we should remove all observables from Core's API's. When an +API consumer is interested in the *state changes itself* it absolutely makes +sense to expose this as an Observable. Good examples of this is exposing +plugin config as this is state that changes over time to which a plugin should +directly react to. + +This is important in the context of synchronous lifecycle methods and context +handlers since exposing convenient API's become very ugly: + +*(3.1): exposing Observable-based API's through the route handler context:* ```ts // Before: Using an asynchronous context provider coreSetup.http.registerRouteHandlerContext(coreId, 'core', async (context, req) => { @@ -66,7 +92,7 @@ coreSetup.http.registerRouteHandlerContext(coreId, 'core', async (context, req) coreSetup.http.registerRouteHandlerContext(coreId, 'core', async (context, req) => { return { elasticsearch: { - // (2.1.1) We can expose a convenient API by doing a lot of work + // (3.1.1) We can expose a convenient API by doing a lot of work adminClient: () => { callAsInternalUser: async (...args) => { adminClient = await coreSetup.elasticsearch.adminClient$.pipe(take(1)).toPromise(); @@ -77,7 +103,7 @@ coreSetup.http.registerRouteHandlerContext(coreId, 'core', async (context, req) return adminClient.asScoped(req).callAsCurrentUser(args); } }, - // (2.1.2) A slightly more awkward API to consume, but easier to build up + // (3.1.2) Or a lazy approach which perpetuates the problem to consumers: dataClient: async () => { const dataClient = await coreSetup.elasticsearch.dataClient$.pipe(take(1)).toPromise(); dataClient.asScoped(req), @@ -87,6 +113,40 @@ coreSetup.http.registerRouteHandlerContext(coreId, 'core', async (context, req) }); ``` +### Complete example code +```ts +export class Plugin { + public setup(core: CoreSetup) { + // Async setup is possible and any operations involving asynchronous API's + // will still block until these API's are ready, (savedObjects find only + // resolves once the elasticsearch client has established a connection to + // the cluster). The difference is that these details are now internal to + // the API. + (async () => { + const docs = await context.core.savedObjects.client.find({...}); + ... + await context.core.savedObjects.client.update(...); + })(); + } +} +``` + +```ts +export class Plugin { + public async setup(core: CoreSetup) { + return { + ping: async () => { + // async & await isn't necessary here, but makes example a bit clearer. + // Note that the elasticsearch client no longer exposes an adminClient$ + // observable. + const result = await core.elasticsearch.adminClient.callAsInternalUser('ping', ...); + return result; + } + }; + } +} +``` + ### 3. & 4. Core only exposes it's API's through the Plugin Lifecycle Context ```ts @@ -133,99 +193,43 @@ export class Plugin { ``` # Drawbacks - -Why should we *not* do this? Please consider: - -- implementation cost, both in term of code size and complexity -- the impact on teaching people Kibana development -- integration of this feature with other existing and planned features -- cost of migrating existing Kibana plugins (is it a breaking change?) - -There are tradeoffs to choosing any path. Attempt to identify them here. +Not being able to block on a lifecycle method also means plugins can no longer +be certain that all setup is complete before they reach the start lifecycle. +Plugins will have to manage such state internally. Core will still expose +special API's that are able block the setup lifecycle such as registering +Saved Object migrations, but this will be limited to operations where the risk +of blocking all of kibana starting up is limited. # Alternatives -## 1. Just make lifecycle functions synchronous -1. Lifecycle functions are synchronous functions, they can perform asynchronous - operations but Core doesn't wait for these to complete. - -2. Plugins continue to expose their API's by returning from a lifecycle -function. There are no convenient "context" or the luxury of delaying exposing -the full API until all dependencies are available. Each API function will have -to wait for it's dependencies to become available. - -```ts -export class Plugin { - public setup(core: CoreSetup) { - return { - search: (id) => { - return core.elasticsearch.adminClient$.pipe( - last(), - map((adminClient) => { - return adminClient.callAsInternalUser('endpoint', id); - }) - ).toPromise(); - }, - getObject: (id) => { - return core.savedObjects.client$.pipe( - last(), - map((soClient) => { - return soClient.find(id); - }) - ).toPromise(); - } - } - } -} -``` - -## 2. Expose Plugin API's as Observables - -The main benefit of this approach is Plugin authors have full control over -which dependencies they want to block on and can fully control the behaviour ( -e.g. if elasticsearch isn't available within 5 seconds, expose the API anyway -knowing some functions will always fail). - -1. Lifecycle functions are synchronous functions, they can perform asynchronous - operations but Core doesn't wait for these to complete. - -2. Plugins register their API's with Core as Observables - -```ts -interface CoreSetup { - ... - registerSetupApi( - name: string, - provider: () => BehaviourSubject[T]> - ), -} -``` - -```ts -export class Plugin { - public setup(core: CoreSetup) { - core.registerSetupApi( - name: 'data', - provider: () => { - // Here plugin chooses to "block" exposing it's API until both saved - // objects and elasticsearch clients are available. - return combineLatest( - core.elasticsearch.adminClient$, - core.savedObjects.client$ - ).pipe(map((adminClient, savedObjectsClient) => { - return { - search: (id) => { - return adminClient.callAsInternalUser('endpoint', id); - }, - getObject: (id) => { - return savedObjectsClient.get(id); - } - } - })) - } - ) - } -} -``` +## 1. Introduce a lifecycle/context provider timeout +Lifecycle methods and context providers would timeout after X seconds and any +API's they expose would not be available if the timeout had been reached. + +Drawbacks: +1. For lifecycle methods: there would be no way to recover from a timeout, + once a timeout had been reached the API will remain unavailable. + + Context providers have the benefit of being re-created for each handler + call, so a single timeout would not permanently disable the API. + +3. Plugins have less control over their behaviour. When an upstream server + becomes unavailable, a plugin might prefer to keep retrying the request + indefinitely or only timeout after more than X seconds. + +2. (minor) Introduces an additional failure condition that needs to be handled. + Consumers should handle the API not being available and error responses from + the API itself. + +## 2. Treat anything that blocks Kibana from starting up as a bug +Effectively do what we've always been doing. We can improve on the status quo +by logging detailed diagnostic info on any plugins that appear to be blocking +startup. + +Drawbacks: +1. Since plugins load serially, even if they don't block startup, all the + delays could add up and potentially make startup very slow. +2. This opens up the potential for a third-party plugin to effectively "break" + kibana which creates a bad user experience. # Adoption strategy