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

feature: refactor and extend log_exporter benchmark #2143

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

Conversation

demurgos
Copy link
Contributor

Design discussion issue: #2031

Changes

This commit refactors and extends the log_exporter benchmark.

  1. Rename to be more precise: FooWithFuture becomes FooAsyncTrait, FooWithoutFuture becomes FooSync
  2. Add variant using native Rust support for async traits
  3. Share workload code between implementations
  4. Use blackbox hint for implementations under test
  5. Expand module documentation
  6. Move each impl into its own submodule

Merge requirement checklist

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

@demurgos demurgos requested a review from a team as a code owner September 24, 2024 21:10
@demurgos demurgos force-pushed the feature/bench-log-exporter-native-async branch from 21785fd to 6468e84 Compare September 24, 2024 21:12
Copy link

codecov bot commented Sep 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.0%. Comparing base (99d24b7) to head (de4ee51).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #2143   +/-   ##
=====================================
  Coverage   79.0%   79.0%           
=====================================
  Files        121     121           
  Lines      20945   20945           
=====================================
  Hits       16561   16561           
  Misses      4384    4384           

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

This commit refactors and extends the `log_exporter` benchmark.
1. Rename to be more precise: `FooWithFuture` becomes `FooAsyncTrait`, `FooWithoutFuture` becomes `FooSync`
2. Add variant using native Rust support for async traits
3. Share workload code between implementations
4. Use blackbox hint for implementations under test
5. Expand module documentation
6. Move each impl into its own submodule
@demurgos demurgos force-pushed the feature/bench-log-exporter-native-async branch from 6468e84 to abbd9f5 Compare September 24, 2024 21:20
@cijothomas
Copy link
Member

@demurgos Thanks in advance for your patience! I am clearing my backlog and will get to this soon!

pub struct NoOpExporterAsyncNative;

impl LogExporterAsyncNative for NoOpExporterAsyncNative {
#[inline]
Copy link
Member

Choose a reason for hiding this comment

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

can you add this consistently to other ones too, to ensure a fair comparison.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I missed the #[inline], I thought I applied it everywhere. I'll update my PR.

use super::*;

/// Async style using Rustc support for async traits
pub trait LogExporterAsyncNative: Send + Sync + Debug {
Copy link
Member

@cijothomas cijothomas Oct 8, 2024

Choose a reason for hiding this comment

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

From the benchmarks, this looks to be close enough with sync counterpart. Wondering if there is a reason we can't try this in the Exporter traits? The <T> for output might make things complex...?
(I only added this benchmarks to see if there is actually some price to be paid for introducing async.)

@demurgos Do you have some bandwidth to modify existing exporter trait to use async native style? The existing benchmarks should show improvement with this change, based on what i observe in this PR.

(Still pretty new to Rust async, so I might be missing something obvious!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey 👋
I was also interested in trying to impl a version using native async. I'm pretty familiar with it, but the lib is pretty large. I'll try to send something, but I can't promise any timeline.

@cijothomas
Copy link
Member

@lalitb Can you help continue this?

@lalitb
Copy link
Member

lalitb commented Nov 26, 2024

@lalitb Can you help continue this?

Had a quick look. The impl Future approach is indeed much faster (around ~8 times faster in my local tests), which makes sense as it uses static dispatch with future type is known at compile time and so avoids overhead of heap allocations and dynamic dispatch that comes with using Pin<Box<dyn Future>> or async-trait. However, it requires Rust 1.75 or newer since impl Trait in traits was only stabilized in that version. Since our current msrv is 1.71, we can't use this approach without updating our Rust version.

Before going forward, it's good to discuss if we should bump-up our msrv to 1.75 for otel-sdk, and all exporters.

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

Successfully merging this pull request may close these issues.

3 participants