Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migrate core domain services to packages: Dependency analysis on remaining server-side services #134137

Closed
lukeelmers opened this issue Jun 9, 2022 · 9 comments
Assignees
Labels
chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@lukeelmers
Copy link
Member

lukeelmers commented Jun 9, 2022

Part of #134112

After doing the initial work of moving server-side service types to packages, let's stop and come up with a plan for migrating the remaining services. The obvious next step will be to tackle the HTTP service, as it's used by all remaining plugins. We’ll need to handle the isolation issues between http/elasticsearch/context. Once this part is done, the next services should be way easier.

Remaining services:

  • Capabilities (registers routes)
  • Context (bad isolation with HTTP, cross imports of types between the two)
  • CoreApp (registers routes)
  • CoreUsageData
  • Deprecations (registers routes)
  • Elasticsearch (bad isolation with HTTP, cross imports of types between the two)
  • Http / externalUrl / CSP / HttpResources (main issue here)
  • I18n (registers routes)
  • Metrics
  • Plugins (depends on CoreSetup/Start, this one will be fun too as it needs to be top level)
  • Preboot
  • Rendering
  • SavedObjects (depends on ES->HTTP)
  • Status (registers routes)
  • UiSettings (depends on SO->ES->HTTP)
  • HttpResources (depends on http)

The task here is to do a dependency analysis and come up with a clear plan for migrating the remaining items.

@lukeelmers lukeelmers added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Jun 9, 2022
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@pgayvallet
Copy link
Contributor

The next step (and the main difficulty) will be the splitting / ordering of the http, context and elasticsearch domains

Regarding http and context:

- Both domains import mocks from the other one

E.g

import { contextServiceMock } from '../context/context_service.mock';

and

import { httpServerMock } from '../../http/http_server.mocks';

- Both domains import types from the other one

e.g

import type { RequestHandler, RequestHandlerContext } from '../..';

and

import { IContextProvider, IContextContainer } from '../context';

- The http service depends on the context service

export interface PrebootDeps {
context: InternalContextPreboot;
}

In theory, given the http service has a direct dependency on the context service, the dependencies should only be in that order, and no context type should depends on http types.

In practice, the some types of the two domains are tightly coupled around the 'request handler' context. The http domain exposes the RequestHandlerContext type, and the context domain exposes the IContextProvider types.

In short, we have a terrible isolation between those two services, and at the moment I don't see a clear path toward splitting them.

@pgayvallet
Copy link
Contributor

pgayvallet commented Jun 29, 2022

This is actually a bit worse than that. I actually missed that IContextProvider explicitly depends on RequestHandlerContext

export type IContextProvider<
Context extends RequestHandlerContext,

And RequestHandlerContext has a core property that is of type CoreRequestHandlerContext

and this type is the whole context exposed to plugins via the router handler context:

export interface CoreRequestHandlerContext {
savedObjects: {
client: SavedObjectsClientContract;
typeRegistry: ISavedObjectTypeRegistry;
getClient: (options?: SavedObjectsClientProviderOptions) => SavedObjectsClientContract;
getExporter: (client: SavedObjectsClientContract) => ISavedObjectsExporter;
getImporter: (client: SavedObjectsClientContract) => ISavedObjectsImporter;
};
elasticsearch: {
client: IScopedClusterClient;
};
uiSettings: {
client: IUiSettingsClient;
};
deprecations: {
client: DeprecationsClient;
};
}

Meaning that the context and http domains actually depends on types from other domains, most of them for which the domain's service actually depends on the http types:

  • http types depends on other types
  • other service implementation depends on http service implementation

At this point, I don't really see any other option that starting by creating the public type packages for all the domains that are referenced from CoreRequestHandlerContext, and have a 'reversed dependency' between the public types packages (where http will for example depends on savedObjects) and the internal implementation (where savedObjects will depends on http)...

An alternative would be to stop this per-domain public type separation, and to have a single public type package for core browser/shared/server areas...

@pgayvallet
Copy link
Contributor

I created #135703 to track the migration of the server-side http domain

@pgayvallet
Copy link
Contributor

There are 4 services remaining on the server-side:

  • httpResources
  • rendering
  • plugins
  • coreApp

Dependency analysis

Only listing the unmigrated domains dependencies:

httpResources

Depends on rendering

import { InternalRenderingServicePreboot, InternalRenderingServiceSetup } from '../rendering';

rendering

Depends on plugins

import type { UiPlugins } from '../plugins';

plugins

No dependencies, except the core lifecycle contracts, both public and internal, that aren't migrated yet.

coreApp

Depends on httpResources

import { HttpResources, HttpResourcesServiceToolkit } from '../http_resources';

Depends on plugins

import { UiPlugins } from '../plugins';

Topological ordering

plugins -> rendering -> httpResources -> coreApp

Will create the remaining issues and update #134112 accordingly.

Keeping this issue open just in case, but we should likely be able to close it right now.

@pgayvallet
Copy link
Contributor

pgayvallet commented Sep 27, 2022

Okay, it would have been too easy, it's not actually as trivial as I described it...

The plugins service depends on the internal core lifecycle contracts

import { InternalCorePreboot, InternalCoreSetup, InternalCoreStart } from '../internal_types';

which, and I missed it, have references to services that are higher in the dependency chain (rendering and httpResources)

export interface InternalCoreSetup {
analytics: AnalyticsServiceSetup;
capabilities: CapabilitiesSetup;
context: InternalContextSetup;
docLinks: DocLinksServiceSetup;
http: InternalHttpServiceSetup;
elasticsearch: InternalElasticsearchServiceSetup;
executionContext: InternalExecutionContextSetup;
i18n: I18nServiceSetup;
savedObjects: InternalSavedObjectsServiceSetup;
status: InternalStatusServiceSetup;
uiSettings: InternalUiSettingsServiceSetup;
environment: InternalEnvironmentServiceSetup;
rendering: InternalRenderingServiceSetup;
httpResources: InternalHttpResourcesSetup;
logging: InternalLoggingServiceSetup;
metrics: InternalMetricsServiceSetup;
deprecations: InternalDeprecationsServiceSetup;
coreUsageData: InternalCoreUsageDataSetup;
}

Which introduces a cyclic dependency via these internal types....

@pgayvallet
Copy link
Contributor

I think we should consider that plugins should / will always be at the highest level in the dependency tree (a leaf), and try to work around with that statement.

We will probably have to introduce a -base package for the plugin domains containing the types that are used by other domains, which should break the cyclic dependency. Gonna take a look at that.

@TinaHeiligers
Copy link
Contributor

@pgayvallet can you remember why the Jira issue for this is marked as blocked?

@pgayvallet
Copy link
Contributor

IIRC it was more marked as 'awaiting' than blocked.

But I think we should be good closing this issue now, WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

4 participants