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

[Discuss] New Platform Service vs Plugin Architecture #47065

Closed
stacey-gammon opened this issue Oct 1, 2019 · 18 comments
Closed

[Discuss] New Platform Service vs Plugin Architecture #47065

stacey-gammon opened this issue Oct 1, 2019 · 18 comments
Labels
discuss impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@stacey-gammon
Copy link
Contributor

stacey-gammon commented Oct 1, 2019

Summary

I'd like to discuss some challenges with our current paradigms for plugin and service development and propose some ideas.

Terminology

  • Plugin - a top level folder, can also be called a top level domain or service. Dependencies are automatically injected into the plugin and you cannot have circular dependencies between plugins.
  • Domain - a set of common services and/or functionality. A top level domain is called a plugin today.
  • Sub domain - also a set of common services and/or functionality, but nested inside another domain.
  • Service - Historically we've used the service terminology when discussing focused areas of functionality within a given domain. This issue asks the question though, whether there is any reason to make a distinction between these two concepts. A search services domain could be within the data services domain.

A plugin is a domain, but a domain is not always a plugin, depending on where the folder is located.

Current Goals

**1. [Requirement] Avoid circular dependencies. **

This isn't a goal, it's a requirement, but I have it listed here because there may be times we can't support 2. because of 1.

2. An organized, hierarchical folder structure for services to ease discoverability

We want to avoid 1,000s of folders at any given level.

Proposed goals

3. Ease of changing this hierarchy at a later time.

We've already started changing our minds for some structures. It's inevitable that as more services and domains come about, better organizational structures will appear and some re-arrangement will be necessary.

4. Consistency between domains at any level (only difference between a top and nested domain is access path)

You hit this goal, you get 3 for free. If you want to move a sub-domain to be a top level domain, or a top-level domain to be a sub domain, you should only have to move folders and fix references. There isn't any reason a top level domain should be treated specially or written in a different way.

5. Leaf services should be highly focused, small services

This makes them easy to read, test and reason about.

Proposals for now

1. Formalize and encourage the Service interface

Supports goal 3 and 4.

Just like we have the Plugin service. Even though the platform team has followed this pattern consistently with core, other teams are exposing their sub services differently.

2. Replace Plugin terminology with Service terminology

Supports goal 3 and 4.

Current:

- data
   - public
     - plugin.ts
       - class DataPlugin extends Plugin<>

Proposed:

- data
   - public
     - data_service.ts
       - class DataService extends Service<>

Proposals for the future

3. Allow for modularity within a sub service that has both client and server side code

Supports goal 3 and 4.

Current:

- data
   - public
     - search_service
      - es_search_service
  - server
    - search_service
      - es_search_service

Proposed:

- data
   - public
   - server
   - services
     - search_service
       - public
       - server
       - services
        - es_search_service
          - public
          - server

4. Use dependency injection for services

I'm not sure of the technical feasibility of this one, or if it's even desired, but I suspect we will want this to avoid circular dependencies. We can punt on this until/if it actually becomes a problem, but wanted to mention it in case others start to see circular references as a pain point.

Discussion

I bring this up now because I started working on the search service inside the data plugin and realized it has no dependencies on anything else that is supposed to be inside the data plugin. It felt like a very small and focused "plugin" and conforming to the plugin architecture made it consistent, familiar, and easy to read. Along the same line, I made esSearch a plugin as well. It depended on the search plugin, but by separating it out, it made it familiar and easy to read as well. The esSearch plugin.ts file was small and focused - just a few lines to register the ES search strategy. The search plugin.ts file was also small and focused - just a few lines to expose the search strategy registration points.

I decided to make them top level plugins because by conforming to this structure it made them focused, structured, and consistent.

But the problem with this approach is that it contradicts 2. - going this way you'll end up with 1,000s of plugins at the top level, making code hard to navigate and find.

I want to find a way where we can continue to make small, focused, isolated "services", no matter where in the hierarchy of domains they fall. Choosing a parent domain then should have no affect on the code you are writing inside that service. Write your code, decide where it belongs, change it later if a better organizational path ends up making more sense. I'm pretty sure that as we add more services, we'll come up with better organizational structures. It's easy now to have everything within a single plugin built completely differently from another plugin.

