-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[data.search] Handle warnings inside of headers #103744
[data.search] Handle warnings inside of headers #103744
Conversation
Pinging @elastic/kibana-app-services (Team:AppServices) |
@elasticmachine merge upstream |
jenkins, test this (had to abort for Jenkins upgrade) |
Following up on our call - I think it would be better if the search API exposed two fields:
In this PR we could use it only for warnings, but I would make a follow up PR to get rid of |
@elasticmachine merge upstream |
I was giving this some more thought today and I'm not sure this will work, as a search can complete and still have warnings. So I'm not sure what the enum of statuses would look like. Maybe we can chat a little bit more about this approach or possibly move it to a follow-up PR. |
Then what about aiming for -
A successful response will have an OK status, but may or may not have a warning(s?). |
@lizozom I removed the |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
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.
Ok, lets merge as is and wait for feedback 👍🏻
💚 Build SucceededMetrics [docs]Page load bundle
History
To update your PR or re-run it, just comment with: cc @lukasolson |
It is interesting that one of our sample dashboards shows a real warning now: Seems like the warning is a deprecation message which shouldn't concern our users, but should be addressed by us instead. Seems like we need a way to separate real or actionable by-user warnings from such deprecation warnings which should be handled by us? Maybe there are other types of warnings that we have to mute :( |
Also opened a fix: #108825 |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
* [data.search] Handle warnings inside of headers * Update docs * Add tests * Remove isWarningResponse Co-authored-by: Kibana Machine <[email protected]>
* [data.search] Handle warnings inside of headers * Update docs * Add tests * Remove isWarningResponse Co-authored-by: Kibana Machine <[email protected]>
Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync. |
* [data.search] Handle warnings inside of headers * Update docs * Add tests * Remove isWarningResponse Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
* [data.search] Handle warnings inside of headers * Update docs * Add tests * Remove isWarningResponse Co-authored-by: Kibana Machine <[email protected]> Co-authored-by: Kibana Machine <[email protected]>
Summary
Closes #98764.
This PR adds support to the
data.search
services to surfacewarning
properties that are returned in headers from Elasticsearch responses. It also adds developer examples and adds support toSearchSource
for handling these types of warnings directly.Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers
Plugin API changes
The
data.search
service now returns awarning
property that includes any warnings returned from Elasticsearch in the headers.