[BUG] Decide Elasticsearch bulk export success from HTTP status and the errors flag#4297
Open
thc1006 wants to merge 11 commits into
Open
[BUG] Decide Elasticsearch bulk export success from HTTP status and the errors flag#4297thc1006 wants to merge 11 commits into
thc1006 wants to merge 11 commits into
Conversation
thc1006
added a commit
to thc1006/opentelemetry-cpp
that referenced
this pull request
Jul 24, 2026
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.25% 81.25% +0.01%
==========================================
Files 446 447 +1
Lines 18872 18904 +32
==========================================
+ Hits 15332 15359 +27
- Misses 3540 3545 +5
🚀 New features to boost your workflow:
|
thc1006
added a commit
to thc1006/opentelemetry-cpp
that referenced
this pull request
Jul 25, 2026
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
…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>
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>
An errors:false body was trusted even when it carried no item results, so
a truncated or unrelated response such as {"errors":false} or an empty
"items" array reported a dropped batch as a success. The check now
requires "items" to be an array whose size matches the number of records
exported: both the sync and async paths pass that count, and the async
handler stores it alongside the response.
Also move the non-2xx status check inside the exception guard so building
its failure reason cannot terminate a noexcept caller on allocation
failure, drop the ?pretty query now that the parser no longer depends on
whitespace, and log a synchronous non-2xx response once from Export()
rather than a second time with the full body from the response handler.
Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
Review found the added items.size() == records check to be half-baked schema validation beyond the scope of open-telemetry#4295: it checks array length but not that each entry is a real operation result, and it would reject a legitimate filter_path response. A 2xx response whose top-level "errors" is false is Elasticsearch's authoritative batch-success signal, so the condition returns to 2xx + JSON object + boolean errors. Also clear failure_reason on entry so a success cannot leave a stale reason behind, and document the exception path's reason as best-effort. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
thc1006
force-pushed
the
bugfix/elasticsearch-response-handling
branch
from
July 25, 2026 09:28
d751066 to
229ee19
Compare
thc1006
marked this pull request as ready for review
July 25, 2026 19:12
…response-handling # Conflicts: # CHANGELOG.md
6 tasks
Add two response-parser tests and soften one doc comment; no change to the validator logic. - TreatsThe2xxRangeAsTheSuccessBand: 199/200/299/300 pin the status boundary. - DoesNotRequireItemsForSuccess: a body with errors:false and no items (e.g. filter_path-filtered) is a success, locking out re-adding an items-count check. - Reword the header comment so it no longer claims every 2xx means every operation was applied; a successful 2xx with errors:false is treated as batch success.
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
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
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. The 2xx range is pinned at its 199/200/299/300 boundaries, and a success witherrors:falseand noitems(as with afilter_path-filtered response) is covered so the batch-level policy cannot silently regress to an item-count check.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. Exporter-level HTTP wiring coverage will follow once the completion-state race in #4298 is resolved, since a fake session that responded immediately would otherwise deadlock the synchronous path on that pre-existing bare wait.Verification
HTTP 500 + {"errors":false} -> failure, the 2xx boundary cases, and the items-optional success case./_bulkresponse; parsing no longer depends on pretty-printing..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.