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

[thc1006]

Fixes #4295

Short summary for review: #4297 (comment)

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. The parser holds each acknowledgement to a string _index and an integer status, and stops there. _index is checked for presence and type only, never compared: an alias resolves to the concrete index behind it, so the name a conforming server reports is not always the name the request was addressed to. Nothing beyond _index, status and a non-null error changes the verdict.

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 on both layers, and that is a compatibility choice rather than a property of the endpoint. Elasticsearch documents the bulk response as HTTP 200, and an index operation as 201 for a new document or 200 for an overwrite, so 202, 206 and 299 are values a compliant server will not send. Accepting the band keeps the check working against proxies and Elasticsearch compatible servers that answer inside it, and it costs nothing here because every record is written with an index operation: there is no create returning 409 for a duplicate or delete returning 404 that could hide in the widened range. The boundaries are pinned at 199/200/299/300 by a case, so narrowing this to 200 and 200/201 later is a two line change with its edges already held. Say if you would rather have it narrow now.

The band is compared in the type the number was parsed as, not through an int. The value comes from the server, is_number_integer() holds for unsigned as well, and nlohmann does not range check get<int>(), so 2^32 + 200 narrows straight back into the band on a 32 bit int and reads as applied. Comparing directly also removes the need for a rejected-status sentinel: an earlier revision returned the first rejected status as an int and used 0 to mean "nothing was rejected", which cannot tell those apart, so {"errors":false,"items":[{"index":{"status":0}}]} was reported as a successful write. Both are covered by cases at 0, -0, -1, 199, 200, 201, 299, 300, 2^32 + 200 and UINT64_MAX, plus a non-integer and a string.

The status is what makes the response two things rather than one, so the synchronous handler now keeps both halves together. It publishes the status and the body only on the transition that records the outcome, and Export takes them in a single call. Reading them one lock at a time, with each response overwriting them unconditionally, let a client that delivers two responses for one request pair a status from one with a body from the other, and that pair is what the verdict is computed from. The asynchronous handler keeps its body in a local for the same reason, since nothing outside the call reads it.

An operation that did not apply says so twice, and both are read the same way whichever value the flag has. Elasticsearch documents error as present only on a failed operation, and the errors:true path already relied on that to name the first failure. Under errors:false it was ignored, so {"index":{"status":201,"error":{"type":"mapper_parsing_exception"}}} was accepted while the same body with errors:true was rejected. That is the same shape as the items entry problem above: one body, two verdicts, chosen by a flag the broken responder also controls. The status alone does not cover it, since the contradiction is between the status and the error rather than between the status and the flag. Both signals are now collected in the pass that already validates the items, which also removes the second walk the errors:true branch needed.

A null member is not a cause. find reports a key that is present holding null, and a serialiser that writes absent optionals that way is saying the operation applied, which is what the flag says too, so rejecting it would fail exports that are fine and catch nothing. Both shapes have a case.

The check stops there. It never asks what an operation's status means beyond the band, and it reads no other member of the result.

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 stopped installing it when it merged on 4 August, so excluding this one follows the convention rather than standing against it. On the reading that held before that, the file would belong 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 would rather it were not reachable as a header at all.

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 accepted statuses are pinned on both sides and for both places they are read: 199, 200, 201, 202, 299 and 300 against the request, where only 200 passes, and the same set against an operation, where 200 and 201 pass. 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 a response pairing the two statuses an applied operation answers with. 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.

Three things a later reading turned up

One success band, said once. The response status was compared as a plain int against 200 and 299, and each operation status went through a separate predicate that compares in the type nlohmann parsed it as. Same band, two spellings, and only one of them safe against a number the server chooses. Both go through one pair of overloads now, documented in one place, and the response status reaches the signed one through a cast an int cannot lose anything to. The two overloads are not one signed parameter because an operation status arrives as JSON: is_number_integer() is true for unsigned as well, and 2^32 + 200 narrows back into the band on a 32 bit int.

Worth saying that the HTTP side was never at risk of that. StatusCode is a uint16_t, so reaching the int parameter widens rather than narrows. What changed is that one rule is now written once rather than twice.

An acknowledgement has to name the index it wrote to. Every Elasticsearch index result carries _index, on a failed operation as much as an applied one, so a result without one is not an answer to an index operation. This now requires it, and that is a tightening: a backend that answers a bulk request without _index in its index results was accepted before and is a failed export now.

Presence and type only, deliberately. The name reported is the index the write resolved to, and an alias or a date math index makes that different from the one submitted, so comparing it against options_.index_ would fail an ordinary setup. That is also why the exporter's own index name is not passed down.

