Skip to content

Commit 385ae3c

Browse files
committed
[EXPORTER] Cancel instead of finish the ES session on export timeout
FinishSession() waits for the in-flight transfer to complete, which defeats the purpose of the response deadline for HTTP clients (e.g. curl) whose worker thread blocks on the transfer itself. Cancel the session instead when the deadline expires. Addresses review feedback from open-telemetry#4530 (review)
1 parent 4620459 commit 385ae3c

1 file changed

Lines changed: 40 additions & 3 deletions

File tree

‎exporters/elasticsearch/test/es_log_record_exporter_test.cc‎

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -140,8 +140,19 @@ class SilentSession final : public http_client::Session
140140
void SendRequest(std::shared_ptr<http_client::EventHandler>) noexcept override {}
141141

142142
bool IsSessionActive() noexcept override { return true; }
143-
bool CancelSession() noexcept override { return true; }
144-
bool FinishSession() noexcept override { return true; }
143+
bool CancelSession() noexcept override
144+
{
145+
cancel_called_ = true;
146+
return true;
147+
}
148+
bool FinishSession() noexcept override
149+
{
150+
finish_called_ = true;
151+
return true;
152+
}
153+
154+
bool cancel_called_ = false;
155+
bool finish_called_ = false;
145156
};
146157

147158
class SilentHttpClient final : public http_client::HttpClient
@@ -150,12 +161,15 @@ class SilentHttpClient final : public http_client::HttpClient
150161
std::shared_ptr<http_client::Session> CreateSession(
151162
opentelemetry::nostd::string_view) noexcept override
152163
{
153-
return std::make_shared<SilentSession>();
164+
session_ = std::make_shared<SilentSession>();
165+
return session_;
154166
}
155167

156168
bool CancelAllSessions() noexcept override { return true; }
157169
bool FinishAllSessions() noexcept override { return true; }
158170
void SetMaxSessionsPerConnection(std::size_t) noexcept override {}
171+
172+
std::shared_ptr<SilentSession> session_;
159173
};
160174
#endif // !ENABLE_ASYNC_EXPORT
161175

@@ -220,6 +234,29 @@ TEST(ElasticsearchLogsExporterTests, ExportReturnsOnTimeoutWhenClientNeverRespon
220234

221235
EXPECT_EQ(result, opentelemetry::sdk::common::ExportResult::kFailure);
222236
}
237+
238+
// Regression test: on a timed-out export, Export() used to call session->FinishSession()
239+
// regardless of the outcome. A real HTTP client (e.g. curl) blocks its FinishSession() on
240+
// the in-flight transfer completing, which is exactly the hang the deadline exists to avoid,
241+
// so the timeout path must cancel the session instead of finishing it.
242+
TEST(ElasticsearchLogsExporterTests, ExportCancelsSessionOnTimeoutInsteadOfFinishing)
243+
{
244+
logs_exporter::ElasticsearchExporterOptions options("localhost", 9200, "logs",
245+
/*response_timeout=*/1);
246+
auto http_client = std::make_shared<SilentHttpClient>();
247+
auto exporter = std::unique_ptr<sdklogs::LogRecordExporter>(
248+
new logs_exporter::ElasticsearchLogRecordExporter(options, http_client));
249+
250+
auto record = exporter->MakeRecordable();
251+
record->SetBody("this export should cancel its session, not finish it");
252+
253+
auto result = exporter->Export(nostd::span<std::unique_ptr<sdklogs::Recordable>>(&record, 1));
254+
255+
ASSERT_EQ(result, opentelemetry::sdk::common::ExportResult::kFailure);
256+
ASSERT_NE(http_client->session_, nullptr);
257+
EXPECT_TRUE(http_client->session_->cancel_called_);
258+
EXPECT_FALSE(http_client->session_->finish_called_);
259+
}
223260
#endif // !ENABLE_ASYNC_EXPORT
224261

225262
// Attempt to write a log to an invalid host/port, test that the Export() returns failure

0 commit comments

Comments
 (0)