[Core] Deprecate async lifecycles

[rudolf]

From Lifecycles RFC: https://github.com/elastic/kibana/blob/6042a01f385078587e8e09418ef07f69ec2dadf7/rfcs/text/0007_lifecycle_unblocked.md

Subtasks

• Deprecate async lifecycles by logging a warning if a plugin returns a promise. Migrate most plugins to synchronous lifecycle #89562
• Remove async lifecycles from Core's Plugin interface and introduce the temporary AsyncPlugin interface Migrate most plugins to synchronous lifecycle #89562
• Update migration guide and other documentation examples. Migrate most plugins to synchronous lifecycle #89562
• Provide a way to access config synchronously Core should provide a synchronous API for reading config values #82723

Unmigrated plugins

Plugins that still depend on async behaviour after #89562:

• server-side
  • fleet [Fleet] Migrate server-side plugin to synchronous lifecycle #90581
• client-side
  • vis_type_table Migrate vis_type_table plugin to synchronous lifecycle #90582

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[lukeelmers]

Provide a way to access config synchronously (?)

What about a way to access uiSettings synchronously on the server?

Currently on the server uiSettings.get returns a promise, but is sync on the client.

For plugins which have synchronous registries containing items that rely on specific uiSettings, in a setup lifecycle method we'd need to await those settings before registering stuff:

// server/plugin.ts
async setup(core, { foo }) {
  const dateFormat = await core.uiSettings.get('dateFormat');

  // foo.registerSomething is sync
  foo.registerSomething(new MyRegistryItem({ dateFormat }));
}

class MyRegistryItem {
  constructor({ dateFormat }) {
    this.dateFormat = dateFormat;
    return ...
  }
}

[joshdover]

Tricky thing about this is that UiSettings are stored as a Saved Object, one per space. On the client, we actually cache this document in memory, which is less than ideal, but hasn't caused any major problems since there is only one space active in the browser at a time.

The server-side is a bit different since it's serving many spaces simultaneously. That said, a given UiSettingsClient only has access to the specific space it was configured for. I think the simplest option would be to configure a RouteHandlerContextProvider on the server that fetches the UiSettings and configures the service with those settings, all before the route handler executes. This would allow for synchronous access without needing to worry about a complicated caching mechanism. Example:

// Register a context provider
core.http.registerRouteHandlerContext('myService', async (context) => {
  // Fetch all the settings for this request
  const cachedUiSettings = await context.core.uiSettings.client.getAll();
  // Wire up MyService with the cached settings for this request
  // These can be accessed synchronously by MyService
  const myService = new MyService({ uiSettings: cachedUiSettings });
  return myService;
});

// Use context API
core.http.createRouter().get(
  { path: '/api/example', validate: false },
  async (context, req, res) => {
    const thing = context.myService.doThing();
    return res.ok({ body: thing });
  }
}

It's also worth noting that the example above actually isn't valid since we don't expose a core.uiSettings.get API on CoreSetup. You have to get access to client instances via coreStart.uiSettings.asScopedToClient in start or via context.core.uiSettings.client in an HTTP route handler. Registry items on the server can't simply rely on a single value for a UiSetting since this value may be different for each request based on the spaces that request is made in.

[rudolf]

@lukeelmers do you have a concrete example of where we currently have such a dependency on sync uisettings? It feels like this would create a situation where kibana finishes the setup lifecycle in a way that's dependant on a uisetting, but if that uisetting ever changes Kibana will have to be restarted for it to take effect.

[lukeelmers]

@rudolf It came to mind as we were working through plans to make the data plugin's aggs infrastructure (data.search.aggs) available on the server.

Specifically, different agg types which depend on core.uiSettings are registered during setup:

kibana/src/plugins/data/public/search/search_service.ts

Lines 92 to 100 in 008b0fd

	const aggTypesSetup = this.aggTypesRegistry.setup();
	const aggTypes = getAggTypes({
	query,
	uiSettings: core.uiSettings,
	getInternalStartServices,
	});
	aggTypes.buckets.forEach(b => aggTypesSetup.registerBucket(b));
	aggTypes.metrics.forEach(m => aggTypesSetup.registerMetric(m));

Both the registration & creation of the agg type definition occur synchronously. Some of the agg types rely on a value retrieved from core.uiSettings.get. Take filters for example, where the query language is needed in the default param value (L64):

kibana/src/plugins/data/public/search/aggs/buckets/filters.ts

