Introducing "tiered dependencies" in observability plugins

[jasonrhodes]

This document explains the convention of "tiered dependencies" so we can begin to enforce it in code reviews by referencing a single document.

Please take a look at the Markdown document being introduced in this PR and put any questions or concerns in the comments of the PR.

Open Questions

• Why is this important? I've seen new plugin-to-plugin dependencies being added casually in PRs recently, which will make this problem harder and harder to solve, the further we double down on it. It's happening because we're all sharing things more, which is great! Let's make sure sharing doesn't create more feature blockages, and enable as much safe, hassle-free sharing between our teams as possible!
• How will we enforce this? To start with, it will be a manual convention enforced by code review. This is better than what we have today, and a document like this allows us to explain the reasoning in a code review without having to discuss everything from the start each time. However, if we find this system works, we should look into how to codify and enforce it via linting, build, etc.
• What if we need more than 3 tiers? I don't want to get too bogged down in this question, but it's fair. I contemplated numbering the tiers "10", "20", "30" to allow for space in between, but I don't think it's necessary. If we need more tiers, we'll adjust the guidance. For now, this should be plenty to get us started.
• What other information does this documentation need? Perhaps a "where should my shared code go?" flow chart type section, to clarify as much as possible where newly shared/extracted code should go?

[apmmachine]

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

• /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
• /oblt-deploy-serverless : Deploy a serverless Kibana instance using the Observability test environments.
• run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

[CoenWarmer]

Thanks for this clear write up @jasonrhodes. It lays bare some of the problems I encountered while refactoring the Observability app. We already moved the majority of shared stuff into Observability Shared (which I assume by the plan proposed is a Share Tier plugin).

Three questions I have:

1. Observability Rule Registry

The Obs Rule Registry is currently housed in the Observability app.
The Observability app is an End User Tier plugin. As multiple End User Tier plugins need to register rules, should the registry also be moved to Observability Shared (or even TriggersActionsUi as that offers a lot of alerting related UI)? Or, as the registry deals with Alerts, should it be housed in a Data Level Tier plugin? Would be interesting to have @XavierM's perspective here too.

2. TriggersActionsUi

Is this plugin considered a Share Tier plugin?

3. Labelling and enforcement

I'm concerned that if we don't get the enforcement right it will not be possible to maintain a clean tier structure for long.

What is the general plan for understanding what plugins are of which tier? Perhaps we could denote that in kibana.jsonc.

Will there be an effort made to invest in tooling (ie ESLint rules) to help contributors catch incorrect tier inclusions? Preferably before it reaches the PR stage. I'd be interested to help if that would be welcomed.

[cauemarcondes]

LGTM. Thanks for writing it down.

[tommyers-elastic]

Seems like a good approach. Is there a place for plugins to define their tier in code? And is there any way to track how many intra-tier dependencies currently exist?

[jasonrhodes]

@CoenWarmer great questions!

We already moved the majority of shared stuff into Observability Shared (which I assume by the plan proposed is a Share Tier plugin).

Yep!

1. Observability Rule Registry

The Obs Rule Registry is currently housed in the Observability app.
The Observability app is an End User Tier plugin. As multiple End User Tier plugins need to register rules, should the registry also be moved to Observability Shared (or even TriggersActionsUi as that offers a lot of alerting related UI)? Or, as the registry deals with Alerts, should it be housed in a Data Level Tier plugin? Would be interesting to have @XavierM's perspective here too.

I think for sure it should be extracted from the end user tier. Where it goes from there probably depends on how it might be used. The deeper down it can go, the more things can depend on it, so if it doesn't need access to any other data access clients, that could be a good tier for it. We probably need to flesh out the "flow chart" to help us answer the question of where exactly things go (and whether these tier names are perfect yet, too).

2. TriggersActionsUi

Is this plugin considered a Share Tier plugin?

As I understand it, that's managed by one of the central Kibana teams, right? If so, I'd consider it to be part of "Kibana core" from the observability perspective.

3. Labelling and enforcement

