[BUG] Decide Elasticsearch bulk export success from HTTP status and the errors flag

[thc1006]

Fixes #4295

The bug

Both export paths decided whether a bulk write succeeded with body.find("\"failed\" : 0"). failed is per-shard information for one item, not the batch outcome (which is the top-level errors flag), 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, expected_items, reason) that both paths call. A non-2xx status is a failure first. Then the body has to be a JSON object with a boolean errors flag and an items array holding one operation result per operation the request submitted, and errors has to be false. A malformed body counts as a failure, and the first item error is reported so the log says why.

The count is there because the exporter posts an unfiltered /_bulk with exactly one index operation per record, and Elasticsearch answers those with one entry in items per operation. Without it, {"errors":false} on its own was reported as a successful write of the whole batch. An exporter that claims success without a write acknowledgement gives the caller no reason to retry, which is the worse of the two directions to be wrong in, and a misconfigured proxy or a non-Elasticsearch endpoint answering with coincidentally shaped JSON both landed there.

Matching the count is not enough on its own. Elasticsearch answers each operation with an object keyed by the action name, so {"errors":false,"items":[null,null]} for a two record batch had the right length and was reported as a successful write of both records. That requirement already existed, but only on the errors:true path, where the walk that extracts the first item error skips anything that is not an object. The same body was therefore a failure with errors:true and a success with errors:false, so a responder that sends neither shape correctly picked the verdict with a flag it also controls. The requirement now runs before errors is read, and the skip inside the walk goes with it.

An acknowledgement is an entry with exactly one member, named index, holding a result object with a status. Elasticsearch keys each entry by the action it answers and the exporter writes every record with an index action, so that is what an answer to this request looks like, and the bulk schema makes status a required member of the result alongside _index.

Nothing weaker ties the entry to the operation that was sent. Being an object was not the line it looked like: {"errors":false,"items":[{},{}]} matched the count and was an object per entry. Neither is carrying some action or other: {"unknown":{"status":201}} answers something this exporter never submitted, and {"index":{"status":201},"delete":{}} answers one operation with two. The count check is what makes those dangerous rather than untidy. A hundred records answered with a hundred filler entries reads as a successful write, the processor drops the batch, and the records are gone.

The status is then read against the flag rather than instead of it. A false errors asserts that every operation was applied, so {"errors":false,"items":[{"index":{"status":400}}]} is the response contradicting itself, and a response that contradicts itself is not evidence that anything was written. A conforming server never sends that combination, which is why this rejects nothing the flag alone would have accepted.

2xx is the whole success band here, and that is a property of this exporter rather than a policy choice: every record is written with an index operation, which Elasticsearch answers with 200 or 201. There is no create returning 409 for a duplicate or delete returning 404 to weigh, so the band is the same one the response's own HTTP status uses, pinned at 200/299/300 by a case.

The check stops there. It never asks what an operation's status means beyond the band, and it does not read error bodies under errors:false, since the status already covers them.

An earlier revision of this branch had the count check and dropped it, on the grounds that a filter_path response need not carry items. That reasoning does not apply here: this exporter never sends filter_path. If it ever does, the request side should say so rather than the parser accepting two incompatible response shapes.

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 500 with {"errors":false} was reported as kSuccess. 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 for Export to pass in.

Structure

The helper lives in include/opentelemetry/exporters/elasticsearch/detail/es_bulk_response.h so tests can reach it, following the detail/ pattern already used by ext/http/client/detail/default_factory.h. It is a detail header rather than an anonymous-namespace function in the .cc (an earlier revision put it there, which is why it could not be tested).

It is excluded from the installed package, both the header and the detail directory, since the file name pattern alone still leaves an empty directory behind in the package. The component's *.h glob would otherwise ship it, and it includes <nlohmann/json.hpp>, which would make nlohmann a public dependency of the installed Elasticsearch headers for the first time. es_log_recordable.h is the precedent one line above in the same call: it is the only other header here that includes nlohmann, and it is already excluded for the same reason. Verified by installing into a clean prefix and listing what lands under include/opentelemetry/exporters/elasticsearch, which is now just es_log_record_exporter.h.

Worth your call rather than mine: the only other detail/ directory in the tree is ext/http/client/detail, and it is installed today, so detail/ has meant "public but unstable" here rather than "private". #4327 stops installing it, which would make this exclusion the convention rather than the exception, but that PR is not merged and I would rather not lean on it. By the reading that holds today the file belongs in src/ instead, which would need no exclusion at all. I kept it where the tests can reach it and excluded it, and I am happy to move it if you prefer the convention held.

On the Bazel side the header is not in the exporter target's hdrs. hdrs is a target's public interface, so listing it there would let a Bazel consumer include a header the CMake package deliberately does not install. It has its own target, visible only to this package, and the exporter reaches it through implementation_deps so it is not re-exported; the test depends on it directly.

Checked rather than assumed. A consumer that depends only on :es_log_record_exporter:

fatal error: opentelemetry/exporters/elasticsearch/detail/es_bulk_response.h: No such file or directory

and the same consumer with :es_bulk_response added builds. This is the repository's first implementation_deps and its first target level visibility, so say if you would rather the header simply stayed in hdrs and the two package surfaces differed, the way //ext:headers currently does.

One other thing in this diff that is not strictly part of the fix: the bulk URI drops ?pretty. The old check depended on pretty printed whitespace, so asking the server for it no longer serves any purpose, but it does change the request and I would rather name it than have it found.

