[SDK] BatchSpanProcessor: wait for a full batch instead of draining partial batches

[yswdqz]

fix #4449

Problem

BatchSpanProcessor currently wakes up whenever the buffer is non-empty and
then drains the entire buffer in a tight loop. Under steady load this produces
exports that are much smaller than max_export_batch_size, causing:

• Up to 2x more gRPC export requests than necessary
• Higher CPU usage due to repeated serialization and wakeups

Changes

• Change the worker wait predicate from !buffer_.empty() to
  buffer_.size() >= max_export_batch_size_.
• Export() now only drains the entire buffer when a force flush is pending
  or the processor is shutting down; on normal wakeups it exports at most one
  batch of max_export_batch_size spans.

This preserves ForceFlush/Shutdown semantics while making the normal
export path strictly batch-oriented.

Performance

I will add it later.

Checklist

• CHANGELOG.md updated for non-trivial changes
• Unit tests have been added (No new tests have been added, existing batch_span_processor_test and
  batch_span_processor_test_stress all pass)
• Changes in public API reviewed (no public API changes;)

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: yswdqz / name: yswdqz (3d02bbf, 54771ba, d003137)

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.62%. Comparing base (93f16f3) to head (b3fa617).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4466      +/-   ##
==========================================
+ Coverage   82.62%   82.62%   +0.01%     
==========================================
  Files         512      512              
  Lines       20138    20141       +3     
==========================================
+ Hits        16637    16640       +3     
  Misses       3501     3501              

Files with missing lines	Coverage Δ
sdk/src/trace/batch_span_processor.cc	85.72% <100.00%> (+0.24%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[marcalff]

Thanks for the fix.

Please see clang-format errors, either run clang-format or apply this manually:

diff --git a/sdk/src/trace/batch_span_processor.cc b/sdk/src/trace/batch_span_processor.cc
index 2231b7b..1a1a7d8 100644
--- a/sdk/src/trace/batch_span_processor.cc
+++ b/sdk/src/trace/batch_span_processor.cc
@@ -250,8 +250,8 @@ void BatchSpanProcessor::Export()
 
   std::uint64_t notify_force_flush =
       synchronization_data_->force_flush_pending_sequence.load(std::memory_order_acquire);
-  bool should_drain = notify_force_flush != 0 ||
-                      synchronization_data_->is_shutdown.load(std::memory_order_acquire);
+  bool should_drain =
+      notify_force_flush != 0 || synchronization_data_->is_shutdown.load(std::memory_order_acquire);
 
   do
   {

[denizariyan]

Export() now only drains the entire buffer when a force flush is pending
or the processor is shutting down; on normal wakeups it exports at most one
batch of max_export_batch_size spans.

notify_force_flush (i.e., synchronization_data_->force_flush_pending_sequence) is a monotonically increasing counter that increases on every call to ForceFlush. This would mean this condition would be permanently true after the first time one calls BatchSpanProcessor::ForceFlush.

Hence while (should_drain) below never becomes while (false), so Export() keeps draining to empty on every call which means the one-batch-per-wakeup behavior this PR adds never takes effect after the first call to BatchSpanProcessor::ForceFlush.

I think something like this would actually solve that problem.

Suggested change

	bool should_drain = notify_force_flush != 0 ||
	synchronization_data_->is_shutdown.load(std::memory_order_acquire);
	bool should_drain =
	notify_force_flush >
	synchronization_data_->force_flush_notified_sequence.load(std::memory_order_acquire) ||
	synchronization_data_->is_shutdown.load(std::memory_order_acquire);

Maybe you could add some tests to confirm/check?

This issue could be tested with a test exporter that records how many spans each Export() call receives and pauses inside the first call, so the worker is held mid-export and you control what's in the buffer when it resumes. You could then add a couple spans while it's paused mid export to see that it will still drain less than the max batch size immediately instead of returning after exporting one batch as the PR suggests IF the BatchSpanProcessor::ForceFlush was ever called.

[ThomsonTan]

should_drain is doing two orthogonal jobs here: how many records to take, and whether to keep looping. Because is_shutdown alone now sets it, it hands the entire buffer to a single Export() call, bypassing max_export_batch_size. Before this PR, a program that never called ForceFlush had notify_force_flush == 0, so DrainQueue() still chunked; now a backlog of 3000 with max_export_batch_size = 2048 goes out as one 3000-span request instead of 2048 + 952. If the receiver rejects the oversized message (max_receive_message_length defaults to 4 MiB), the spans are already consumed and the result is discarded at the line - and at process exit there is no retry.

Suggest keeping the cap unconditional and letting should_drain control only the loop.