Skip to content

Commit 84fe660

Browse files
authored
[BUGFIX] Complete an OTLP HTTP request on every state its client can end on (#4453)
1 parent fb6b450 commit 84fe660

3 files changed

Lines changed: 117 additions & 0 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,11 @@ Increment the:
4040
* [CONFIGURATION] Cleanup build targets and docs
4141
[#4486](https://github.com/open-telemetry/opentelemetry-cpp/pull/4486)
4242

43+
* [BUGFIX] Complete an OTLP HTTP request that its client ends with
44+
`Destroyed`, `ReadError` or `WriteError`, rather than logging the state
45+
and leaving the caller waiting
46+
[#4425](https://github.com/open-telemetry/opentelemetry-cpp/issues/4425)
47+
4348
* [CONFIGURATION] Build the configured resource detectors in SdkBuilder, apply
4449
the `detection.attributes` include/exclude filter to the detected attributes,
4550
and merge the resource per the resource SDK specification.

‎exporters/otlp/src/otlp_http_client.cc‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,10 @@ class ResponseHandler : public http_client::EventHandler
186186
opentelemetry::nostd::string_view reason) noexcept override
187187
{
188188
// need to modify stopping_ under lock before calling callback
189+
//
190+
// Every state the client can end a request on. A state missing from here leaves the caller
191+
// waiting for a callback that never comes. The exchange below decides ordering, so a state
192+
// arriving after a response does not report a second time.
189193
bool need_stop = false;
190194
switch (state)
191195
{
@@ -195,6 +199,9 @@ class ResponseHandler : public http_client::EventHandler
195199
case http_client::SessionState::SSLHandshakeFailed:
196200
case http_client::SessionState::TimedOut:
197201
case http_client::SessionState::NetworkError:
202+
case http_client::SessionState::Destroyed:
203+
case http_client::SessionState::ReadError:
204+
case http_client::SessionState::WriteError:
198205
case http_client::SessionState::Cancelled: {
199206
need_stop = true;
200207
}

‎exporters/otlp/test/otlp_http_exporter_custom_client_test.cc‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,8 +2,10 @@
22
// SPDX-License-Identifier: Apache-2.0
33

44
#include <gtest/gtest.h>
5+
#include <atomic>
56
#include <chrono>
67
#include <functional>
8+
#include <initializer_list>
79
#include <string>
810
#include <utility>
911
#include "gmock/gmock.h"
@@ -104,6 +106,31 @@ class OtlpHttpExporterCustomClientTestPeer : public ::testing::Test
104106
return {new OtlpHttpClient(MakeOtlpHttpClientOptions(), http_client), http_client};
105107
}
106108

109+
// Records the outcome. A request that never settles calls back no times and one that settles
110+
// twice calls back twice, so both failures show in the count rather than only in a timeout.
111+
static void ExportOneRequest(OtlpHttpClient &otlp_client,
112+
const std::shared_ptr<std::atomic<int>> &calls,
113+
const std::shared_ptr<sdk::common::ExportResult> &result)
114+
{
115+
auto arena = std::make_unique<google::protobuf::Arena>();
116+
auto *request =
117+
google::protobuf::Arena::Create<proto::collector::trace::v1::ExportTraceServiceRequest>(
118+
arena.get());
119+
auto *response =
120+
google::protobuf::Arena::Create<proto::collector::trace::v1::ExportTraceServiceResponse>(
121+
arena.get());
122+
123+
otlp_client.Export(
124+
*request, std::move(arena), response,
125+
[calls, result](opentelemetry::sdk::common::ExportResult outcome,
126+
google::protobuf::Message *) {
127+
calls->fetch_add(1, std::memory_order_release);
128+
*result = outcome;
129+
return true;
130+
},
131+
1);
132+
}
133+
107134
// A non-zero request budget keeps the export asynchronous, so it returns while the session runs.
108135
static void ExportOneRequest(OtlpHttpClient &otlp_client)
109136
{
@@ -248,6 +275,84 @@ TEST_F(OtlpHttpExporterCustomClientTestPeer, ForceFlushReportsSuccessOnceTheSess
248275
EXPECT_TRUE(otlp_client.ForceFlush(std::chrono::milliseconds{50}));
249276
}
250277

278+
// A client that finishes a transfer on ReadError, WriteError or Destroyed ends the request: the
279+
// result callback runs once, and ForceFlush returns rather than waiting out its deadline.
280+
TEST_F(OtlpHttpExporterCustomClientTestPeer, ATerminalClientEventEndsTheRequest)
281+
{
282+
for (const auto state :
283+
{http_client::SessionState::ReadError, http_client::SessionState::WriteError,
284+
http_client::SessionState::Destroyed})
285+
{
286+
SCOPED_TRACE(static_cast<int>(state));
287+
288+
auto client = http_client::HttpClientTestFactory::Create();
289+
auto no_send_client = std::static_pointer_cast<http_client::nosend::HttpClient>(client);
290+
auto session = std::static_pointer_cast<http_client::nosend::Session>(no_send_client->session_);
291+
292+
std::shared_ptr<opentelemetry::ext::http::client::EventHandler> pending;
293+
EXPECT_CALL(*session, SendRequest)
294+
.WillRepeatedly(
295+
[&pending](std::shared_ptr<opentelemetry::ext::http::client::EventHandler> callback) {
296+
pending = std::move(callback);
297+
});
298+
299+
OtlpHttpClient otlp_client(MakeOtlpHttpClientOptions(std::chrono::seconds{30}), client);
300+
301+
auto calls = std::make_shared<std::atomic<int>>(0);
302+
auto result = std::make_shared<sdk::common::ExportResult>(sdk::common::ExportResult::kSuccess);
303+
ExportOneRequest(otlp_client, calls, result);
304+
ASSERT_NE(pending, nullptr);
305+
306+
pending->OnEvent(state, "");
307+
308+
EXPECT_EQ(1, calls->load(std::memory_order_acquire))
309+
<< "the request was not ended by the state the client finished on";
310+
EXPECT_EQ(sdk::common::ExportResult::kFailure, *result);
311+
312+
// The deadline is short on purpose. Without the request being ended above this waits it out.
313+
EXPECT_TRUE(otlp_client.ForceFlush(std::chrono::milliseconds{50}));
314+
}
315+
}
316+
317+
// The other direction. A client is free to report one of those states after it has already
318+
// delivered a response, and the outcome the caller was given must not be replaced or repeated.
319+
TEST_F(OtlpHttpExporterCustomClientTestPeer, ALateTerminalEventDoesNotReportASecondTime)
320+
{
321+
for (const auto state :
322+
{http_client::SessionState::ReadError, http_client::SessionState::WriteError,
323+
http_client::SessionState::Destroyed})
324+
{
325+
SCOPED_TRACE(static_cast<int>(state));
326+
327+
auto client = http_client::HttpClientTestFactory::Create();
328+
auto no_send_client = std::static_pointer_cast<http_client::nosend::HttpClient>(client);
329+
auto session = std::static_pointer_cast<http_client::nosend::Session>(no_send_client->session_);
330+
331+
std::shared_ptr<opentelemetry::ext::http::client::EventHandler> pending;
332+
EXPECT_CALL(*session, SendRequest)
333+
.WillRepeatedly(
334+
[&pending](std::shared_ptr<opentelemetry::ext::http::client::EventHandler> callback) {
335+
pending = std::move(callback);
336+
});
337+
338+
OtlpHttpClient otlp_client(MakeOtlpHttpClientOptions(std::chrono::seconds{30}), client);
339+
340+
auto calls = std::make_shared<std::atomic<int>>(0);
341+
auto result = std::make_shared<sdk::common::ExportResult>(sdk::common::ExportResult::kFailure);
342+
ExportOneRequest(otlp_client, calls, result);
343+
ASSERT_NE(pending, nullptr);
344+
345+
http_client::nosend::Response sent;
346+
sent.Finish(*pending);
347+
ASSERT_EQ(1, calls->load(std::memory_order_acquire));
348+
349+
pending->OnEvent(state, "");
350+
351+
EXPECT_EQ(1, calls->load(std::memory_order_acquire))
352+
<< "a state arriving after the response reported the request a second time";
353+
}
354+
}
355+
251356
} // namespace otlp
252357
} // namespace exporter
253358
OPENTELEMETRY_END_NAMESPACE

0 commit comments

Comments
 (0)