Lines 54 to 65 in 008b0fd

	new BucketAggType(
	{
	name: BUCKET_TYPES.FILTERS,
	title: filtersTitle,
	createFilter: createFilterFilters,
	customLabels: false,
	params: [
	{
	name: 'filters',
	default: [
	{ input: { query: '', language: uiSettings.get('search:queryLanguage') }, label: '' },
	],

I'm sure this is something we could refactor around if necessary, but hopefully it illustrates the use case. We have a lot of synchronous registries floating around, and as uiSettings is quite a common dependency, I would expect this issue to come up in other areas too as there is an increasing need for many of our main services (index patterns, search, query, expressions) to be available on the server.

[mshustov]

I think the simplest option would be to configure a RouteHandlerContextProvider on the server that fetches the UiSettings and configures the service with those settings, all before the route handler executes.

It would work with a couple of limitations:

• the request handler can have obsolete uiSettings.
• a plugin must not change uiSettings. like here. We need to update the uiSettings client interface to restrict access to setters. And yes, it will diverge client-server API even more.

This would allow for synchronous access without needing to worry about a complicated caching mechanism.

Yeah, uiSettings can be changed via rest API or another Kibana instance, so we cannot implement an in-memory cache.

Specifically, different agg types which depend on core.uiSettings are registered during setup

@lukeelmers I think @rudolf meant that the use of core.uiSettings in a lifecycle in a sync manner is a fundamental problem. Given your example with filters. If a plugin registres API based on search: queryLanguage' value on setup, how it would behave when a user switches search: queryLanguage' in UI? filters had been registered with the old value. The only way to fix this inconsistency is to reload a page (btw I don't see requiresPageReload: true for this setting). But the time of life for a web server >> the time of life for a web page. We cannot reload a web-server nor re-run a setup lifecycle on a ui settings change. Plugins have to respect the changeable nature of the ui settings.

[rudolf]

It seems to me like moving data.search.aggs to the server-side will require re-thinking the setup phase. On the client we can do setup with a uiSetting because if the uiSettings are changed we can tell the user to refresh their browser for these settings to take effect (IMO it would be better if this wasn't necessary, but there's at least a workaround).

On the server-side if we register an aggregation that uses uiSettings.get('search:queryLanguage') that setting will remain in effect until the Kibana server is restarted which users have no control over. So we will have to consider how to make this reactive which might mean instead of a synchronous accessor uiSettings will have to expose an observable that emits every time a new value is written to uiSettings.

[mshustov]

will have to expose an observable that emits every time a new value is written to uiSettings.

Does ES / SO provide ability to emit update events?

...uiSettings can be changed via rest API or another Kibana instance, ...

[lukeelmers]

btw I don't see requiresPageReload: true for this setting

Thanks for pointing this out, @restrry! I'll investigate further, sounds like a bug we hadn't noticed yet

So we will have to consider how to make this reactive which might mean instead of a synchronous accessor uiSettings will have to expose an observable that emits every time a new value is written to uiSettings.

Yeah, the filters is just one example, and admittedly should probably be refactored anyway. But considering the widespread use of uiSettings in our plugins, the need to make many of our services available on the server, and the synchronous nature of (most of) our registries which have been built to date, this will not be an isolated problem.

uiSettings as an observable is definitely one way we could solve this on the server, and might also enable us to eventually remove the client-side browser refresh, so if it's technically feasible this feels like it'd be helpful.

[lukeelmers]

might also enable us to eventually remove the client-side browser refresh

...he says before forgetting that there already is a uiSettings.get$ observable on the client 🤦

[rudolf]

Does ES / SO provide ability to emit update events?

Not that I'm aware of. We can optimistically emit if we receive a write, but would have to poll to be sure another node didn't perform a write. This would probably have to be rather frequent to be useful, but polling every minute for instance for a document that usually doesn't receive a lot of changes feels like a waste.

I'm not sure if it's feasible, but it would be better if we could somehow only read the setting when the service / registry receives an incoming request. That would also make reading these ui settings spaces compatible.

[mattkime]

I'm running into the sync/async uiSettings access issue in some common code. Moving to async access in most places works well. However, field formatters rely on uiSettings values and field formatters can't be async. Therefore its necessary to snapshot uiSettings values and provide them to the formatter. I suspect this will need to happen in a few places and it would be nice to have it provided at the api level.

[joshdover]

Did a new audit today in #74191 to figure how many plugins will require significant changes to drop async lifecycles. Looking very promising! Cross posting my results here:

• 26 plugins that could be completely fixed by exposing a sync config API:
  • OSS: apm_oss, console, usage_collection, vis_type_timelion
  • X-Pack: actions, alerts, apm, canvas, case, cloud, code, encrypted_saved_objects, enterprise_search, event_log, global_search, index_lifecycle_management, infra, ingest_manager, licensing, lists, maps, observability, remove_clusters, security_solution, snapshot_restore, task_manager
• 3 plugins need other, more significant changes:
  • OSS: telemetry (writes some config data to a SavedObject during start, probably easy to refactor)
  • X-Pack: monitoring (lots of things in setup), security (calls the async http.createCookieSessionStorageFactory API during setup)

[pgayvallet]

With #88981 merged, core now provides a synchronous API to retrieve the plugin's (and the legacy) configuration. This should unblock most of the plugins.

[mshustov]

Should we create an issue to migrate all the async lifecycles to the sync counterparts? It might be faster if the Core team performs most of the migrations. Afterward, we need to create a meta issue for all the remaining affected solutions and communicate the timeline.

[pgayvallet]

Should we create an issue to migrate all the async lifecycles to the sync counterparts? It might be faster if the Core team performs most of the migrations

Yea, I was planning on doing that: try to adapt as many plugins as possible myself, and only then create a meta issue with the remaining unmigrated plugins. Should have the inventory in a few days.

Will also change the Plugin signatures to sync at the same time, and introduce the deprecated AsyncPlugin interface which would be our current Plugin. This should help identifying the remaining unmigrated plugins.

[pgayvallet]

#89562 has been merged. Only two plugins were not migrated to synchronous lifecycle (fleet and vis_type_table).

I created separate issues for those, and updated the description of this one.