Skip to content

Commit 3fc79c5

Browse files
committed
[BUG] Decide Elasticsearch bulk export success from status and errors
Both export paths decided whether a bulk write had succeeded by searching the response body for the literal `"failed" : 0`. That field is per-shard information for a single item rather than the outcome of the batch, which is the top-level `errors` flag, and the literal baked in pretty-printing whitespace besides. It was wrong in both directions: a batch holding a rejected item read as success, and a compact successful response read as failure. One helper, `IsBulkResponseSuccessful(status_code, body, expected_items, reason)`, now answers for both paths. A non-2xx status fails first. Then the body has to be a JSON object carrying a boolean `errors` flag and an `items` array with one operation result per operation the request submitted, and `errors` has to be false. A malformed body is a failure rather than a silent success, and the reason the helper reports is the one written to the log. Squashed from fourteen commits so the rebase onto main resolves once rather than at every step. No content change: the tree is identical to the one the fourteen produced. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
1 parent 30322cf commit 3fc79c5

6 files changed

Lines changed: 1009 additions & 40 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -210,6 +210,11 @@ Increment the:
210210
* [BUG] Wait on a completion state in the Elasticsearch exporter
211211
[#4298](https://github.com/open-telemetry/opentelemetry-cpp/pull/4298)
212212

213+
* [BUG] Decide Elasticsearch bulk export success from the HTTP status, the
214+
errors flag, and one acknowledgement per record that names its target index
215+
and says the operation applied
216+
[#4297](https://github.com/open-telemetry/opentelemetry-cpp/pull/4297)
217+
213218
* [BUG] Make SocketAddr string parsing safe and reject malformed addresses
214219
[#4292](https://github.com/open-telemetry/opentelemetry-cpp/pull/4292)
215220

‎exporters/elasticsearch/BUILD‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,24 @@ load("@rules_cc//cc:cc_test.bzl", "cc_test")
66

77
package(default_visibility = ["//visibility:public"])
88

9+
# The bulk response parser is an implementation detail that the CMake package
10+
# also leaves out. hdrs is a target's public interface, so it is kept in its own
11+
# target that only this package can depend on, and reached through
12+
# implementation_deps so the exporter does not re-export it.
13+
cc_library(
14+
name = "es_bulk_response",
15+
hdrs = [
16+
"include/opentelemetry/exporters/elasticsearch/detail/es_bulk_response.h",
17+
],
18+
strip_include_prefix = "include",
19+
tags = ["es"],
20+
visibility = ["//exporters/elasticsearch:__pkg__"],
21+
deps = [
22+
"//api",
23+
"@github_nlohmann_json//:json",
24+
],
25+
)
26+
927
cc_library(
1028
name = "es_log_record_exporter",
1129
srcs = [
@@ -16,6 +34,7 @@ cc_library(
1634
"include/opentelemetry/exporters/elasticsearch/es_log_record_exporter.h",
1735
"include/opentelemetry/exporters/elasticsearch/es_log_recordable.h",
1836
],
37+
implementation_deps = [":es_bulk_response"],
1938
linkopts = select({
2039
"//bazel:windows": [
2140
"-DEFAULTLIB:advapi32.lib",
@@ -43,6 +62,7 @@ cc_test(
4362
"test",
4463
],
4564
deps = [
65+
":es_bulk_response",
4666
":es_log_record_exporter",
4767
"@com_google_googletest//:gtest_main",
4868
"@curl",

‎exporters/elasticsearch/CMakeLists.txt‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,12 @@ otel_add_component(
3434
"*.h"
3535
PATTERN
3636
"es_log_recordable.h"
37+
EXCLUDE
38+
PATTERN
39+
"es_bulk_response.h"
40+
EXCLUDE
41+
PATTERN
42+
"detail"
3743
EXCLUDE)
3844

3945
if(OTELCPP_INSTALL)
@@ -55,4 +61,10 @@ if(OTELCPP_BUILD_TESTING)
5561
TARGET es_log_record_exporter_test
5662
TEST_PREFIX exporter.
5763
TEST_LIST es_log_record_exporter_test)
64+
65+
# The synchronous wiring cases wait on a completion, and a regression that
66+
# stops delivering one hangs the job rather than reporting. A bound turns that
67+
# back into a red test. Not the asynchronous ones: their fake calls back from
68+
# inside SendRequest() and Export() returns without waiting.
69+
set_tests_properties(${es_log_record_exporter_test} PROPERTIES TIMEOUT 120)
5870
endif() # OTELCPP_BUILD_TESTING
Lines changed: 208 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,208 @@
1+
// Copyright The OpenTelemetry Authors
2+
// SPDX-License-Identifier: Apache-2.0
3+
4+
#pragma once
5+
6+
#include <cstddef>
7+
#include <nlohmann/json.hpp>
8+
#include <string>
9+
10+
#include "opentelemetry/version.h"
11+
12+
OPENTELEMETRY_BEGIN_NAMESPACE
13+
namespace exporter
14+
{
15+
namespace logs
16+
{
17+
namespace detail
18+
{
19+
20+
/**
21+
* What the bulk request itself has to answer with. Elasticsearch answers a bulk request with 200,
22+
* and the rest of the 2xx band does not say the batch was written. 202 is the one that matters:
23+
* it says the request was accepted and the processing is not finished, and may never be. Reading
24+
* that as success is not a conservative reading of a vague answer, it is the opposite of what the
25+
* answer says, and a success here lets the caller drop the records it just handed over.
26+
*/
27+
inline bool IsSuccessfulBulkStatus(int status_code) noexcept
28+
{
29+
return 200 == status_code;
30+
}
31+
32+
/**
33+
* Whether an acknowledged operation status is one that applied the operation. Elasticsearch
34+
* answers 201 when it created the document and 200 when it replaced an existing one.
35+
*
36+
* Comparing the json value rather than extracting one keeps this noexcept without relying on the
37+
* type check to make an extraction safe. The check still has to be here: without it a float 200.5
38+
* would answer for 200.
39+
*/
40+
inline bool IsAcknowledgedStatus(const nlohmann::json &status) noexcept
41+
{
42+
return status.is_number_integer() && (status == 200 || status == 201);
43+
}
44+
45+
/**
46+
* Decide whether an Elasticsearch bulk response reports the whole batch as written.
47+
*
48+
* Callers include noexcept response handlers, so nothing may escape. Anything that stops the body
49+
* being inspected counts as a failed export.
50+
*
51+
* This reads the fields the bulk response documents to decide an outcome. It is not a check
52+
* against a responder that is trying to be believed: a repeated key, for one, is folded before
53+
* this sees the document, so a body carrying both "errors": true and "errors": false arrives as
54+
* whichever one the parser kept, and nothing here can tell that the other was ever sent.
55+
*
56+
* @param status_code the response status, which the caller passes rather than this reading the
57+
* body alone: "errors" describes item outcomes and cannot override a transport error
58+
* @param body the raw response body
59+
* @param expected_items the number of index operations the request submitted
60+
* @param failure_reason a best-effort explanation when this returns false
61+
* @return true only when the status is 200, "errors" is false, and "items" holds exactly
62+
* expected_items index results that each name a target index, acknowledge a status the
63+
* operation applied under, and carry no error
64+
*/
65+
inline bool IsBulkResponseSuccessful(int status_code,
66+
const std::string &body,
67+
std::size_t expected_items,
68+
std::string &failure_reason) noexcept
69+
{
70+
failure_reason.clear();
71+
#if OPENTELEMETRY_HAVE_EXCEPTIONS
72+
try
73+
{
74+
#endif
75+
// Inside the try: building a reason allocates.
76+
// Not the band an operation status is held to. This one answers for the request, and an
77+
// operation that applied answers 201 for a document it created, which the request itself
78+
// never says.
79+
if (!IsSuccessfulBulkStatus(status_code))
80+
{
81+
failure_reason = "unexpected HTTP status " + std::to_string(status_code);
82+
return false;
83+
}
84+
85+
const nlohmann::json parsed = nlohmann::json::parse(body, nullptr, false);
86+
if (parsed.is_discarded() || !parsed.is_object())
87+
{
88+
failure_reason = "the response body is not a JSON object";
89+
return false;
90+
}
91+
92+
const auto errors = parsed.find("errors");
93+
if (errors == parsed.end() || !errors->is_boolean())
94+
{
95+
failure_reason = "the response body has no boolean \"errors\" field";
96+
return false;
97+
}
98+
99+
// The request is an unfiltered /_bulk, so "items" is always there and holds one entry per
100+
// operation. The next three checks decide whether this body answers the request that was sent.
101+
const auto items = parsed.find("items");
102+
if (items == parsed.end() || !items->is_array())
103+
{
104+
failure_reason = "the response body has no \"items\" array";
105+
return false;
106+
}
107+
108+
if (items->size() != expected_items)
109+
{
110+
failure_reason = "the response acknowledges " + std::to_string(items->size()) + " of " +
111+
std::to_string(expected_items) + " submitted operations";
112+
return false;
113+
}
114+
115+
// Each entry is keyed by the action it answers, and every record goes out as an index
116+
// operation. Read before "errors", which a body that is not this answer does not get to decide.
117+
// An operation that was not applied says so twice, through its status and through an "error"
118+
// member. Both are read here so that neither reading depends on the "errors" flag, which a
119+
// response that contradicts itself also controls.
120+
const nlohmann::json *rejected = nullptr;
121+
const nlohmann::json *item_error = nullptr;
122+
for (const auto &item : *items)
123+
{
124+
if (!item.is_object() || item.size() != 1)
125+
{
126+
failure_reason = "the response has an \"items\" entry that is not one operation result";
127+
return false;
128+
}
129+
130+
const auto operation = item.find("index");
131+
if (operation == item.end() || !operation->is_object())
132+
{
133+
failure_reason = "the response does not acknowledge the submitted index operation";
134+
return false;
135+
}
136+
137+
// Elasticsearch names the index it wrote to in every index result, whether the operation
138+
// applied or not. Presence and type only: the name it reports is the index the write
139+
// resolved to, which an alias or a date math index makes different from the one that was
140+
// submitted, so it is not something to compare against.
141+
const auto target = operation->find("_index");
142+
if (target == operation->end() || !target->is_string())
143+
{
144+
failure_reason = "the response acknowledges an index operation with no target index";
145+
return false;
146+
}
147+
148+
const auto status = operation->find("status");
149+
if (status == operation->end() || !status->is_number_integer())
150+
{
151+
failure_reason = "the response acknowledges an index operation with no status";
152+
return false;
153+
}
154+
155+
if (rejected == nullptr && !IsAcknowledgedStatus(*status))
156+
{
157+
rejected = &(*status);
158+
}
159+
160+
// A null holds no cause, and a serialiser that writes absent optionals as null is saying
161+
// the operation applied, which is what the flag says too. Only a cause contradicts it.
162+
const auto error = operation->find("error");
163+
if (item_error == nullptr && error != operation->end() && !error->is_null())
164+
{
165+
item_error = &(*error);
166+
}
167+
}
168+
169+
if (!errors->get<bool>())
170+
{
171+
// A false flag claims every operation was applied; hold the items to that claim.
172+
if (rejected != nullptr)
173+
{
174+
failure_reason =
175+
"the response reports no errors but acknowledges operation status " + rejected->dump();
176+
return false;
177+
}
178+
if (item_error != nullptr)
179+
{
180+
failure_reason =
181+
"the response reports no errors but an item carries error " + item_error->dump();
182+
return false;
183+
}
184+
return true;
185+
}
186+
187+
// Name the first item error rather than only saying that something failed.
188+
if (item_error != nullptr)
189+
{
190+
failure_reason = "at least one item failed, first error: " + item_error->dump();
191+
return false;
192+
}
193+
194+
failure_reason = "the response reports errors";
195+
return false;
196+
#if OPENTELEMETRY_HAVE_EXCEPTIONS
197+
}
198+
catch (...)
199+
{
200+
return false;
201+
}
202+
#endif
203+
}
204+
205+
} // namespace detail
206+
} // namespace logs
207+
} // namespace exporter
208+
OPENTELEMETRY_END_NAMESPACE

0 commit comments

Comments
 (0)