Skip to content
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

[BUG] search detector API doesn't recognize _source field #669

Closed
ylwu-amzn opened this issue Sep 13, 2022 · 4 comments · Fixed by #749 or #764
Closed

[BUG] search detector API doesn't recognize _source field #669

ylwu-amzn opened this issue Sep 13, 2022 · 4 comments · Fixed by #749 or #764
Assignees
Labels
bug Something isn't working Priority-Medium v2.5.0 'Issues and PRs related to version v2.5.0'

Comments

@ylwu-amzn
Copy link
Collaborator

ml-commons is using the similar way to build search APIs and we received one bug opensearch-project/ml-commons#419.

I guess AD has the same issue. Refer to the ml-commons issue for more details of root cause.

Related AD code AbstractSearchAction.java#L85

@ylwu-amzn ylwu-amzn added bug Something isn't working untriaged labels Sep 13, 2022
@amitgalitz amitgalitz self-assigned this Nov 29, 2022
@amitgalitz
Copy link
Member

amitgalitz commented Dec 5, 2022

For returning Anomaly Detector’s in our search response we have additional logic to re-parse our response into an AnomalyDetector object in-order to keep the correct order of the response after excluding the ui_metdata. When we add _source to exclude/include specific fields it changes the order of the response by default. Because we have implemented this logic, if we add the ability for example to exclude timefield than we will fail in re-parsing the detector object.

The two ways to add the ability to use _source is to write an additional AnomalyDetetctor parse method and find a non-redundant way to change toXContent to accommodate for the change while trying to keep the order or to stop caring about the order of the response body.

I believe adding this capability is not important enough to change the order of our response, so for now I’ll split up the issue into two issues and first release a bug fix for other search API’s (anomaly results, detector tasks)

@ohltyler
Copy link
Member

ohltyler commented Dec 6, 2022

@amitgalitz I understand that we take the raw responses & parse into AnomalyDetector objs, but what does this have to do with persisting any order? Also, our API doesn't ensure that they will be returned in any particular order: https://opensearch.org/docs/latest/monitoring-plugins/ad/api/#search-detector

@amitgalitz amitgalitz linked a pull request Dec 7, 2022 that will close this issue
@amitgalitz
Copy link
Member

@amitgalitz I understand that we take the raw responses & parse into AnomalyDetector objs, but what does this have to do with persisting any order? Also, our API doesn't ensure that they will be returned in any particular order: https://opensearch.org/docs/latest/monitoring-plugins/ad/api/#search-detector

After further consideration, and your point as well that we don't technically ensure an order in the JSON response, I removed the part where we re-parse the detector and added the ability to use _source in the query but now we don't ensure that the order is exactly the same which is something that isn't 100% necessary for JSON.

@kaituo kaituo added the v2.5.0 'Issues and PRs related to version v2.5.0' label Dec 19, 2022
@kaituo kaituo reopened this Dec 19, 2022
@kaituo
Copy link
Collaborator

kaituo commented Dec 19, 2022

@amitgalitz Can you forward port your changes to the main/3.0.0 branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Priority-Medium v2.5.0 'Issues and PRs related to version v2.5.0'
Projects
None yet
5 participants