Skip to content

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

Description

@thc1006

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    needs-triageIndicates an issue or PR lacks a `triage/foo` label and requires one.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions