-
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] Removing RequestHandlerContext from the low-level data.search API. #78461
Comments
+1 for the approach where scoped clients are passed in const results = await data.search.search({
uiSettings: myScopedUiSettingsClient,
esClient: myScopedEsClient,
}, searchRequest, options); I think it was already the plan that Alerting will create those scoped clients and pass them in (@mikecote correct me if that is wrong). Also, I like this approach because it decouples the search service from knowing anything about the Kibana request object and Kibana context objects, which seems like a big win. As @lukeelmers points out, it is not even clear how to construct a "fake" Kibana context object. And receiving Kibana request object is a too tight of a coupling to the Kibana core, it is not even a generic "request object", it is specifically an HTTP request object, and making the search service be consumable only in presence of an HTTP request object seems too limiting. And, as also @lukeelmers mentions, Alerting is not even using a real HTTP request object, they are "faking" it. Thus if there is any incompatibility or if in the future one would construct the scoped clients in some other way (not from request objects), we will require changes to the search service, whereas if the search service receives just the clients from the beginning we avoid all those potential problems. Basically would suggest something like this, where it is up to the consumer to pass in scoped or not-scoped UI Settings and ES clients: const client = data.search.createClient({ uiSettings, es });
const results = await client.search(searchRequest, options); Consider testing the search service, if the search service receives directly the scoped clients, you create test doubles for them and write your tests, done. If, on the other hand, the search service receives a Kibana request, then in your tests you will need to mock that request instead, then somehow make sure that that request is transformed into the scoped clients internally, but in unit tests there is no way to run the core which would transform requests into the scoped clients, so it would be just two layers of mocks. Also, even if we decide for search service to receive the Kibana request, I would suggest considering to make an intermediate step where that request is converted to scoped clients and search service receives just the scoped clients: const client = await data.search.createScopedClient(request);
// which internally calls:
public createScopedClient (request) {
const so = core.savedObjects.getScopedClient(request);
const uiSettings = core.uiSettings.asScopedToClient(so);
const es = core.elasticsearch.client.asScoped(request);
return this.createClient({ uiSettings, es });
} |
i think passing scoped clients in might be fine for places we know we can limit this to just a few, but with search requests devs can add their search strategies, and we don't want to limit them access to services. That's why I think we should go with first proposal (passing KibanaRequest in) here, but we may consider passing scoped clients in some other places. |
Correct, we are currently passing in scoped clients which shouldn't be a problem (non-breaking change). We are also planning to expose the fake request to the alert executor in scenarios they want to scope something else (ex: ML client). |
++ These are both excellent points, and are good reasons to lean toward the approach of passing in scoped services (aside from the fact that it is also more explicit)
Yeah I guess we are really talking about two things here:
For 1) I am thinking I would lean toward passing the scoped services, otherwise you will get to a point where the interdependencies between services essentially require passing the whole request all the time (example: Both index patterns & field formats require the whole For 2) I could go either way. I don't think there's anything inherently wrong with limiting what scoped services are available to search strategies as we can always add more over time. However you could also make the argument that the search service is primarily an abstraction over the ES client, which requires passing a TLDR: My vote at this point would either be: A) passing scoped services everywhere, or B) passing scoped services everywhere except for the search service, which would work with a raw Either way it seems there are no concerns with removing the |
I'm definitely behind passing in services. Modifying index pattern services would be easy and it looks like the field formatters service already works this way. I'm not familiar enough with search strategies to have an opinion on it. It does sound like a special case. |
Ahh, you are right -- I was mistaken. Thanks for pointing this out. |
I'm going to go ahead and close this issue now, since it looks like we have a plans in the low-level search roadmap to go with I believe the only other service we'd need to change at this point to explicitly pass in services is index patterns. If anyone has any other comments/concerns, feel free to re-open the discussion. |
The problem
While working on exposing SearchSource on the server, I noticed that the low-level search service requires
RequestHandlerContext
to be provided each time it is called.As far as I can tell, this is only used for two things in the service:
Based on the way search strategies are designed, this entire context also gets passed to each individual search strategy.
While this makes things convenient for folks building custom search strategies, it is going to make things more complex for services like Alerting and Reporting, which rely on creating a "fake"
KibanaRequest
in order to authenticate on behalf of a specific user. Given the current design of low-level search, Alerting/Reporting would need to take the additional step of constructing a fakeRequestHandlerContext
as well.While creating fake context doesn't sound too complex, it actually raises a bigger problem around search strategies: How can you properly fake a request context when any custom context can be registered? Since we have no way of knowing which parts of the
RequestHandlerContext
search strategies are relying on, we risk breaking those strategies by not having the entire context mocked.In fact, this problem already exists in the
SearchSource
implementation, where context is being faked: If there's a search strategy which relies on anything but esClient or uiSettings, they won't have those items available in the request context when accessed viaSearchSource
.Why worry about this now?
Currently all of the search strategies registered in Kibana are in the
data
ordata_enhanced
plugins, however we know that work is being done to add an EQL strategy, while other teams are making plans for migrating to the search service to use async search.If we are going to make a change to the interface, now is the time to do it before adoption increases -- otherwise it will become very difficult to make this change.
Proposal
Here's my proposal which @elastic/kibana-app-arch discussed offline and we generally seem aligned on:
Since core's
RequestHandlerContext
can already be derived from aKibanaRequest
, let's adjust the API to only take in aKibanaRequest
, and use that to access any other scoped services that are needed.Internally, this means that instead of accessing esClient and uiSettings like this:
We would access it like this:
And each search strategy would receive the
KibanaRequest
instead of theRequestHandlerContext
, and therefore could use the same approach as above to access the various scoped services that they need.We could also provide a top-level
asScoped
method on the server, similar to how core services are designed, so that you wouldn't need to provide the request with every call tosearch
:Alternatives
An alternative would be to stay away from
KibanaRequest
altogether, and instead require every consumer of the service to provide their own scoped services:This delegates more work to the consumer of the service, however it is also the most explicit way of handling the dependencies if
KibanaRequest
feels like overkill.Another option would be to still take in the
KibanaRequest
, but limit what is passed to the search strategies to only include the scopedesClient
anduiSettings
. This is similar to what the Alerting service does in alert executors, where only a specific list of services get injected, and more can be added over time as needed. This also carries the benefit of "saving" folks from registering custom strategies from doing the extra work to access scoped services as outlined above, since we would be doing it for them. The tradeoff is that it's more restrictive: With aKibanaRequest
they can pretty much do whatever they want in a search strategy, while a fixed set of injected services puts them in more of a box.Next steps
The goal of this discussion is to get alignment on two things:
After we have alignment, it will be a matter of adjusting the service itself to accommodate these changes. The sooner we do this, the better, as we expect usage of the service to become more widespread in the near future.
cc @elastic/kibana-app-arch @elastic/kibana-platform @stacey-gammon @kobelb
The text was updated successfully, but these errors were encountered: