[RFC] Allow 'leasing' of Expression constructs

[clintandrewhall]

RFC - Allow "Leasing" of Expression Constructs

Related: #104266
Related: #105439

Summary

This PR enhances the Expressions executor and plugin-provided service to allow a consumer to register functions, types and renderers on a temporary basis-- that I've called 'leasing'-- returning a single callback to remove them from the runtime at an appropriate time, (e.g. lease on plugin mount, remove on plugin unmount). This prevents unexpected leaking of expression functions to other plugins that were not intended or expected.

Problem

Currently, a plugin registers its Expression constructs (renderers, functions, types) during plugin setup, (or is intended to). This poses a number of difficulties:

• Once registered, the constructs are available globally to any plugin consuming Expressions, (e.g. Lens functions are available in Canvas, Canvas functions in Lens, etc).
  • Indeed, many proposals have been offered throughout 7.x to deal with this problem: a private flag, namespacing, forking, etc. None have been implemented.
• This creates a "contract leak", where Expression constructs that were never shared explicitly by a plugin (e.g. added to the public or common folder) are still shared implicitly.
• If registered in setup or start of a plugin, the constructs-- which may be few, like Lens, or many, like Canvas-- are included in the page bundle... regardless of if that construct is actually used in contexts beyond that plugin.
  • Plugins like Canvas can add hundreds of kb to the page bundle while the functions themselves aren't used (or at least, intended to be used) outside of the specific plugin.
  • While the function body can be loaded asynchronously, the definition must be included in the bundle.
    • Experiments to use the async approach still add a large amount to the page bundle.
    • [RFC] Experiment: split Canvas Expression Definitions from async Expression logic #104990
• While some plugins have opted to add the constructs on mount or by other asynchronous means, once the plugin is loaded, the constructs have still "polluted" the global context.
• We lack any standard, philosophy or best practice around how to manage the relationship between a plugin and the global context, (e.g. when to register and under what circumstances).

All of this makes passivity impossible. Expression-based constructs, once registered, become global, period.

Example

As an example, see the change that became necessary in #104264 when Canvas moved its function registration to plugin mount rather than setup: the Expressions Explorer example had been using an implicit expression, markdown, rather than the explicitly-shared expression, markdown_vis. Notice that the Canvas markdown function is not even in the public directory. This is a perfect example of the "contract leak" the expressions registration dilemma causes.

While only in an example app, the consequence is clear: changing the markdown expression in Canvas, regardless of how minor or measured the change from a Canvas perspective, would have broken this plugin.

Proposal

In #104264, Canvas moved its registration to the mount of the plugin, but they still pollute the Expressions plugin, (they are unavailable on mount, but are available globally on unmount).

This PR evens this out by providing new methods (and philosophy) that keeps plugins from polluting the Expressions plugin, while still giving a means to register their constructs when the plugin is active.

New Methods

1. Provide methods that allow consumers to register more than one construct at a time.
2. Provide methods to remove constructs by name.
3. Provide "lease" functions that return a single, simple callback to remove all constructs registered.

New Philosophy

1. Plugins that register expression constructs meant to remain in the global context should register them in the setup portion of their plugin.
2. Plugins that provide expression constructs for other plugins to consume should either place them in common or public (as appropriate), or in a shared package. They should avoid registering them themselves in the global context.
3. Plugins that intend to keep their expression constructs within the execution of their own application should lease them on mount and remove them on unmount.

Impact

This change should be passive to most plugins, but constructs that were registered in Canvas are already not available as of #104264. Other plugins can simply "opt-in" as they see fit.

We should also begin to create shared packages of expressions we view as "common" or "global" as soon as possible.

Making this change and establishing these standards now, before more expressions or expressions-consuming solutions are written, would benefit the entire ecosystem moving forward. This change is also passive in the sense no one is required to do anything different... but continuing to "pollute" the executor carries a known risk, (with a mitigation).

Request for Comment

