-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Implement error_trace parameter for HTTP Bulk requests #19985
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
Implement error_trace parameter for HTTP Bulk requests #19985
Conversation
Signed-off-by: Sergei Ustimenko <[email protected]>
Signed-off-by: Sergei Ustimenko <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #19985 +/- ##
=========================================
Coverage 73.27% 73.27%
+ Complexity 71553 71540 -13
=========================================
Files 5789 5789
Lines 327144 327153 +9
Branches 47156 47157 +1
=========================================
+ Hits 239715 239731 +16
+ Misses 68235 68222 -13
- Partials 19194 19200 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
owaiskazi19
left a comment
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.
Overall LG. Can we also add a rest based test https://github.com/opensearch-project/OpenSearch/tree/main/rest-api-spec/src/main/resources/rest-api-spec/test/bulk?
qa/smoke-test-http/src/test/java/org/opensearch/http/DetailedErrorsDisabledIT.java
Show resolved
Hide resolved
Signed-off-by: Sergei Ustimenko <[email protected]>
…ulk-api-error-trace
|
@owaiskazi19 thanks for the quick review! I've smoothen rough edges and added more rest tests (also to |
|
❕ Gradle check result for 1731418: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
rest-api-spec/src/main/resources/rest-api-spec/test/bulk/100_error_traces.yml
Show resolved
Hide resolved
Signed-off-by: Sergei Ustimenko <[email protected]>
|
❌ Gradle check result for 6855608: null Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Sergei Ustimenko <[email protected]>
|
❌ Gradle check result for 91995ff: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
…ulk-api-error-trace Signed-off-by: Sergei Ustimenko <[email protected]>
|
last commit failed with: clickmerged latest main just in case |
|
❌ Gradle check result for f27f08b: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
Signed-off-by: Sergei Ustimenko <[email protected]>
|
The but the #16159 is closed. Perhaps it needs to be reopened... 🤔 |
Did you run |
@owaiskazi19 I’m pretty sure I did 😅 |
Signed-off-by: Andrew Ross <[email protected]>
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.
…oject#19985) * Implement error_trace parameter for bulk requests Signed-off-by: Sergei Ustimenko <[email protected]> * Add the CHANGELOG.md entry Signed-off-by: Sergei Ustimenko <[email protected]> * Polish RestChannel and add more testing Signed-off-by: Sergei Ustimenko <[email protected]> * Exclude error_traces REST tests from the backwards compatibility runs Signed-off-by: Sergei Ustimenko <[email protected]> --------- Signed-off-by: Sergei Ustimenko <[email protected]> Signed-off-by: Andrew Ross <[email protected]> Co-authored-by: Andrew Ross <[email protected]>
…oject#19985) * Implement error_trace parameter for bulk requests Signed-off-by: Sergei Ustimenko <[email protected]> * Add the CHANGELOG.md entry Signed-off-by: Sergei Ustimenko <[email protected]> * Polish RestChannel and add more testing Signed-off-by: Sergei Ustimenko <[email protected]> * Exclude error_traces REST tests from the backwards compatibility runs Signed-off-by: Sergei Ustimenko <[email protected]> --------- Signed-off-by: Sergei Ustimenko <[email protected]> Signed-off-by: Andrew Ross <[email protected]> Co-authored-by: Andrew Ross <[email protected]>
Description
This PR adds support for showing detailed stack traces when Bulk HTTP Apis, such as
_bulk,_msearch, etc. are called.The
error_tracewas already implemented from the beginning and it works for Bulk APIs when an internal error (e.g. an NPE) occurs. Detailed stack traces are not included in the response (when some items of the batch have failed) the request itself is treated as success, which it should. Thus the serialization of response doesn't use theerror_tracerequest parameter which, for the proper serialization of stack traces should be converted torest.exception.stacktrace.skipparameter. So,rest.exception.stacktrace.skipuses fallback which istrue(i.e. skip printing stack traces).This change properly transforms
error_tracetorest.exception.stacktrace.skipin all cases, so all REST requests serialize stack traces consistently as per REST request parameters.Related Issues
Resolves #19945
Check List
- [ ] API changes companion pull request created, if applicable.- [ ] Public documentation issue/PR created, if applicable.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.