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.
What happens
ElasticsearchLogRecordExporter::Shutdowntakes a timeout and never reads it(
exporters/elasticsearch/src/es_log_record_exporter.cc):The parameter is commented out at the signature, so the caller's deadline has no effect,
and the return is
truewhether or not anything was flushed. The call is not instanteither:
HttpClient::FinishAllSessionsloops untilsessions_is empty, callingFinishSession()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
Shutdownexists 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::Shutdownpasses a timeout down expecting it to mean something.
For contrast, both OTLP clients do honour theirs:
OtlpHttpClient::ShutdownandOtlpGrpcClient::Shutdowneach callForceFlush(timeout)and return its result. TheElasticsearch 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:
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.
Shutdownsetsis_shutdown_whilean
Exportthat has already passed its ownisShutdown()check may still be registering asession, 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.