-
Notifications
You must be signed in to change notification settings - Fork 85
ES|QL support #233
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
ES|QL support #233
Changes from 18 commits
a307db2
9c35f22
6f99055
086a592
e30e0f9
7746c14
76303d8
1fb29f7
5d47f2f
c291e24
22e72e9
af6e24a
4ce6fa4
4ed69ff
0725f98
cfb36f3
a92a71e
d4f559d
65eb675
789f467
fefe6a0
e108c87
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -74,6 +74,7 @@ class LogStash::Inputs::Elasticsearch < LogStash::Inputs::Base | |||||||
| require 'logstash/inputs/elasticsearch/paginated_search' | ||||||||
| require 'logstash/inputs/elasticsearch/aggregation' | ||||||||
| require 'logstash/inputs/elasticsearch/cursor_tracker' | ||||||||
| require 'logstash/inputs/elasticsearch/esql' | ||||||||
|
|
||||||||
| include LogStash::PluginMixins::ECSCompatibilitySupport(:disabled, :v1, :v8 => :v1) | ||||||||
| include LogStash::PluginMixins::ECSCompatibilitySupport::TargetCheck | ||||||||
|
|
@@ -96,15 +97,18 @@ class LogStash::Inputs::Elasticsearch < LogStash::Inputs::Base | |||||||
| # The index or alias to search. | ||||||||
| config :index, :validate => :string, :default => "logstash-*" | ||||||||
|
|
||||||||
| # The query to be executed. Read the Elasticsearch query DSL documentation | ||||||||
| # for more info | ||||||||
| # https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl.html | ||||||||
| # The query to be executed. DSL or ES|QL (when `response_type => 'esql'`) query shape is accepted. | ||||||||
| # Read the following documentations for more info | ||||||||
| # Query DSL: https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl.html | ||||||||
| # ES|QL: https://www.elastic.co/guide/en/elasticsearch/reference/current/esql.html | ||||||||
| config :query, :validate => :string, :default => '{ "sort": [ "_doc" ] }' | ||||||||
|
|
||||||||
| # This allows you to speccify the response type: either hits or aggregations | ||||||||
| # where hits: normal search request | ||||||||
| # aggregations: aggregation request | ||||||||
| config :response_type, :validate => ['hits', 'aggregations'], :default => 'hits' | ||||||||
| # This allows you to speccify the response type: one of [hits, aggregations, esql] | ||||||||
| # where | ||||||||
| # hits: normal search request | ||||||||
| # aggregations: aggregation request | ||||||||
| # esql: ES|QL request | ||||||||
| config :response_type, :validate => %w[hits aggregations esql], :default => 'hits' | ||||||||
|
||||||||
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review note: moved to private area
Uh oh!
There was an error while loading. Please reload this page.