Guarded with OPENTELEMETRY_HAVE_EXCEPTIONS so the try/catch compiles under the -fno-exceptions Bazel config. <nlohmann/json.hpp> stays included by the .cc because it still calls GetJSON().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:true with no extractable item error, malformed and empty bodies, and a missing or non-boolean errors field. The 2xx range is pinned at its 199/200/299/300 boundaries. The acknowledgement count is pinned in both directions: no items, items:null, too few, too many, and the exact count. Entries that do not acknowledge an index operation are pinned separately: null entries, scalars, a nested array, {}, an action the exporter never submitted, index holding a scalar, two members in one entry, two members where one of them is valid, index with no status, and the same null shape with errors:true, which has to fail whichever way the flag reads. That case is mutation checked: removing the index lookup from the helper while leaving the member count and the status requirement turns it red, so it pins the operation identity rather than the JSON shape around it. A separate case covers the response contradicting itself: a 400 under errors:false, one rejection among several acknowledgements, and the 200/299/300 band boundary. The shared success and rejection fixtures now carry the status and _index a real bulk response has; they were abridged to what the old substring check looked at.

Now that #4298 has landed, a fake session that responds from inside SendRequest() no longer deadlocks the synchronous path, so three cases run through the exporter itself rather than through the helper:

• an accepted bulk response is a successful export,
• the rejected item from [BUG] Elasticsearch exporter decides bulk success by substring instead of the errors field #4295, whose shard counter still reads "failed" : 0, is a failed export,
• a 500 carrying a body the parser would otherwise accept is a failed export.

The third is the one the helper tests cannot give you. A handler that stored a fixed status, or an Export() that never asked for one, passes every helper case.

Those three drive the synchronous path, and they skip in the configuration the coverage job builds. code.coverage configures all-options-abiv2-preview, which turns on ENABLE_ASYNC_EXPORT, so nothing there called Export() at all and six lines of this change went unexecuted: the handler's submitted_operations_ member, the bulk URI, the handler construction, and the parse with its failure log.

Two more cases cover that path. Export() returns before a response arrives there, so the parsed result decides only which internal log line is written, and they read it through a captured log handler the way batch_span_processor_test does: an accepted response logs no export failure, a rejected one does. Measured with lcov on the coverage preset, the six lines go from zero hits to covered. Of the two, only the rejected one discriminates: putting the substring check back on the asynchronous path makes it fail, because "failed" : 0 appears in a body that has a rejected item.

[  FAILED  ] ElasticsearchLogsExporterAsyncTests.ARejectedResponseIsReportedAsAFailure

The accepted one passes either way and is there as its control.

Both sets are fixtures that skip in SetUp rather than cases that compile out. gtest_add_tests registers from the source, so a case missing from the binary is still handed to CTest, and a gtest filter that matches nothing exits zero, which reports a pass without running. Putting the skip in SetUp rather than at the top of each body also keeps GTEST_SKIP, which returns, from leaving the rest of a body unreachable, which MSVC reports as C4702 and the maintainer mode jobs turn into an error.

The catch (...) guard in the helper stays uncovered. It is unreachable by input, since the parser rejects malformed bodies rather than throwing, and reaching it needs allocation fault injection, which has no precedent here.

The fake client is the same one #4331 adds to this file. Whichever lands first, the other drops the duplicate when it rebases.

Verification

• [ PASSED ] 16 tests. with WITH_ELASTICSEARCH=ON: eleven helper cases, the recordable and client construction cases, and the three above. The two asynchronous cases skip in that configuration; WITH_ASYNC_EXPORT_PREVIEW=ON runs them and skips the three synchronous ones, [ PASSED ] 15 tests. Both configurations build with no warnings under OTELCPP_MAINTAINER_MODE=ON.
• Reverting Export() to the old substring check and rebuilding fails exactly the two cases that should discriminate:

[  FAILED  ] ElasticsearchLogsExporterWiringTests.RejectedItemIsAFailedExport
[  FAILED  ] ElasticsearchLogsExporterWiringTests.ServerErrorIsAFailedExportEvenWithAnAcceptedBody

AcceptedBulkResponseIsASuccessfulExport still passes there, which is correct: the happy path works under either check, so it is not a discriminator.

• Success is decided from the compact /_bulk response; parsing no longer depends on pretty-printing.

• ./ci/do_ci.sh format exits 0 with no diff (clang-format 18, cmake-format 0.6.13, buildifier 3.5.0).

• The lines the coverage report still marks in es_bulk_response.h are the defensive
  catch (...) that keeps anything from escaping the noexcept response handlers. No response
  body reaches it, since the parser rejects malformed and invalid-UTF-8 input before any
  throwing call, so exercising it would mean injecting an allocation failure through a global
  operator new override. That is program-wide machinery this repository does not use
  elsewhere, and it behaves differently in the shared-library configurations, so I left the
  guard uncovered rather than add it. I can add it if you would rather have the coverage.

• clang-tidy was measured against main rather than in isolation, and over the test target so that the test file is compiled as well as the exporter. On the all-options-abiv2-preview preset both trees report the same three checks and the same twenty two warning lines, and nothing in es_bulk_response.h, so the branch adds nothing to warning_limit. The include-what-you-use jobs are green on all three presets.

[codecov]

Codecov Report

❌ Patch coverage is 94.11765% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.17%. Comparing base (11fa0db) to head (09848c8).
⚠️ Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
.../exporters/elasticsearch/detail/es_bulk_response.h	93.55%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4297      +/-   ##
==========================================
- Coverage   81.29%   81.17%   -0.12%     
==========================================
  Files         447      451       +4     
  Lines       19028    19250     +222     
==========================================
+ Hits        15467    15624     +157     
- Misses       3561     3626      +65     

Files with missing lines	Coverage Δ
...orters/elasticsearch/src/es_log_record_exporter.cc	50.38% <100.00%> (+38.17%)	⬆️
.../exporters/elasticsearch/detail/es_bulk_response.h	93.55% <93.55%> (ø)

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.