Refactor BatchLogProcessor

[utpilla]

Towards #2478

Changes

• Refactor BatchLogProcessor to wake up the background thread ONLY when needed BatchProcessor shutdown issue #2478 (comment)
• Address the shutdown issue

Performance

There is a significant performance improvement mainly because of mpsc:Receiver not being excessively active (unlike how it is on main branch). It looks like there is a significant amount of synchronization overhead with senders and receivers actively working on the channel all the time. With this PR, the channel receivers are only woken up when needed so the logging threads incur a lesser synchronization overhead cost.

I updated the Logging_Comparable_To_Appender benchmark and logs stress test to use a BatchProcessor with a no-op exporter and ran the tests. Here are the results:

• 25% improvement in throughput with stress test results going up from 12M (main) to 15 M
• ~46% improvement in benchmarks with the latency measurement going down from 420ns (main) to 227 ns.

Merge requirement checklist

• CONTRIBUTING guidelines followed
• Unit tests added/updated (if applicable)
• Appropriate CHANGELOG.md files updated for non-trivial, user-facing changes
• Changes in public API reviewed (if applicable)

[codecov]

Codecov Report

Attention: Patch coverage is 59.34066% with 37 lines in your changes missing coverage. Please review.

Project coverage is 77.9%. Comparing base (73859c3) to head (08687d1).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
opentelemetry-sdk/src/logs/log_processor.rs	59.3%	37 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #2494     +/-   ##
=======================================
- Coverage   77.9%   77.9%   -0.1%     
=======================================
  Files        123     123             
  Lines      22881   22943     +62     
=======================================
+ Hits       17838   17875     +37     
- Misses      5043    5068     +25     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[lalitb]

Excellent approach! Please make this ready for review once you feel it's ready :)

[lalitb]

should we use compare_exchange instead of swap. This will avoid redundant writes if the export_log_message_sent is already true?

[utpilla]

I used swap because it's less verbose. Note that both swap and compare_exchange need exclusive access to the cache line. More importantly, a compare_exchangeoperation would need exclusive access even to perform the comparison.

I pushed some new changes. I have added a new if check using atomic load to avoid aggressively calling swap. We would now call swap only if the atomic load value returned false. Let me know what you think.

[cijothomas]

we can get rid of logs vec and instead create a fresh array and pass its slice to export method.?

[utpilla]

For using array, we need to know the size of the array beforehand which we don't because we don't know how many items are present in the channel. It might be less than max_export_batch_size.

[cijothomas]

we can allocate an array of max_export_batch_size, and slice it at the required length. Anyway, this is not a blocker.

[utpilla]

Always stack allocating a big array even to export very few logs seems wasteful.

What's the benefit of using an array instead of a vec here? We are reusing the same vec for each export anyway so allocation isn't a concern.

[cijothomas]

same as flush - we need to ensure logs are continued to be exported until it finishes all logs received upto the point shutdown was invoked.

[cijothomas]

Need to fix flush/shutdown, but good to merge and follow up.

Also, I am unsure of the perf testing, as any perf test would instantly fill up the channel, so majority of items would be taking the "drop_due_to_full" path. Developing a testing strategy for BatchProcessor is also good candidate for another PR. Irrespective of that, the direction in this PR looks correct to me.

[utpilla]

Also, I am unsure of the perf testing, as any perf test would instantly fill up the channel, so majority of items would be taking the "drop_due_to_full" path.

Since we are using a no-op exporter, the channel should be getting emptied really quick as well. But yes, it would be nice to verify that objectively instead of manual reasoning.