Skip to content

Commit

Permalink
Draft RFC for unblocking kibana startup
Browse files Browse the repository at this point in the history
  • Loading branch information
@rudolf
rudolf committed Sep 16, 2019
1 parent 5d7d0c9 commit 07ab8d4
Showing 1 changed file with 105 additions and 101 deletions.
206 changes: 105 additions & 101 deletions rfcs/text/0006_lifecycle_unblocked.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<
Expand All @@ -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) => {
Expand All @@ -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();
Expand All @@ -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),
Expand All @@ -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
Expand Down Expand Up @@ -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<T>(
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
Expand Down

0 comments on commit 07ab8d4

Please sign in to comment.