Skip to content

Commit 4a4b935

Browse files
committed
[CHORE] Hold the status helper to its own precondition, and take the log level with the handler
Three small ones. IsAcknowledgedStatus() is noexcept and reached straight for get(), leaving the type check to whoever called it. The one caller does check, so nothing is wrong today, but the two ways a later one could get it wrong are not symmetrical: a string throws, and out of a noexcept function that is std::terminate rather than a wrong answer, while a float does not throw at all, it truncates, so 200.5 would have answered for 200. The check is now where the assumption is. The asynchronous cases saved and restored the log handler but not the log level. The level is process wide, another case in the same binary can lower it, and a failure the runtime filtered out looks exactly like one the parser never reported, so they would have stopped discriminating without saying so. This is not hypothetical in this file: the debug case added earlier passed for that reason until the level went in beside the handler. The header now says what it does not do. A repeated key is folded before the document reaches this, so a body carrying both "errors": true and "errors": false arrives as whichever one the parser kept, and nothing here can tell the other was sent. Deciding an outcome from the documented fields is what this is for; standing up to a responder that is trying to be believed is not. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>
1 parent c8a6715 commit 4a4b935

2 files changed

Lines changed: 33 additions & 1 deletion

File tree

‎exporters/elasticsearch/include/opentelemetry/exporters/elasticsearch/detail/es_bulk_response.h‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,21 @@ inline bool IsAppliedStatus(nlohmann::json::number_integer_t value) noexcept
4747
return 200 == value || 201 == value;
4848
}
4949

50-
/** Whether an acknowledged operation status is one that applied the operation. */
50+
/**
51+
* Whether an acknowledged operation status is one that applied the operation.
52+
*
53+
* The type check is here rather than left to the caller, although the one caller does it too.
54+
* This is noexcept, and get() on a value that is not a number throws, so a caller that forgot
55+
* would not get a wrong answer, it would get std::terminate. A float does not throw: it
56+
* truncates, so 200.5 would answer for 200.
57+
*/
5158
inline bool IsAcknowledgedStatus(const nlohmann::json &status) noexcept
5259
{
60+
if (!status.is_number_integer())
61+
{
62+
return false;
63+
}
64+
5365
if (status.is_number_unsigned())
5466
{
5567
return IsAppliedStatus(status.get<nlohmann::json::number_unsigned_t>());
@@ -64,6 +76,11 @@ inline bool IsAcknowledgedStatus(const nlohmann::json &status) noexcept
6476
* Callers include noexcept response handlers, so nothing may escape. Anything that stops the body
6577
* being inspected counts as a failed export.
6678
*
79+
* This reads the fields the bulk response documents to decide an outcome. It is not a check
80+
* against a responder that is trying to be believed: a repeated key, for one, is folded before
81+
* this sees the document, so a body carrying both "errors": true and "errors": false arrives as
82+
* whichever one the parser kept, and nothing here can tell that the other was ever sent.
83+
*
6784
* @param status_code the response status, which the caller passes rather than this reading the
6885
* body alone: "errors" describes item outcomes and cannot override a transport error
6986
* @param body the raw response body

‎exporters/elasticsearch/test/es_log_record_exporter_test.cc‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -668,13 +668,25 @@ class ElasticsearchLogsExporterAsyncTests : public ::testing::Test
668668
handler_ = nostd::shared_ptr<opentelemetry::sdk::common::internal_log::LogHandler>(
669669
new CapturingLogHandler());
670670
opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler(handler_);
671+
672+
// The level as well as the handler. It is process wide and another case in this binary can
673+
// lower it, and a failure filtered out at runtime looks exactly like one the parser never
674+
// reported, so these cases would stop discriminating without saying so.
675+
previous_level_ = opentelemetry::sdk::common::internal_log::GlobalLogHandler::GetLogLevel();
676+
opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogLevel(
677+
opentelemetry::sdk::common::internal_log::LogLevel::Error);
678+
restore_level_ = true;
671679
#endif
672680
}
673681

674682
void TearDown() override
675683
{
676684
// Put back what was found, rather than a fresh default that would displace a handler another
677685
// case in this binary had installed.
686+
if (restore_level_)
687+
{
688+
opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogLevel(previous_level_);
689+
}
678690
if (handler_)
679691
{
680692
opentelemetry::sdk::common::internal_log::GlobalLogHandler::SetLogHandler(previous_handler_);
@@ -697,6 +709,9 @@ class ElasticsearchLogsExporterAsyncTests : public ::testing::Test
697709

698710
nostd::shared_ptr<opentelemetry::sdk::common::internal_log::LogHandler> handler_;
699711
nostd::shared_ptr<opentelemetry::sdk::common::internal_log::LogHandler> previous_handler_;
712+
opentelemetry::sdk::common::internal_log::LogLevel previous_level_ =
713+
opentelemetry::sdk::common::internal_log::LogLevel::Warning;
714+
bool restore_level_ = false;
700715
};
701716

702717
class ElasticsearchLogsExporterDebugLoggingTests : public ::testing::Test

0 commit comments

Comments
 (0)