-
Notifications
You must be signed in to change notification settings - Fork 8k
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
Expose serverless Elasticsearch client from core #169674
Comments
Pinging @elastic/kibana-core (Team:Core) |
Sharing my opinion here:
This is ihmo the correct design, but, I think it's important to note that we would need a common interface for the subset (intersection) of the APIs that are exactly the same between the two environment Otherwise it will become a nightmare for developers. (which is why this is something that needs to be discussed/confirmed with the client team as soon as we can ihmo) So if we assume we have 3 interfaces total (don't mind the names):
That way, most usages of the client could be done without needing to know the current environment. Developers can just use the const client: CommonClient = core.elasticsearch.getClient(); // returns a `TraditionalClient` - note that `getClient` would likely be deprecated in favor of `getTraditionalClient`
// do anything in any env given the interface only has APIs compatible with both envs.
client.someCommonApi() And then use either of the specialized interface when they know they're facing a specific env: const client: = core.elasticsearch.getServerlessClient() // returns a `ServerlessClient`
// I did not force cast to CommonClient, so I can use common + serverless APIs
client.someServerlessApi() just for sugar, we could even introduce a third API, const client: = core.elasticsearch.getCommonClient() // returns a `CommonClient`
// can only use common APIs (or at least only common APis are defined on the interface)
client.someCommonApi()
So, just to make sure I understand, it would mean having a single interface and a single implementation (right?), with all APIs, both serverless and traditional, and let the plugin owners having the responsibility to know when it's safe to call serverless-only and/or traditional-only API. The and a single implementation part is important ihmo, as otherwise it seems very developer error prone (having nullpointer exception when trying to do If we can have a single implementation for this, then even if less elegant, I agree that it could be an option. In the end, the developers will have to know if they are in serverless or traditional environments anyway when manipulating env-specific APIs, so maybe the way is to just expose everything from a single interface (which is technically what we're doing today - we have a single client exposing some APIs that are only working against traditional ES) |
It looks like now we have a Serverless-specific ES client to use: https://github.com/elastic/elasticsearch-serverless-js We might be able to take this task now. |
The initial idea was the to be able to use a common and 2 specialized interfaces, as described in #169674 (comment). I'm not sure this is the approach that was taken in https://github.com/elastic/elasticsearch-serverless-js, as I don't see anything closely or remotely looking to a "common traditional and serverless client interface" in that package. which would mean we are back on square zero: finding a solution on our own to put squares into triangles and circles? |
Q: Why do we need 3 clients? Common + 2 specialized? I naively think this is automatically done by TS as in: type ESClient = ClassicESClient | ServerlessESClient;
function myFunctionThatCallsES(esClient: ESClient) {
await esClient.aCommonAPI(); // TS is OK with this
await esClient.aServerlessSpecificAPI(); // TS complains because it might not exist if `ESClient` is `ClassicESClient`
if (isServerlessClient(esClient)) {
await esClient.aServerlessSpecificAPI(); // TS is now happy
}
} Am I missing any point? |
I was taking a look at the serverless client, and I noticed one important gotcha: import { Client as TraditionalClient } from '@elastic/elasticsearch';
import { Client as ServerlessClient } from '@elastic/elasticsearch-serverless';
const ClientConstructor = flavor === 'serverless' ? ServerlessClient : TraditionalClient;
const client = new ClientConstructor({ ...clientOptions });
client.cluster.stats(); // TS claims that the API does not exist
if (client instanceof TraditionalClient) {
client.cluster.stats(); // TS is now happy
} This is exactly as we expected... However... the truth is that API is supported by Serverless, but only to internal clients like Kibana. The Serverkess client correctly hides internal-only APIs to avoid customers hitting a dead end when trying to use an API that isn't supported in that offering. Another example is the request our ES service uses to validate the connectivity and compatibility of the ES nodes: client.nodes.info({
filter_path: ['nodes.*.version', 'nodes.*.http.publish_address', 'nodes.*.ip'],
}) We may need to instantiate the traditional client and expose it as very-internal, trust me, I know what I'm doing to cover these scenarios. |
The more I read about the current serverless client the less convinced I become that we should be using it directly in Kibana. The tension I'm seeing is in the intentions. The specs that build the serverless clients:
The problem is the current serverless clients only reflect (and implement) the first point. This means we need some special knowledge of which excluded functionalities are hidden/removed/changed on serverless in order to preserve them. I'm not sure giving access to the traditional client as a "super client" adequately covers this case. It does know about "internal" serverless endpoints (which is "good enough" right now) but it won't know about any changes to their req/res/options or which are truly removed. [EDIT] So it would be the "trust me i know what I'm doing" client, but we may be fighting TS and can already do "super user" using the transport directly. It might be simplest if we (not necessarily Kibana folks) leverage the knowledge in the elasticsearch spec repo to find all APIs that are IMO exposing this as one |
++ I'd love it if the serverless client provided a way to use the internal APIs (maybe under At the same time, I'm not sure having While we come to an agreement on the best way to expose the internal APIs, I think leveraging the traditional client workaround for them is safer than what we have today: We'll definitely need a follow-up on those internal usages to adapt them to an actual client that supports Serverless internal APIs. But I think it's best if we don't delay the introduction of the Serverless client for this reasons. WDYT? |
Yeah I see your point. I suppose with the type union we have the base intersection covered. Anything that is not compatible with the union you'd have to do a type narrowing before using the client. And exposing a bypass client in some way that is trackable does make sense. Overall I'd still prefer avoiding this work of the "trust me I know what I'm doing client", but perhaps you're right about the immediate benefit being worth it. |
I'm working on a PoC to know if there are other gotchas like if both clients are not in sync, you have a lot of issues in the common APIs. I'll see where it takes me. |
There are no plans to include anything internal in the serverless client, as it is intended primarily for public use. I had a conversation going with @lukeelmers a while back about whether or not a third Kibana-specific version of the client would ever be necessary, especially once a separate serverless client was deemed necessary. We kept putting off the decision because it remained unclear whether the effort was worth it. From my perspective, it would be ideal to not have to generate even more clients. Whenever I add a feature to the stack client, I have to figure out if/how to port it to serverless. Doing so usually takes an extra day of porting and testing each change. So a third client would increase that per-feature overhead even more. There are many different ways we could solve this problem, and I'd love to get a better understanding of all your needs regarding internal APIs to see if there's a solution that doesn't add much extra overhead. |
Well, simply put, Kibana is by right allowed to use those internal APIs (the kibana user has operator permissions), and is using themin practice (both in Core/Platform functionalities, and from some plugins - management plugins coming to mind first as obvious rightful callers). So the question is: how do we get to get an interface that trully reflects which Serverless APIs Kibana is allowed to call with the right API signatures? Because (and correct me if I'm wrong) atm the divergences between the traditional and serverless clients are just APIs being fully removed from the serverless one, right? So for now, the traditional client is a superset of the serverless one (still right?), so in theory we can simply keep using he traditional client's interface for serverless, for now. However... We assume (and here too, please correct us if we're wrong, we really have no visibility on what you're all intending to do with that serverless client - this discussion being a proof...), that at some point, the divergences will become more important (params/outpout properties differences - such as the "version" field being removed from stats on serverless, signatures mismatch, and so on). At that point, we would no longer be able to safely use the traditional client's interface when performing requests against serverless. But we would still need to be able to perform calls to internal APIs, so using that "public facade" version of the serverless client would still not be viable... So yeah, I return the question: what should we all do 😄? |
Thanks for your response @JoshMock !
Are you thinking we could minimise effort by providing the subset of internal serverless APIs Kibana uses? It's tricky to introspect which ES endpoints Kibana uses, not the least because current/past usage is not a reliable indication of future usage needs. Or are there approaches you had in mind?
Ideally we'd have a way for Kibana developers to get a reliable indication of available ES APIs (and their options, once/if they diverge). Leveraging TypeScript is for sure the best way to do this. I think if we can just get to a place where we have the full interface of available APIs for serverless we can build it such that if they ever diverge we can detect it. Perhaps taking some kind of manual action on Kibana's side (as we currently have a precedent for this anyway, but usually as a temporary measure until clients are released). |
Also some properties have been removed from serverless. e.g. So, it is already the case that you can't trust the stack client's types to accurately represent the same type of request on serverless. That said, there is an expectation that the stack client should continue to work when pointed at a serverless client. Missing APIs will return a My current idea is to provide a separate library for Kibana that does not include its own client implementation, but rather patches |
This would be awesome! Are you imagining for this patching logic to live in Kibana? |
It doesn't necessarily have to. A standalone package could in theory just add functions to |
Got it. I was thinking of any reasonable measure that'll help remove the burden of maintaining the "Kibana client". How much effort would this be? Happy to meet and chat about possible approaches 😄 |
Going to meet with @jloleysens and @afharo next week about Kibana internal API support. 🚀 |
Moving forward, there will be two flavors of the Elasticsearch JS client: the stateful client that exists today which aims to be compatible with APIs that are shared between stateful and serverless, and a stateless client which will be able to talk to ES APIs that are only available in serverless Elasticsearch.
Currently Kibana isn't using any Serverless-only ES APIs that would require the stateless client, but it is expected that we will have a need for this over time.
We need to define and implement a strategy for handling these two different clients from within core.
Broadly there are at least two directions we could take here:
The text was updated successfully, but these errors were encountered: