open-telemetry / open-telemetry/opentelemetry-cpp
[BUG] Elasticsearch async exporter counts a session's completion zero or twice
Nobody has claimed this yet.
- Dominant language
- C++
- Stars
- 1.4k
- Forks
- 632
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 75
Description
Describe your environment
main at 11fa0db0, Linux, GCC 14.2, -DWITH_ELASTICSEARCH=ON -DWITH_ASYNC_EXPORT_PREVIEW=ON. AsyncResponseHandler only exists under ENABLE_ASYNC_EXPORT.
Steps to reproduce
Two exports through a fake HttpClient injected via the public constructor. The first session reports two terminal events, which is what the curl path does on a setup failure (see below). The second never calls back.
int session = 0;
auto client = std::make_shared<FakeHttpClient>([&session](http_client::EventHandler &handler) {
if (++session == 1)
{
handler.OnEvent(http_client::SessionState::ConnectFailed, "");
handler.OnEvent(http_client::SessionState::CreateFailed, "");
}
// session 2 never calls back
});
ExportOnce(exporter); // session 1
ExportOnce(exporter); // session 2, still in flight
const bool flushed = exporter.ForceFlush(std::chrono::milliseconds{20});
What is the expected behavior?
flushed == false. One of the two sessions has not finished.
What is the actual behavior?
[repro] ForceFlush with one session still running returned true
Immediately, in 0 ms. finished_session_counter_ reached 2 from a single session, so finished >= running was already true.
I ran this on top of the fix in #4337, so this is not the timeout defect reported in #4336. The counters are monotonic and compared with >=, which makes the overshoot permanent: every later flush also returns one session early.
Additional context
AsyncResponseHandler accounts a session's completion neither at most once nor at least once.
More than once. OnResponse and every handled terminal OnEvent call result_callback_ directly, and the callback increments finished_session_counter_. There is no guard. Two in-tree paths produce two terminal callbacks for one session:
HttpOperation::SendAsyncdispatchesConnectFailedand returns non-CURLE_OKwhenSetup()fails (ext/src/http/client/curl/http_operation_curl.cc, thecode != CURLE_OKbranch).Session::SendRequestthen takes itselsebranch and dispatchesCreateFailedfor the same handler (ext/src/http/client/curl/http_client_curl.cc). This is deterministic, not a race.- The async completion lambda in
Session::SendRequestuses two independentifblocks rather thanelse if, so an operation that was aborted and also has a response firesOnEvent(Cancelled)andOnResponse.
Fewer than once. ReadError, WriteError and Destroyed fall into default: break, so result_callback_ never fires and the session is never counted as finished. The synchronous handler treats all three as terminal, Destroyed since #4298 and the other two in #4331, so the two handlers in the same file currently disagree.
Suggested shape
Route every outcome through one idempotent step:
void CompleteOnce(sdk::common::ExportResult result) noexcept
{
bool expected = false;
if (completed_.compare_exchange_strong(expected, true, std::memory_order_acq_rel))
{
result_callback_(result);
}
}
with OnResponse and every terminal OnEvent going through it, and the switch enumerating all fifteen states so -Wswitch catches a future addition rather than a default: swallowing it. The guard is also what makes widening the terminal set safe, since it removes the double-completion objection to treating ReadError, WriteError and Destroyed as terminal here the way the synchronous handler does.
Worth covering: each of the three ignored states finishing one session as a failure, a response followed by another terminal event counting once, a cancellation followed by a late response counting once, and a duplicate terminal event counting once.
Two things I would rather flag than fold in. The two if blocks and the non-returning gzip failure branch in Session::SendRequest are shared HTTP client behaviour, so fixing them there would remove the double-fire for every consumer, but that is a separate change. And an exporter side guard is still warranted either way, because EventHandler is a public extension point and an injected client is free to emit whatever it likes.
I am happy to open the PR for the exporter side. It overlaps #4297 and #4337 in the same file, so I would rather land those first unless you would prefer it sooner.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start by locating AsyncResponseHandler and its result_callback_ usage, then read the ConnectFailed/CreateFailed paths in ext/src/curl/http_operation_curl.cc and ext/src/curl/http_client_curl.cc. Add coverage for ignored terminal states and duplicate callbacks; done means each session contributes exactly once and ForceFlush stays false while another session is running.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, elasticsearch
- Domain
- observability-sre
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100