From 4f56aa334c580992e6947fc739a27cabac1e735b Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 3 Aug 2026 01:48:45 +0800 Subject: [PATCH 1/7] [BUG] End the Elasticsearch exporter's wait on a read or write error ReadError and WriteError left completion_ at Pending, so if either was the last callback the synchronous Export() waited until its predicate could never become true. Both now record CompletionState::Failure like the other terminal states, and are logged at error level to match them. Neither state is emitted by the in-tree curl client, so this only reaches a consumer that supplies its own HTTP client through the factory, but for that consumer the export never returned. The new cases drive a fake HTTP client that delivers its callbacks from inside SendRequest(), before Export() reaches the wait, so they also cover a completion recorded ahead of the waiter: a response, a read error, a write error, a session destroyed while pending, and a session destroyed after a response, which stays successful because the first outcome recorded is the one reported. Fixes #4330 --- CHANGELOG.md | 3 + .../src/es_log_record_exporter.cc | 6 +- .../test/es_log_record_exporter_test.cc | 169 ++++++++++++++++++ 3 files changed, 176 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 07b490d4b0..7765d3beaa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,9 @@ Increment the: ## [Unreleased] +* [BUG] End the Elasticsearch exporter's wait on a read or write error + [#4331](https://github.com/open-telemetry/opentelemetry-cpp/pull/4331) + * [BUILD] Run the ext_http component install test on Windows [#4326](https://github.com/open-telemetry/opentelemetry-cpp/pull/4326) diff --git a/exporters/elasticsearch/src/es_log_record_exporter.cc b/exporters/elasticsearch/src/es_log_record_exporter.cc index a87a514f01..9679e53578 100644 --- a/exporters/elasticsearch/src/es_log_record_exporter.cc +++ b/exporters/elasticsearch/src/es_log_record_exporter.cc @@ -182,10 +182,12 @@ class ResponseHandler : public http_client::EventHandler recordCompletion(CompletionState::Failure); break; case http_client::SessionState::ReadError: - OTEL_INTERNAL_LOG_DEBUG("[ES Log Exporter] Read error"); + OTEL_INTERNAL_LOG_ERROR("[ES Log Exporter] Read error"); + recordCompletion(CompletionState::Failure); break; case http_client::SessionState::WriteError: - OTEL_INTERNAL_LOG_DEBUG("[ES Log Exporter] Write error"); + OTEL_INTERNAL_LOG_ERROR("[ES Log Exporter] Write error"); + recordCompletion(CompletionState::Failure); break; case http_client::SessionState::Cancelled: OTEL_INTERNAL_LOG_ERROR("[ES Log Exporter] (manually) cancelled"); diff --git a/exporters/elasticsearch/test/es_log_record_exporter_test.cc b/exporters/elasticsearch/test/es_log_record_exporter_test.cc index a65c0b4c1c..c14ee8a9db 100644 --- a/exporters/elasticsearch/test/es_log_record_exporter_test.cc +++ b/exporters/elasticsearch/test/es_log_record_exporter_test.cc @@ -4,7 +4,9 @@ #include "opentelemetry/exporters/elasticsearch/es_log_record_exporter.h" #include "opentelemetry/common/timestamp.h" #include "opentelemetry/exporters/elasticsearch/es_log_recordable.h" +#include "opentelemetry/ext/http/client/http_client.h" #include "opentelemetry/logs/severity.h" +#include "opentelemetry/nostd/function_ref.h" #include "opentelemetry/nostd/span.h" #include "opentelemetry/nostd/string_view.h" #include "opentelemetry/nostd/utility.h" @@ -19,6 +21,8 @@ #include #include #include +#include +#include #include #include #include "nlohmann/json.hpp" @@ -142,3 +146,168 @@ TEST(ElasticsearchLogRecordableTests, BasicTests) EXPECT_EQ(actual, expected); } + +// --------------------------------------------------------------------------- +// Synchronous completion path. +// +// A fake HTTP client drives a scripted sequence of callbacks from inside +// SendRequest(), which runs before the exporter reaches waitForResponse(). Every +// case here therefore also covers a completion recorded before the wait starts, +// the notification a bare cv_.wait() would have missed. +// --------------------------------------------------------------------------- +namespace +{ +namespace http_client = opentelemetry::ext::http::client; + +// Accepted by both the substring check and a top level "errors": false parse, so +// these cases keep meaning the same thing whichever success check is in place. +constexpr const char *kAcceptedBody = + R"({"took":30,"errors":false,"items":[{"index":{"_shards":{"failed" : 0}}}]})"; + +class FakeResponse : public http_client::Response +{ +public: + FakeResponse(http_client::StatusCode status, const std::string &body) + : status_(status), body_(body.begin(), body.end()) + {} + const http_client::Body &GetBody() const noexcept override { return body_; } + bool ForEachHeader( + nostd::function_ref) const noexcept override + { + return true; + } + bool ForEachHeader( + const nostd::string_view &, + nostd::function_ref) const noexcept override + { + return true; + } + http_client::StatusCode GetStatusCode() const noexcept override { return status_; } + +private: + http_client::StatusCode status_; + http_client::Body body_; +}; + +class FakeRequest : public http_client::Request +{ +public: + void SetMethod(http_client::Method) noexcept override {} + void SetUri(nostd::string_view) noexcept override {} + void SetSslOptions(const http_client::HttpSslOptions &) noexcept override {} + void SetBody(http_client::Body &) noexcept override {} + void AddHeader(nostd::string_view, nostd::string_view) noexcept override {} + void ReplaceHeader(nostd::string_view, nostd::string_view) noexcept override {} + void SetTimeoutMs(std::chrono::milliseconds) noexcept override {} + void SetCompression(const http_client::Compression &) noexcept override {} + void EnableLogging(bool) noexcept override {} + void SetRetryPolicy(const http_client::RetryPolicy &) noexcept override {} +}; + +using EventScript = std::function; + +class FakeSession : public http_client::Session +{ +public: + explicit FakeSession(EventScript script) : script_(std::move(script)) {} + std::shared_ptr CreateRequest() noexcept override + { + return std::make_shared(); + } + void SendRequest(std::shared_ptr handler) noexcept override + { + script_(*handler); + } + bool IsSessionActive() noexcept override { return false; } + bool CancelSession() noexcept override { return true; } + bool FinishSession() noexcept override { return true; } + +private: + EventScript script_; +}; + +class FakeHttpClient : public http_client::HttpClient +{ +public: + explicit FakeHttpClient(EventScript script) : script_(std::move(script)) {} + std::shared_ptr CreateSession(nostd::string_view) noexcept override + { + return std::make_shared(script_); + } + bool CancelAllSessions() noexcept override { return true; } + bool FinishAllSessions() noexcept override { return true; } + void SetMaxSessionsPerConnection(std::size_t) noexcept override {} + +private: + EventScript script_; +}; + +opentelemetry::sdk::common::ExportResult ExportWith(EventScript script) +{ + auto client = std::make_shared(std::move(script)); + logs_exporter::ElasticsearchExporterOptions options; + logs_exporter::ElasticsearchLogRecordExporter exporter(options, client); + auto record = exporter.MakeRecordable(); + return exporter.Export(nostd::span>(&record, 1)); +} +} // namespace + +// The synchronous wait exists only when the exporter is built without async export, so these cases +// skip rather than compile out: gtest_add_tests reads the source, and a case that disappeared from +// the binary would still be registered with CTest. +#ifdef ENABLE_ASYNC_EXPORT +# define SKIP_WITHOUT_SYNC_EXPORT() \ + GTEST_SKIP() << "Export() returns without waiting when async export is enabled" +#else +# define SKIP_WITHOUT_SYNC_EXPORT() (void)0 +#endif + +TEST(ElasticsearchLogsExporterSyncTests, ResponseRecordedBeforeTheWaitIsStillSeen) +{ + SKIP_WITHOUT_SYNC_EXPORT(); + const auto result = ExportWith([](http_client::EventHandler &handler) { + FakeResponse response(200, kAcceptedBody); + handler.OnResponse(response); + }); + EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kSuccess); +} + +TEST(ElasticsearchLogsExporterSyncTests, ReadErrorEndsTheWait) +{ + SKIP_WITHOUT_SYNC_EXPORT(); + const auto result = ExportWith([](http_client::EventHandler &handler) { + handler.OnEvent(http_client::SessionState::ReadError, ""); + }); + EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kFailure); +} + +TEST(ElasticsearchLogsExporterSyncTests, WriteErrorEndsTheWait) +{ + SKIP_WITHOUT_SYNC_EXPORT(); + const auto result = ExportWith([](http_client::EventHandler &handler) { + handler.OnEvent(http_client::SessionState::WriteError, ""); + }); + EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kFailure); +} + +TEST(ElasticsearchLogsExporterSyncTests, SessionDestroyedWhilePendingEndsTheWait) +{ + SKIP_WITHOUT_SYNC_EXPORT(); + const auto result = ExportWith([](http_client::EventHandler &handler) { + handler.OnEvent(http_client::SessionState::Destroyed, ""); + }); + EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kFailure); +} + +// The first outcome recorded is the one reported, so tearing the session down after a response has +// arrived does not turn a successful export into a failure. +TEST(ElasticsearchLogsExporterSyncTests, SessionDestroyedAfterAResponseKeepsTheSuccess) +{ + SKIP_WITHOUT_SYNC_EXPORT(); + const auto result = ExportWith([](http_client::EventHandler &handler) { + FakeResponse response(200, kAcceptedBody); + handler.OnResponse(response); + handler.OnEvent(http_client::SessionState::Destroyed, ""); + }); + EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kSuccess); +} From 4dcfc60488d8cf3361ed8c332acb18e8dbcf2798 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 3 Aug 2026 02:46:24 +0800 Subject: [PATCH 2/7] Keep the I/O error logs at debug Recording a completion is first writer wins, so a read or write error that arrives after a response has already succeeded does not change the result Export() reports. Logging it at error level announced a failure the caller was never told about. Destroyed is the existing precedent in this switch: it ends the wait the same way and stays at debug for the same reason. Covered by IoErrorAfterAResponseKeepsTheSuccess. --- .../src/es_log_record_exporter.cc | 7 +++++-- .../test/es_log_record_exporter_test.cc | 18 +++++++++++++++++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/exporters/elasticsearch/src/es_log_record_exporter.cc b/exporters/elasticsearch/src/es_log_record_exporter.cc index 9679e53578..9f59db2d45 100644 --- a/exporters/elasticsearch/src/es_log_record_exporter.cc +++ b/exporters/elasticsearch/src/es_log_record_exporter.cc @@ -182,11 +182,14 @@ class ResponseHandler : public http_client::EventHandler recordCompletion(CompletionState::Failure); break; case http_client::SessionState::ReadError: - OTEL_INTERNAL_LOG_ERROR("[ES Log Exporter] Read error"); + OTEL_INTERNAL_LOG_DEBUG("[ES Log Exporter] Read error"); + // Kept at debug, like Destroyed above: recording is first writer wins, so this can + // arrive after a response has already succeeded, and an error line there would not + // describe the outcome Export() reports. recordCompletion(CompletionState::Failure); break; case http_client::SessionState::WriteError: - OTEL_INTERNAL_LOG_ERROR("[ES Log Exporter] Write error"); + OTEL_INTERNAL_LOG_DEBUG("[ES Log Exporter] Write error"); recordCompletion(CompletionState::Failure); break; case http_client::SessionState::Cancelled: diff --git a/exporters/elasticsearch/test/es_log_record_exporter_test.cc b/exporters/elasticsearch/test/es_log_record_exporter_test.cc index c14ee8a9db..e09152ba22 100644 --- a/exporters/elasticsearch/test/es_log_record_exporter_test.cc +++ b/exporters/elasticsearch/test/es_log_record_exporter_test.cc @@ -22,7 +22,6 @@ #include #include #include -#include #include #include #include "nlohmann/json.hpp" @@ -311,3 +310,20 @@ TEST(ElasticsearchLogsExporterSyncTests, SessionDestroyedAfterAResponseKeepsTheS }); EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kSuccess); } + +// Same rule for the two states this change makes terminal. It is why they log at debug rather +// than error: reaching one of them says nothing about the result Export() goes on to report. +TEST(ElasticsearchLogsExporterSyncTests, IoErrorAfterAResponseKeepsTheSuccess) +{ + SKIP_WITHOUT_SYNC_EXPORT(); + for (const auto state : + {http_client::SessionState::ReadError, http_client::SessionState::WriteError}) + { + const auto result = ExportWith([state](http_client::EventHandler &handler) { + FakeResponse response(200, kAcceptedBody); + handler.OnResponse(response); + handler.OnEvent(state, ""); + }); + EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kSuccess); + } +} From a8bcd0b4e8d60075e97383b4e805b3bcc54083e8 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 3 Aug 2026 03:16:49 +0800 Subject: [PATCH 3/7] Pin every terminal state, not just the two being fixed The seven states #4298 made terminal had no test. A future edit that drops one back to a bare log would only show up as a hung export in the field. The companion case covers the other direction: progress states must not decide the result on their own, or a response arriving after them would never be consulted. --- .../test/es_log_record_exporter_test.cc | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/exporters/elasticsearch/test/es_log_record_exporter_test.cc b/exporters/elasticsearch/test/es_log_record_exporter_test.cc index e09152ba22..54a6c74ed8 100644 --- a/exporters/elasticsearch/test/es_log_record_exporter_test.cc +++ b/exporters/elasticsearch/test/es_log_record_exporter_test.cc @@ -280,6 +280,48 @@ TEST(ElasticsearchLogsExporterSyncTests, ReadErrorEndsTheWait) EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kFailure); } +// The whole contract in one place. Every state that ends a session has to leave a result behind, +// otherwise a client that emits it last strands the wait. The seven states other than the two +// this change adds have behaved this way since #4298 but had no test. +// +// A regression here surfaces as a CTest timeout rather than a failed assertion, because a state +// that stops being terminal leaves Export() waiting with nothing left to wake it. +TEST(ElasticsearchLogsExporterSyncTests, EveryTerminalStateEndsTheWaitInFailure) +{ + SKIP_WITHOUT_SYNC_EXPORT(); + const http_client::SessionState terminal[] = { + http_client::SessionState::CreateFailed, http_client::SessionState::ConnectFailed, + http_client::SessionState::SendFailed, http_client::SessionState::SSLHandshakeFailed, + http_client::SessionState::TimedOut, http_client::SessionState::NetworkError, + http_client::SessionState::Cancelled, http_client::SessionState::ReadError, + http_client::SessionState::WriteError, http_client::SessionState::Destroyed}; + + for (const auto state : terminal) + { + SCOPED_TRACE(static_cast(state)); + const auto result = + ExportWith([state](http_client::EventHandler &handler) { handler.OnEvent(state, ""); }); + EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kFailure); + } +} + +// The other side of the same contract: a state that only reports progress must not complete the +// export on its own, or a response that arrives afterwards is never consulted. +TEST(ElasticsearchLogsExporterSyncTests, ProgressStatesDoNotDecideTheResult) +{ + SKIP_WITHOUT_SYNC_EXPORT(); + const auto result = ExportWith([](http_client::EventHandler &handler) { + handler.OnEvent(http_client::SessionState::Created, ""); + handler.OnEvent(http_client::SessionState::Connecting, ""); + handler.OnEvent(http_client::SessionState::Connected, ""); + handler.OnEvent(http_client::SessionState::Sending, ""); + handler.OnEvent(http_client::SessionState::Response, ""); + FakeResponse response(200, kAcceptedBody); + handler.OnResponse(response); + }); + EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kSuccess); +} + TEST(ElasticsearchLogsExporterSyncTests, WriteErrorEndsTheWait) { SKIP_WITHOUT_SYNC_EXPORT(); From 206c68f42467e57448064c15cab96ddb62ffc9f3 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 3 Aug 2026 03:21:22 +0800 Subject: [PATCH 4/7] Report a read or write error only when it is the result Choosing between an error line that can describe an export the caller was told succeeded, and a debug line the default log level hides, was a false choice. recordCompletion() now reports whether it was the write that decided the outcome, and the log follows that. So an I/O error that ends the export says so at error level, and one that arrives after a response has already succeeded says nothing. Also: give the cases a CTest timeout, since a wait that stops returning would otherwise stall the job rather than fail it; and drop the claim that the request timeout arrives as a TimedOut event, which nothing dispatches. --- exporters/elasticsearch/CMakeLists.txt | 4 ++ .../src/es_log_record_exporter.cc | 37 ++++++++++++------- .../test/es_log_record_exporter_test.cc | 22 ++++++++++- 3 files changed, 47 insertions(+), 16 deletions(-) diff --git a/exporters/elasticsearch/CMakeLists.txt b/exporters/elasticsearch/CMakeLists.txt index cfd4ce01db..4ce3a11f85 100644 --- a/exporters/elasticsearch/CMakeLists.txt +++ b/exporters/elasticsearch/CMakeLists.txt @@ -55,4 +55,8 @@ if(BUILD_TESTING) TARGET es_log_record_exporter_test TEST_PREFIX exporter. TEST_LIST es_log_record_exporter_test) + + # These cases exist to catch a wait that never returns. Without a per test + # bound a regression stalls the job instead of failing it. + set_tests_properties(${es_log_record_exporter_test} PROPERTIES TIMEOUT 30) endif() # BUILD_TESTING diff --git a/exporters/elasticsearch/src/es_log_record_exporter.cc b/exporters/elasticsearch/src/es_log_record_exporter.cc index 9f59db2d45..885b3c5cd3 100644 --- a/exporters/elasticsearch/src/es_log_record_exporter.cc +++ b/exporters/elasticsearch/src/es_log_record_exporter.cc @@ -108,8 +108,8 @@ class ResponseHandler : public http_client::EventHandler /** * A method the user calls to block their thread until the request has either produced a - * response or failed. The longest duration is the timeout of the request, set by - * SetTimeoutMs(), which arrives here as a TimedOut session event. + * response or failed. It has no deadline of its own and relies on the HTTP client reporting + * one of the terminal session states. */ bool waitForResponse() { @@ -182,15 +182,19 @@ class ResponseHandler : public http_client::EventHandler recordCompletion(CompletionState::Failure); break; case http_client::SessionState::ReadError: - OTEL_INTERNAL_LOG_DEBUG("[ES Log Exporter] Read error"); - // Kept at debug, like Destroyed above: recording is first writer wins, so this can - // arrive after a response has already succeeded, and an error line there would not - // describe the outcome Export() reports. - recordCompletion(CompletionState::Failure); + // Reported only when this is the outcome Export() returns. Recording is first writer + // wins, so either of these can arrive after a response has already succeeded, and an + // error line there would describe a failure the caller was never told about. + if (recordCompletion(CompletionState::Failure)) + { + OTEL_INTERNAL_LOG_ERROR("[ES Log Exporter] Read error"); + } break; case http_client::SessionState::WriteError: - OTEL_INTERNAL_LOG_DEBUG("[ES Log Exporter] Write error"); - recordCompletion(CompletionState::Failure); + if (recordCompletion(CompletionState::Failure)) + { + OTEL_INTERNAL_LOG_ERROR("[ES Log Exporter] Write error"); + } break; case http_client::SessionState::Cancelled: OTEL_INTERNAL_LOG_ERROR("[ES Log Exporter] (manually) cancelled"); @@ -211,22 +215,27 @@ class ResponseHandler : public http_client::EventHandler * Record the outcome of the request, first writer wins, then release any waiter. Keeping the * first outcome means a session destroyed after a successful response does not overwrite it. */ - void recordCompletion(CompletionState state) + /// Returns whether this call is the one that decided the outcome. + bool recordCompletion(CompletionState state) { + bool recorded = false; { std::unique_lock lk(mutex_); - recordCompletionLocked(state); + recorded = recordCompletionLocked(state); } cv_.notify_all(); + return recorded; } /// As recordCompletion(), for callers that already hold mutex_ and notify themselves. - void recordCompletionLocked(CompletionState state) + bool recordCompletionLocked(CompletionState state) { - if (completion_ == CompletionState::Pending) + if (completion_ != CompletionState::Pending) { - completion_ = state; + return false; } + completion_ = state; + return true; } // Define a condition variable and mutex diff --git a/exporters/elasticsearch/test/es_log_record_exporter_test.cc b/exporters/elasticsearch/test/es_log_record_exporter_test.cc index 54a6c74ed8..1c156dae8d 100644 --- a/exporters/elasticsearch/test/es_log_record_exporter_test.cc +++ b/exporters/elasticsearch/test/es_log_record_exporter_test.cc @@ -353,8 +353,8 @@ TEST(ElasticsearchLogsExporterSyncTests, SessionDestroyedAfterAResponseKeepsTheS EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kSuccess); } -// Same rule for the two states this change makes terminal. It is why they log at debug rather -// than error: reaching one of them says nothing about the result Export() goes on to report. +// Same rule for the two states this change makes terminal, and the reason their error line is +// conditional: reaching one of them says nothing on its own about the result Export() reports. TEST(ElasticsearchLogsExporterSyncTests, IoErrorAfterAResponseKeepsTheSuccess) { SKIP_WITHOUT_SYNC_EXPORT(); @@ -369,3 +369,21 @@ TEST(ElasticsearchLogsExporterSyncTests, IoErrorAfterAResponseKeepsTheSuccess) EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kSuccess); } } + +// The mirror image, and the case where the error line is the only diagnostic the caller gets: +// the I/O error is recorded first, so a response arriving afterwards does not rescue the export. +TEST(ElasticsearchLogsExporterSyncTests, IoErrorBeforeAResponseKeepsTheFailure) +{ + SKIP_WITHOUT_SYNC_EXPORT(); + for (const auto state : + {http_client::SessionState::ReadError, http_client::SessionState::WriteError}) + { + SCOPED_TRACE(static_cast(state)); + const auto result = ExportWith([state](http_client::EventHandler &handler) { + handler.OnEvent(state, ""); + FakeResponse response(200, kAcceptedBody); + handler.OnResponse(response); + }); + EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kFailure); + } +} From 4f1716d88032037c069a7f42b325ca870ce2c9ad Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 3 Aug 2026 05:59:34 +0800 Subject: [PATCH 5/7] Include for the braced state lists Reported by include-what-you-use, which runs with warning_limit 0. --- exporters/elasticsearch/test/es_log_record_exporter_test.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/exporters/elasticsearch/test/es_log_record_exporter_test.cc b/exporters/elasticsearch/test/es_log_record_exporter_test.cc index 1c156dae8d..47e1d884f3 100644 --- a/exporters/elasticsearch/test/es_log_record_exporter_test.cc +++ b/exporters/elasticsearch/test/es_log_record_exporter_test.cc @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include "nlohmann/json.hpp" From 88255841b903868d8c7ef08e3f5337915f471f16 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 3 Aug 2026 07:47:25 +0800 Subject: [PATCH 6/7] Skip in SetUp so no case body ends in unreachable code GTEST_SKIP returns, so a skip at the top of each body left the rest of that body unreachable. MSVC reports it as C4702 and the maintainer mode jobs turn warnings into errors, which failed the abiv2 build at lines 268 and 272. A fixture that skips in SetUp keeps every case in the binary, which gtest_add_tests still needs, and leaves no early return behind. --- .../test/es_log_record_exporter_test.cc | 43 +++++++++---------- 1 file changed, 20 insertions(+), 23 deletions(-) diff --git a/exporters/elasticsearch/test/es_log_record_exporter_test.cc b/exporters/elasticsearch/test/es_log_record_exporter_test.cc index 47e1d884f3..0897c3cd31 100644 --- a/exporters/elasticsearch/test/es_log_record_exporter_test.cc +++ b/exporters/elasticsearch/test/es_log_record_exporter_test.cc @@ -254,17 +254,22 @@ opentelemetry::sdk::common::ExportResult ExportWith(EventScript script) // The synchronous wait exists only when the exporter is built without async export, so these cases // skip rather than compile out: gtest_add_tests reads the source, and a case that disappeared from -// the binary would still be registered with CTest. +// the binary would still be registered with CTest. The skip goes in SetUp rather than at the top of +// each body, because GTEST_SKIP returns and leaves the rest of the body unreachable, which MSVC +// reports as C4702 and the maintainer mode jobs turn into an error. +class ElasticsearchLogsExporterSyncTests : public ::testing::Test +{ +protected: + void SetUp() override + { #ifdef ENABLE_ASYNC_EXPORT -# define SKIP_WITHOUT_SYNC_EXPORT() \ - GTEST_SKIP() << "Export() returns without waiting when async export is enabled" -#else -# define SKIP_WITHOUT_SYNC_EXPORT() (void)0 + GTEST_SKIP() << "Export() returns without waiting when async export is enabled"; #endif + } +}; -TEST(ElasticsearchLogsExporterSyncTests, ResponseRecordedBeforeTheWaitIsStillSeen) +TEST_F(ElasticsearchLogsExporterSyncTests, ResponseRecordedBeforeTheWaitIsStillSeen) { - SKIP_WITHOUT_SYNC_EXPORT(); const auto result = ExportWith([](http_client::EventHandler &handler) { FakeResponse response(200, kAcceptedBody); handler.OnResponse(response); @@ -272,9 +277,8 @@ TEST(ElasticsearchLogsExporterSyncTests, ResponseRecordedBeforeTheWaitIsStillSee EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kSuccess); } -TEST(ElasticsearchLogsExporterSyncTests, ReadErrorEndsTheWait) +TEST_F(ElasticsearchLogsExporterSyncTests, ReadErrorEndsTheWait) { - SKIP_WITHOUT_SYNC_EXPORT(); const auto result = ExportWith([](http_client::EventHandler &handler) { handler.OnEvent(http_client::SessionState::ReadError, ""); }); @@ -287,9 +291,8 @@ TEST(ElasticsearchLogsExporterSyncTests, ReadErrorEndsTheWait) // // A regression here surfaces as a CTest timeout rather than a failed assertion, because a state // that stops being terminal leaves Export() waiting with nothing left to wake it. -TEST(ElasticsearchLogsExporterSyncTests, EveryTerminalStateEndsTheWaitInFailure) +TEST_F(ElasticsearchLogsExporterSyncTests, EveryTerminalStateEndsTheWaitInFailure) { - SKIP_WITHOUT_SYNC_EXPORT(); const http_client::SessionState terminal[] = { http_client::SessionState::CreateFailed, http_client::SessionState::ConnectFailed, http_client::SessionState::SendFailed, http_client::SessionState::SSLHandshakeFailed, @@ -308,9 +311,8 @@ TEST(ElasticsearchLogsExporterSyncTests, EveryTerminalStateEndsTheWaitInFailure) // The other side of the same contract: a state that only reports progress must not complete the // export on its own, or a response that arrives afterwards is never consulted. -TEST(ElasticsearchLogsExporterSyncTests, ProgressStatesDoNotDecideTheResult) +TEST_F(ElasticsearchLogsExporterSyncTests, ProgressStatesDoNotDecideTheResult) { - SKIP_WITHOUT_SYNC_EXPORT(); const auto result = ExportWith([](http_client::EventHandler &handler) { handler.OnEvent(http_client::SessionState::Created, ""); handler.OnEvent(http_client::SessionState::Connecting, ""); @@ -323,18 +325,16 @@ TEST(ElasticsearchLogsExporterSyncTests, ProgressStatesDoNotDecideTheResult) EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kSuccess); } -TEST(ElasticsearchLogsExporterSyncTests, WriteErrorEndsTheWait) +TEST_F(ElasticsearchLogsExporterSyncTests, WriteErrorEndsTheWait) { - SKIP_WITHOUT_SYNC_EXPORT(); const auto result = ExportWith([](http_client::EventHandler &handler) { handler.OnEvent(http_client::SessionState::WriteError, ""); }); EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kFailure); } -TEST(ElasticsearchLogsExporterSyncTests, SessionDestroyedWhilePendingEndsTheWait) +TEST_F(ElasticsearchLogsExporterSyncTests, SessionDestroyedWhilePendingEndsTheWait) { - SKIP_WITHOUT_SYNC_EXPORT(); const auto result = ExportWith([](http_client::EventHandler &handler) { handler.OnEvent(http_client::SessionState::Destroyed, ""); }); @@ -343,9 +343,8 @@ TEST(ElasticsearchLogsExporterSyncTests, SessionDestroyedWhilePendingEndsTheWait // The first outcome recorded is the one reported, so tearing the session down after a response has // arrived does not turn a successful export into a failure. -TEST(ElasticsearchLogsExporterSyncTests, SessionDestroyedAfterAResponseKeepsTheSuccess) +TEST_F(ElasticsearchLogsExporterSyncTests, SessionDestroyedAfterAResponseKeepsTheSuccess) { - SKIP_WITHOUT_SYNC_EXPORT(); const auto result = ExportWith([](http_client::EventHandler &handler) { FakeResponse response(200, kAcceptedBody); handler.OnResponse(response); @@ -356,9 +355,8 @@ TEST(ElasticsearchLogsExporterSyncTests, SessionDestroyedAfterAResponseKeepsTheS // Same rule for the two states this change makes terminal, and the reason their error line is // conditional: reaching one of them says nothing on its own about the result Export() reports. -TEST(ElasticsearchLogsExporterSyncTests, IoErrorAfterAResponseKeepsTheSuccess) +TEST_F(ElasticsearchLogsExporterSyncTests, IoErrorAfterAResponseKeepsTheSuccess) { - SKIP_WITHOUT_SYNC_EXPORT(); for (const auto state : {http_client::SessionState::ReadError, http_client::SessionState::WriteError}) { @@ -373,9 +371,8 @@ TEST(ElasticsearchLogsExporterSyncTests, IoErrorAfterAResponseKeepsTheSuccess) // The mirror image, and the case where the error line is the only diagnostic the caller gets: // the I/O error is recorded first, so a response arriving afterwards does not rescue the export. -TEST(ElasticsearchLogsExporterSyncTests, IoErrorBeforeAResponseKeepsTheFailure) +TEST_F(ElasticsearchLogsExporterSyncTests, IoErrorBeforeAResponseKeepsTheFailure) { - SKIP_WITHOUT_SYNC_EXPORT(); for (const auto state : {http_client::SessionState::ReadError, http_client::SessionState::WriteError}) { From f60d20195fc6a479aebd44fd2dacbb6bc00372a6 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 3 Aug 2026 09:29:40 +0800 Subject: [PATCH 7/7] Keep the skip fixture out of external linkage clang-tidy's misc-use-internal-linkage counts a file scope test fixture, which took the abiv1-preview preset to 134 against a limit of 133. --- exporters/elasticsearch/test/es_log_record_exporter_test.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/exporters/elasticsearch/test/es_log_record_exporter_test.cc b/exporters/elasticsearch/test/es_log_record_exporter_test.cc index 0897c3cd31..216b724d07 100644 --- a/exporters/elasticsearch/test/es_log_record_exporter_test.cc +++ b/exporters/elasticsearch/test/es_log_record_exporter_test.cc @@ -257,6 +257,8 @@ opentelemetry::sdk::common::ExportResult ExportWith(EventScript script) // the binary would still be registered with CTest. The skip goes in SetUp rather than at the top of // each body, because GTEST_SKIP returns and leaves the rest of the body unreachable, which MSVC // reports as C4702 and the maintainer mode jobs turn into an error. +namespace +{ class ElasticsearchLogsExporterSyncTests : public ::testing::Test { protected: @@ -267,6 +269,7 @@ class ElasticsearchLogsExporterSyncTests : public ::testing::Test #endif } }; +} // namespace TEST_F(ElasticsearchLogsExporterSyncTests, ResponseRecordedBeforeTheWaitIsStillSeen) {