-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
Elasticsearch: fix support for multiple indices #71183
Conversation
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.
Left a few comments, maybe we should quickly talk about that on Monday's meeting.
@@ -207,7 +207,7 @@ func (c *baseClientImpl) createMultiSearchRequests(searchRequests []*SearchReque | |||
header: map[string]interface{}{ | |||
"search_type": "query_then_fetch", | |||
"ignore_unavailable": true, | |||
"index": c.indices, |
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.
Something we should ideally also do is to dedupe the array. With a "daily" pattern and the index as [logs-]YYYY.MM.DD,[test],[metrics-]YYYY.MM.DD
we would correctly end up in multiple [logs-]YYYY.MM.DD
and [metrics-]YYYY.MM.DD
indices, but test
will also be duplicated, i.e. ending in a total of logs-2023.07.05,test,metrics-2023.07.05,logs-2023.07.06,test,metrics-2023.07.06,logs-2023.07.07,test,metrics-2023.07.07
for 3 days.
@@ -210,7 +211,7 @@ func TestClient_Index(t *testing.T) { | |||
jHeader, err := simplejson.NewJson(headerBytes) | |||
require.NoError(t, err) | |||
|
|||
assert.Equal(t, test.indexInRequest, jHeader.Get("index").MustStringArray()) | |||
assert.Equal(t, strings.Join(test.indexInRequest, ","), jHeader.Get("index").MustString()) |
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.
would be great if we ensure with a test that multiple indices are joined.
@@ -207,7 +207,7 @@ func (c *baseClientImpl) createMultiSearchRequests(searchRequests []*SearchReque | |||
header: map[string]interface{}{ | |||
"search_type": "query_then_fetch", | |||
"ignore_unavailable": true, | |||
"index": c.indices, | |||
"index": strings.Join(c.indices, ","), |
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.
Even though this seems to work and I might have pointed you in a wrong direction (sorry), I am starting to be a bit unsure if that's the right approach. After reading https://www.elastic.co/guide/en/elasticsearch/reference/current/search-multi-search.html
index
(Optional, string or array of strings) Data streams, indices, and aliases to search. Supports wildcards (*). Specify multiple targets as an array.
If this parameter is not specified, the <target> request path parameter is used as a fallback.
We should either use an array here or use the correct <target>
in the request path parameter. WDYT? We can also discuss with the rest of the squad next week.
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.
Definitely! Let's discuss this next week.
Thanks for the review @svennergr , let's discuss this more in depth. |
I haven't head a detailed look, but I suspect this #67276 broke it. We decided to change comma separated list to array to be consistent with frontend. So maybe we could just revert it to fix the issue. And then we can decide if we want to refactor/change the logic. |
i did some testing on how "frontend-mode" handles this and:
{
"index": "logs"
} this also works if you put more indexes comma-separated, like {
"index": "logs1,logs2"
}
{
"index": [
"logs-2023.07.08",
"logs-2023.07.09",
"logs-2023.07.10"
]
}
|
after some thinking, i think [3] from above can be considered unsupported. i see two ways to achieve this:
func (ip *staticIndexPattern) GetIndices(timeRange backend.TimeRange) ([]string, error) {
if ip.indexName != "" {
- return []string{ip.indexName}, nil
+ return strings.Split(ip.indexName, ","), nil
} else {
return []string{}, nil
} i think the single-string approach is more robust, WDYT? @ivanahuckova @matyax |
I am a bit concerned with the statement in the Elastic docs https://www.elastic.co/guide/en/elasticsearch/reference/current/search-multi-search.html
The other fix could be to rely on this statement
and remove the |
Thanks everyone |
Originally reported in an escalation, this PR fixes support for multiple configured indices, as a comma-separated list.
Which issue(s) does this PR fix?:
Fixes #71092
Special notes for your reviewer:
To verify:
Expected results: without the fix, no data is returned by the data source; with the code changes, it should work as usual.