[RFC] Handler interface

[epixa]

This is still a work in progress, but I wanted to post it since it overlaps with at least one other RFC that is in flight.

View Rendered RFC

[skip-ci]

[elasticmachine]

Pinging @elastic/kibana-platform

[epixa]

@joshdover Take a look at this. It's not completely done yet, but I think it overlaps with your application mounting RFC. Specifically, I think the mounting interface should function as a handler rather than us passing entire plugin contracts down into application business logic, as you mentioned here.

[joshdover]

Gahh woops, I hit "Ready for Review" on the wrong PR.

[joshdover]

@elastic/kibana-platform This is now ready for review.

[lukeelmers]

Handlers are asynchronous functions registered with core services invoked to respond to events like a HTTP request, or mounting an application.

Might just be me, but this RFC would be clearer with an additional example that uses handlers for application mounting or something besides HTTP requests.

As the "Drawbacks" section accurately points out, handlers as a pattern are commonly used for HTTP routes, but people might not necessarily associate them with other areas. A concrete example could help bridge that gap.

[joshdover]

@lukeelmers Additional examples have been added!

[lukeelmers]

those are super helpful & much clearer - thanks!

[joshdover]

@elastic/kibana-platform Still looking for approvals here. Once I have one approval, I'll put this RFC into the "final comment period" and will presume that it's good enough to accept.

[stacey-gammon]

How does context get created when the request argument is only known at the time of execution?

Like I'd think this route handler would have to be something like:

sync routeHandler(context, request) {
  context.getElasticsearch(request).search();
}

[epixa]

The context gets created on a per-request basis, so in this example each route handler would technically be getting access to a different Elasticsearch client that is already pre-configured for the current request.

@joshdover We should make this super clear in the RFC since it's pretty much the most important detail of this whole proposal :)

[stacey-gammon]

Looks nice! This is interesting from my perspective from dealing with actions and I'm wondering how it fits. If I'm following this all correctly, Handlers use context as well as optional additional arguments. The extra arguments are data that is only known at the time the handler is executed, whereas context is known and established at the time the plugins start up. Does that sound accurate?

My Actions are somewhat similar, but handlerContext type data would be passed into the constructor, and data that is only known at the time the action handler is executed, is passed in as a single triggerContext argument.

So it might look something like this:

// myaction.ts
class MyAction {
  constructor(context) {
    this.context = context;
  }

  execute(dynamicContext) {
    this.context.elasticsearch.search(dynamicContext);
  }
}

// myplugin.ts
setup({ core }) {
   const handlerContext = somehowCreateHandlerContext(core);
  actionRegistry.set(MY_ACTION_ID, new MyAction(handlerContext));
}

// somewhereelse.ts
// someone kicks off the action and passes in the data only known at that time
actionRegistry.get(MY_ACTION_ID).execute(dynamicContext);

So if actions want to use a similar Api it seems like the context should be passed in when it's executed... but then I'm still not sure how context gets created using the providers and the dynamic data.

[mshustov]

it's an implementation detail, but imo important don't forget to make context extension lazy (as a getter?) and execute this function every time when someone reads from context.[extension point] otherwise context may have stale version of the client.

[epixa]

I think context needs to be generated immediately before invoking the handler. The idea is that context does not change once its corresponding handler begins executing, so even if the ES client were to update at the service level, a handler that has already started executing would continue to use the original client until it finishes executing.

[epixa]

@joshdover We should call this out specifically in the RFC because it's an important detail.

[joshdover]

@stacey-gammon I've coalesced answers to your questions here, but let me know if I missed anything.

The extra arguments are data that is only known at the time the handler is executed, whereas context is known and established at the time the plugins start up. Does that sound accurate?

No, context is created right before a handler is executed by calling each context provider and constructing an object from the return values. I've added a section to the RFC that explains how and when context is created.

how args fits in to both the provider and the handler function. Are they the same or totally unrelated?

The arguments available to context providers will differ by service. In the HTTP example, context providers would have access to request but not toolkit because context providers cannot intercept the execution of the handler. (This is not middleware).

The service that implements the Handler Context pattern will have to define which arguments are available to context providers and which are available to handlers themselves.

is the http.router one the only one that uses args?

Unlikely, I'll add some more fleshed out examples in the RFC itself, but one off the top of my head is for the ApplicationService's mount handler. Context providers could be provided access to the App definition that is about to be mounted in order to provided scoped services. For example, a context provider that returns a Telemetry Client that automatically prefixes telemetry keys with the app's id.

I'm not sure I'm sold on this pattern vs passing context into functions provided on the plugin contract. All examples I can think of seem a little more straightforward if you just use the plugin contracts. Like it's only one pattern I have to learn about.

I think one of the main benefits of using this pattern over plugin contracts is that this pattern does not require the developer to know all of the available APIs in every plugin in order to get a useful set of functionality in a handler.

Without handler context, I think many developers would re-implement the same logic over-and-over (possibly with bugs) rather than being able to leverage TypeScript/Intellisense to explore what's available in the context argument.

[epixa]

Another benefit of this pattern is that you don't need to pass around core and dependent services throughout all of your business logic. Contextual handlers allow for service capabilities to be injected directly where you need them.

With this proposal implemented, we won't even need to pass things like the saved object client to plugin lifecycle functions because you'll be able to access it directly from your handlers.

[joshdover]

This RFC is now in the final comment period. If you have any comments about a fundamental problem with this proposal, make them now. Otherwise this will be accepted on Monday, June 17th.

[epixa]

Since this RFC is about a broad concept rather than a specific feature, I'm fine proceeding with this RFC as-is, but this detail is an important problem to address for implementations. Realistically, I think this means that core must provide plugin developers a mechanism to build a context since by nature a plugin won't even know about plugins that it does not depend on, so they can't properly construct the context themselves.

[joshdover]

Agreed. We may actually need to expose a service to plugins that allows them to offer context registration and construction. This service would handle the dependency management aspect of constructing context.

This needs to be built before any plugins begin offering context in their handlers.