Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor BatchLogProcessor #2494

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

utpilla
Copy link
Contributor

@utpilla utpilla commented Jan 3, 2025

Towards #2478

Changes

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)

Copy link

codecov bot commented Jan 3, 2025

Codecov Report

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

Project coverage is 77.8%. Comparing base (82ed8e0) to head (7291998).

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.8%   -0.1%     
=======================================
  Files        123     123             
  Lines      22888   22950     +62     
=======================================
+ Hits       17839   17876     +37     
- Misses      5049    5074     +25     

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

@lalitb
Copy link
Member

lalitb commented Jan 3, 2025

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

@utpilla utpilla marked this pull request as ready for review January 3, 2025 19:55
@utpilla utpilla requested a review from a team as a code owner January 3, 2025 19:55
@lalitb lalitb added the integration tests Run integration tests label Jan 6, 2025
// Check if the a control message for exporting logs is already sent to the worker thread.
// If not, send a control message to export logs.
// `export_log_message_sent` is set to false ONLY when the worker thread has processed the control message.
if !self.export_log_message_sent.swap(true, Ordering::Relaxed) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@utpilla utpilla Jan 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration tests Run integration tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants