ESQL: Default ESQL version on old clients#107438
ESQL: Default ESQL version on old clients#107438elasticsearchmachine merged 4 commits intoelastic:mainfrom
Conversation
Soon we will require an ESQL version for all ESQL requests. Kibana is sending it already. The official clients will start sending it in version 8.14+. This defaults the version for the official clients before 8.14. It does so by reading a magic header the clients send, `x-elastic-client-meta` and detecting the version string for any clients that might possibly support ESQL - `es=8.11`, `es=8.12`, and `es=8.13`. If we receive that we'll default to the first version of ESQL. This doesn't do anything for old versions of kibana.
|
Pinging @elastic/es-analytical-engine (Team:Analytics) |
|
If anyone is using ESQL outside of the official client you'll need to start sending the |
alex-spies
left a comment
There was a problem hiding this comment.
Thanks a lot @nik9000 !
I think this can be merged as-is, but left some comments as maybe this can be simplified a little.
| } | ||
| String product = restRequest.header(PRODUCT_ORIGIN); | ||
| if ("kibana".equals(product)) { | ||
| if (clientMeta.contains("es=8.9")) { |
There was a problem hiding this comment.
For someone who doesn't know the headers sent by Kibana, why do we check for exactly 8.9?
I expected something like "check client meta and use 🚀 if it's 8.13 or lower". I guess we don't want to set default values for 8.10 and lower (no ESQL at that time), but Kibana still sends the old 8.9 header for some reason?
Wouldn't it be simpler to just look for es=8.x.y with a regex, check that x < 14 and if so, use 🚀 ? This could also avoid having to special case for Kibana, so we wouldn't have to check the product origin header at all.
There was a problem hiding this comment.
I'll leave a comment. The short version is that they haven't upgraded the js client for a while. All of the versions of kibana that support ESQL were on the 8.9 version of the javascript client. Also! The version of kibana that we do want to send is on the 8.13 version of the client. Not 8.14. Thus, what we've got. I'll leave a comment.
| if (clientMeta.contains("es=8.1") == false) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Nit: this is technically redundant as the next line only is executed if a stronger condition than this applies.
I'd remove this as it can be misleading. (Looks as if we were specialcasing for 8.1, while we actually just look for 8.1x .)
There was a problem hiding this comment.
Yeah. I was thinking, "oh, this'll be faster". But that's silly.
There was a problem hiding this comment.
By faster - I mean faster in the cases where it isn't 8.1x - but I'm not even sure there will be clients in the 8.2x space. That's a long time from now.
| EsqlQueryRequest esqlRequest = new EsqlQueryRequest(); | ||
| esqlRequest.esqlVersion("whatever"); | ||
| FakeRestRequest restRequest = new FakeRestRequest(); | ||
| restRequest.getHttpRequest().getHeaders().put(CLIENT_META, List.of("es=8.13.0")); |
There was a problem hiding this comment.
nit: could randomize/parameterize the client meta version.
| } | ||
|
|
||
| public void testNoVersionForNewClient() { | ||
| assertEsqlVersion("es=8.14", randomProduct(), nullValue(String.class)); |
There was a problem hiding this comment.
super nit: could use future versions as well, 8.15, ..., 8.21 (an 8.2x might be nice since we do some substring logic)
There was a problem hiding this comment.
++ - I'll randomize to 8.2x as well.
| } | ||
|
|
||
| public void testNoVersionForAlreadySet() { | ||
| EsqlQueryRequest esqlRequest = new EsqlQueryRequest(); |
There was a problem hiding this comment.
nit/bikeshed: since we do not really test the handling of RestEsqlQueryAction and the tests are based on EsqlQueryRequests anyway - we only really test , we could a) move the tests to EsqlQueryRequestsTets and b) make defaultVersionForOldClients a method of EsqlQueryRequest.
There was a problem hiding this comment.
I thought about this one and didn't want to. I like the defaulting method in the REST handler because it's an "incoming layer" hack sorta thing. And that implies sticking it in these tests. It'd be reasonably to put it in the request, but I'm the one typing so I'm going to keep it here.
| static final String PRODUCT_ORIGIN = "x-elastic-product-origin"; | ||
| static final String CLIENT_META = "x-elastic-client-meta"; | ||
|
|
||
| static void defaultVersionForOldClients(EsqlQueryRequest esqlRequest, RestRequest restRequest) { |
There was a problem hiding this comment.
nit/bikeshed: This could also be a method of EsqlQueryRequest since it just mutates its version.
Also, in the added tests we do not really test the handling of RestEsqlQueryAction, the tests are based on EsqlQueryRequests. We could move the tests to EsqlQueryRequestsTests.
There was a problem hiding this comment.
Same as above - yeah, it totally could. But I, personally, think of it as a REST layer hack rather than a request hack.
Soon we will require an ESQL version for all ESQL requests. Kibana is sending it already. The official clients will start sending it in version 8.14+. This defaults the version for the official clients before 8.14. It does so by reading a magic header the clients send,
x-elastic-client-metaand detecting the version string for any clients that might possibly support ESQL -es=8.11,es=8.12, andes=8.13. If we receive that we'll default to the first version of ESQL.I've also made this work for kibana versions 8.12 and 8.13 - discover will default to the old version of ESQL.