I'm seeking general feedback, but also the following specific feedback:

• Philosophical "rules" above.
  • What effect does this have on embeddables? How would, say Lens, "embed" their expressions with the embeddable to ensure they are available in an embeddable container, (e.g. Dashboard) while not polluting the global context? Or are we "doomed" to always require expressions used by embeddables to always be globally available?
• Naming conventions and terminology.
  • Do we need to strike a difference between register and lease? In other words, should we change register to return a callback that is simply ignored by the consumer, or is the syntactic sugar of lease more explicit and useful?
• Should we add register() and lease() which takes multiple types of construct?
  • What signature is best?
    • lease( { renderers: ..., types: ...} ) or lease([...stuff]) and use a TS type-guard?
• Should this logic be more strict in some way?

[elasticmachine]

Pinging @elastic/kibana-presentation (Team:Presentation)

[elasticmachine]

Pinging @elastic/kibana-app-services (Team:AppServices)

[clintandrewhall]

@elasticmachine merge upstream

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: bdbc19d
• Storybooks Preview

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
expressions	1563	1619	+56

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
canvas	1.6MB	1.6MB	+238.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
canvas	23.6KB	23.3KB	-227.0B
expressions	213.1KB	215.3KB	+2.2KB
total			+2.0KB

Unknown metric groups

API count

id	before	after	diff
expressions	1994	2072	+78

History

• 💔 Build #137907 failed 26dcaf2103c6075bfdf1d94ca439d6ac46318f28
• 💔 Build #137705 failed 22046d6

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

[stacey-gammon]

When I read through the problems listed and try to distil them, I think this boils down to a page load bundle size issue only. If the page bundle size was not affected at all, do any problems remain?

Once registered, the constructs are available globally to any plugin consuming Expressions, (e.g. Lens functions are available in Canvas, Canvas functions in Lens, etc).

If this didn't affect page load time, I would consider this a feature, not a bug. We don't yet expose the expression language in other applications, but it's been discussed many times, and if we had that functionality, I would consider it a bad UX if a user couldn't copy an expression string from Canvas to Lens or to Dashboard, and have it work.

This creates a "contract leak", where Expression constructs that were never shared explicitly by a plugin (e.g. added to the public or common folder) are still shared implicitly.

Taking a hypothetical example, it sounds like we have a Plugin A that does something like this: PluginA::setup(, { pluginB }) => pluginB.add(myImplementation) but Plugin A expected myImplementation to be private. However, it's public because PluginB also hasPluginB::start() => ({ get(id) }) that exposes all registry items to other plugins.

Assuming this had no affect on page size, why is it problematic that myImplementation may be accessible publically?

While some plugins have opted to add the constructs on mount or by other asynchronous means, once the plugin is loaded, the constructs have still "polluted" the global context.

If you are talking about plugins adding registry implementations after the setup lifecycle, I consider this an anti-pattern, and something that should be solved with #105439. It sounds there like again, the only issue is page bundle size.

We lack any standard, philosophy or best practice around how to manage the relationship between a plugin and the global context, (e.g. when to register and under what circumstances).

Yes, but I don't know if this is something we can solve. Even if we had recommendations, I don't think we can control what PluginB, in the example above, chooses to expose.

If I try to condense those problems down, I come up with:

Can we improve our registry pattern to avoid increasing the page load bundle size significantly?

Some problems that can cause this:

1. If registry implementations are very large.
2. If a plugin adds many custom implementations to a registry, especially if those custom implementations may not even be used.

Canvas registers many ExpressionFunctions that aren't used by end users outside of Canvas (yet), because we don't expose the raw expression string anywhere else in the UI. However, we've talked about it a lot, and, a developer might want to use those functions. If I understand correctly, a developer using an expression function is what caused this issue to be raised in the first place.

IMO, an ideal solution is something that helps us avoid page bloat for all types of registries, but allows the implementations in them to still be globally accessible, and available to all plugins, at all times (predictability). I like the idea of only loading the implementations when they are needed, but it sounds like that might not solve the page bundle size, according to your experiment Clint. Maybe this is worth digging into a bit more (cc @lukeelmers )

I think allowing expression functions to be registered and unregistered at any time has the potential to cause a lot of issues. For example, while you are intending to use this new functionality in mount and unmount, so they are available only while the user is on the Canvas application, it's up to the expression function producer to do that. There is nothing stopping a producer from leasing and unleasing at unpredictable times. How would a consumer of the expressions registry tell if a function they wanted to use was leased and may stop being available at any time?

[lukeelmers]

From the beginning, the whole promise of Expressions has been that they could eventually be used anywhere in Kibana. So I share Stacey's concern about allowing expression functions to be registered/unregistered at any time. We should be clear about which functions are public and which are private (which I believe we have a solution for -- my thoughts on that below), and then should be very careful about breaking the public ones.

Indeed, many proposals have been offered throughout 7.x to deal with this problem: a private flag, namespacing, forking, etc. None have been implemented.

I understand the concern here and how implicit dependencies on functions can cause confusion, however I am not sure how this isn't already solved by the existing fork in the expressions service. This is explained in the source:

kibana/src/plugins/expressions/common/service/expressions_services.ts

Lines 154 to 163 in 0ef3cbe

	/**
	* Create a new instance of `ExpressionsService`. The new instance inherits
	* all state of the original `ExpressionsService`, including all expression
	* types, expression functions and context. Also, all new types and functions
	* registered in the original services AFTER the forking event will be
	* available in the forked instance. However, all new types and functions
	* registered in the forked instances will NOT be available to the original
	* service.
	*/
	fork: () => ExpressionsService;

In other words, a plugin can:

• Register its public/shared functions with the expressions service
• Use fork which will inherit everything registered to the existing expressions service, including the public/shared items you registered previously
• Register any private functions to your fork, and have your plugin use the fork internally

Does this not solve the problem you are describing above? The "fork" and "no-fork" approaches are not mutually exclusive: you can use both to control sharing of the expression functions.

If you are talking about plugins adding registry implementations after the setup lifecycle, I consider this an anti-pattern, and something that should be solved with #105439. It sounds there like again, the only issue is page bundle size.

++ Agree on this point, but will add my thoughts to that thread. Performing setup operations inside mount breaks the guarantees that the plugin lifecycles are designed to provide.

Can we improve our registry pattern to avoid increasing the page load bundle size significantly?

I know I've said this before, but I think designing more registries to be async could solve a lot of these problems. Then for situations like Canvas, where a bunch of functions are registered during setup, you could instead register a promise that does an async import resolving to the registry item. And in start, the first time someone needs to retrieve a function, the promise is resolved & the result is then cached for subsequent use.

[clintandrewhall]

Thanks for this so far. Let's see if I can address all of the concerns... to be clear, I'm not proposing this PR as the solution, but rather demonstrating the need/a way to sort it.

From the beginning, the whole promise of Expressions has been that they could eventually be used anywhere in Kibana.

Expressions, but not all Expression Functions. We've gone to great lengths to restrict access to code that is not intended to be shared, and yet we're saying that anything registered at run time should be globally accessible. Canvas has no dependents, and yet we're forcing Canvas to share its code with the rest of Kibana simply by utilizing a common service... and forcing the Kibana application to load all of the Canvas code up-front... even if the user has no interest in loading Canvas.

Canvas is currently discussing how to handle deprecations with #101377 ... Expressions, especially those that render output, are incredibly if not impossible to migrate automatically. We're already concerned about how to handle this with end-users in the Canvas UI, but to be concerned about where else in Kibana our functions may be consumed at run time is an enormous burden to bear.

would consider it a bad UX if a user couldn't copy an expression string from Canvas to Lens or to Dashboard, and have it work.

