introduce start pahse for plugins service for parity with client side

src/core/server/index.ts

@@ -18,7 +18,7 @@
  */
 import { ElasticsearchServiceSetup } from './elasticsearch';
 import { HttpServiceSetup, HttpServiceStart } from './http';
-import { PluginsServiceSetup } from './plugins';
+import { PluginsServiceSetup, PluginsServiceStart } from './plugins';
 
 export { bootstrap } from './bootstrap';
 export { ConfigService } from './config';
@@ -47,6 +47,7 @@ export {
   PluginInitializerContext,
   PluginName,
   PluginSetupContext,
+  PluginStartContext,
 } from './plugins';
 
 /** @public */
@@ -58,6 +59,13 @@ export interface CoreSetup {
 
 export interface CoreStart {
   http: HttpServiceStart;
+  plugins: PluginsServiceStart;
 }
 
-export { HttpServiceSetup, HttpServiceStart, ElasticsearchServiceSetup, PluginsServiceSetup };
+export {
+  HttpServiceSetup,
+  HttpServiceStart,
+  ElasticsearchServiceSetup,
+  PluginsServiceSetup,
+  PluginsServiceStart,
+};

src/core/server/legacy/legacy_service.test.ts

@@ -35,7 +35,7 @@ import { ElasticsearchServiceSetup } from '../elasticsearch';
 import { HttpServiceStart } from '../http';
 import { loggingServiceMock } from '../logging/logging_service.mock';
 import { DiscoveredPlugin, DiscoveredPluginInternal } from '../plugins';
-import { PluginsServiceSetup } from '../plugins/plugins_service';
+import { PluginsServiceSetup, PluginsServiceStart } from '../plugins/plugins_service';
 import { LegacyPlatformProxy } from './legacy_platform_proxy';
 
 const MockKbnServer: jest.Mock<KbnServer> = KbnServer as any;
@@ -52,6 +52,7 @@ let setupDeps: {
 
 let startDeps: {
   http: HttpServiceStart;
+  plugins: PluginsServiceStart;
 };
 
 const logger = loggingServiceMock.create();
@@ -82,6 +83,9 @@ beforeEach(() => {
     http: {
       isListening: () => true,
     },
+    plugins: {
+      contracts: new Map(),
+    },
   };
 
   config$ = new BehaviorSubject<Config>(
@@ -317,6 +321,9 @@ describe('once LegacyService is set up without connection info', () => {
     http: {
       isListening: () => false,
     },
+    plugins: {
+      contracts: new Map(),
+    },
   };
   beforeEach(async () => {
     await legacyService.setup(setupDeps);
@@ -379,6 +386,9 @@ describe('once LegacyService is set up in `devClusterMaster` mode', () => {
       http: {
         isListening: () => false,
       },
+      plugins: {
+        contracts: new Map(),
+      },
     });
 
     expect(MockClusterManager.create.mock.calls).toMatchSnapshot(
@@ -407,6 +417,9 @@ describe('once LegacyService is set up in `devClusterMaster` mode', () => {
       http: {
         isListening: () => false,
       },
+      plugins: {
+        contracts: new Map(),
+      },
     });
 
     expect(MockClusterManager.create.mock.calls).toMatchSnapshot(

src/core/server/plugins/index.ts

@@ -17,7 +17,7 @@
  * under the License.
  */
 
-export { PluginsService, PluginsServiceSetup } from './plugins_service';
+export { PluginsService, PluginsServiceSetup, PluginsServiceStart } from './plugins_service';
 
 /** @internal */
 export { isNewPlatformPlugin } from './discovery';
@@ -29,4 +29,4 @@ export {
   PluginInitializer,
   PluginName,
 } from './plugin';
-export { PluginInitializerContext, PluginSetupContext } from './plugin_context';
+export { PluginInitializerContext, PluginSetupContext, PluginStartContext } from './plugin_context';

src/core/server/plugins/plugin.test.ts

@@ -179,6 +179,44 @@ test('`setup` initializes plugin and calls appropriate lifecycle hook', async ()
   expect(mockPluginInstance.setup).toHaveBeenCalledWith(setupContext, setupDependencies);
 });
 
+test('`start` fails if setup is not called first', async () => {
+  const manifest = createPluginManifest();
+  const plugin = new PluginWrapper(
+    'some-plugin-path',
+    manifest,
+    createPluginInitializerContext(coreContext, manifest)
+  );
+
+  await expect(plugin.start({} as any, {} as any)).rejects.toThrowErrorMatchingInlineSnapshot(
+    `"Plugin \\"some-plugin-id\\" can't be started since it isn't set up."`
+  );
+});
+
+test('`start` calls plugin.start with context and dependencies', async () => {
+  const manifest = createPluginManifest();
+  const plugin = new PluginWrapper(
+    'plugin-with-initializer-path',
+    manifest,
+    createPluginInitializerContext(coreContext, manifest)
+  );
+  const context = { any: 'thing' } as any;
+  const deps = { otherDep: 'value' };
+
+  const pluginStartContract = { contract: 'start-contract' };
+  const mockPluginInstance = {
+    setup: jest.fn(),
+    start: jest.fn().mockResolvedValue(pluginStartContract),
+  };
+  mockPluginInitializer.mockReturnValue(mockPluginInstance);
+
+  await plugin.setup({} as any, {} as any);
+
+  const startContract = await plugin.start(context, deps);
+
+  expect(startContract).toBe(pluginStartContract);
+  expect(mockPluginInstance.start).toHaveBeenCalledWith(context, deps);
+});
+
 test('`stop` fails if plugin is not set up', async () => {
   const manifest = createPluginManifest();
   const plugin = new PluginWrapper(

src/core/server/plugins/plugin.ts

@@ -21,7 +21,7 @@ import { join } from 'path';
 import typeDetect from 'type-detect';
 import { ConfigPath } from '../config';
 import { Logger } from '../logging';
-import { PluginInitializerContext, PluginSetupContext } from './plugin_context';
+import { PluginInitializerContext, PluginSetupContext, PluginStartContext } from './plugin_context';
 
 /**
  * Dedicated type for plugin name/id that is supposed to make Map/Set/Arrays
@@ -129,11 +129,14 @@ export interface DiscoveredPluginInternal extends DiscoveredPlugin {
  *
  * @public
  */
-export interface Plugin<TSetup, TPluginsSetup extends Record<PluginName, unknown> = {}> {
-  setup: (
-    pluginSetupContext: PluginSetupContext,
-    plugins: TPluginsSetup
-  ) => TSetup | Promise<TSetup>;
+export interface Plugin<
+  TSetup,
+  TStart,
+  TPluginsSetup extends Record<PluginName, unknown> = {},
+  TPluginsStart extends Record<PluginName, unknown> = {}
+> {
+  setup: (core: PluginSetupContext, plugins: TPluginsSetup) => TSetup | Promise<TSetup>;
+  start: (core: PluginStartContext, plugins: TPluginsStart) => TStart | Promise<TStart>;
   stop?: () => void;
 }
 
@@ -143,9 +146,12 @@ export interface Plugin<TSetup, TPluginsSetup extends Record<PluginName, unknown
  *
  * @public
  */
-export type PluginInitializer<TSetup, TPluginsSetup extends Record<PluginName, unknown> = {}> = (
-  coreContext: PluginInitializerContext
-) => Plugin<TSetup, TPluginsSetup>;
+export type PluginInitializer<
+  TSetup,
+  TStart,
+  TPluginsSetup extends Record<PluginName, unknown> = {},
+  TPluginsStart extends Record<PluginName, unknown> = {}
+> = (core: PluginInitializerContext) => Plugin<TSetup, TStart, TPluginsSetup, TPluginsStart>;
 
 /**
  * Lightweight wrapper around discovered plugin that is responsible for instantiating
@@ -155,7 +161,9 @@ export type PluginInitializer<TSetup, TPluginsSetup extends Record<PluginName, u
  */
 export class PluginWrapper<
   TSetup = unknown,
-  TPluginsSetup extends Record<PluginName, unknown> = Record<PluginName, unknown>
+  TStart = unknown,
+  TPluginsSetup extends Record<PluginName, unknown> = Record<PluginName, unknown>,
+  TPluginsStart extends Record<PluginName, unknown> = Record<PluginName, unknown>
 > {
   public readonly name: PluginManifest['id'];
   public readonly configPath: PluginManifest['configPath'];
@@ -166,7 +174,7 @@ export class PluginWrapper<
 
   private readonly log: Logger;
 
-  private instance?: Plugin<TSetup, TPluginsSetup>;
+  private instance?: Plugin<TSetup, TStart, TPluginsSetup, TPluginsStart>;
 
   constructor(
     public readonly path: string,
@@ -197,6 +205,21 @@ export class PluginWrapper<
     return await this.instance.setup(setupContext, plugins);
   }
 
+  /**
+   * Calls `start` function exposed by the initialized plugin.
+   * @param startContext Context that consists of various core services tailored specifically
+   * for the `start` lifecycle event.
+   * @param plugins The dictionary where the key is the dependency name and the value
+   * is the contract returned by the dependency's `start` function.
+   */
+  public async start(startContext: PluginStartContext, plugins: TPluginsStart) {
+    if (this.instance === undefined) {
+      throw new Error(`Plugin "${this.name}" can't be started since it isn't set up.`);
+    }
+
+    return await this.instance.start(startContext, plugins);
+  }
+
   /**
    * Calls optional `stop` function exposed by the plugin initializer.
    */
@@ -224,7 +247,7 @@ export class PluginWrapper<
     }
 
     const { plugin: initializer } = pluginDefinition as {
-      plugin: PluginInitializer<TSetup, TPluginsSetup>;
+      plugin: PluginInitializer<TSetup, TStart, TPluginsSetup, TPluginsStart>;
     };
     if (!initializer || typeof initializer !== 'function') {
       throw new Error(`Definition of plugin "${this.name}" should be a function (${this.path}).`);

src/core/server/plugins/plugin_context.ts

@@ -25,7 +25,7 @@ import { ClusterClient } from '../elasticsearch';
 import { HttpServiceSetup } from '../http';
 import { LoggerFactory } from '../logging';
 import { PluginWrapper, PluginManifest } from './plugin';
-import { PluginsServiceSetupDeps } from './plugins_service';
+import { PluginsServiceSetupDeps, PluginsServiceStartDeps } from './plugins_service';
 
 /**
  * Context that's available to plugins during initialization stage.
@@ -61,6 +61,13 @@ export interface PluginSetupContext {
   };
 }
 
+/**
+ * Context passed to the plugins `start` method.
+ *
+ * @public
+ */
+export interface PluginStartContext {} // eslint-disable-line @typescript-eslint/no-empty-interface
+
 /**
  * This returns a facade for `CoreContext` that will be exposed to the plugin initializer.
  * This facade should be safe to use across entire plugin lifespan.
@@ -144,3 +151,23 @@ export function createPluginSetupContext<TPlugin, TPluginDependencies>(
     },
   };
 }
+
+/**
+ * This returns a facade for `CoreContext` that will be exposed to the plugin `start` method.
+ * This facade should be safe to use only within `start` itself.
+ *
+ * This is called for each plugin when it starts, so each plugin gets its own
+ * version of these values.
+ *
+ * @param coreContext Kibana core context
+ * @param plugin The plugin we're building these values for.
+ * @param deps Dependencies that Plugins services gets during start.
+ * @internal
+ */
+export function createPluginStartContext<TPlugin, TPluginDependencies>(
+  coreContext: CoreContext,
+  deps: PluginsServiceStartDeps,
+  plugin: PluginWrapper<TPlugin, TPluginDependencies>
+): PluginStartContext {
+  return {};
+}

src/core/server/plugins/plugins_service.ts

@@ -38,6 +38,11 @@ export interface PluginsServiceSetup {
   };
 }
 
+/** @internal */
+export interface PluginsServiceStart {
+  contracts: Map<PluginName, unknown>;
+}
+
 /** @internal */
 export interface PluginsServiceSetupDeps {
   elasticsearch: ElasticsearchServiceSetup;
@@ -45,7 +50,10 @@ export interface PluginsServiceSetupDeps {
 }
 
 /** @internal */
-export class PluginsService implements CoreService<PluginsServiceSetup> {
+export interface PluginsServiceStartDeps {} // eslint-disable-line @typescript-eslint/no-empty-interface
+
+/** @internal */
+export class PluginsService implements CoreService<PluginsServiceSetup, PluginsServiceStart> {
   private readonly log: Logger;
   private readonly pluginsSystem: PluginsSystem;
 
@@ -80,7 +88,11 @@ export class PluginsService implements CoreService<PluginsServiceSetup> {
     };
   }
 
-  public async start() {}
+  public async start(deps: PluginsServiceStartDeps) {
+    this.log.debug('Plugins service starts plugins');
+    const contracts = await this.pluginsSystem.startPlugins(deps);
+    return { contracts };
+  }
 
   public async stop() {
     this.log.debug('Stopping plugins service');

src/core/server/plugins/plugins_system.test.mocks.ts

@@ -18,6 +18,8 @@
  */
 
 export const mockCreatePluginSetupContext = jest.fn();
+export const mockCreatePluginStartContext = jest.fn();
 jest.mock('./plugin_context', () => ({
   createPluginSetupContext: mockCreatePluginSetupContext,
+  createPluginStartContext: mockCreatePluginStartContext,
 }));

src/core/server/plugins/plugins_system.test.ts

@@ -17,7 +17,10 @@
  * under the License.
  */
 
-import { mockCreatePluginSetupContext } from './plugins_system.test.mocks';
+import {
+  mockCreatePluginSetupContext,
+  mockCreatePluginStartContext,
+} from './plugins_system.test.mocks';
 
 import { BehaviorSubject } from 'rxjs';
 import { Config, ConfigService, Env, ObjectToConfigAdapter } from '../config';
@@ -105,7 +108,7 @@ test('`setupPlugins` throws if plugins have circular required dependency', async
   );
 });
 
-test('`setupPlugins` throws if plugins have circular optional dependency', async () => {
+test('`setupPlugins` && `startPlugins` throws if plugins have circular optional dependency', async () => {
   pluginsSystem.addPlugin(createPlugin('no-dep'));
   pluginsSystem.addPlugin(createPlugin('depends-on-1', { optional: ['depends-on-2'] }));
   pluginsSystem.addPlugin(createPlugin('depends-on-2', { optional: ['depends-on-1'] }));
@@ -131,27 +134,58 @@ Array [
 `);
 });
 
-test('`setupPlugins` correctly orders plugins and returns exposed values', async () => {
+test('correctly orders plugins and returns exposed values for "setup" and "start"', async () => {
+  interface Contracts {
+    setup: Record<PluginName, unknown>;
+    start: Record<PluginName, unknown>;
+  }
   const plugins = new Map([
-    [createPlugin('order-4', { required: ['order-2'] }), { 'order-2': 'added-as-2' }],
-    [createPlugin('order-0'), {}],
+    [
+      createPlugin('order-4', { required: ['order-2'] }),
+      {
+        setup: { 'order-2': 'added-as-2' },
+        start: { 'order-2': 'started-as-2' },
+      },
+    ],
+    [
+      createPlugin('order-0'),
+      {
+        setup: {},
+        start: {},
+      },
+    ],
     [
       createPlugin('order-2', { required: ['order-1'], optional: ['order-0'] }),
-      { 'order-1': 'added-as-3', 'order-0': 'added-as-1' },
+      {
+        setup: { 'order-1': 'added-as-3', 'order-0': 'added-as-1' },
+        start: { 'order-1': 'started-as-3', 'order-0': 'started-as-1' },
+      },
+    ],
+    [
+      createPlugin('order-1', { required: ['order-0'] }),
+      {
+        setup: { 'order-0': 'added-as-1' },
+        start: { 'order-0': 'started-as-1' },
+      },
     ],
-    [createPlugin('order-1', { required: ['order-0'] }), { 'order-0': 'added-as-1' }],
     [
       createPlugin('order-3', { required: ['order-2'], optional: ['missing-dep'] }),
-      { 'order-2': 'added-as-2' },
+      {
+        setup: { 'order-2': 'added-as-2' },
+        start: { 'order-2': 'started-as-2' },
+      },
     ],
-  ] as Array<[PluginWrapper, Record<PluginName, unknown>]>);
+  ] as Array<[PluginWrapper, Contracts]>);
 
   const setupContextMap = new Map();
+  const startContextMap = new Map();
 
   [...plugins.keys()].forEach((plugin, index) => {
     jest.spyOn(plugin, 'setup').mockResolvedValue(`added-as-${index}`);
+    jest.spyOn(plugin, 'start').mockResolvedValue(`started-as-${index}`);
 
     setupContextMap.set(plugin.name, `setup-for-${plugin.name}`);
+    startContextMap.set(plugin.name, `start-for-${plugin.name}`);
 
     pluginsSystem.addPlugin(plugin);
   });
@@ -160,6 +194,10 @@ test('`setupPlugins` correctly orders plugins and returns exposed values', async
     setupContextMap.get(plugin.name)
   );
 
+  mockCreatePluginStartContext.mockImplementation((context, deps, plugin) =>
+    startContextMap.get(plugin.name)
+  );
+
   expect([...(await pluginsSystem.setupPlugins(setupDeps))]).toMatchInlineSnapshot(`
 Array [
   Array [
@@ -188,7 +226,39 @@ Array [
   for (const [plugin, deps] of plugins) {
     expect(mockCreatePluginSetupContext).toHaveBeenCalledWith(coreContext, setupDeps, plugin);
     expect(plugin.setup).toHaveBeenCalledTimes(1);
-    expect(plugin.setup).toHaveBeenCalledWith(setupContextMap.get(plugin.name), deps);
+    expect(plugin.setup).toHaveBeenCalledWith(setupContextMap.get(plugin.name), deps.setup);
+  }
+
+  const startDeps = {};
+  expect([...(await pluginsSystem.startPlugins(startDeps))]).toMatchInlineSnapshot(`
+Array [
+  Array [
+    "order-0",
+    "started-as-1",
+  ],
+  Array [
+    "order-1",
+    "started-as-3",
+  ],
+  Array [
+    "order-2",
+    "started-as-2",
+  ],
+  Array [
+    "order-3",
+    "started-as-4",
+  ],
+  Array [
+    "order-4",
+    "started-as-0",
+  ],
+]
+`);
+
+  for (const [plugin, deps] of plugins) {
+    expect(mockCreatePluginStartContext).toHaveBeenCalledWith(coreContext, startDeps, plugin);
+    expect(plugin.start).toHaveBeenCalledTimes(1);
+    expect(plugin.start).toHaveBeenCalledWith(startContextMap.get(plugin.name), deps.start);
   }
 });
 
@@ -271,3 +341,38 @@ Array [
 ]
 `);
 });
+
+test('can start without plugins', async () => {
+  await pluginsSystem.setupPlugins(setupDeps);
+  const pluginsStart = await pluginsSystem.startPlugins({});
+
+  expect(pluginsStart).toBeInstanceOf(Map);
+  expect(pluginsStart.size).toBe(0);
+});
+
+test('`startPlugins` only starts plugins that were setup', async () => {
+  const firstPluginToRun = createPlugin('order-0');
+  const secondPluginNotToRun = createPlugin('order-not-run', { server: false });
+  const thirdPluginToRun = createPlugin('order-1');
+
+  [firstPluginToRun, secondPluginNotToRun, thirdPluginToRun].forEach((plugin, index) => {
+    jest.spyOn(plugin, 'setup').mockResolvedValue(`setup-as-${index}`);
+    jest.spyOn(plugin, 'start').mockResolvedValue(`started-as-${index}`);
+
+    pluginsSystem.addPlugin(plugin);
+  });
+  await pluginsSystem.setupPlugins(setupDeps);
+  const result = await pluginsSystem.startPlugins({});
+  expect([...result]).toMatchInlineSnapshot(`
+Array [
+  Array [
+    "order-1",
+    "started-as-2",
+  ],
+  Array [
+    "order-0",
+    "started-as-0",
+  ],
+]
+`);
+});

src/core/server/plugins/plugins_system.ts

@@ -22,8 +22,8 @@ import { pick } from 'lodash';
 import { CoreContext } from '../core_context';
 import { Logger } from '../logging';
 import { DiscoveredPlugin, DiscoveredPluginInternal, PluginWrapper, PluginName } from './plugin';
-import { createPluginSetupContext } from './plugin_context';
-import { PluginsServiceSetupDeps } from './plugins_service';
+import { createPluginSetupContext, createPluginStartContext } from './plugin_context';
+import { PluginsServiceSetupDeps, PluginsServiceStartDeps } from './plugins_service';
 
 /** @internal */
 export class PluginsSystem {
@@ -56,8 +56,8 @@ export class PluginsSystem {
       }
 
       this.log.debug(`Setting up plugin "${pluginName}"...`);
-
-      const pluginDepContracts = [...plugin.requiredPlugins, ...plugin.optionalPlugins].reduce(
+      const pluginDeps = new Set([...plugin.requiredPlugins, ...plugin.optionalPlugins]);
+      const pluginDepContracts = Array.from(pluginDeps).reduce(
         (depContracts, dependencyName) => {
           // Only set if present. Could be absent if plugin does not have server-side code or is a
           // missing optional dependency.
@@ -84,6 +84,43 @@ export class PluginsSystem {
     return contracts;
   }
 
+  public async startPlugins(deps: PluginsServiceStartDeps) {
+    const contracts = new Map<PluginName, unknown>();
+    if (this.satupPlugins.length === 0) {
+      return contracts;
+    }
+
+    this.log.info(`Starting [${this.satupPlugins.length}] plugins: [${[...this.satupPlugins]}]`);
+
+    for (const pluginName of this.satupPlugins) {
+      this.log.debug(`Starting plugin "${pluginName}"...`);
+      const plugin = this.plugins.get(pluginName)!;
+      const pluginDeps = new Set([...plugin.requiredPlugins, ...plugin.optionalPlugins]);
+      const pluginDepContracts = Array.from(pluginDeps).reduce(
+        (depContracts, dependencyName) => {
+          // Only set if present. Could be absent if plugin does not have server-side code or is a
+          // missing optional dependency.
+          if (contracts.has(dependencyName)) {
+            depContracts[dependencyName] = contracts.get(dependencyName);
+          }
+
+          return depContracts;
+        },
+        {} as Record<PluginName, unknown>
+      );
+
+      contracts.set(
+        pluginName,
+        await plugin.start(
+          createPluginStartContext(this.coreContext, deps, plugin),
+          pluginDepContracts
+        )
+      );
+    }
+
+    return contracts;
+  }
+
   public async stopPlugins() {
     if (this.plugins.size === 0 || this.satupPlugins.length === 0) {
       return;

src/core/server/server.ts

@@ -65,9 +65,11 @@ export class Server {
 
   public async start() {
     const httpStart = await this.http.start();
+    const plugins = await this.plugins.start({});
 
     const startDeps = {
       http: httpStart,
+      plugins,
     };
 
     await this.legacy.start(startDeps);

src/legacy/server/kbn_server.d.ts

@@ -25,6 +25,7 @@ import {
   HttpServiceStart,
   ConfigService,
   PluginsServiceSetup,
+  PluginsServiceStart,
 } from '../../core/server';
 import { ApmOssPlugin } from '../core_plugins/apm_oss';
 import { CallClusterWithRequest, ElasticsearchPlugin } from '../core_plugins/elasticsearch';
@@ -86,6 +87,7 @@ export default class KbnServer {
       core: {
         http: HttpServiceStart;
       };
+      plugins: PluginsServiceStart;
     };
     stop: null;
     params: {

src/legacy/server/kbn_server.js

@@ -67,6 +67,7 @@ export default class KbnServer {
         core: {
           http: startDeps.http,
         },
+        plugins: startDeps.plugins,
       },
       stop: null,
       params: {

src/plugins/testbed/server/index.ts

@@ -19,7 +19,13 @@
 
 import { map, mergeMap } from 'rxjs/operators';
 
-import { Logger, PluginInitializerContext, PluginName, PluginSetupContext } from 'kibana/server';
+import {
+  Logger,
+  PluginInitializerContext,
+  PluginName,
+  PluginSetupContext,
+  PluginStartContext,
+} from 'kibana/server';
 import { TestBedConfig } from './config';
 
 class Plugin {
@@ -49,6 +55,20 @@ class Plugin {
     };
   }
 
+  public start(startContext: PluginStartContext, deps: Record<PluginName, unknown>) {
+    this.log.debug(
+      `Starting up TestBed testbed with core contract [${Object.keys(
+        startContext
+      )}] and deps [${Object.keys(deps)}]`
+    );
+
+    return {
+      getStartContext() {
+        return startContext;
+      },
+    };
+  }
+
   public stop() {
     this.log.debug(`Stopping TestBed`);
   }