In addition, I suspect that we will start to encounter circular dependencies within plugins which will force us to change our hierarchies. Consider this:

2019-10-01

We may end up having highly specialized services (TopNavQueryBar) mixed in with super generic services (Search) which could end up causing circular dependencies. In the above example, hypothetically speaking we decide to add a "create alert" button inside the query bar and now it has a dependency on Alerting which has a dependency on search (this isn't a great real life example because it'd actually be pluggable and alerting would register the button in it's own plugin).

Some of these are hypothetical problems that may never actually arise. I could be totally wrong. So my proposal is: do some of these now, consider some of these for later if it does start to be a problem. I bring it up now so developers who might encounter the hypothetical issues can bring it up and come back here. And also to get others thoughts on this! Maybe there are developers out there who can come up with a concrete example for circular dependencies.

Do now:

  • Service interface
  • Use Service terminology instead of plugin terminology
  • e.g. instead of plugin.ts call it data_services.ts
  • Formalize sub service architecture, without enforcing it.

Maybe do later:

  • Service modularization
  • Service dependency injection
  • Enforce functional implementations at leaf services only.

Thoughts?

cc @epixa @peterschretlen

@stacey-gammon stacey-gammon added discuss Team:AppArch Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Oct 2, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-app-arch (Team:AppArch)

@elasticmachine
Copy link
Contributor

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

@mattkime
Copy link
Contributor

mattkime commented Oct 2, 2019

How would plugins and services differ aside from their position in the hierarchy?

@ppisljar
Copy link
Member

ppisljar commented Oct 2, 2019

one of the suggestions at the time was the possibility for two level folder structure, where we could have actual plugins nested inside a subfolder inside plugins. something like:
(indexPatterns, query and expressions are plugins ... data is just a folder to group indexPatterns and query)

plugins
  data
    indexPatterns
      plugin.ts
    query
      plugin.ts
   ...
  expresions
    plugin.ts
  ...

i think it shouldn't be too much work to make this possible. this way we could have multiple small plugins without polluting the plugins folder.

@stacey-gammon
Copy link
Contributor Author

How would plugins and services differ aside from their position in the hierarchy?

If we went with the future proposals which include dependency injection, they wouldn't. The only difference is that a plugin is a top level domain. And as such I suppose that plugin can be disabled/enabled but not individual services (at least not using the same mechanism).

If we go with just the "now" proposals, plugins would need to initialize sub services/domains manually. E.g.

class DataServicesPlugin extends Service<> {
  constructor(initializer) { // <-- automatically injected
    this.searchServices = new SearchServices(initializer) // <-- manually passed
  }

  setup(core, deps) { // <--- automatically injected
    return {
      search: this.searchService.setup(core, deps) // <-- manually passed
    }
  }

  start(core, deps) { // <--- automatically injected
    return {
      search: this.searchService.start(core, deps) // <-- manually passed
    }
  }
}

@ppisljar - I like that proposal as well. If they are real plugins then that solves the circular references and you get the automatic dependency injection. I think then the discussion is still though, should these be called nested services or nested plugins?

Though one downside (?) is that access paths then don't match folder structure. e.g.

class AlertingServicesPlugin extends Service<> {
 setup(core, deps) {
   deps.search.doSomething(); // <-- `deps.search` is the "access path", while folder structure is `data/search`.  Not sure if this is confusing.
 }
}

@mattkime
Copy link
Contributor

mattkime commented Oct 2, 2019

I'd describe this topic as composition of plugins and it makes sense to me that we'd be discussing it at this stage of the process. Its important to address.

IMO, unless there is a significant difference (yes, thats open to interpretation) between plugins and services I'd just assume refer to them as plugins and nested (or sub? something else?) plugins as it communicates similarity and hierarchy in a way that plugins and services does not.

@epixa
Copy link
Contributor

epixa commented Oct 2, 2019

There are concrete differences between plugins and services today:

Services, by convention, are classes with lifecycle functions that instantiate the state of their particular domain and expose APIs for others to use.

Plugins are effectively services that also:

  • Are dynamically discovered by their location on the file system
  • Contain static manifest files that are or will be used for dependencies, ui/server code flags, native controllers (code to be executed in a privileged context before any other plugin code runs anywhere)
  • Can be disabled
  • Automatically have dependencies injected into their setup/start routines

In essence, a "plugin" is a service that you've wrapped with a pattern that allows it to integrate with the core plugin service.

I also want to help frame this discussion: we're proposing changes to the model designed in the original rearchitecture and now new platform efforts. That's OK because we learn news things and we certainly have no illusions that Kibana's new architecture is perfect, but this should mean we're approaching this in terms of what is problematic with the conventions, terminology, or general architecture that we have today and what are the things we should do to fix that.

@stacey-gammon
Copy link
Contributor Author

Can be disabled

This is a good point to consider. If we group thinks solely based on logical, domain, hierarchies, the disable/enable flag can end up covering a lot of smaller discrete services.

I think I actually just came up with a real circular dependency issue with grouping by domain. Consider the following top level domains:

  • embeddables
  • ui_actions
  • data

we have another x-pack plugin called advanced_ui_actions which includes per panel time range but if we really wanted to group by domain, not by type, that should actually be under a advanced_data plugin because it deals with time and TimeRange is under the data plugin.

If it was, we now have a circular dependency because Per panel time range depends on Embeddables but Embeddables depends on data because it has time, query and filters as first class concepts (optionally, not all embeddables use those).

...actually that still isn't a real issue because advanced_data is in x-pack, but all it would take is open sourcing per panel time range for it to be a real issue... an actual issue doesn't feel far off..

@stacey-gammon
Copy link
Contributor Author

One more benefit of the modular approach is an obvious place to put a single README.md file that can cover both server and public details.

@stacey-gammon
Copy link
Contributor Author

Another similar question. Should routes mimick the path? e.g. should it be api/search or api/data/search?

@joshdover
Copy link
Contributor

1. Formalize and encourage the Service interface

This sounds like a good idea. And I think we can take it further by emphasizing that the Plugin interface is really just a specialized form of the Service interface that makes it compatible with the core plugin service.

type LifecycleMethod = (...args: any[]) => any;

interface Service<TSetup extends LifecycleMethod, TStart extends LifecycleMethod> {
  setup(...args: Parameters<TSetup>): ReturnType<TSetup>;
  start(...args: Parameters<TStart>): ReturnType<TStart>;
  stop(): void;
}

type Plugin<
  TSetup = void,
  TStart = void,
  TPluginsSetup extends object = object,
  TPluginsStart extends object = object
> = Service<
  (core: CoreSetup, plugins: TPluginsSetup) => TSetup,
  (core: CoreStart, plugins: TPluginsStart) => TStart
>;

2. Replace Plugin terminology with Service terminology

Supports goal 3 and 4.

Current:

- data
   - public
     - plugin.ts
       - class DataPlugin extends Plugin<>

Proposed:

- data
   - public
     - data_service.ts
       - class DataService extends Service<>

It seems to me we would get the same benefits of "portability" if we kept all plugin.ts files extremely small and instead just used these classes to wire up services. For instance:

// my_plugin/server/plugin.ts

import { DataService } from './services/data';

interface MyPluginSetup = { data: ReturnType<DataService['setup']> };
interface MyPluginStart = { data: ReturnType<DataService['start']> };

export class MyPlugin implements Plugin<MyPluginSetup, MyPluginStart> {
  private readonly dataService = new DataService();

  public setup(core: CoreSetup) {
    return { data: this.dataService.setup(core) };
  }

  public start(core: CoreStart) {
    return { data: this.dataService.start(core) };
  }

  public stop() {
    this.dataService.stop(core);
  }
}

With this pattern, you'd never really need to "move" MyPlugin since it doesn't actually do anything. Instead, you'd move DataService. Though when combined with the type suggestion above, you also could move MyPlugin without breaking the "all services should conform to the Service interface" rule since Plugin extends Service.


For the hierarchy considerations, I think the main consideration we should be making here is whether or not a domain or sub-domain should be togglable (enable/disable from kibana.yml) on its own. If yes, then that service/domain should be its own plugin. If not, then it should belong to a broader plugin.

If a top-level plugin also wishes to allow it's sub-domains/sub-services to be disable-able, it still can. It just needs to implement this functionality itself. I don't think this is a concern that Core could solve in any meaningful way beyond what plugins can already do today by reading config.


Regarding circular dependencies, I think when these arise it's more indicative of a code smell than an architectural problem with the Platform. Typically, this indicates tight-coupling or a need for reorganization.

I would like to see a good example that couldn't be solved by either:

  • "Hoisting" the dependency parts up to a shared dependency further up the chain; or
  • De-coupling domains by exposing APIs for plugins register themselves with other plugins.
    • For instance, in the whiteboard example above, I'd be surprised to see the data plugin depending on alerting in any way. This seems like tight coupling that could be avoided by the data plugin exposing some registerAlertProvider function or similar (not sure on the exact reason for this dependency in this example).

The more circular dependencies we have, the harder refactoring and iterating on new solutions becomes. I think introducing any magic dependency injector beyond what we already have would be a mistake that we would pay for for years to come.

@stacey-gammon
Copy link
Contributor Author

👍 that makes sense to me @joshdover.

@rudolf
Copy link
Contributor

rudolf commented Oct 25, 2019

The more circular dependencies we have, the harder refactoring and iterating on new solutions becomes. I think introducing any magic dependency injector beyond what we already have would be a mistake that we would pay for for years to come.

👍

  1. Consistency between domains at any level (only difference between a top and nested domain is access path)
  2. Leaf services should be highly focused, small services

I see value in creating a formalized interface for services where a service encompasses a particular domain and agree with @joshdover's comment about this providing portability so that services can be moved between plugins.

However, I'm cautious about extending this to "sub-services" or "leaf-services" unless the sub-service has an obvious lifecycle that benefits from having lifecycle methods defined. There are a couple of places in Core where it feels like we've forced this pattern a bit too much. One could argue this improves portability, but I think when it comes to tiny services this reason alone would be over-engineering. If we ever need to move a sub-service into a service it would be trivial to refactor ("You A'int Gonna Need It").

Adhering to the lifecycle methods adds additional type boilerplate and makes it hard not to separate documentation from the implementation. It also constrains API's to a lifecycle, while a service like the ChromeService might choose not to expose getDocLinks from it's setup() lifecycle, it might be perfectly valid for the ChromeService to use this function internally during setup. It feels like lifecycle management and "gate keeping" of whet gets exposed should be a domain/top-level service concern not something every piece of functionality or shared code needs to be aware of.

Some concrete examples:

Doc links service

export class DocLinksService {
public start({ injectedMetadata }: StartDeps): DocLinksStart {
const DOC_LINK_VERSION = injectedMetadata.getKibanaBranch();
const ELASTIC_WEBSITE_URL = 'https://www.elastic.co/';
const ELASTICSEARCH_DOCS = `${ELASTIC_WEBSITE_URL}guide/en/elasticsearch/reference/${DOC_LINK_VERSION}/`;
return deepFreeze({
DOC_LINK_VERSION,
ELASTIC_WEBSITE_URL,
links: {

Do we loose anything if we use a function?

export function getDocLinks(injectedMetadata: InjectedMetadataStart) {
  const DOC_LINK_VERSION = injectedMetadata.getKibanaBranch();
  const ELASTIC_WEBSITE_URL = 'https://www.elastic.co/';
  const ELASTICSEARCH_DOCS = `${ELASTIC_WEBSITE_URL}guide/en/elasticsearch/reference/${DOC_LINK_VERSION}/`;

  return deepFreeze({
    DOC_LINK_VERSION,
    ELASTIC_WEBSITE_URL,
    links: {

Doc title service

export interface ChromeDocTitle {
/**
* Changes the current document title.
*
* @example
* How to change the title of the document
* ```ts
* chrome.docTitle.change('My application title')
* chrome.docTitle.change(['My application', 'My section'])
* ```
*
* @param newTitle The new title to set, either a string or string array
*/
change(newTitle: string | string[]): void;
/**
* Resets the document title to it's initial value.
* (meaning the one present in the title meta at application load.)
*/
reset(): void;
/** @internal */
__legacy: {
setBaseTitle(baseTitle: string): void;
};
}
const defaultTitle: string[] = [];
const titleSeparator = ' - ';
/** @internal */
export class DocTitleService {
private document = { title: '' };
private baseTitle = '';
public start({ document }: StartDeps): ChromeDocTitle {
this.document = document;
this.baseTitle = document.title;
return {
change: (title: string | string[]) => {
this.applyTitle(title);
},
reset: () => {
this.applyTitle(defaultTitle);
},

As an ES6 class:

export class DocTitleService {
  private document = { title: '' };
  private baseTitle = '';

  public constructor(document: Document){
    this.document = document;
    this.baseTitle = document.title;
  }

  /**
   * Changes the current document title.
   *
   * @example
   * How to change the title of the document
   * ```ts
   * chrome.docTitle.change('My application title')
   * chrome.docTitle.change(['My application', 'My section'])
   * ```
   *
   * @param newTitle The new title to set, either a string or string array
   */
  public change(title: string | string[]) {
    this.applyTitle(title);
  }

  /**
   * Resets the document title to it's initial value.
   * (meaning the one present in the title meta at application load.)
   */
  public reset() {
    this.applyTitle(defaultTitle);
  }

In short, I like the idea of standardising the service lifecycle pattern, but we should only apply it where it adds value. The deeper down the plugin/service tree we go the less value I see it adding and the more it gets in the way.

@lukeelmers
Copy link
Member

I know I'm a month late to catch up on this, but I wanted to voice support for the idea of a formalized service interface as well, which could serve as a base to the plugin interface as Josh describes above.

Regarding circular dependencies, I think when these arise it's more indicative of a code smell than an architectural problem with the Platform. Typically, this indicates tight-coupling or a need for reorganization.

100% agree with this. If we hit a circular dependency, we should have a bias toward finding out whether it truly needs to be there in the first place, rather than having the solution be creating another domain/plugin. If down the road we end up with a giant flat /src/plugins directory full of single-service plugins, we will have done something wrong IMO.

As an aside: My sense is that data probably shouldn't ever need to have dependencies on other stateful services/plugins outside of core. I feel like data is the one plugin that most other plugins will depend on in one way or another, so if we can avoid adding stateful dependencies to it, we greatly reduce the likelihood of hitting new circular dependencies as we build out new services. This thread isn't the right place for that discussion though, just mentioning it here as it relates to the example above 😉

@stacey-gammon
Copy link
Contributor Author

stacey-gammon commented Dec 11, 2019

We talked more about circular dependencies in meeting today. There are more use case of dependencies, though in most cases a registry should solve the problem. For example, we talked about embeddables and cross linking between different solutions:

  • Uptime wants to embed an APM metric, APM wants to embed uptime metric.

You can solve this via registries because you don't have to actually add the other plugin as a dependency, you can do something like:

 // In APM plugin:
  if (embeddableFactories.get(INFRA)) {
   // render infra metric
  }

 // In Infra plugin:
  if (embeddableFactories.get(APM)) {
   // render APM metric
  }

It was mentioned will could cause us to lose some type safety (though I'm not sure I follow how).

This also forces us to always use registries these ways, which maybe is too restrictive?

You could solve this by importing static code, but this is not always possible. You can also solve this by breaking your plugin into multiple plugins (the current workaround), but this creates the flat domain structure.

In addition to the cross solution embedding, is the cross solution linking, which is a common scenario.

@stacey-gammon
Copy link
Contributor Author

Noting some things that have changed since my last comment.

  1. We discourage accessing a service through a generic registry when that service is known at compile time.

Good:

const fooBarService = fooBarPlugin.getFooBarService();

Discouraged:

const fooBarService = fooPlugin.getService(BAR);

Why? The former makes it obvious you need to declare a dependency on plugin bar. If you forget, you will get random ci failures (plugin registration order, when not explicit, is not guaranteed). It also avoids the need to use declare module and have type mappings for type safety (see #67551).

  1. I mentioned the cross solution linking in the issue above, but that particular instance is getting more attention now in Plugin URLs should be part of the plugin's contract #66682

@exalate-issue-sync exalate-issue-sync bot added impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort labels Jun 21, 2021
@pgayvallet
Copy link
Contributor

@stacey-gammon do you think we should keep this issue open, or are we good closing?

@stacey-gammon
Copy link
Contributor Author

Good to close!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. loe:small Small Level of Effort Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

9 participants