I'm concerned that if we don't get the enforcement right it will not be possible to maintain a clean tier structure for long.

What is the general plan for understanding what plugins are of which tier? Perhaps we could denote that in kibana.jsonc.

Will there be an effort made to invest in tooling (ie ESLint rules) to help contributors catch incorrect tier inclusions? Preferably before it reaches the PR stage. I'd be interested to help if that would be welcomed.

Fair concern! I think the kibana.jsonc file is the right place to designate a plugin's tier. I would also very much welcome any help in exploring linting or other tooling options for enforcement, as I don't have bandwidth to explore this myself at the moment.

[jasonrhodes]

Thanks, @tommyers-elastic

Is there a place for plugins to define their tier in code?

My first inclination is to designate the tier in the kibana.jsonc file. We'll need to check with the Kibana core team to see if we can add keys there or if there is an approval process, etc., but it should be possible.

And is there any way to track how many intra-tier dependencies currently exist?

I imagine there will be a way to compute this by walking the plugin tree and looking in the kibana.jsonc files, but someone would likely need to script this, as I don't think it'd be something available to us without a little work. It shouldn't be hard to compute, though, I expect.

[dgieselaar]

@jasonrhodes thanks for your work here! Solving this would remove a lot of the friction around sharing functionality between Observability plugins. Quite often the answer to a product question is not necessarily "it might take a bit" but "we can't do it because of circular dependencies" so seeing this solved would be a big boost.

That being said, I don't think this gets to the root of the problem, which is that circular dependencies are a self-imposed limitation. It doesn't have to work this way. In most cases, the APIs from the downstream dependency that the upstream plugin wants to use, are not expected to be used by the methods exposed in the upstream plugin's start or setup contract. What I'd like to see here is a much simpler solution: expose core.onPluginStart(<pluginName>, <callback>) and core.onPluginSetup(<pluginName>, <callback>) methods that will fire the callback if the specified plugin has completed start or setup, with its contract. This is an implicit dependency, so it shouldn't have to be specified in package.json. The only additional work needed would be to move the types for the start and setup contract to a separate package so it can be consumed by the upstream plugin without creating a static dependency on the downstream dependency. cc @pgayvallet

[jasonrhodes]

@dgieselaar thanks for weighing in!

That being said, I don't think this gets to the root of the problem, which is that circular dependencies are a self-imposed limitation. It doesn't have to work this way.

I know what you mean here, but I actually think that in many cases, our desire for circular dependencies is a smell that would lead to other issues if we circumnavigated it. Personally, I find the idea of being free to create smaller, more focused plugins and packages with clear, limited dependencies to be a much simpler solution overall, and allows us to clearly specify what we need and what we don't in more places than we are able to do now.

What I'd like to see here is a much simpler solution: expose core.onPluginStart(<pluginName>, <callback>) and core.onPluginSetup(<pluginName>, <callback>) methods that will fire the callback if the specified plugin has completed start or setup, with its contract. This is an implicit dependency, so it shouldn't have to be specified in package.json.

Let me try to understand this setup.

• APM wants to use the metrics client exposed from Infra:setup
• Infra wants to use a getTraces method exposed from APM:start

If I understand the onPluginStart() solution right, this might look something like:

// inside Infra
setup(core: CoreSetup) {
  core.onPluginStart('apm', (apmStart) => {
    // here I can use apmStart.getTraces()
  }
}

But this wouldn't let Infra define a method that makes use of getTraces and also is available for use throughout all of Infra, right? It seems like you can only make use of the depencency method(s) inside the callback. I might be misunderstanding, but this feels to me like it makes for a very complicated set of limitations around how code must be written. Is there a better example here for how Infra could define an exported method (for internal Infra plugin use) that makes use of the getTraces method exposed in APM?

Ultimately I agree that many things we are trying to depend on are much more likely to be closer to static dependencies and the circular dep problem is coming from the runtime execution only. But I think the solution to that is making it easier to extract things out of our main plugins and make use of explicitly static dependencies via packages, leaving behind only what needs direct access to core/runtime flow. Tiers are, imo, a way to make it clear how to extract things safely, and eventually end up with very lightweight/thin plugins.

[dgieselaar]

But this wouldn't let Infra define a method that makes use of getTraces and also is available for use throughout all of Infra, right? It seems like you can only make use of the depencency method(s) inside the callback. I might be misunderstanding, but this feels to me like it makes for a very complicated set of limitations around how code must be written. Is there a better example here for how Infra could define an exported method (for internal Infra plugin use) that makes use of the getTraces method exposed in APM?

It depends on what "internal Infra plugin use" means. But here's an example (with some convoluted naming to make it more explicit):

// inside Infra
setup(core: CoreSetup, pluginsSetup: PluginsSetup ) {

  registerInfraAPIEndpoints({
  	core,
	plugins: pluginsSetup,
	getApmTracesMethod: ( ) => {
		return new Promise(( resolve, reject ) => {
			core.onPluginStart('apm', ( apmStart ) => {
				resolve(apmStart.getTraces);
			});
		});
  })
}

Ultimately I agree that many things we are trying to depend on are much more likely to be closer to static dependencies and the circular dep problem is coming from the runtime execution only. But I think the solution to that is making it easier to extract things out of our main plugins and make use of explicitly static dependencies via packages, leaving behind only what needs direct access to core/runtime flow. Tiers are, imo, a way to make it clear how to extract things safely, and eventually end up with very lightweight/thin plugins.

I mostly agree, but the main issue for me is that I think there's a cost to extracting things out of a plugin (bootstrapping a plugin which I think is not something we can generate today, it's a tedious copy & paste process), there's things like CODEOWNERS, git history etc which all become more complicated. I don't think organising a change like this is cheap (esp cross-team), and I would rather have a simple API that solves practical problems today, with minimal effort on all teams.

An example: The Observability AI Assistant plugin depends on ML for some functionality, but some of the ML UI is now integrated into Observability. That means that the Assistant button is now missing on 6 of the 13 views in Serverless. To get around this, and to conform to this approach, the ML plugin would have to split up their plugin. That requires a significant effort on the ML team's part, which have their own roadmap, so I wouldn't expect this to happen for a while. An event-based API would be more appealing in this case I think.

[dgieselaar]

@pgayvallet opened an issue to discuss an event-based API here: #166688

[jasonrhodes]

It depends on what "internal Infra plugin use" means. But here's an example (with some convoluted naming to make it more explicit):

// inside Infra
setup(core: CoreSetup, pluginsSetup: PluginsSetup ) {

  registerInfraAPIEndpoints({
  	core,
	plugins: pluginsSetup,
	getApmTracesMethod: ( ) => {
		return new Promise(( resolve, reject ) => {
			core.onPluginStart('apm', ( apmStart ) => {
				resolve(apmStart.getTraces);
			});
		});
  })
}

Thanks, this makes sense. It's a bit more complicated than the normal "import and use" flow that most engineers would be used to and expect to work, but it's not terrible and matches the pattern most of us use for plugin bootstrapping already.

Ultimately I agree that many things we are trying to depend on are much more likely to be closer to static dependencies and the circular dep problem is coming from the runtime execution only. But I think the solution to that is making it easier to extract things out of our main plugins and make use of explicitly static dependencies via packages, leaving behind only what needs direct access to core/runtime flow. Tiers are, imo, a way to make it clear how to extract things safely, and eventually end up with very lightweight/thin plugins.

I mostly agree, but the main issue for me is that I think there's a cost to extracting things out of a plugin (bootstrapping a plugin which I think is not something we can generate today, it's a tedious copy & paste process), there's things like CODEOWNERS, git history etc which all become more complicated. I don't think organising a change like this is cheap (esp cross-team), and I would rather have a simple API that solves practical problems today, with minimal effort on all teams.

Yeah, plugins are heavy. I'm hoping that by encouraging this type of extraction, we can move toward making more and more things into packages, which are a good deal lighter (not free, either, but lighter). I think overall that cost is usually worth it to detangle all of the concerns we have mashed together at the moment.

An example: The Observability AI Assistant plugin depends on ML for some functionality, but some of the ML UI is now integrated into Observability. That means that the Assistant button is now missing on 6 of the 13 views in Serverless. To get around this, and to conform to this approach, the ML plugin would have to split up their plugin. That requires a significant effort on the ML team's part, which have their own roadmap, so I wouldn't expect this to happen for a while. An event-based API would be more appealing in this case I think.

I don't totally follow this but I understand that we can't really ask other teams outside of Observability to get on board with this type of reorganization work, so in those cases we'll probably need a different escape hatch. I'm hoping we might be able to demonstrate the value of this kind of system within observability and then maybe make the case to wider Kibana after that (or discover it didn't work, and why, if that's the case).

@pgayvallet opened an issue to discuss an event-based API here: #166688

I'll take a look! At the very least, it'll be good to have this kind of escape hatch where it's needed, because we won't always be able to wait for another team to extract things to make them shareable. But if we adopt this system, I hope we don't use the escape hatch too often because, in my opinion, it's much harder to follow and encourages doubling down on patterns that create more problems for us all in the long run.

[jasonrhodes]

Another issue to consider, brought up by @tommyers-elastic:

I think i have a use-case that breaks the kibana plugin tiering. We are planning to build a K8s API in Kibana. I could see it utilizing the asset-manager plugin to make asset type queries, but it's not "visible section of the Observability UI".

Where should it sit?

You can boil this problem down to "what happens if share tier plugins want to use each other?" In the suggested system, this wouldn't work. The options for solving this are (a) more tiers, (b) share tier plugins accept injected dependencies so they can use each other, (c) share tier moves to packages.

(a) is bad, because it's unscalable. If a new K8s API needs the assets API, separating it into its own "plugin tier" is arbitrary and complicated. What happens when some other plugin wants to use the K8s API? More tiers? It won't scale.

(b) works, but is complicated because you can re-introduce plugin circular dependencies that appear in injected dependencies. I think this is probably an anti-pattern, and we should avoid injected dependencies in plugins and instead push that pattern out to packages, where all runtime dependencies are always injected.

(c) That brings us to the packages solution, which is the best, I think. We should probably strongly discourage accepting injected (runtime) dependencies in plugin methods and use packages for things that are designed that way. In the example above, the K8s API should likely be a package which accepts the assets client (and any other data clients it needs, or core deps, etc) as injected parameters.

Ultimately, this may mean that the share tier disappears and is replaced entirely by packages. If that happens, we can simplify the tier model and rely on static dependency management even more, which I think is a win overall.

[weltenwort]

Thanks for the succinct write-up. As you know from our conversations about this over the years, I think this is a useful approach to reduce the risk of getting blocked by circular dependencies.

Regarding accessing APIs of same-tier plugins, I also see the occasional need for it. One pattern we could apply on top of the tiered structure is to put a registry in a lower-tier plugin and share the accompanying types via a package. One example would be locators: The Kibana platform offers a locator registry on the low-tier share plugin, which is pretty safe to depend on. Two "End User Tier" plugins could use each other's locators by fetching them from that registry using IDs in constants in a package and type the return value using interface from the same package. This avoids cycles both on the runtime level and the typescript project reference level.

flowchart TD

plugin_a(Plugin A)
plugin_b(Plugin B)
share_plugin(`share` plugin)
type_package(Package with IDs and types)

plugin_a -->|register locator A| share_plugin
plugin_a -->|get locator B| share_plugin
plugin_b -->|register locator B| share_plugin
plugin_b -->|get locator A| share_plugin
plugin_a --> type_package
plugin_b --> type_package

Loading

The event-based approach described above sounds like it only "circumvents" the cycle check by moving it to the runtime phase and thereby not checking it at all. Two plugins waiting on each other would cause a deadlock that wouldn't be discovered statically. And in order for it to be type-safe the plugins would still need to share the interfaces, which would introduce a cycle on the TS project reference level. Unless one factors them out into a package, that is.

Just one more comment, which is purely a matter of taste: While writing the above I stumbled over the tier numbering several times. The "End User Tier" feels like a "higher tier" to me, but has a lower number than the "Share Tier". To me the "End User Tier" would make more sense as a "Tier 3". 🤷

[dgieselaar]

The event-based approach described above sounds like it only "circumvents" the cycle check by moving it to the runtime phase and thereby not checking it at all. Two plugins waiting on each other would cause a deadlock that wouldn't be discovered statically.

I don't think this is true?

And in order for it to be type-safe the plugins would still need to share the interfaces, which would introduce a cycle on the TS project reference level.

This is true, but the types of contracts should be fairly small in most cases, and it's still not close to the amount of work that would be needed to separate entire plugins.

[dgieselaar]

@weltenwort can you also clarify what you mean here, specifically the "introduce a cycle" part:

And in order for it to be type-safe the plugins would still need to share the interfaces, which would introduce a cycle on the TS project reference level.

[weltenwort]

@weltenwort can you also clarify what you mean here, specifically the "introduce a cycle" part:

And in order for it to be type-safe the plugins would still need to share the interfaces, which would introduce a cycle on the TS project reference level.

Maybe I'm misremembering, but I seem to remember that our CI runs a linter with auto-fix, that adds tsconfig references for every imported other package/plugin (https://github.com/elastic/kibana/blob/dc34810aff1b855fc2e7157e3d2dee574840b63a/packages/kbn-lint-ts-projects-cli/rules/reference_used_pkgs.ts). And in those tsconfig project references cycles are not allowed either, at least according to https://github.com/elastic/kibana/blob/main/docs/developer/best-practices/typescript.asciidoc#project-references.

[dgieselaar]

@weltenwort ah I see what you mean. The types for the contracts would live in packages, removing the cyclical dependencies.

[jasonrhodes]

I'd like to think that moving logical chunks of shared code into packages isn't significantly more work than moving types there, and probably has lots of other benefits in the long run. It won't work for all cases, but I see it as a huge improvement for us overall in many cases.

[jasonrhodes]

@weltenwort thanks for weighing in!

Regarding accessing APIs of same-tier plugins, I also see the occasional need for it. One pattern we could apply on top of the tiered structure is to put a registry in a lower-tier plugin and share the accompanying types via a package. One example would be locators: The Kibana platform offers a locator registry on the low-tier share plugin, which is pretty safe to depend on. Two "End User Tier" plugins could use each other's locators by fetching them from that registry using IDs in constants in a package and type the return value using interface from the same package. This avoids cycles both on the runtime level and the typescript project reference level.

I'm not opposed to this idea, and I think we'll likely need something like it. We'll have to make some tricky trade-off decisions around complexity and abstraction (when should we use a registry vs just extracting code like locators out of the end user tier, etc), but at least we'll have a plan for how to do each, which is the main goal here.

Just one more comment, which is purely a matter of taste: While writing the above I stumbled over the tier numbering several times. The "End User Tier" feels like a "higher tier" to me, but has a lower number than the "Share Tier". To me the "End User Tier" would make more sense as a "Tier 3". 🤷

Yeah that's not set in stone yet, I already switched it around once just recently 😆

[CoenWarmer]

Thinking about it more, if we decide to move forward with this, I strongly feel we need to have enforcement via CI checks (and preferably checks before a PR hits the CI) in place.

If we don't, it will work for a short time while this topic is top of mind for engineers (assuming it even is at the moment of merging), but once that moment passes it only takes one merge to break the convention again. Who knows if and when we catch it afterwards.

[sorenlouv]

Thinking about it more, if we decide to move forward with this, I strongly feel we need to have enforcement via CI checks (and preferably checks before a PR hits the CI) in place.

Are you referring to the tiered dependencies or the API approach? And in either case, do you have any suggestions for how this check should work?

[jasonrhodes]

Thinking about it more, if we decide to move forward with this, I strongly feel we need to have enforcement via CI checks (and preferably checks before a PR hits the CI) in place.

@CoenWarmer I agree, although I don't want that to block us from agreeing on the approach and getting it described in writing, which will hopefully help us clarify exactly what we want to lint/test.

[jasonrhodes]

For those who have responded to this PR, thank you so much. I appreciate the conversation that's happened here and I'd like to get us to a place where we have a rough agreement on how to approach our plugin and package organization going forward, especially with Observability going through organizational changes.

To that end, I've added a commit where I've introduced a summary section to the doc that explains the primary things we're agreeing to do, across the Observability Kibana world, to optimize for moving fast, sharing when it makes sense, and resisting over-abstraction.

You can see that summary here: https://github.com/elastic/kibana/blob/7bd3a892114a31d8eb432291753a22a9d4873743/x-pack/plugins/observability_shared/dev_docs/how-we-manage-dependencies.md#plan-summary

Note: I've also renamed the "data access tier" to the "core-only tier" which more accurately represents what that tier needs to be, any plugin that only needs access to Kibana core services can live in that tier. If nobody objects to that rename, I'll update the images before I merge these docs.

If I hear no major objections to this latest commit, I'd like to merge these docs sometime next week and begin to think through how we might introduce (a) tier identification, and (b) dependency linting to prevent this problem from continuing to grow. (A few people gave a thumbs up in email only, such as @andrewvc and @ruflin.)

[dgieselaar]

@jasonrhodes no objections to merging, but before adding ci checks/linting, I think we should have a sync conversation first, because I (and others) have yet to be convinced this is not a dead end, and whether it is something we should be enforcing.

[jasonrhodes]

@dgieselaar happy to talk. I understood the concerns in this thread, at least, to be about being careful to require people to extract things into new plugins/packages, so the idea with these guidelines is not to require that, but only to disallow dependencies between our "end user" plugins. I'm hoping we can find agreement at least on that, and then clarify the rest so that everyone is comfortable with the general direction. I'll schedule something for next week.

[jasonrhodes]

Update: We had a few good meetings involving myself, @dgieselaar, @sqren, @lukeelmers, @stevedodson, and others on the Obs and Kibana sides. The conclusion from that meeting is as follows:

• Pierre's event-based core API system will be reviewed and merged. From the beginning the plan would need to be to remove this API and only have it be a temporary solution until we (Kibana platform) have something more permanent in place. Perhaps we would even mark it as deprecated from day 1. So we would intend to make a breaking change and remove this API once a longer-term solution like service-level dependencies is in place.
• By avoiding using this event-based API and instead focusing on the tiered plugin proposal outlined in this PR, the transition to a longer-term solution would likely be something teams could do in a non-breaking way.
• But by using the event-based API, teams would need to commit to making removal a priority when the time comes. Because using the API would make it easier to defer work on refactoring/restructuring plugins and services, this might mean more tech debt accumulates if teams aren’t careful, making it more challenging to refactor the API out later.

Note: The Kibana core team is currently trying to plan a roadmap for Kibana moving forward, and this dependency management topic is top of mind.

Based on this conclusion, I'm going to merge this proposal as a documented convention, also noting that:

• Until and unless we implement some kind of enforcement of these plugin tier rules, we should have no expectation that they will be effective, so we should commit to exploring enforcement options ASAP
• The developer experience (DX) for breaking code into smaller pieces in Kibana is not optimal at the time, and we need to work with the Kibana platform teams to improve this so that we engineers aren't incentivized away from good practices due to poor DX
• We will need to revise these dependency guidelines often to try to keep them relevant and effective

[jbudz]

This is failing a casing check: https://buildkite.com/elastic/kibana-on-merge/builds/36841#018b3957-a498-42ef-9ec4-87b45ab980db/848-849.

Reverted with 9080836.

I'll follow up on the CI side to see if we can partially run checks on documentation pull requests.

[jasonrhodes]

Thanks, @jbudz -- it would be great to run that check on all PRs, or to modify the check to exclude .md{x} files perhaps.