The cases had no bound. gtest_add_tests registered them with no timeout, and what these cases catch fails by hanging rather than by asserting, which stops the job instead of reporting. They carry a 120 second CTest timeout now: 27 registered entries, three of which the suite disables and CTest therefore skips, and all 24 enabled ones carry it, read back from ctest --show-only=json-v1 rather than assumed from having written it.

Against the same header with the target index check taken out, the case that holds it fails on all three of its new shapes: no _index at all, an _index that is a number, and one that is null.

Verification

• Twenty four cases, none failing in any configuration, and 100% of them passing through ctest -R exporter.Elasticsearch. WITH_ELASTICSEARCH=ON gives [ PASSED ] 20 tests. with the four asynchronous cases skipped, and WITH_ASYNC_EXPORT_PREVIEW=ON gives [ PASSED ] 19 tests. with the five synchronous wiring cases skipped instead. A build with error logging compiled out, -DOTEL_INTERNAL_LOG_LEVEL=0, gives [ PASSED ] 15 tests. and skips the nine that read the outcome out of the log, because below error level an absent log cannot be told from one that was never written. Removing that skip makes two of them fail on correct code, 2 of 2. All 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). Worth running rather than cmake-format --check: the job reformats in place and diffs, and it wraps a comment narrower than --check accepts with the same config, so --check reported clean on a file the job then rewrote.

• 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 96.15385% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.91%. Comparing base (60c3d11) to head (4a4b935).

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4297      +/-   ##
==========================================
+ Coverage   82.61%   82.91%   +0.30%     
==========================================
  Files         511      512       +1     
  Lines       20132    20205      +73     
==========================================
+ Hits        16631    16750     +119     
+ Misses       3501     3455      -46     

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	95.78% <95.78%> (ø)

🚀 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.

[thc1006]

Eighteen days and no human has looked at this yet. The one review on it is Copilot reporting that it could not run because the requester was out of quota, and CI has been green throughout. So here is the short entry point that CONTRIBUTING.md asks for, because the description above is long and I suspect that is part of why this is easier to skip than to start.

What it does. Both export paths decided a bulk write had succeeded with body.find("\"failed\" : 0"). That substring is per shard information about one item rather than the batch outcome, and it bakes in pretty printed 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. One helper now decides it from the HTTP status, the top level errors flag, and one acknowledgement per record the request submitted.

Where to look, if you would rather spot check than read it all.

• exporters/elasticsearch/src/es_log_record_exporter.cc, +50/-40, is the entire production change. Everything else is the new helper, its tests, and build files.
• detail/es_bulk_response.h is the file worth reading. It is the whole decision in one function.
• The behaviour a user would notice: a response that does not acknowledge every record is now a failure rather than a success. That is the point of the change, and the description says why an exporter claiming success without an acknowledgement is the worse direction to be wrong in.
• The synchronous path used to log a non-2xx status and still return success, so HTTP 500 with {"errors":false} was reported as kSuccess. That is fixed here too.

Two choices I flagged as open, which I am now closing rather than leaving on you. I raised them because both are firsts for this repository, not because I think they are wrong, and re-reading the description I can see that asking for two decisions on top of a review is a good reason to defer the whole thing.

1. The helper is a detail/ header, excluded from the installed package. It is a header rather than an anonymous namespace function in the .cc so the tests can reach it, which an earlier revision could not. The install output is verified against a clean prefix.
2. On the Bazel side it has its own package private target reached through implementation_deps, so a Bazel consumer cannot include a header the CMake package deliberately does not install. Checked both ways: a consumer depending only on :es_log_record_exporter fails to find the header, and the same consumer with :es_bulk_response builds.

Both stay as they are unless you would rather they did not. Say the word on either and I will change it, but nothing is waiting on an answer.

If this is still unlooked-at once it leaves draft I will bring it to the C++ SIG meeting, which is what CONTRIBUTING.md suggests at this point. Not a nudge, just so you know where it goes next rather than it sitting here indefinitely.

Edited 16 Aug. It is a draft now, and that is my doing rather than anything about the code. Reading it alongside #4331 and #4337 turned up two gaps I had not pinned: no asynchronous case carried a non-200 status through to the parser, and no successful case carried more than one record, so either could have been hard-coded without a test noticing. Both are measured now, with the mutation that used to pass and no longer does. The original text said 18 August; that clock is paused while it sits in draft, and I have folded the follow-up comment in here rather than leave two to read.