open-telemetry / open-telemetry/opentelemetry-cpp

[BUG] Elasticsearch exporter Shutdown ignores its timeout and always reports success

Open
#4,359 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

What happens

ElasticsearchLogRecordExporter::Shutdown takes a timeout and never reads it
(exporters/elasticsearch/src/es_log_record_exporter.cc):

bool ElasticsearchLogRecordExporter::Shutdown(std::chrono::microseconds /* timeout */) noexcept
{
  is_shutdown_ = true;

  // Shutdown the session manager
  http_client_->CancelAllSessions();
  http_client_->FinishAllSessions();

  return true;
}

The parameter is commented out at the signature, so the caller's deadline has no effect,
and the return is true whether or not anything was flushed. The call is not instant
either: HttpClient::FinishAllSessions loops until sessions_ is empty, calling
FinishSession() on each, with no deadline of its own.

Why it matters

Two separate things.

The return value carries no information. A caller cannot distinguish "everything was
flushed" from "sessions were cancelled with data still queued", which is the one question
Shutdown exists to answer.

The wait is unbounded from the caller's point of view. An application that asks for a one
second shutdown can block for as long as the transport takes. LoggerProvider::Shutdown
passes a timeout down expecting it to mean something.

For contrast, both OTLP clients do honour theirs: OtlpHttpClient::Shutdown and
OtlpGrpcClient::Shutdown each call ForceFlush(timeout) and return its result. The
Elasticsearch exporter is the one that does not.

Suggested shape

Flush first with the caller's deadline, then cancel whatever is left, and return what the
flush reported:

is_shutdown_ = true;
const bool flushed = ForceFlush(timeout);
http_client_->CancelAllSessions();
http_client_->FinishAllSessions();
return flushed;

The ordering matters, and flushing has to come first. Cancelling first would leave the
flush nothing to wait for, so it would report success without having waited for anything.

There is an admission race to settle at the same time. Shutdown sets is_shutdown_ while
an Export that has already passed its own isShutdown() check may still be registering a
session, so the flush can miss it. Sharing one mutex between the shutdown flag and the
registration closes that.

Context

Not a regression. This is how the exporter has always behaved, and #4337 leaves it alone
deliberately so that its own change stays reviewable; its description says so. Filing it
separately so the gap is recorded rather than implied by that pull request's scope.

Happy to send a patch if the shape above looks right, or to leave it if a maintainer would
rather fold it into a broader lifecycle change.

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 in exporters/elasticsearch/src/es_log_record_exporter.cc and compare ElasticsearchLogRecordExporter::Shutdown with OtlpHttpClient::Shutdown and OtlpGrpcClient::Shutdown. Trace ForceFlush, HttpClient::FinishAllSessions, and session registration to resolve the admission race between Export and Shutdown. Done means the caller timeout is honored, remaining sessions are cancelled afterward, and Shutdown returns the flush result.

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
Mostly clear
Newbie friendliness
58/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.