Interpreter plugin imports from other plugins

[timroes]

Currently several new visualization expression functions (https://github.com/elastic/kibana/blob/master/src/legacy/core_plugins/interpreter/public/functions/) import from another plugin via import. The effected functions:

• tilemap
• timelion_vis
• tsvb
• vega

That is an issue, since that means Kibana won't start up anymore if you disabled any of the respective plugins. So e.g. if you put metrics.enabled: false in your kibana.yml to disable the TSVB plugin, trying to start Kibana, will now resolve in the following error, (when trying to access the page):

ERROR in ./src/legacy/core_plugins/interpreter/public/functions.tsvb.js
Module not found: Error: Can't resolve `plugins/metrics/kbn_vis_types/request_handler` in ...

That would be a breaking change and not the desired behavior. We basically have two possible solutions.

1. Make sure the actual interpreter functions are being placed inside the plugin they belong to, i.e move the timelion_vis function into the timelion plugin, etc.
2. Make sure that the code we import in those functions, is not directly imported, but instead use some kind of code sharing mechanism (similar to server.expose, but for client side code), so that it will continue working even if the plugin is disabled.

We need to investigate further how those options actually work out with the way interpreter functions are found and how that import block with disabled plugins actually work, e.g. if we would move the timelion_vis function into the timelion plugin, and disable that via kibana.yml, would the interpreter plugin still find the timelion_vis function and the import in it to it's own plugin would still fail?

cc @stacey-gammon @ppisljar @lukeelmers

[elasticmachine]

Pinging @elastic/kibana-app

[ppisljar]

i prefer the first option, however currently, canvas plugin system doesn't check if plugin is enabled or not ... if it will find its files it will load them.

[stacey-gammon]

Yea, sounds like this is going to be an issue for canvas too. Code from a disabled plugin could potentially be executed. I had a chat with @epixa about this and we thought maybe this would be an option:

Using the legacy plugin system.

• Update the kbn interpreter to be able to take function definitions as an array of arguments at runtime. - When it gets them at runtime, it includes them, then
• For the 8 offending functions, we could define them using legacy system, uiExports whatever, then inject them into the interpreter.
• Just expose kbnInterpreter.addFunction function to push a function on that stack

But, it's a lot more work to also handle the canvas ones since it's way more than 8 bad functions, or whatever the number is. Other options we considered:

• Option 1: Rush forward New Platform client side plugin work, try to rush forward the migration
  • likely to be a large amount of work with a lot of dependencies still on the platform team.
• Option 2: Migrate just the offending expression functions (vega, timelion) to New Platform server
  Would have to be server side because client side isn’t ready, so this option is out.
• Option 3: Revert the new visualizations using canvas pipeline.
  • I actually thought the biggest reason not to do this was the breaking change going into 7.0, that's when I realized that the breaking change actually isn't going into 7.0, (which spurred this separate discussion: [DISCUSS] Visualization saved object migration plan #29382)

Option 3 would suck because I know there has been a TON of work to incorporate this and reverting it would probably also be a huge amount of work... although this is mostly speculation on my part. @ppisljar - is there any easy path for adding like a flag to still use the previous implementations or has that code been pretty much entirely thrown out and replaced with the new system? #25996 is a pretty big PR but a lot of it seems to be new functionality. So I figure I should at least ask the question for cost of effort involved, just in case we can't find a decent hack.

[epixa]

It's worth mentioning for those that missed it: we've agreed to move canvas plugins to the new platform in early 7.x. Possibly 7.1. There are details to be figured out, but it seems very doable.

So we don't need to rearchitect the world to solve this problem. That's one of the reasons why a temporary solution like moving the handful of offending functions into the legacy plugin system is appealing.

[stacey-gammon]

one of the reasons why a temporary solution like moving the handful of offending functions into the legacy plugin system is appealing.

Yea, but @epixa, it's not a handful if we have to solve it for Canvas too, it's a ton.

Maybe it wouldn't be considered a blocker since Kibana starts up, but it would mean that it's possible that Canvas code executes even if it's disabled. Granted, you'd have to really try to get there. Esp since the saved object migration isn't happening in 7.0, you couldn't just go in and edit the visualize saved object. You'd have to like... create a whole new plugin that specifically uses the expression language and interpreter and then executes a canvas function. Perhaps given how unlikely that is, it's okay to ship 7.0 like that. And at least none of our plugins would be executing canvas code. I think I convinced myself it's okay to do the hacky legacy solution.

[ppisljar]

we'll take a look at using legacy plugin system for mitigating this problem, i'll update this issue once we know more.

[ppisljar]

i wanted to mention, that in my first attempt to move interpreter to OSS, i used legay plugin system to some extent, where every legacy plugin could register paths that interpreter plugin system would then scan.

However that was then said to break some major features of canvas (loading canvas plugins without restarting kibana).

[stacey-gammon]

Thanks @ppisljar. I'm trying to find the history of that conversation but having a tough time. @elastic/kibana-canvas - can someone update here with any additional issues with that approach?

How important is "loading canvas plugins without restarting Kibana"? In the notes on the Canvas plugin system, it seemed like this is only for client side only functions? I see:

For server, requires a restart.
[Joe] may not need to reboot for serverside functions. [Rashid] unsure…
This is an open question.

Then @monfera mentioned in a comment that common functions also require a reboot right now.

If that's the case, it doesn't really sound like requiring a reboot is that big of an issue?

Were there any other issues with using the legacy plugin system? If the reboot thing is the only issue, any objections to using legacy anyway?

[stacey-gammon]

Should be closed after the bundling PR @chrisdavies and @ppisljar did.

[ppisljar]

Why ? Bundling didn't really affect this. We moved away from canvas plugin system, but we never solved this problem.. interpreter is still importing from other kibana plugins. Disabling timelion will most likely still prevent kibana from starting.

[stacey-gammon]

Disabling timelion will most likely still prevent kibana from starting.

Only if the expression language is turned on, right? This sounds like a blocker otherwise.

My mistake then, I thought bundling solved it, I'll have to dig further into how it works. We would like to move all the package code into the core plugin anyhow.

[ppisljar]

i am not 100%, would need to check out, but anyway this should be solved as part of stabilization for 7.0

[chrisdavies]

I would think this was solved as part of the bundling process, too. But I haven't verified.

[ppisljar]

in interpreter core plugin functions still reference other plugins (like timelion function references timelion plugin), which will crash server if those plugins are not enabled. Doesn't happen with the switch off but does with the switch (use pipeline in visualize switch) on.

currently i am blocked on registries being in the package