open-telemetry / open-telemetry/opentelemetry-cpp

[BUG] Elasticsearch async exporter counts a session's completion zero or twice

Open
#4,338 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

needs-triage
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::SendAsync dispatches ConnectFailed and returns non-CURLE_OK when Setup() fails (ext/src/http/client/curl/http_operation_curl.cc, the code != CURLE_OK branch). Session::SendRequest then takes its else branch and dispatches CreateFailed for the same handler (ext/src/http/client/curl/http_client_curl.cc). This is deterministic, not a race.
  • The async completion lambda in Session::SendRequest uses two independent if blocks rather than else if, so an operation that was aborted and also has a response fires OnEvent(Cancelled) and OnResponse.

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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.