Yes and no. We're basically saying the same thing, but in different ways. I'm arguing that up-front loading of code is inherently wasteful, but also that the sharing of that code must be intentional. In my PR before, an example plugin broke because it was relying on a Canvas expression we had zero intention of being used anywhere else... Canvas has "zero dependents" currently, remember.

I don't think @wylieconlon or @flash1293 have any expectation (or intention to support) that Lens Expressions "should just render" in Canvas outside of an embeddable. They certainly don't "just work" that way today.

however I am not sure how this isn't already solved by the existing fork in the expressions service

Canvas does fork the Expressions service, but my understanding from you and @ppisljar and @streamich was that we wanted forks of the service to stop. Canvas can certainly fork the service to keep our code private. But if Lens were to do that, would their embeddables work?

the only issue is page bundle size.

I'm not convinced that's the only issue here. It's a big one, to be sure. I don't think we should be loading so much code up front for Canvas, and I can get that as small as I can (#104990). Even if we fork and separate the definitions from the implementations, we're still loading code other plugins won't use. That feels incorrect to me.

But I'm also concerned about the implicit vs. explicit sharing of code. We literally block people from importing the source code unless we add it to /public or /common and export it.

I'm all for forking the Expressions Service and explicitly sharing functions, but I think it's heavy handed... I'm not sure though, at this point.

[clintandrewhall]

Action Items

I think I need to either:

1/ expand #104990 to async all of our functions, and/or
2/ move all of our functions to a fork.

Both would serve in replacement of #104264 ... but I'm still concerned about the page bundle.

[stacey-gammon]

Canvas has "zero dependents" currently, remember.

Yea, this is a good point, and a bug, imo. That example plugin should have listed Canvas as a dependency if it required the specific markdown function to be available.

I've discussed this tripping point with @ppisljar before, which is touched on in this closed issue. We talked about discouraging access like embeddableStart.get<LensEmbeddableFactory>(LENS_EMBEDDABLE_TYPE) and instead encouraging lensStart.getLensEmbeddableFactory(), which would force the plugin to list Lens as a dependency, and is better for types.

However, that's just a recommendation, and it'd still be possible for a developer to use the generic registry and not add a dependency. I suspect this is even more difficult to solve with expressions because it's a string. At least with the embeddable example, the plugin would probably import LENS_EMBEDDABLE_TYPE directly from the lens plugin, which is a big giveaway.

For example, what happens if a developer hard codes an expression string and uses a certain expression function they know is registered by PluginA, so they do the right thing and list PluginA as a dependency, but later, the PluginA code is refactored and moved to PluginB. I don't think we would deterministically fail, and the consuming plugin would be depending on the wrong plugin.

we're still loading code other plugins won't use. That feels incorrect to me..
I'm also concerned about the implicit vs. explicit sharing of code. We literally block people from importing the source code unless we add it to /public or /common and export it.

I'm still trying to understand the root problem here. Can we try the "5 whys" approach? Why does it feel incorrect? What is problematic? I'm not being facetious, I'm really trying to understand. Is it a problem that it's shared, or is it a problem that it's not obvious it's shared? Is the main issue migrations/BWC?

[ppisljar]

i need to dig a bit deeper but here are some initial thoughts i have:

it seems we have two problems listed above:

• need for private function inside expressions to prevent contract leaking:
  we currently allow forking but discourage its use as it has a lot of flaws and we plan to remove it in the future.
  we discussed private flags before but they wouldn't fully solve the problem
  currently the idea is that you shouldn't expose types for your function on your contract if you don't want it used but its true its still possible to use it (without type safety, or thru canvas ui)

• bundle size problems
  async loading of function implementations only gets us so far. we would want to prevent function definitions being loaded as well.
  this is something we still want to explore, if we could either load definitions async as well, group definitions in namespaces and async load those or something similar.

I am gonna list some of the problems we know of with forking, and i think they all apply to suggested solution as well:

• migrating saved objects: if we are using a fork (or function is not registered) we can't run migrations on that function as we can't find its definition (which would contain migrations). save for extracting and injecting references. this is a blocker issue ifwe have a migration in place or one of functions (that are in fork or would be unregistered) references a saved object.

we do have functions in canvas referencing other saved objects ... all panels using them will all break with 8.0 unless we fix this

• running reporting: we don't support doing csv reports on panels/apps backed by expressions yet but its one of our top items. when enabled it will be cruical that reporting can execute the same expression that the panel/app did so all of the functions should be accessible in expression registry (not in fork, not unregistered)

• running alerting: alerting also doesn't support expressions yet but is also an important feature we are looking toward. Same here, alerting will need to run the same expression so all functions need to be available (not in fork, not unregistered)

• backgrounding search sessions: search sessions don't support expressions yet either but its our top priority to add that support. When added it will be just as with reporting and alerting crucial to be able to run same expression (so no functions in fork or unregistered)

apps using fork will not be able to support backgrounding search sessions, alerting, reporting and won't be able to properly support migrations and saved object reference extraction/injection.

unless we find a solution for private functions that would not break any of the above i would not be going forward with it.

[clintandrewhall]

A Quick Comment

Yea, this is a good point, and a bug, imo. That example plugin should have listed Canvas as a dependency if it required the specific markdown function to be available.

I think that's the disconnect here, @stacey-gammon: that example plugin didn't have a dependency on Canvas. It didn't import any of its code, require any APIs or anything else. The markdown function was invoked from a default string in a textbox.

Even if we look past that, unless the person writing the plugin and making the default of that textbox specifically knew the markdown Expression Function came from Canvas, they wouldn't know to make it a dependency. And your point about moving code around is also correct. So the dependency declaration is pretty much meaningless.

But worst of all: Canvas didn't know markdown-- or any of the functions it registers but doesn't export-- was being used anywhere, (much less in a simple textbox being piped to the Expressions Renderer). This is why we need to sort out how the Expressions Plugin registers, manages and provides Expression Language evaluation.

The anti-pattern in this case is unknowingly adding functionality considered internal to a global state to be used anywhere else. And this is why we need to fix the bug we have in Canvas, (see below).

But we also need to start organizing our truly common Expression constructs into packages or the Expressions Plugin itself.

Closing the RFC

Based on the comments here, and conversations elsewhere, I'm going to abandon this PR/RFC in favor of work proposed here:

• [canvas] Simplify and organize Canvas plugin lifecycle #105675

While a Nice Try™️ at keeping Canvas from polluting the Expressions plugin with its functions, I've come to believe that we need a much more holistic and philosophical solution to how Expression constructs are registered in (and persisted throughout) Kibana.

Canvas will sort the plugin lifecycle as defined in the issue above, but also fix a bug where Expression constructs are registered twice: once with its fork and once with the global plugin.

In addition, I'm volunteering myself to prep a "history of expressions and their use" to guide conversations in the Expressions Working Group, (cc: @petrklapka) I think we really need to revisit a few topics:

• which Canvas Expressions should be moved out of Canvas and into either a package or the Expressions Plugin?
• how do we "protect" Expressions constructs that are deemed internal or private beyond prefixes or obscurity?
• how do we register Expressions constructs to the global scope without penalizing plugins/chrome that don't or won't use them, (e.g. page bundle bloat)?
• how do we tackle the list @ppisljar has brought around alerting, reporting and others?

Thanks for the feedback here, all. While we still have some of the same issues we had when I started this RFC, the good news is those conversations have started.

[stacey-gammon]

We can't prevent a plugin from storing and exposing state/functionality passed to it via a lifecycle contract method. I think our stance should be:

• Plugins should assume any state/functionality passed to another plugin's lifecycle contract method is public.
• If a plugin needs a privacy guarantee, it should only use statically imported utilities, as we guarantee those are stateless.