ES|QL support #233
ES|QL support #233
Conversation
…esql option, validations to make sure both LS and ES support the ESQL execution.
… adds by default - might be users are looking for by default.
…/info and add docinfo* fields in ineffective fields list.
Fix the condition to correctly compares supported LS version.
…t timestampt converter to LogStash::Timestamp, dotted fields extended to nested fields.
jsvd
left a comment
jsvd
left a comment
There was a problem hiding this comment.
first round of review, overall it looks good, I'll give it a spin today/tomorrow to check on the overall user experience.
…tting the result into target if defined. Debug logs added which can help to investigate query and its result.
|
|
||
| private | ||
|
|
||
| def get_query_object |
There was a problem hiding this comment.
review note: moved to private area
|
One use case that concerns me is the common default pattern of ES creating a "field.keyword" for each "field", which results in an error in the plugin during The ways to not have this is to have a dedicated mapping without this overlap or being explicit about what to keep using Also the error is not very helpful given it's coming straight from not sure yet what the solution should be, but at least catching this particular nesting scenario and bubbling up a warning saying "you can't keep top level and nested fields". |
I was wrong, if there is a |
Right! |
yaauie
left a comment
yaauie
left a comment
There was a problem hiding this comment.
Overall this looks to be on track.
- I'd like to include the client-side mitigation of queries that come back with inner sub-fields to prevent crashes
- I'd like to align with the filter plugin for which parameter to specify the ESQL query in; if we determine that is better to use
esql_queryin the filter due to the filter's inability to distinguish a QueryString query from an ES|QL query, I'd like to use it here too. - I would prefer more validation of inputs; a user shouldn't be able to configure ESQL with irrelevant things like
slicesordocinfo.
docs/index.asciidoc
Outdated
| |This plugin |4.23.0+ (4.x series) or 5.2.0+ (5.x series) | ||
| |=== | ||
|
|
||
| To configure ES|QL query in the plugin, set the `response_type` to `esql` and provide your ES|QL query in the `query` parameter. |
There was a problem hiding this comment.
This feels cumbersome to me.
Could we align with the proposal in the filter PR to provide an ESQL query with esql_query instead of requring the configuration of multiple separate parameters?
In this case, since the input plugin does require a JSON-encoded object for its query parameter when using the Query DSL, we could auto-detect that a given query parameter is ESQL (unlike the ES filter, which uses a QueryString query as its query parameter)
There was a problem hiding this comment.
When we had a discussion with @jsvd about this, we had a similar idea to deprecate this response_type and replace with query_type in the future. And through the experience as I do see, introducing new param is not a difficult, deprecation -> obseletion -> removal is a long headache process.
From this point of view, I would support adding minimal change but I am open to apply changes if anyone has strong opinion.
There was a problem hiding this comment.
I've left a separate note on how to do it.
I don't personally care much about removing the response_type right away, but if a user starts using ESQL I'd like them to not start new usages of a config that we'd like to deprecate.
Since this is effectively a rename, we can easily use the with_deprecated_alias helper from NormalizeConfigSupport.
Co-authored-by: Rye Biesemeyer <[email protected]> Co-authored-by: João Duarte <[email protected]>
…yntax fix, unit test errors fix.
lib/logstash/inputs/elasticsearch.rb
Outdated
| # hits: normal search request | ||
| # aggregations: aggregation request | ||
| # esql: ES|QL request | ||
| config :response_type, :validate => %w[hits aggregations esql], :default => 'hits' |
There was a problem hiding this comment.
Migrating to query_type with auto-detection of ESQL queries would be pretty straight-forward with the NormalizeConfigSupport mixin:
| config :response_type, :validate => %w[hits aggregations esql], :default => 'hits' | |
| config :response_type, :validate => %w[hits aggregations], :deprecated => "use `query_type`" | |
| config :query_type, :validate => %w[hits aggregations esql] # default depends on query shape |
def register
+ @query_type = normalize_config("query_type") do |normalizer|
+ normalizer.with_deprecated_alias("response_type")
+ end || (@query.start_with?('{') ? 'hits' : 'esql')There was a problem hiding this comment.
I was thinking to add the deprecation right after this ES|QL change.
One agreement we need to decide is naming. I personally do not like hits, aggregations along with esql. They indicate different contexts. I had options dsl_search, dsl_aggregation and esql.
Let me please know your opinion: I can either apply with change if we quickly come with agreement or create an issue follow up right after this PR.
There was a problem hiding this comment.
I was thinking to add the deprecation right after this ES|QL change.
If someone starts using this feature, I would rather that their never-possible-before configuration feels "stable" and doesn't require them to go back and deal with deprecation warnings for things that we knew about before shipping the feature.
They indicate different contexts
This is a very good point.
The current response_type only makes sense in the context of DSL-based queries.
So: what if we were to keep response_type, but constrain its use to query_type => dsl?
This would mean:
query_type => dsl: allows use ofresponse_typequery_type => esql: prohibits use ofresponse_type- unspecified
query_typecould have a sensible default based on the shape ofquery:- if it looks like JSON, then it's
dsl - if it looks like ES|QL then it's
esql - else we error helpfully
- if it looks like JSON, then it's
There was a problem hiding this comment.
Introducing query_type and keep using response_type was my initial design and we with @jsvd thinking if we can still simplify without introducing new param (and came to agreement in our 1:1 to support wth response_type and deprecate it in the future).
However, considering the behavior and user experience, I do also strongly support this (introducing query_type at high level which other params follow) structural (query type at the high level, then depth details such as what response shape going to be parsed, etc..) logic.
I have applied it with this commit.
FYI: current CI snapshot unit test steps are broken (CIs with release versions are fine) due to core openssl.jar and uri gem miss but I have run on my local with local LS to verify change and unit/integration tests.
docs/index.asciidoc
Outdated
| |This plugin |4.23.0+ (4.x series) or 5.2.0+ (5.x series) | ||
| |=== | ||
|
|
||
| To configure ES|QL query in the plugin, set the `response_type` to `esql` and provide your ES|QL query in the `query` parameter. |
There was a problem hiding this comment.
I've left a separate note on how to do it.
I don't personally care much about removing the response_type right away, but if a user starts using ESQL I'd like them to not start new usages of a config that we'd like to deprecate.
Since this is effectively a rename, we can easily use the with_deprecated_alias helper from NormalizeConfigSupport.
…pply method to avoid null checks at runtime.
… It validates the shape to send a valid query type to the ES.
jsvd
left a comment
jsvd
left a comment
There was a problem hiding this comment.
Overall LGTM, it think it's worth capturing any feedback that hasn't been address yet in the PR (like deprecation of response_type), in a set of issues linked to this PR.
I tested this in multiple scenarios and error conditions and I didn't find any UX issue, so for me 🚢 it!
3 participants
Description
ES|QL support:
query_typeparams, acceptsdsloresqloption.query_typeisesql, make sure we accept meaningful inputs and do not allowresponse_type,index, etc.. DSL related params.METADATAwhich adds_id,_versionto the response entriessize,search_api,targetif users configure{a.b.c: 'val'}=>{'a':{'b':{'c':'val'}}})Sample minimal config to test:
Author's check
Logs