[BUG] Decide Elasticsearch bulk export success from HTTP status and the errors flag#4297
Draft
thc1006 wants to merge 7 commits into
Draft
[BUG] Decide Elasticsearch bulk export success from HTTP status and the errors flag#4297thc1006 wants to merge 7 commits into
thc1006 wants to merge 7 commits into
Conversation
…string
Both export paths decided whether a bulk write succeeded with
body.find("\"failed\" : 0") == std::string::npos
which is wrong in both directions. "failed" is per-shard information for
a single item, so a batch where one item was rejected and another
reported "failed": 0 was recorded as a success and the rejected records
were silently lost. The literal also contains pretty-printing spaces, so
a compact response never matched and a completely successful export was
reported as a failure.
Both paths now parse the body and check the top level "errors" field,
which is what the bulk API defines as the outcome for the batch, and
report the first item error rather than only that something went wrong.
A malformed or unparseable body counts as a failure. The asynchronous
path also checks the HTTP status, which it previously ignored while the
synchronous path did not.
nlohmann/json.hpp was already included by this file and already linked
in both the CMake and Bazel targets, so this adds no dependency.
Fixes open-telemetry#4295
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4297 +/- ##
==========================================
- Coverage 81.31% 81.26% -0.05%
==========================================
Files 445 447 +2
Lines 18859 18904 +45
==========================================
+ Hits 15333 15360 +27
- Misses 3526 3544 +18
🚀 New features to boost your workflow:
|
AsyncResponseHandler::OnResponse is noexcept, and the helper it now calls can throw: dump() and the string concatenation that builds the failure reason both allocate. clang-tidy reported bugprone-exception-escape on OnResponse, which was a warning this branch added rather than one that was already there. The helper is now noexcept and absorbs anything thrown while inspecting the body, which is the right answer anyway: a response that cannot be inspected is not a successful export. The default reason is set inside the try so the handler itself does no allocation. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
The Bazel noexcept configuration compiles with -fno-exceptions, where try is a hard error rather than a warning, so the previous commit broke that job. The repository already has an idiom for this, OPENTELEMETRY_HAVE_EXCEPTIONS, used the same way in attribute_utils.h. Nothing is lost when it is off: without exceptions an allocation failure terminates rather than unwinding, so there is nothing for the handler to catch in the first place. The macro arrives through opentelemetry/version.h, which includes macros.h with an IWYU pragma export, so no new include is needed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Codecov reported 0% patch coverage: none of the 34 changed lines were exercised. The reason was structural rather than an oversight about which tests to write. The helper sat in an anonymous namespace inside the .cc, so no test translation unit could reach it, and the existing Elasticsearch tests are almost all DISABLED_ because they need a live server. It is a pure function from a response body to a verdict, so it belongs somewhere a test can call it. It moves to a detail header, following the detail/ pattern already used by ext/http/client/detail/default_factory.h, and is added to the Bazel hdrs so the test target picks it up. Four tests cover it: pretty-printed and compact successes, a batch with one rejected item where the reason must name the underlying error, an unusable body, and a missing or non-boolean errors field. The first two are the cases the old substring check got wrong in each direction. nlohmann/json.hpp is no longer used by the .cc and is removed from it. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
IWYU flagged es_log_record_exporter.cc for a missing nlohmann/json.hpp: it calls GetJSON().dump() at the point it builds the request body, which is a direct use of the type. When the bulk response helper moved to its own header I removed the include with it, but this use remained, and IWYU treats reaching it through es_log_recordable.h as an indirect dependency. The include goes back. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Review of open-telemetry#4297 found the synchronous path reported a non-2xx response as a success: ResponseHandler::OnResponse only logged the status and unconditionally set response_received_, waitForResponse returned true, and Export then checked the body alone, so HTTP 500 with a body of {"errors":false} became kSuccess. My description claiming the sync path already checked the status was wrong: it observed the status but never let it affect the ExportResult. IsBulkResponseSuccessful now takes the status code and treats a non-2xx as a failure before looking at the body. The top-level "errors" flag is item-level and cannot override a transport or application error. Both paths call it: the async handler drops its duplicated status check, and the sync handler stores the status so Export can pass it in. Tests assert the invariant directly, including HTTP 500 with {"errors":false} on the sync-shaped path, plus the errors:true generic reason and the missing item-error branches. A full handler-level mock across HttpClient/Session is out of scope: the exporter has no such mock today (its network tests are DISABLED_), and the status invariant is what the bug was, so it is covered at the validator instead. Fixes open-telemetry#4295 Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
thc1006
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4295
Draft while CI runs. Two rounds of review folded in.
The bug
Both export paths decided whether a bulk write succeeded with
body.find("\"failed\" : 0").failedis per-shard information for one item, not the batch outcome (which is the top-levelerrorsflag), and the literal even baked in pretty-printing whitespace, so it was wrong in both directions: a batch with a rejected item read as success, and a compact successful response read as failure.The fix
A single
IsBulkResponseSuccessful(status_code, body, reason)that both paths call. It treats a non-2xx status as a failure first, then parses the body and checks the top-levelerrorsflag, reporting the first item error so the log says why. A malformed body counts as a failure.The HTTP status matters and was the second-round finding: the synchronous path previously only logged a non-2xx status and still returned success, so
HTTP 500with{"errors":false}was reported askSuccess. The status is now part of the result on both paths; the async handler drops its duplicated check and the sync handler stores the status forExportto pass in.Structure
The helper lives in
include/opentelemetry/exporters/elasticsearch/detail/es_bulk_response.hso tests can reach it, following thedetail/pattern already used byext/http/client/detail/default_factory.h. It is adetailheader rather than an anonymous-namespace function in the.cc(an earlier revision put it there, which is why it could not be tested). This exporter's publices_log_recordable.halready exposesnlohmann::json, so the parser being in an installed header does not add a new dependency surface, but it is a deliberatedetailheader and worth a maintainer's eye.Guarded with
OPENTELEMETRY_HAVE_EXCEPTIONSso thetry/catchcompiles under the-fno-exceptionsBazel config.<nlohmann/json.hpp>stays included by the.ccbecause it still callsGetJSON().dump()directly.Tests
The helper's cases are covered directly: pretty and compact success, a rejected item (reason names the underlying error), a non-2xx status with
{"errors":false}(the sync false-success invariant),errors:truewith no extractable item error, malformed and empty bodies, and a missing or non-booleanerrorsfield.A full handler-level mock across
HttpClient/Session/Exportis out of scope here: the exporter has no such mock today (its network tests areDISABLED_), and the defect was the status/body decision, which is what these tests pin. Building that mock is worth its own change.Verification
HTTP 500 + {"errors":false} -> failure..cclocally (it needs the exporter's full include set), so CI is the authority onwarning_limit; the change adds onedetailheader and no new file-scope entity in the.cc.