[dx] Dependency check script for plugins

[clintandrewhall]

Summary

Dealing with circular dependencies between plugins has become a sharp pain point for anyone developing plugins in Kibana.

Providing dependencies to a plugin

First, a plugin defines its dependencies in its kibana.jsonc file as one of three types:

• required - the dependency must be present and enabled -- will be guaranteed in the lifecycle
• optional - the dependency can be missing or disabled -- will be undefined in the lifecycle
• requiredBundle - the dependency is required as static code only -- will not be present in the lifecycle

Missing or circular dependencies are detected by the Kibana platform when it starts.

Providing dependencies in code

Our plugins are written in and type-checked by Typescript. As such, each plugin needs to maintain Typescript types defining what the platform is providing. This is done manually, and there is no enforcement mechanism between that and the plugin Typescript types. If these dependency definitions are inconsistent or stale, it can lead to host of issues:

• optional plugins typed as required that are disabled crash the plugin at runtime;
• plugins that are no longer used still included in dependency checks;
• plugins marked as required or optional that are actually required bundles.
• etc.

Dependencies with side-effects

One of the interesting things that has come out of this has been identifying plugins that provide dependent logic through side-effects, rather than lifecycles.

As an example, licensing provides a lifecycle contracts, but also a route handler context as middleware for a dependent plugin. Unfortunately, while this dependency can be stated as required in a dependent plugin's kibana.jsonc, the fact that this is a side-effect makes it incredible difficult to understand the dependency without searching the code.

So the side-effect is more or less hidden from developers. This is likely why we see other plugins using the lifecycle logic, or copy-pasting licensing check code [1, 2], or relying on the route context side-effect.

Proposed (initial) solution

This script is an initial attempt to both identify these problems and surface a plugin's dependencies in a useful way. In addition, the script will warn if the classes aren't typed well, not typed at all, or even don't extend the core Plugin class.

For side-effects, identifying them is key, and then refactoring the plugins to provide appropriate logic in the start or setup contracts.

Next steps

• refine the logic
• write tests
• add --fix option

I'm also considering (in another PR), outputting a consistent type definition file-- perhaps kibana.d.ts-- to the plugin from which the implementing classes could Omit<> or Pick<> the relevant contracts.

[CoenWarmer]

Thanks for your effort @clintandrewhall. Some thoughts:

You mention circular dependencies as a pain point for Kibana and the impetus for this PR. Perhaps I'm misunderstanding, but the Kibana dev server will automatically stop at runtime when a circular dependency is found. I suppose my question is, at what point during development (including dev, CI and build) is this script intended to run?

The other issue that is noted is keeping the setup and start types of a plugin in sync with its dependencies in kibana.jsonc. This is very promising and would definitely be an improvement in DX. It wouldn't necessarily need the terminal UI part for it to be useful, as long as it automatically generates types in a specific subfolder of the plugin which could then be included in the plugin.ts file. Excited to see where this goes next.

[clintandrewhall]

Thanks @CoenWarmer ! Replies below:

You mention circular dependencies as a pain point for Kibana and the impetus for this PR. Perhaps I'm misunderstanding, but the Kibana dev server will automatically stop at runtime when a circular dependency is found.

That's correct. But as we discovered building Serverless projects, if the types and kibana.jsonc are mismatched, the dev server will make the wrong assumptions:

• If the kibana.jsonc in pluginFoo says pluginBar is optional, but in the type it's required, and the plugin is disabled, the failure will only be found at runtime when pluginFoo tries to do something with the pluginBar dependency, (it will be undefined).
• If the kibana.jsonc in pluginFoo says pluginBar is required, but in the type it's optional, the dev server will not start pluginFoo if pluginBar is disabled... even though it's been built to treat it as optional.

I suppose my question is, at what point during development (including dev, CI and build) is this script intended to run?

Right now, it's for dev as a sanity check. Depending on what we think of it, I could see it being expanded to run in CI, similar to how we output bundle tables. I also want to auto-fix types to reflect the manifest when we bootstrap, (but that requires a process to automatically create types for each plugin, etc).

It wouldn't necessarily need the terminal UI part for it to be useful, as long as it automatically generates types in a specific subfolder of the plugin which could then be included in the plugin.ts file.

💯 - I want to do this, specifically, create consistently-named types and put them in common, then auto-fix the plugins to use them. But I didn't want to go there unless we thought it was worthwhile, (sounds like it is).

Thank you for the feedback!

[elasticmachine]

Pinging @elastic/kibana-operations (Team:Operations)

[elasticmachine]

Pinging @elastic/appex-sharedux (Team:SharedUX)

[elasticmachine]

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

[jbudz]

Code review and I tested all CLI options. For evaluating correctness of the graph and plugin lifecycle, I didn't see any issues but I'm operating at a distance from this conceptually. Approving for operations as it's a development dependency, but if there's anyone else on operations that wants to take a second pass it would be helpful.

[sorenlouv]

Changes to x-pack/plugins/apm/scripts/infer_route_return_types/index.ts lgtm

[kdelemme]

Tested locally with observability and it seems to work well. I've changed some dependencies from required to optional in the plugin type definitions, and they are reported "required" as expected.

[kdelemme]

Suggested change

	node scripts/plugin_check --plugin pluginName
	node scripts/plugin_check --dependencies pluginName

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 906e9ed

Failed CI Steps

• FTR Configs #87
• APM Cypress Tests #1
• APM Cypress Tests #3

Test Failures

• [job] [logs] FTR Configs #87 / Session View API (basic) Session view - /internal/session_view/io_events - with a basic license "before all" hook for "/internal/session_view/io_events fails when an invalid api version is specified"

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
@kbn/docs-utils	1	5	+4

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
@kbn/docs-utils	0	1	+1

Unknown metric groups

API count

id	before	after	diff
@kbn/docs-utils	1	5	+4
@kbn/plugin-check	-	1	+1
total			+5

References to deprecated APIs

id	before	after	diff
@kbn/core-lifecycle-browser	2	3	+1
@kbn/core-saved-objects-browser	2	3	+1
@kbn/core-saved-objects-browser-internal	171	173	+2
savedObjectsTagging	44	45	+1
total			+5

History

• 💛 Build #187551 was flaky 07e52e6
• 💚 Build #186919 succeeded 6ace3b1
• 💔 Build #185180 failed c77805a3ba2441cfe993631a429cd173b2a7b2aa
• 💚 Build #184866 succeeded b4f398d0359f2a7fe5688b44a458576bcdeefb12
• 💔 Build #184138 failed e798020aec7f34fbb2c1d632594eb305fd15207d
• 💔 Build #181585 failed 0935d63b40bbf08808904c74c90ba3b379d65397

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream