Data plugin: Suggested enhance pattern

[lizozom]

Summary

This PR is a suggestion for a simple pattern to enhance an OSS plugin from x-pack.

Related to #62998

The term "enhance" refers to cases where the x-pack plugin changes the functionality or expands the API of an OSS plugin. It does not refer to cases where an OSS plugin has a registry that multiple plugins can register to (i.e. search strategy registry).

How does it work?

• The OSS plugin exposes an enhance function on the setup contract.
• The enhance function enriches the plugin by using passed in enhancements or passes them down to sub-plugins, by calling their enhance function.
• The corresponding x-pack plugin should call enhance function from it's setup contract. (This is important because enhancements should be done, before consuming those services).
• If any enhancement requires a dependency from the start contract, it should use core.getStartServices internally.

Empty APIs

Another part of the suggested pattern (but is not demoed here), is explained in detail by @streamich in this comment.

To allow OSS plugins easily consume those enhanced APIs, an OSS plugin may expose empty functions that do nothing in OSS (return undefined \ null \ false \ throw an exception, depending on the usecase). The enhance function will provide the implementation for those functions, hence enabling those features.

Checklist

Delete any items that are not applicable to this PR.

• Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• Documentation was added for features that require explanation or tutorials
• Unit or functional tests were updated or added to match the most common scenarios
• This was checked for keyboard-only and screenreader accessibility
• This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
• This was checked for cross-browser compatibility

For maintainers

• This was checked for breaking API changes and was labeled appropriately

[elasticmachine]

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

[lizozom]

@elasticmachine merge upstream

[ppisljar]

not sure how different this is from what we are currently doing, except that if we want many overrides we can do them all in one method. (so its easier to add more overrides)

[ppisljar]

should it take in Partial ? will we be providing enhancements from different plugins ? (one plugin providing search enhancement and another one providing index pattern enhancement) or do we want to limit it to single plugin ?

if so, lets add a check to make sure we are not calling this twice (and throw)

[ppisljar]

also, lets mark this as internal, i would prefer if we keep it under separate key to make it very clear
data.__internal.enhance()

[ppisljar]

another taught: should we rather provide this functions on per service basis ? (data.search.enhance()) or even on per item basis: data.search.setSearchInterceptor() (which is what we are currently doing)?

[lizozom]

1. I don't think we want to take in a partial because this is a pattern for a plugin enhancing it's own matching OSS plugin. Not for a registry or so.
2. I agree about making it internal. I think calling the function data._enhance should be enough.
3. We do have it in a per service basis (to propagate the enhancements). However, I think that one of the benefits of having an enhance method rather than a setInterceptor method, is the explicitness of it.

An additional change here, is that enhancements are managed during the setup phase, while before we enhanced during the start phase (which was giving me trouble in the Background Sessions PR. IN the same PR I also have multiple enhancements, getting actual value from the pattern suggested here.

[lizozom]

@elasticmachine merge upstream

[ppisljar]

code LGTM

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 73208e9

Build metrics

page load bundle size

id	value	diff	baseline
data	1.4MB	+1.2KB	1.4MB
dataEnhanced	178.0KB	+34.0B	178.0KB
total		+1.2KB

History

• 💔 Build #67708 failed ef12dfb
• 💔 Build #67704 failed c743e25
• 💚 Build #67457 succeeded f2871fa
• 💚 Build #67183 succeeded 0b95e19
• 💚 Build #66987 succeeded d8383db

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

[lukasolson]

Code LGTM. Pulled down and tested OSS/trial license and things were functioning as expected.

[stacey-gammon]

As far as a generic pattern goes, this is a bit hard to follow, since the types are specific to the plugin, but here are my initial thoughts:

I don't think we'll be able to come up with a generic solution that fits every use case - many of the use cases are very situationally dependent. For example - the embeddable enhancement is an enhancement to a registry item, and can add state, so needs to provide a migration function. This kind of enhancement pattern I tried to capture here. It's somewhat generic but I don't think it extends to the use case here because you aren't extending instances that belong to a registry. You are intercepting a function call (if I'm following correctly), so no possibility of manipulating state, so no migration concerns.

@streamich - I think your diagram here:

is supposed to be what the current enhancements does. We should support multiple "enhancers", and you could do something like:

Which is what you are getting at too, with your diagrams? If so, I think we are on the same page. The problem now is that only one enhancer is supported, which I agree should probably change.