From 5d7d0c98910d7221802c1b6fc8cf7001b96d1c60 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Wed, 11 Sep 2019 15:03:33 +0200 Subject: [PATCH 01/15] Draft RFC for unblocking plugin lifecycle methods --- rfcs/text/0006_lifecycle_unblocked.md | 250 ++++++++++++++++++++++++++ 1 file changed, 250 insertions(+) create mode 100644 rfcs/text/0006_lifecycle_unblocked.md diff --git a/rfcs/text/0006_lifecycle_unblocked.md b/rfcs/text/0006_lifecycle_unblocked.md new file mode 100644 index 0000000000000..e1e5d1f90c618 --- /dev/null +++ b/rfcs/text/0006_lifecycle_unblocked.md @@ -0,0 +1,250 @@ +- Start Date: 2019-09-11 +- RFC PR: (leave this empty) +- Kibana Issue: (leave this empty) + +# Summary + +Prevent plugin lifecycle functions from blocking Kibana startup + +# Basic example + +If the proposal involves a new or changed API, include a basic code example. +Omit this section if it's not applicable. + +# Motivation +We should make it impossible for a single plugin lifecycle function to stall +all of kibana. + +### Background: +Plugin lifecycle functions are async (promise-returning) functions. Core runs +these functions in series and waits for each plugin's lifecycle function to +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. + +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 +### 2. Synchronous Context provider functions + +```ts +export type IContextProvider< + TContext extends Record, + TContextName extends keyof TContext, + TProviderParameters extends any[] = [] +> = ( + context: Partial, + ...rest: TProviderParameters +) => TContext[TContextName]; +``` + +*(2.1): exposing elasticsearch through the route handler context:* +```ts +// Before: Using an asynchronous context provider +coreSetup.http.registerRouteHandlerContext(coreId, 'core', async (context, req) => { + const adminClient = await coreSetup.elasticsearch.adminClient$.pipe(take(1)).toPromise(); + const dataClient = await coreSetup.elasticsearch.dataClient$.pipe(take(1)).toPromise(); + return { + elasticsearch: { + adminClient: adminClient.asScoped(req), + dataClient: dataClient.asScoped(req), + }, + }; +}); + +// After: Using a synchronous context provider +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 + adminClient: () => { + callAsInternalUser: async (...args) => { + adminClient = await coreSetup.elasticsearch.adminClient$.pipe(take(1)).toPromise(); + return adminClient.asScoped(req).callAsinternalUser(args); + }, + callAsCurrentUser: async (...args) => { + adminClient = await coreSetup.elasticsearch.adminClient$.pipe(take(1)).toPromise(); + return adminClient.asScoped(req).callAsCurrentUser(args); + } + }, + // (2.1.2) A slightly more awkward API to consume, but easier to build up + dataClient: async () => { + const dataClient = await coreSetup.elasticsearch.dataClient$.pipe(take(1)).toPromise(); + dataClient.asScoped(req), + }, + }, + }; +}); +``` + +### 3. & 4. Core only exposes it's API's through the Plugin Lifecycle Context + +```ts +// CoreSetup no longer contains any API's like elasticsearch, saved objects +interface CoreSetup { + withContext((context: SetupContext) => void), + registerSetupApi( + name: T, + provider: (context: Partial) => Promise + ), +} +``` + +```ts +export class Plugin { + public setup(core: CoreSetup) { + // withContext builds up an updated context and executes the passed in + // handler in the same 'tick'. + core.withContext(async (context: SetupContext) => { + // the only reason for passing an async function is so that we can use + // await internally to make it easier to do asynchronous operations in + // series. withContext completely ignores the return value of the handler + const docs = await context.core.savedObjects.client.find({...}); + ... + await context.core.savedObjects.client.update(...); + }); + } +} +``` + +### 5. Plugins register their API's with the Plugin Lifecycle Context +```ts +export class Plugin { + public async setup(core: CoreSetup) { + core.registerSetupApi( + name: 'data', + provider: (context: SetupContext) => ({ + ping: () => { + return context.core.elasticsearch.adminClient.callAsInternalUser('ping', ...) + }) + ); + } +} +``` + +# 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. + +# 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); + } + } + })) + } + ) + } +} +``` + +# Adoption strategy + +If we implement this proposal, how will existing Kibana developers adopt it? Is +this a breaking change? Can we write a codemod? Should we coordinate with +other projects or libraries? + +# How we teach this + +What names and terminology work best for these concepts and why? How is this +idea best presented? As a continuation of existing Kibana patterns? + +Would the acceptance of this proposal mean the Kibana documentation must be +re-organized or altered? Does it change how Kibana is taught to new developers +at any level? + +How should this feature be taught to existing Kibana developers? + +# Unresolved questions + +Optional, but suggested for first drafts. What parts of the design are still +TBD? \ No newline at end of file From ee1ec45713774c60eebe04bfda200c47abb67864 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Mon, 16 Sep 2019 18:15:56 +0200 Subject: [PATCH 02/15] Draft RFC for unblocking kibana startup --- rfcs/text/0006_lifecycle_unblocked.md | 271 ++++++++++++-------------- 1 file changed, 123 insertions(+), 148 deletions(-) diff --git a/rfcs/text/0006_lifecycle_unblocked.md b/rfcs/text/0006_lifecycle_unblocked.md index e1e5d1f90c618..c54a0a913d6bb 100644 --- a/rfcs/text/0006_lifecycle_unblocked.md +++ b/rfcs/text/0006_lifecycle_unblocked.md @@ -4,38 +4,35 @@ # Summary -Prevent plugin lifecycle functions from blocking Kibana startup - -# Basic example - -If the proposal involves a new or changed API, include a basic code example. -Omit this section if it's not applicable. +Prevent plugin lifecycle methods from blocking Kibana startup by making the +following changes: +1. Synchronous lifecycle methods +2. Synchronous context provider functions +3. Core should not expose API's as observables # Motivation -We should make it impossible for a single plugin lifecycle function to stall -all of kibana. - -### Background: -Plugin lifecycle functions are async (promise-returning) functions. Core runs -these functions in series and waits for each plugin's lifecycle function to -resolve before calling the next. This allows plugins to depend on the API's -returned from other plugins. +Plugin lifecycle methods and context provider functions are async +(promise-returning) functions. Core runs these functions in series and waits +for each plugin's lifecycle function to 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. +With the current design, a single lifecycle method or context provider that +blocks will block all of Kibana from starting up. -3. A new Plugin Lifecycle Context is introduced for exposing API's to plugins. +We should make it impossible for a single plugin lifecycle function to +stall all of kibana. -4. Core only exposes it's API's through the Plugin Lifecycle Context. +# Detailed design -5. Plugins register their API's with the Plugin Lifecycle Context in order to - expose functionality to other plugins. +### 1. Synchronous lifecycle methods +Lifecycle methods 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. -### 1. Synchronous lifecycle functions -### 2. Synchronous Context provider functions +### 2. Synchronous Context Provider functions +Making context provider functions synchronous guarantees that a context +handler will never be blocked by registered context providers. They can expose +asynchronous API's which could potentially have blocking behaviour. ```ts export type IContextProvider< @@ -48,7 +45,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 every time 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 +88,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 +99,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,153 +109,107 @@ coreSetup.http.registerRouteHandlerContext(coreId, 'core', async (context, req) }); ``` -### 3. & 4. Core only exposes it's API's through the Plugin Lifecycle Context - -```ts -// CoreSetup no longer contains any API's like elasticsearch, saved objects -interface CoreSetup { - withContext((context: SetupContext) => void), - registerSetupApi( - name: T, - provider: (context: Partial) => Promise - ), -} -``` - +### 4. Complete example code +*(4.1) Doing async operations in a plugin's setup lifecycle* ```ts export class Plugin { public setup(core: CoreSetup) { - // withContext builds up an updated context and executes the passed in - // handler in the same 'tick'. - core.withContext(async (context: SetupContext) => { - // the only reason for passing an async function is so that we can use - // await internally to make it easier to do asynchronous operations in - // series. withContext completely ignores the return value of the handler + // 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(...); - }); + })(); } } ``` -### 5. Plugins register their API's with the Plugin Lifecycle Context +*(4.2) Exposing an API from a plugin's setup lifecycle* ```ts export class Plugin { public async setup(core: CoreSetup) { - core.registerSetupApi( - name: 'data', - provider: (context: SetupContext) => ({ - ping: () => { - return context.core.elasticsearch.adminClient.callAsInternalUser('ping', ...) - }) - ); + 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; + } + }; } } ``` -# 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?) +*(4.3) Exposing an observable free Elasticsearch API from the route context* +```ts +coreSetup.http.registerRouteHandlerContext(coreId, 'core', async (context, req) => { + return { + elasticsearch: { + adminClient: coreSetup.elasticsearch.adminClient.asScoped(req), + dataClient: coreSetup.elasticsearch.adminClient.asScoped(req), + }, + }; +}); +``` -There are tradeoffs to choosing any path. Attempt to identify them here. +# Drawbacks +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. +## 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. -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. +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. -```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(); - } - } - } -} -``` + Context providers have the benefit of being re-created for each handler + call, so a single timeout would not permanently disable the API. -## 2. Expose Plugin API's as Observables +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. It also isn't able + to expose detailed error information to downstream consumers such as + specifying which host or service is unavailable. -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). +5. (minor) Introduces an additional failure condition that needs to be handled. + Consumers should handle the API not being available in setup, as well as, + error responses from the API itself. Since remote hosts like Elasticsearch + could go down even after a successful setup, this effectively means API + consumers have to handle the same error condition in two places. -1. Lifecycle functions are synchronous functions, they can perform asynchronous - operations but Core doesn't wait for these to complete. +## 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. -2. Plugins register their API's with Core as Observables +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. -```ts -interface CoreSetup { - ... - registerSetupApi( - name: string, - provider: () => BehaviourSubject[T]> - ), -} -``` +# Adoption strategy (WIP) -```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); - } - } - })) - } - ) - } -} -``` +Making context provider functions synchronous (2) and not exposing core API's +as observables (3) would require the least amount of change from plugins since +adoption on these API's are still fairly low. -# Adoption strategy +Having synchronous lifecycle methods (1) would have a bigger impact on plugins +since most NP shims were built with asynchronous methods in mind. -If we implement this proposal, how will existing Kibana developers adopt it? Is -this a breaking change? Can we write a codemod? Should we coordinate with -other projects or libraries? - -# How we teach this +# How we teach this (TBD) What names and terminology work best for these concepts and why? How is this idea best presented? As a continuation of existing Kibana patterns? @@ -245,6 +221,5 @@ at any level? How should this feature be taught to existing Kibana developers? # Unresolved questions - -Optional, but suggested for first drafts. What parts of the design are still -TBD? \ No newline at end of file +Are the drawbacks worth the benefits or can we live with Kibana potentially +being blocked for the sake of convenient asynchronous lifecycle stages? \ No newline at end of file From a125d8dda134eb5541f2078117f4e64f3ce9219b Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Tue, 24 Sep 2019 22:08:40 +0200 Subject: [PATCH 03/15] Rename rfc from 0006 to 0007 --- .../{0006_lifecycle_unblocked.md => 0007_lifecycle_unblocked.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename rfcs/text/{0006_lifecycle_unblocked.md => 0007_lifecycle_unblocked.md} (100%) diff --git a/rfcs/text/0006_lifecycle_unblocked.md b/rfcs/text/0007_lifecycle_unblocked.md similarity index 100% rename from rfcs/text/0006_lifecycle_unblocked.md rename to rfcs/text/0007_lifecycle_unblocked.md From e91b83cd1dd8f09f669d430391e401f32a5b8dab Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Tue, 24 Sep 2019 22:30:57 +0200 Subject: [PATCH 04/15] Add references to TC39 top-level await --- rfcs/text/0007_lifecycle_unblocked.md | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/rfcs/text/0007_lifecycle_unblocked.md b/rfcs/text/0007_lifecycle_unblocked.md index c54a0a913d6bb..bb4dad7127ef6 100644 --- a/rfcs/text/0007_lifecycle_unblocked.md +++ b/rfcs/text/0007_lifecycle_unblocked.md @@ -194,11 +194,22 @@ 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. +A parallel can be drawn between Kibana's async plugin initialization and the TC39 +proposal for [top-level await](https://github.com/tc39/proposal-top-level-await). +> enables modules to act as big async functions: With top-level await, +> ECMAScript Modules (ESM) can await resources, causing other modules who +> import them to wait before they start evaluating their body + +They believe the benefits outweigh the risk of modules blocking loading since: + - [developer education should result in correct usage](https://github.com/tc39/proposal-top-level-await#will-top-level-await-cause-developers-to-make-their-code-block-longer-than-it-should) + - [there are existing unavoidable ways in which modules could block loading such as infinite loops or recursion](https://github.com/tc39/proposal-top-level-await#does-top-level-await-increase-the-risk-of-deadlocks) + + 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. + delays add up and increase the startup time. +2. This opens up the potential for a bug in Elastic or third-party plugins to + effectively "break" kibana which creates a bad user experience. # Adoption strategy (WIP) From aa1a7885ecd6a8081c60af6b7a945d48882c96e3 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Tue, 1 Oct 2019 15:30:49 +0200 Subject: [PATCH 05/15] Update with review suggestion Co-Authored-By: Court Ewing --- rfcs/text/0007_lifecycle_unblocked.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rfcs/text/0007_lifecycle_unblocked.md b/rfcs/text/0007_lifecycle_unblocked.md index bb4dad7127ef6..32346ec52a4c4 100644 --- a/rfcs/text/0007_lifecycle_unblocked.md +++ b/rfcs/text/0007_lifecycle_unblocked.md @@ -102,7 +102,7 @@ coreSetup.http.registerRouteHandlerContext(coreId, 'core', async (context, req) // (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), + return dataClient.asScoped(req); }, }, }; @@ -233,4 +233,4 @@ How should this feature be taught to existing Kibana developers? # Unresolved questions Are the drawbacks worth the benefits or can we live with Kibana potentially -being blocked for the sake of convenient asynchronous lifecycle stages? \ No newline at end of file +being blocked for the sake of convenient asynchronous lifecycle stages? From 977a43bff27c8f33b87b619b0ee3eb4c0153102f Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Wed, 2 Oct 2019 13:07:00 +0200 Subject: [PATCH 06/15] Update RFC#0007 --- rfcs/text/0007_lifecycle_unblocked.md | 126 ++++++++++++++++++-------- 1 file changed, 90 insertions(+), 36 deletions(-) diff --git a/rfcs/text/0007_lifecycle_unblocked.md b/rfcs/text/0007_lifecycle_unblocked.md index 32346ec52a4c4..74a633c215386 100644 --- a/rfcs/text/0007_lifecycle_unblocked.md +++ b/rfcs/text/0007_lifecycle_unblocked.md @@ -27,7 +27,11 @@ stall all of kibana. ### 1. Synchronous lifecycle methods Lifecycle methods 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. +no plugin lifecycle function can block other plugins or core from starting up. + +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. ### 2. Synchronous Context Provider functions Making context provider functions synchronous guarantees that a context @@ -137,8 +141,7 @@ export class Plugin { // 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; + return await core.elasticsearch.adminClient.callAsInternalUser('ping', ...); } }; } @@ -158,12 +161,26 @@ coreSetup.http.registerRouteHandlerContext(coreId, 'core', async (context, req) ``` # Drawbacks -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. +Not being able to block on a lifecycle method means plugins can no longer be +certain that all setup is "complete" before they expose their API's or reach +the start lifecycle. + +A plugin might want to poll an external host to ensure that the host is up in +its setup lifecycle before making network requests to this host in it's start +lifecycle. In effect, the plugin is polling the world to construct a snapshot +of state which drives future behaviour. Modeling this with lifecycle functions +is insufficient since it assumes that any state constructed in the setup +lifecycle is static and won't and can't be changed in the future. + +For example: a plugin's setup lifecycle might poll for the existence of a +custom Elasticsearch index and if it doesn't exist, create it. Should there be +an Elasticsearch restore which deletes the index, the plugin wouldn't be able +to gracefully recover by simply running it's setup lifecycle a second time. + +The once-off nature of lifecycle methods are incompatible with the real-world +dynamic conditions under which plugins run. Not being able to block a +lifecycle method is, therefore, only a drawback when plugins are authored under +the false illusion of stability. # Alternatives ## 1. Introduce a lifecycle/context provider timeout @@ -171,7 +188,11 @@ 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, +1. A blocking setup lifecycle makes it easy for plugin authors to fall into + the trap of assuming that their plugin's behaviour can continue to operate + based on the snapshot of conditions present during setup. + +2. 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 @@ -183,16 +204,18 @@ Drawbacks: to expose detailed error information to downstream consumers such as specifying which host or service is unavailable. -5. (minor) Introduces an additional failure condition that needs to be handled. +4. (minor) Introduces an additional failure condition that needs to be handled. Consumers should handle the API not being available in setup, as well as, error responses from the API itself. Since remote hosts like Elasticsearch could go down even after a successful setup, this effectively means API consumers have to handle the same error condition in two places. ## 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. +Keep the existing New Platform blocking behaviour, but through strong +conventions and developer awareness minimize the risk of plugins blocking +Kibana's startup indefinetely. By logging detailed diagnostic info on any +plugins that appear to be blocking startup, we can aid system administrators +to recover a blocked Kibana. A parallel can be drawn between Kibana's async plugin initialization and the TC39 proposal for [top-level await](https://github.com/tc39/proposal-top-level-await). @@ -206,31 +229,62 @@ They believe the benefits outweigh the risk of modules blocking loading since: Drawbacks: -1. Since plugins load serially, even if they don't block startup, all the +1. A blocking setup lifecycle makes it easy for plugin authors to fall into + the trap of assuming that their plugin's behaviour can continue to operate + based on the snapshot of conditions present during setup. +2. Since plugins load serially, even if they don't block startup, all the delays add up and increase the startup time. -2. This opens up the potential for a bug in Elastic or third-party plugins to +3. This opens up the potential for a bug in Elastic or third-party plugins to effectively "break" kibana which creates a bad user experience. -# Adoption strategy (WIP) - -Making context provider functions synchronous (2) and not exposing core API's -as observables (3) would require the least amount of change from plugins since -adoption on these API's are still fairly low. - -Having synchronous lifecycle methods (1) would have a bigger impact on plugins -since most NP shims were built with asynchronous methods in mind. - -# How we teach this (TBD) - -What names and terminology work best for these concepts and why? How is this -idea best presented? As a continuation of existing Kibana patterns? - -Would the acceptance of this proposal mean the Kibana documentation must be -re-organized or altered? Does it change how Kibana is taught to new developers -at any level? - -How should this feature be taught to existing Kibana developers? +# Adoption strategy +Adoption and implementation should be handled as follows: +a. Don't expose core API's as observables (3). + This should be implemented first to improve the ergonomics of working with + core API's from inside synchronous context providers and lifecycle functions. + + Plugins consuming observable-based API's generally follow a pattern like the + following which effectively ignores the observable and should be easy to + refactor: + + ```diff + - const adminClient = await coreSetup.elasticsearch.adminClient$.pipe(take(1)).toPromise(); + + const adminClient = coreSetup.elasticsearch.adminClient; + ``` +b. Making context provider functions synchronous (2) + Adoption of context provider functions is still fairly low, so the amount + of change required by plugin authors should be limited. +c. Synchronous lifecycle methods (1) will have the biggest impact on plugins + since many NP plugins and shims have been built with asynchronous lifecycle + methods in mind. + + The following New Platform plugins or shims currently rely on async lifecycle functions: + 1. [region_map](https://github.com/elastic/kibana/blob/6039709929caf0090a4130b8235f3a53bd04ed84/src/legacy/core_plugins/region_map/public/plugin.ts#L68) + 2. [tile_map](https://github.com/elastic/kibana/blob/6039709929caf0090a4130b8235f3a53bd04ed84/src/legacy/core_plugins/tile_map/public/plugin.ts#L62) + 3. [vis_type_table](https://github.com/elastic/kibana/blob/6039709929caf0090a4130b8235f3a53bd04ed84/src/legacy/core_plugins/vis_type_table/public/plugin.ts#L61) + 4. [vis_type_vega](https://github.com/elastic/kibana/blob/6039709929caf0090a4130b8235f3a53bd04ed84/src/legacy/core_plugins/vis_type_vega/public/plugin.ts#L59) + 5. [timelion](https://github.com/elastic/kibana/blob/9d69b72a5f200e58220231035b19da852fc6b0a5/src/plugins/timelion/server/plugin.ts#L40) + 6. [code](https://github.com/elastic/kibana/blob/5049b460b47d4ae3432e1d9219263bb4be441392/x-pack/legacy/plugins/code/server/plugin.ts#L129-L149) + 7. [spaces](https://github.com/elastic/kibana/blob/096c7ee51136327f778845c636d7c4f1188e5db2/x-pack/legacy/plugins/spaces/server/new_platform/plugin.ts#L95) + 8. [licensing](https://github.com/elastic/kibana/blob/4667c46caef26f8f47714504879197708debae32/x-pack/plugins/licensing/server/plugin.ts) + 9. [security](https://github.com/elastic/kibana/blob/0f2324e44566ce2cf083d89082841e57d2db6ef6/x-pack/plugins/security/server/plugin.ts#L96) + +Once this RFC has been merged we should educate teams on the implications to +allow existing New Platform plugins to remove any asynchronous lifecycle +behaviour they're relying on before we change this in core. + +# How we teach this + +Plugin lifecycle methods in the New Platform are no longer asynchronous. +Plugins should treat the setup lifecycle as a place in time to register +functionality with core or other plugins' API's and not as a mechanism to kick +off and wait for any initialization that's required for the plugin to be able +to run. # Unresolved questions -Are the drawbacks worth the benefits or can we live with Kibana potentially +1. Are the drawbacks worth the benefits or can we live with Kibana potentially being blocked for the sake of convenient asynchronous lifecycle stages? + +2. Should core provide conventions or patterns for plugins to construct a + snapshot of state and reactively updating this state and the behaviour it + drives as the state of the world changes? From 6b571232aed0eb180b56953436462703e5c63e07 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Fri, 25 Oct 2019 16:21:51 +0200 Subject: [PATCH 07/15] Apply suggestions from code review Co-Authored-By: Aleh Zasypkin --- rfcs/text/0007_lifecycle_unblocked.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rfcs/text/0007_lifecycle_unblocked.md b/rfcs/text/0007_lifecycle_unblocked.md index 74a633c215386..427ccc016bcea 100644 --- a/rfcs/text/0007_lifecycle_unblocked.md +++ b/rfcs/text/0007_lifecycle_unblocked.md @@ -95,7 +95,7 @@ coreSetup.http.registerRouteHandlerContext(coreId, 'core', async (context, req) // (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(); + const adminClient = await coreSetup.elasticsearch.adminClient$.pipe(take(1)).toPromise(); return adminClient.asScoped(req).callAsinternalUser(args); }, callAsCurrentUser: async (...args) => { @@ -135,7 +135,7 @@ export class Plugin { *(4.2) Exposing an API from a plugin's setup lifecycle* ```ts export class Plugin { - public async setup(core: CoreSetup) { + public setup(core: CoreSetup) { return { ping: async () => { // async & await isn't necessary here, but makes example a bit clearer. From 618e3b338575c76073e7d92238eacecd22e30381 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Mon, 28 Oct 2019 16:44:02 +0100 Subject: [PATCH 08/15] Address review comments from @joshdover and @azasypkin 1. Fleshed out motivation, this RFC doesn't prevent errors, but isolates the impact on the rest of Kibana. 2. Added a footnote explaining that sync lifecycles can still block on sync for loops, so it's not a perfect guarantee (from @azasypkin). 3. Updated IContextProvider type signature in (2) to match latest master 4. Dynamically reloading configuration changes should be limited to a whitelist, risky changes like the Elasticsearch host should still require a complete restart. Added to (3) based on https://github.com/elastic/kibana/pull/45796#discussion_r331277153 5. Added Section 5, "Core should expose a status signal for Core services & plugins" (from @joshdover) 6. Added the drawback that incorrect, but valid config would not block Kibana, and might only be surfaced when the associted API/UI gets used (from @azasypkin) --- rfcs/text/0007_lifecycle_unblocked.md | 117 ++++++++++++++++++-------- 1 file changed, 83 insertions(+), 34 deletions(-) diff --git a/rfcs/text/0007_lifecycle_unblocked.md b/rfcs/text/0007_lifecycle_unblocked.md index 427ccc016bcea..8c83e2a02873a 100644 --- a/rfcs/text/0007_lifecycle_unblocked.md +++ b/rfcs/text/0007_lifecycle_unblocked.md @@ -13,21 +13,27 @@ following changes: # Motivation Plugin lifecycle methods and context provider functions are async (promise-returning) functions. Core runs these functions in series and waits -for each plugin's lifecycle function to resolve before calling the next. This -allows plugins to depend on the API's returned from other plugins. +for each plugin's lifecycle/context provider function to resolve before +calling the next. This allows plugins to depend on the API's returned from +other plugins. With the current design, a single lifecycle method or context provider that -blocks will block all of Kibana from starting up. +blocks will block all of Kibana from starting up. Plugins (including legacy +plugins) rely heavily on this blocking behaviour to ensure that all conditions +required for their plugin's operation are met before their plugin is started +and exposes it's API's. This means a single plugin with a network error that +isn't retried or a dependency on an external host that is down, could block +all of Kibana from starting up. -We should make it impossible for a single plugin lifecycle function to -stall all of kibana. +We should make it impossible for a single plugin lifecycle function to stall +all of kibana. # Detailed design ### 1. Synchronous lifecycle methods -Lifecycle methods 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. +Lifecycle methods are synchronous functions, they can perform async 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 [1]. 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 @@ -36,34 +42,40 @@ operations where the risk of blocking all of kibana starting up is limited. ### 2. Synchronous Context Provider functions Making context provider functions synchronous guarantees that a context handler will never be blocked by registered context providers. They can expose -asynchronous API's which could potentially have blocking behaviour. +async API's which could potentially have blocking behaviour. ```ts export type IContextProvider< - TContext extends Record, - TContextName extends keyof TContext, - TProviderParameters extends any[] = [] + THandler extends HandlerFunction, + TContextName extends keyof HandlerContextType > = ( - context: Partial, - ...rest: TProviderParameters -) => TContext[TContextName]; + context: Partial>, + ...rest: HandlerParameters +) => + | HandlerContextType[TContextName]; ``` ### 3. Core should not expose API's as observables -All Core API's should be reactive, when internal state changes their behaviour +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 every time 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`. +a configuration change. This includes changes to the logging configuration and +might in the future include updating the authentication headers sent to +elasticsearch https://github.com/elastic/kibana/issues/19829. 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. + +> Note: It would not be desirable for Core to dynamically load all +> configuration changes. Changing the Elasticsearch `hosts` could mean Kibana +> is pointing to a completely new Elasticsearch cluster. Since this is a risky +> change to make and would likely require core and almost all plugins to +> completely re-initialize, it's safer to require a complete Kibana restart. 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 @@ -76,7 +88,7 @@ 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 +// Before: Using an async context provider coreSetup.http.registerRouteHandlerContext(coreId, 'core', async (context, req) => { const adminClient = await coreSetup.elasticsearch.adminClient$.pipe(take(1)).toPromise(); const dataClient = await coreSetup.elasticsearch.dataClient$.pipe(take(1)).toPromise(); @@ -118,7 +130,7 @@ coreSetup.http.registerRouteHandlerContext(coreId, 'core', async (context, req) ```ts export class Plugin { public setup(core: CoreSetup) { - // Async setup is possible and any operations involving asynchronous API's + // Async setup is possible and any operations involving async 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 @@ -160,6 +172,25 @@ coreSetup.http.registerRouteHandlerContext(coreId, 'core', async (context, req) }); ``` +### 4. Core should expose a status signal for Core services & plugins +Core should expose a global mechanism for core services and plugins to signal +their status. This is equivalent to the legacy status API +`kibana.Plugin.status` which allowed plugins to set their status to e.g. 'red' +or 'green'. The exact design of this API is outside of the scope of this RFC. + +What is important, is that there is a global mechanism to signal status +changes which Core then makes visible to system administrators in the Kibana +logs and the `/status` HTTP API. Plugins should be able to inspect and +subscribe to status changes from any of their dependencies. + +This will provide an obvious mechanism for plugins to signal that the +conditions which are required for this plugin to operate are not currently +present and manual intervention might be required. Status changes can happen +in both setup and start lifecycles e.g.: + - [setup] a required remote host is down + - [start] a remote host which was up during setup, started returning + connection timeout errors. + # Drawbacks Not being able to block on a lifecycle method means plugins can no longer be certain that all setup is "complete" before they expose their API's or reach @@ -167,7 +198,17 @@ the start lifecycle. A plugin might want to poll an external host to ensure that the host is up in its setup lifecycle before making network requests to this host in it's start -lifecycle. In effect, the plugin is polling the world to construct a snapshot +lifecycle. + +Even if Kibana was using a valid, but incorrect configuration for the remote +host, with synchronous lifecycles Kibana would still start up. Although the +status API and logs would indicate a problem, these might not be monitored +leading to the error only being discovered once someone tries to use it's +functionality. This is an acceptable drawback because it buys us isolation. +Some problems might go unnoticed, but no single plugin should affect the +availability of all other plugins. + +In effect, the plugin is polling the world to construct a snapshot of state which drives future behaviour. Modeling this with lifecycle functions is insufficient since it assumes that any state constructed in the setup lifecycle is static and won't and can't be changed in the future. @@ -232,13 +273,14 @@ Drawbacks: 1. A blocking setup lifecycle makes it easy for plugin authors to fall into the trap of assuming that their plugin's behaviour can continue to operate based on the snapshot of conditions present during setup. -2. Since plugins load serially, even if they don't block startup, all the - delays add up and increase the startup time. -3. This opens up the potential for a bug in Elastic or third-party plugins to - effectively "break" kibana which creates a bad user experience. +2. This opens up the potential for a bug in Elastic or third-party plugins to + effectively "break" kibana. Instead of a single plugin being disabled all + of kibana would be down requiring manual intervention by a system + administrator. # Adoption strategy Adoption and implementation should be handled as follows: + a. Don't expose core API's as observables (3). This should be implemented first to improve the ergonomics of working with core API's from inside synchronous context providers and lifecycle functions. @@ -251,11 +293,13 @@ a. Don't expose core API's as observables (3). - const adminClient = await coreSetup.elasticsearch.adminClient$.pipe(take(1)).toPromise(); + const adminClient = coreSetup.elasticsearch.adminClient; ``` + b. Making context provider functions synchronous (2) Adoption of context provider functions is still fairly low, so the amount of change required by plugin authors should be limited. + c. Synchronous lifecycle methods (1) will have the biggest impact on plugins - since many NP plugins and shims have been built with asynchronous lifecycle + since many NP plugins and shims have been built with async lifecycle methods in mind. The following New Platform plugins or shims currently rely on async lifecycle functions: @@ -270,12 +314,12 @@ c. Synchronous lifecycle methods (1) will have the biggest impact on plugins 9. [security](https://github.com/elastic/kibana/blob/0f2324e44566ce2cf083d89082841e57d2db6ef6/x-pack/plugins/security/server/plugin.ts#L96) Once this RFC has been merged we should educate teams on the implications to -allow existing New Platform plugins to remove any asynchronous lifecycle +allow existing New Platform plugins to remove any async lifecycle behaviour they're relying on before we change this in core. # How we teach this -Plugin lifecycle methods in the New Platform are no longer asynchronous. +Plugin lifecycle methods in the New Platform are no longer async. Plugins should treat the setup lifecycle as a place in time to register functionality with core or other plugins' API's and not as a mechanism to kick off and wait for any initialization that's required for the plugin to be able @@ -283,8 +327,13 @@ to run. # Unresolved questions 1. Are the drawbacks worth the benefits or can we live with Kibana potentially -being blocked for the sake of convenient asynchronous lifecycle stages? +being blocked for the sake of convenient async lifecycle stages? 2. Should core provide conventions or patterns for plugins to construct a snapshot of state and reactively updating this state and the behaviour it drives as the state of the world changes? + +# Footnotes +[1] Synchronous lifecycles can still be blocked by e.g. an infine for loop, +but this would always be unintentional behaviour in contrast to intentional +async behaviour like blocking until an external service becomes available. \ No newline at end of file From 545caaca6b8d9884ae11c0796bdd599891ab8594 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Tue, 29 Oct 2019 16:20:15 +0100 Subject: [PATCH 09/15] Formatting: number ordered list instead of letter for github rendering --- rfcs/text/0007_lifecycle_unblocked.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rfcs/text/0007_lifecycle_unblocked.md b/rfcs/text/0007_lifecycle_unblocked.md index 8c83e2a02873a..5c0d5c1f66819 100644 --- a/rfcs/text/0007_lifecycle_unblocked.md +++ b/rfcs/text/0007_lifecycle_unblocked.md @@ -281,7 +281,7 @@ Drawbacks: # Adoption strategy Adoption and implementation should be handled as follows: -a. Don't expose core API's as observables (3). +1. Don't expose core API's as observables (3). This should be implemented first to improve the ergonomics of working with core API's from inside synchronous context providers and lifecycle functions. @@ -294,11 +294,11 @@ a. Don't expose core API's as observables (3). + const adminClient = coreSetup.elasticsearch.adminClient; ``` -b. Making context provider functions synchronous (2) +2. Making context provider functions synchronous (2) Adoption of context provider functions is still fairly low, so the amount of change required by plugin authors should be limited. -c. Synchronous lifecycle methods (1) will have the biggest impact on plugins +3. Synchronous lifecycle methods (1) will have the biggest impact on plugins since many NP plugins and shims have been built with async lifecycle methods in mind. From e606324c44ae04db33e73aafd2f130c7ce986a59 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Thu, 31 Oct 2019 16:28:36 +0100 Subject: [PATCH 10/15] Apply suggestions from code review Co-Authored-By: Josh Dover --- rfcs/text/0007_lifecycle_unblocked.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rfcs/text/0007_lifecycle_unblocked.md b/rfcs/text/0007_lifecycle_unblocked.md index 5c0d5c1f66819..3dfcae251908a 100644 --- a/rfcs/text/0007_lifecycle_unblocked.md +++ b/rfcs/text/0007_lifecycle_unblocked.md @@ -136,9 +136,9 @@ export class Plugin { // the cluster). The difference is that these details are now internal to // the API. (async () => { - const docs = await context.core.savedObjects.client.find({...}); + const docs = await core.savedObjects.client.find({...}); ... - await context.core.savedObjects.client.update(...); + await core.savedObjects.client.update(...); })(); } } @@ -336,4 +336,4 @@ being blocked for the sake of convenient async lifecycle stages? # Footnotes [1] Synchronous lifecycles can still be blocked by e.g. an infine for loop, but this would always be unintentional behaviour in contrast to intentional -async behaviour like blocking until an external service becomes available. \ No newline at end of file +async behaviour like blocking until an external service becomes available. From 332182a763af70ceb6a38baa27b0a7a59807c989 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Fri, 29 Nov 2019 13:05:05 +0100 Subject: [PATCH 11/15] Update rfcs/text/0007_lifecycle_unblocked.md Co-Authored-By: Josh Dover --- rfcs/text/0007_lifecycle_unblocked.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rfcs/text/0007_lifecycle_unblocked.md b/rfcs/text/0007_lifecycle_unblocked.md index 3dfcae251908a..8258b09139b03 100644 --- a/rfcs/text/0007_lifecycle_unblocked.md +++ b/rfcs/text/0007_lifecycle_unblocked.md @@ -172,7 +172,7 @@ coreSetup.http.registerRouteHandlerContext(coreId, 'core', async (context, req) }); ``` -### 4. Core should expose a status signal for Core services & plugins +### 5. Core should expose a status signal for Core services & plugins Core should expose a global mechanism for core services and plugins to signal their status. This is equivalent to the legacy status API `kibana.Plugin.status` which allowed plugins to set their status to e.g. 'red' From 948f7052af1f2ac0f9ecd91e1910e13123445610 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Fri, 29 Nov 2019 17:14:13 +0100 Subject: [PATCH 12/15] Example of plugin exposing API dependent on internal async operation --- rfcs/text/0007_lifecycle_unblocked.md | 31 +++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/rfcs/text/0007_lifecycle_unblocked.md b/rfcs/text/0007_lifecycle_unblocked.md index 8258b09139b03..a8289c805454b 100644 --- a/rfcs/text/0007_lifecycle_unblocked.md +++ b/rfcs/text/0007_lifecycle_unblocked.md @@ -147,12 +147,35 @@ export class Plugin { *(4.2) Exposing an API from a plugin's setup lifecycle* ```ts export class Plugin { + constructor(private readonly initializerContext: PluginInitializerContext) {} + private async initSavedConfig(core: CoreSetup) { + // Note: pulling a config value here means our code isn't reactive to + // changes, but this is equivalent to doing it in an async setup lifecycle. + const config = await this.initializerContext.config + .create>() + .pipe(first()) + .toPromise(); + try { + const savedConfig = await core.savedObjects.internalRepository.get({...}); + return Object.assign({}, config, savedConfig); + } catch (e) { + if (SavedObjectErrorHelpers.isNotFoundError(e)) { + return await core.savedObjects.internalRepository.create(config, {...}); + } + } + } public setup(core: CoreSetup) { + // savedConfigPromise resolves with the same kind of "setup state" that a + // plugin would have constructed in an async setup lifecycle. + const savedConfigPromise = initSavedConfig(core); 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 savedConfig = await savedConfigPromise; + if (config.allowPing === false || savedConfig.allowPing === false) { + throw new Error('ping() has been disabled'); + } + // Note: the elasticsearch client no longer exposes an adminClient$ + // observable, improving the ergonomics of consuming the API. return await core.elasticsearch.adminClient.callAsInternalUser('ping', ...); } }; @@ -336,4 +359,4 @@ being blocked for the sake of convenient async lifecycle stages? # Footnotes [1] Synchronous lifecycles can still be blocked by e.g. an infine for loop, but this would always be unintentional behaviour in contrast to intentional -async behaviour like blocking until an external service becomes available. +async behaviour like blocking until an external service becomes available. \ No newline at end of file From e934829523689fb54a71360c67502c723bfe8cde Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Fri, 29 Nov 2019 17:22:20 +0100 Subject: [PATCH 13/15] Clarify that context providers won't block kibana, just their handlers --- rfcs/text/0007_lifecycle_unblocked.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/rfcs/text/0007_lifecycle_unblocked.md b/rfcs/text/0007_lifecycle_unblocked.md index a8289c805454b..564f854ab22f3 100644 --- a/rfcs/text/0007_lifecycle_unblocked.md +++ b/rfcs/text/0007_lifecycle_unblocked.md @@ -17,8 +17,9 @@ for each plugin's lifecycle/context provider function to resolve before calling the next. This allows plugins to depend on the API's returned from other plugins. -With the current design, a single lifecycle method or context provider that -blocks will block all of Kibana from starting up. Plugins (including legacy +With the current design, a single lifecycle method that blocks will block all +of Kibana from starting up. Similarly, a blocking context provider will block +all the handlers that depend on that context. Plugins (including legacy plugins) rely heavily on this blocking behaviour to ensure that all conditions required for their plugin's operation are met before their plugin is started and exposes it's API's. This means a single plugin with a network error that From 6d3ddd42b0c90f950ad8cfeb06739af78177b32e Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Thu, 12 Dec 2019 20:51:47 +0100 Subject: [PATCH 14/15] Update adoption strategy as per latest discussion --- rfcs/text/0007_lifecycle_unblocked.md | 110 ++++++++++++++++---------- 1 file changed, 67 insertions(+), 43 deletions(-) diff --git a/rfcs/text/0007_lifecycle_unblocked.md b/rfcs/text/0007_lifecycle_unblocked.md index 564f854ab22f3..27b13ba204ef5 100644 --- a/rfcs/text/0007_lifecycle_unblocked.md +++ b/rfcs/text/0007_lifecycle_unblocked.md @@ -2,6 +2,37 @@ - RFC PR: (leave this empty) - Kibana Issue: (leave this empty) +- [Summary](#summary) +- [Motivation](#motivation) +- [Detailed design](#detailed-design) + - [
    +
  1. Synchronous lifecycle methods
    2. +
](#ollisynchronous-lifecycle-methodsliol) + - [
    +
  1. Synchronous Context Provider functions
    2. +
](#ol-start2lisynchronous-context-provider-functionsliol) + - [
    +
  1. Core should not expose API's as observables
    2. +
](#ol-start3licore-should-not-expose-apis-as-observablesliol) + - [
    +
  1. Complete example code
    2. +
](#ol-start4licomplete-example-codeliol) + - [
    +
  1. Core should expose a status signal for Core services & plugins
    2. +
](#ol-start5licore-should-expose-a-status-signal-for-core-services-amp-pluginsliol) +- [Drawbacks](#drawbacks) +- [Alternatives](#alternatives) + - [
    +
  1. Introduce a lifecycle/context provider timeout
    2. +
](#olliintroduce-a-lifecyclecontext-provider-timeoutliol) + - [
    +
  1. Treat anything that blocks Kibana from starting up as a bug
    2. +
](#ol-start2litreat-anything-that-blocks-kibana-from-starting-up-as-a-bugliol) +- [Adoption strategy](#adoption-strategy) +- [How we teach this](#how-we-teach-this) +- [Unresolved questions](#unresolved-questions) +- [Footnotes](#footnotes) + # Summary Prevent plugin lifecycle methods from blocking Kibana startup by making the @@ -303,60 +334,53 @@ Drawbacks: administrator. # Adoption strategy -Adoption and implementation should be handled as follows: +Although the eventual goal is to have sync-only lifecycles / providers, we +will start by deprecating async behaviour and implementing a 30s timeout as +per alternative (1). This will immediately lower the impact of plugin bugs +while at the same time enabling a more incremental rollout and the flexibility +to discover use cases that would require adopting Core API's to support sync +lifecycles / providers. -1. Don't expose core API's as observables (3). - This should be implemented first to improve the ergonomics of working with - core API's from inside synchronous context providers and lifecycle functions. - - Plugins consuming observable-based API's generally follow a pattern like the - following which effectively ignores the observable and should be easy to - refactor: - - ```diff - - const adminClient = await coreSetup.elasticsearch.adminClient$.pipe(take(1)).toPromise(); - + const adminClient = coreSetup.elasticsearch.adminClient; - ``` - -2. Making context provider functions synchronous (2) - Adoption of context provider functions is still fairly low, so the amount - of change required by plugin authors should be limited. - -3. Synchronous lifecycle methods (1) will have the biggest impact on plugins - since many NP plugins and shims have been built with async lifecycle - methods in mind. - - The following New Platform plugins or shims currently rely on async lifecycle functions: - 1. [region_map](https://github.com/elastic/kibana/blob/6039709929caf0090a4130b8235f3a53bd04ed84/src/legacy/core_plugins/region_map/public/plugin.ts#L68) - 2. [tile_map](https://github.com/elastic/kibana/blob/6039709929caf0090a4130b8235f3a53bd04ed84/src/legacy/core_plugins/tile_map/public/plugin.ts#L62) - 3. [vis_type_table](https://github.com/elastic/kibana/blob/6039709929caf0090a4130b8235f3a53bd04ed84/src/legacy/core_plugins/vis_type_table/public/plugin.ts#L61) - 4. [vis_type_vega](https://github.com/elastic/kibana/blob/6039709929caf0090a4130b8235f3a53bd04ed84/src/legacy/core_plugins/vis_type_vega/public/plugin.ts#L59) - 5. [timelion](https://github.com/elastic/kibana/blob/9d69b72a5f200e58220231035b19da852fc6b0a5/src/plugins/timelion/server/plugin.ts#L40) - 6. [code](https://github.com/elastic/kibana/blob/5049b460b47d4ae3432e1d9219263bb4be441392/x-pack/legacy/plugins/code/server/plugin.ts#L129-L149) - 7. [spaces](https://github.com/elastic/kibana/blob/096c7ee51136327f778845c636d7c4f1188e5db2/x-pack/legacy/plugins/spaces/server/new_platform/plugin.ts#L95) - 8. [licensing](https://github.com/elastic/kibana/blob/4667c46caef26f8f47714504879197708debae32/x-pack/plugins/licensing/server/plugin.ts) - 9. [security](https://github.com/elastic/kibana/blob/0f2324e44566ce2cf083d89082841e57d2db6ef6/x-pack/plugins/security/server/plugin.ts#L96) - -Once this RFC has been merged we should educate teams on the implications to -allow existing New Platform plugins to remove any async lifecycle -behaviour they're relying on before we change this in core. +Adoption and implementation should be handled as follows: + - Adopt Core API’s to make sync lifecycles easier (3) + - Update migration guide and other documentation examples. + - Deprecate async lifecycles / context providers with a warning. Add a + timeout of 30s after which a plugin and it's dependencies will be disabled. + - Refactor existing plugin lifecycles which are easily converted to sync + - Future: remove async timeout lifecycles / context providers + +The following New Platform plugins or shims currently rely on async lifecycle +functions and will be impacted: +1. [region_map](https://github.com/elastic/kibana/blob/6039709929caf0090a4130b8235f3a53bd04ed84/src/legacy/core_plugins/region_map/public/plugin.ts#L68) +2. [tile_map](https://github.com/elastic/kibana/blob/6039709929caf0090a4130b8235f3a53bd04ed84/src/legacy/core_plugins/tile_map/public/plugin.ts#L62) +3. [vis_type_table](https://github.com/elastic/kibana/blob/6039709929caf0090a4130b8235f3a53bd04ed84/src/legacy/core_plugins/vis_type_table/public/plugin.ts#L61) +4. [vis_type_vega](https://github.com/elastic/kibana/blob/6039709929caf0090a4130b8235f3a53bd04ed84/src/legacy/core_plugins/vis_type_vega/public/plugin.ts#L59) +5. [timelion](https://github.com/elastic/kibana/blob/9d69b72a5f200e58220231035b19da852fc6b0a5/src/plugins/timelion/server/plugin.ts#L40) +6. [code](https://github.com/elastic/kibana/blob/5049b460b47d4ae3432e1d9219263bb4be441392/x-pack/legacy/plugins/code/server/plugin.ts#L129-L149) +7. [spaces](https://github.com/elastic/kibana/blob/096c7ee51136327f778845c636d7c4f1188e5db2/x-pack/legacy/plugins/spaces/server/new_platform/plugin.ts#L95) +8. [licensing](https://github.com/elastic/kibana/blob/4667c46caef26f8f47714504879197708debae32/x-pack/plugins/licensing/server/plugin.ts) +9. [security](https://github.com/elastic/kibana/blob/0f2324e44566ce2cf083d89082841e57d2db6ef6/x-pack/plugins/security/server/plugin.ts#L96) # How we teach this -Plugin lifecycle methods in the New Platform are no longer async. -Plugins should treat the setup lifecycle as a place in time to register -functionality with core or other plugins' API's and not as a mechanism to kick -off and wait for any initialization that's required for the plugin to be able -to run. +Async Plugin lifecycle methods and async context provider functions have been +deprecated. In the future all lifecycle methods will by sync only. Plugins +should treat the setup lifecycle as a place in time to register functionality +with core or other plugins' API's and not as a mechanism to kick off and wait +for any initialization that's required for the plugin to be able to run. # Unresolved questions -1. Are the drawbacks worth the benefits or can we live with Kibana potentially -being blocked for the sake of convenient async lifecycle stages? +1. ~~Are the drawbacks worth the benefits or can we live with Kibana potentially +being blocked for the sake of convenient async lifecycle stages?~~ 2. Should core provide conventions or patterns for plugins to construct a snapshot of state and reactively updating this state and the behaviour it drives as the state of the world changes? +3. Do plugins ever need to read config values and pass these as parameters to + Core API’s? If so we would have to expose synchronous config values to + support sync lifecycles. + # Footnotes [1] Synchronous lifecycles can still be blocked by e.g. an infine for loop, but this would always be unintentional behaviour in contrast to intentional From 6042a01f385078587e8e09418ef07f69ec2dadf7 Mon Sep 17 00:00:00 2001 From: Rudolf Meijering Date: Thu, 12 Dec 2019 21:04:23 +0100 Subject: [PATCH 15/15] Fix formatting --- rfcs/text/0007_lifecycle_unblocked.md | 31 ++++++++------------------- 1 file changed, 9 insertions(+), 22 deletions(-) diff --git a/rfcs/text/0007_lifecycle_unblocked.md b/rfcs/text/0007_lifecycle_unblocked.md index 27b13ba204ef5..cb978d3dcd7ba 100644 --- a/rfcs/text/0007_lifecycle_unblocked.md +++ b/rfcs/text/0007_lifecycle_unblocked.md @@ -2,32 +2,19 @@ - RFC PR: (leave this empty) - Kibana Issue: (leave this empty) +## Table of contents - [Summary](#summary) - [Motivation](#motivation) - [Detailed design](#detailed-design) - - [
    -
  1. Synchronous lifecycle methods
    2. -
](#ollisynchronous-lifecycle-methodsliol) - - [
    -
  1. Synchronous Context Provider functions
    2. -
](#ol-start2lisynchronous-context-provider-functionsliol) - - [
    -
  1. Core should not expose API's as observables
    2. -
](#ol-start3licore-should-not-expose-apis-as-observablesliol) - - [
    -
  1. Complete example code
    2. -
](#ol-start4licomplete-example-codeliol) - - [
    -
  1. Core should expose a status signal for Core services & plugins
    2. -
](#ol-start5licore-should-expose-a-status-signal-for-core-services-amp-pluginsliol) + - [
  1. Synchronous lifecycle methods
](#ollisynchronous-lifecycle-methodsliol) + - [
  1. Synchronous Context Provider functions
](#ol-start2lisynchronous-context-provider-functionsliol) + - [
  1. Core should not expose API's as observables
](#ol-start3licore-should-not-expose-apis-as-observablesliol) + - [
  1. Complete example code
](#ol-start4licomplete-example-codeliol) + - [
  1. Core should expose a status signal for Core services & plugins
](#ol-start5licore-should-expose-a-status-signal-for-core-services-amp-pluginsliol) - [Drawbacks](#drawbacks) - [Alternatives](#alternatives) - - [
    -
  1. Introduce a lifecycle/context provider timeout
    2. -
](#olliintroduce-a-lifecyclecontext-provider-timeoutliol) - - [
    -
  1. Treat anything that blocks Kibana from starting up as a bug
    2. -
](#ol-start2litreat-anything-that-blocks-kibana-from-starting-up-as-a-bugliol) + - [
  1. Introduce a lifecycle/context provider timeout
](#olliintroduce-a-lifecyclecontext-provider-timeoutliol) + - [
  1. Treat anything that blocks Kibana from starting up as a bug
](#ol-start2litreat-anything-that-blocks-kibana-from-starting-up-as-a-bugliol) - [Adoption strategy](#adoption-strategy) - [How we teach this](#how-we-teach-this) - [Unresolved questions](#unresolved-questions) @@ -384,4 +371,4 @@ being blocked for the sake of convenient async lifecycle stages?~~ # Footnotes [1] Synchronous lifecycles can still be blocked by e.g. an infine for loop, but this would always be unintentional behaviour in contrast to intentional -async behaviour like blocking until an external service becomes available. \ No newline at end of file +async behaviour like blocking until an external service becomes available.