-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[discuss] Provide a single context service for different handlers #46483
Comments
Pinging @elastic/kibana-platform |
It'd be nice to see the context pattern used more in practice before expanding it's scope and usage like this. I suspect something like this is necessary, but it's going to add layers of complexity to an already-complex concept that we've mostly just reasoned about on paper. If you do go down this route, we should consider a mechanism that allows plugins to compose contexts together rather than the interface of the context service becoming a generic registry. This would allow a plugin to say stuff like "I'd like to create a new context for my extension point that has this extra feature composed onto the features of the HTTP request context", which is different than hardcoding the notion of "scoped" into the API of the context service itself. |
On the frontend, we already have quite a few different places that either have context or should have context in the future:
Right now, if one plugin wanted to add a new context property, they'd need to register with each of these. In addition, if a 3rd party plugin wanted to provide context to its own handlers, they'd need to set up registering each context provider themselves. All in all, I just think that we're trying to solve the service discovery problem in a way that may be more complicated than we need it to be. |
Thanks for opening that issue. As discussed yesterday that's something we would highly appreciate.
This might be a hen egg situation here, because we currently would rather avoid using context because of the above described issue. It would actually solve some of the setup/start lifecycle issues and allow us to register in I would also not necessarily agree with the system getting "more complex" by that change, rather the opposite. In our discussion yesterday we also tried to figure out how many different contexts there might be, and couldn't come up with any other example then the 2 on the serverside (bound to user/unbound) and one on the client side (always bound). |
OTOH, what if a plugin wants to provide API only to a specific extension point?
Does |
@restrry Do you have an example for that case? Because providing something into a context is anyway an API of that plugin, I currently don't see any reason why we the plugin providing that API would want to limit that for a specific consumer (context extension point), especially given that available context extension points will change over time, so meaning every provider must decide with every new extension point if it would want to provide it there or not.
Maybe not right now, this was more meant on a "theoretical" basis. I meant, that everything running in the browser HAS a user, so in contrast to the backend side, there isn't at the same point in time a difference between contexts that are user bound and ones that are not. Sorry I am having a hard time describing that exactly, maybe @joshdover can help out here with better words :D |
Heads up: I haven't forgotten about this and do intend to respond, but I need more time to think it through. @joshdover what sort of timeframe are you looking to make a decision in? |
@epixa I think it'd be good for us to make this change early in the 7.6 development phase if we're going to do it. We're expecting that most plugins will be consuming NP services at this point. |
Can someone provide me concrete examples of where they think shared context is helpful? I think the challenge I'm having in understanding the value here is that I don't have real world examples. @joshdover listed a bunch of context handlers, but I'm not sure why we'd want to share services between them. A couple example interfaces may help me, or at least some example APIs that we'd want to make available in a bunch of different places. The quote The reason I feel something like this might be necessary at some point (maybe now) is for the second problem: if I'm creating a new API that I want to make extensible, how do I leverage the contextual capabilities that are available elsewhere? So for a concrete example of that, if we stop passing Elasticsearch clients to plugin setup, how does a plugin expose a contextual API that includes an Elasticsearch client? e.g. How does the alerting plugin expose an elasticsearch client to alert handlers that is already configured in the context of the user that created the alert? |
I agree that this change would not fix a bug, but would be aimed at reducing boilerplate, duplicated logic, and provide a more consistent development experience. As it is now, plugin developers are at the mercy of both service owners (core services or plugins that "own" handlers and it's associated context) and context owners (core services or plugins that provide context services) to wire up each N context service to M context owners. The way I can see this playing out is that devs may frequently run into issues where context service A (eg. Having a single registry of "render context" may help in this situation, since I expect many handlers responsible for rendering UI have the similar semantics and it's likely consumers would expect the same services to be available in each. However, we haven't had this issue pop up yet, so maybe we're getting a little ahead of ourselves here. |
I think I might be hitting a few concrete use cases where this would greatly simplify my experience. Situation:
So, implementing this right out of the box, I get it wrong. Same situation I hit when developing the Josh told me the way to fix this is to have Just my 2 cents. Maybe once the search plugin gets merged, and maybe this living documentation plugin as well (which is a bit of a side endeavor right now, so we'll see what happens with that), we can have some more concrete examples of where this proposal would simplify a lot of plugin interaction. Oh and I also suspect management will hit the same thing since management page is just like my living documentation plugin. |
Alright, I'm going through using
So I start trying to set up the connections in
But
or I think this is the main point @timroes was making. The consumers of this context are relying on the producers of this context to know how it will be consumed. I don't think this will play well when we introduce third party development into the mix. Anyway, maybe there is still a work around I don't know about, but I will sync back up with ya'll on Monday and see if we can figure it out! Related PR with this exploration is here: #47337 it's not in a compilable state though, I just pushed my interim changes of trying to use |
I would be curious to see an example which involves multiple plugins. Like a 3 plugin example: (1) |
My concern with this proposal remains unchanged: I think this is contrary to the purpose of context. The per-API nature of context was intentional, and despite trade-offs I see it as a valuable thing. That said, my concerns are mostly theoretical and philosophical, and I'm not actively involved in building or supporting the context service, so if there's a strong consensus between the platform team and the consumers of this service about how it should evolve, I accept that. I only have one deal-breaking stipulation: it is imperative that plugins only get access to features from other plugins they explicitly depend on regardless of whether context is constructed per API or whether it is shared across APIs. It's not clear to me whether anyone's proposed otherwise, but it's important enough that I wanted to mention it just in case. |
This topic came up in a discussion between @flash1293 and me yesterday, and after talking about the current state and this suggestion, Joe brought up another imho legit question I wanted to raise here: Leave aside the "user bound context" on serverside, but just giving the client side and serverside (non user bound) context. Is there a specific reason why we at all separate context and plugin API still? Couldn't we just fill the context with all the plugin APIs contracts instead? I find that also worth discussing.
I find that an interesting topic. And I think the context should only contain "dependable" API... but my question is: depending by whom? The plugin that "offers" the context or the one that consumes it? My personal feeling is that the latter would make more sense, since if it just contains APIs the |
Edit: This is responding to the second half of Tim's comment. @timroes I don't have a comprehensive answer in the context (no pun intended) of a shared expression service, but I can respond in terms of the original goals of context and hopefully that is still relevant enough to be useful. Without shared contextThe interface VisContext {
visualizations: VisApi;
elasticsearch: CoreElasticsearch;
} The interface EnhancedVisContext extends VisContext {
enhancedVisualizations: EnhancedVisApi;
} The The With shared contextWith shared context, the interface VisContext extends SharedContext {
visualizations: VisApi;
} When only considering core services (which are always available to everyone), then the only real change here is that there's less work for the visualizations plugin to create the original VisContext. With plugins, So let's assume for a moment that |
After seeing this play out in practice in a few different areas, I think we're seeing context on the client-side being used very differently than on the server-side. Server-side contextOn the server-side, we're using context providers to:
This makes the context providers on the server a bit more complex (and useful) than just the raw APIs they're built on top of. It also means they're more specific to their usages. For instance, a context provider for HTTP routes would be different than a context provider for a background task. Client-side contextOn the client-side, things are a bit different:
Ultimately, many of the motivations in the original RFC don't apply to the use-cases we see on the client-side. Since we're using context providers in such a simple way on the client, it does feel like a lot of unnecessary complexity just to get access to some core APIs. Alternative: Service binderThis leads me to believe that maybe what we really need on the client is a simple way to bind Core and Plugin APIs to a function: interface CoreSetup {
bindServices<T extends (...args: any[] => any)>(
callback: (core: CoreStart, plugins: object) => T
): T;
}
class Plugin {
setup(core) {
core.application.register({
id: 'myApp',
mount: core.bindServices(
(core: CoreStart, plugins: MyPlugins) => ({ element }) => {
// normal app mounting code here
}
),
});
}
} This Advantages of this approach:
Basically this Disadvantages:
Questions:
|
I have several concerns regarding the
Can we get by with existing means? Say use late binding. class Plugin {
setup(core) {
core.application.register({
id: 'myApp',
mount: this.onMount,
});
}
start(core, deps){
this.startContract = { core, deps };
}
onMount = ({element}) => {
const startContract = this.startContract;
if(!startContract) throw new Error('not started yet');
// normal app mounting code here
}
} |
@restrry what you suggest is what I'm currently doing in #47469. Essentially it's the same thing service binder would be doing but you have to implement it yourself in each app plugin. I see this more as a small convenience function than anything else. We could also hide this behind a specialized Plugin class - but of course classical inhertiance has its own problems. I definitely prefer the binder-approach. class MyPlugin extends AppPlugin {
// will be used automatically in the register call done in super.setup
appId: string = 'myApp';
setup(core, deps) {
// this will call application.register and save the deps on a local property
super.setup(core, deps);
}
start(core, deps) {
// this will save the deps on a local property
super.start(core, deps);
}
onMount(element, params, startCore, startDeps) {
}
} |
You could potentially combine the two approaches to re-expose the start contract to the bound setup handler as well. class ExamplePlugin implements Plugin<ExampleSetup, ExampleStart> {
setup(core: CoreSetup, plugins: SetupDeps) {
core.application.register({
id: 'myApp',
mount: core.bindServices(
(core: CoreStart, plugins: StartDeps, example: ExampleStart) => ({ element }) => {
// normal app mounting code here
example.startMethod();
}
),
});
return { setupMethod() {} };
}
start(core: CoreStart, plugins: StartDeps) {
return { startMethod() {} };
}
} This would also make the assumption that a registered application wouldn't mount until it's |
@eliperelman That would have the side effect that |
My proposal is that only the
I think we would need to ensure that the ApplicationService does not execute until after all plugins have executed their As it is now, this is already true because Core's RenderingService does not start executing until all plugins have finished
I am proposing that we replace context on the client-side with this concept. I believe it is easier to use and find than the manual Other benefits of this approach over the manual
|
Alternatively, we can make |
Today we decided to move forward with the |
With the current implementation of Handler Contexts, each Core service or plugin that wants to expose context must have its own
IContextContainer
which requires that each context provider register with each context container in the system.In effect, this means N context providers must register with M context containers. This can easily lead to situations where the services available in one context are not available in another.
Many of these usages of context overlap. For instance, on the frontend, it's likely that all contexts would want to make available the same services. Rather than forcing all context providers to register with all services and plugins that want to expose context to handlers, we could have a single set of context providers for the entire frontend.
Examples
Before
After
Considerations
core.context.registerScopedProvider
and acore.context.registerProvider
for these two different situations.The text was updated successfully, but these errors were encountered: