Use native Rust support for async traits in LogExporter::export() method #2374

lalitb · 2024-12-02T22:34:02Z

Changes

Continuation to #2143, more to show feasibility, performance improvements, and have discussion before raising the PR for eventual review, this PR demonstrate using async traits support in Rust exporter. The support was added in Rust v1.75, while the msrv as of now is v1.70 for otel-sdk

The change is:
Existing:

#[async_trait]
pub trait LogExporter: Send + Sync + Debug {
  async fn export(&mut self, batch: LogBatch<'_>) -> LogResult<()>;
}

PR:

pub trait LogExporter: Send + Sync + Debug {
    fn export<'a>(
        &'a mut self,
        batch: &'a LogBatch<'a>,
    ) -> impl std::future::Future<Output = LogResult<()>> + Send + 'a;
}

Have modified the stress test to use LogExporter to demonstrate the improvement:
main with modified stress test:
main...lalitb:opentelemetry-rust:main-stress-with-exporter
6,348,766 iterations/sec
5,725,142 iterations/sec
6,020,324 iterations/sec
5,814,542 iterations/sec

this PR with modified stress test:
9,550,832 iterations/sec
11,045,635 iterations/sec
10,632,124 iterations/sec
10,212,681 iterations/sec
9,810,356 iterations/sec

There are improvements of ~71%, but it need to bump msrv to 1.75. Also, most of the improvement won't be the periodic

Thanks,
Lalit

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)

cijothomas · 2024-12-02T22:44:37Z

opentelemetry-sdk/src/export/logs/mod.rs

@@ -82,7 +80,11 @@ pub trait LogExporter: Send + Sync + Debug {
    /// A `LogResult<()>`, which is a result type indicating either a successful export (with
    /// `Ok(())`) or an error (`Err(LogError)`) if the export operation failed.
    ///
-    async fn export(&mut self, batch: LogBatch<'_>) -> LogResult<()>;
+    fn export<'a>(
+        &'a mut self,


@lalitb I think it maybe better to first check how to avoid requiring mut reference for exporting, before evaluating the results of this perf test, as that requirement has forced the need of mutex and introduced contention and hence stress tests shows way low throughput than before.

@cijothomas If we remove the mut self-reference from the LogExporter::export() method, it will force the exporter implementation to rely on interior mutability to modify its internal state. This would involve using synchronization mechanisms like locks or atomic operations, which add complexity and overhead. But in the case of SimpleLogProcessor and BatchLogProcessor, these methods are always called sequentially, so there’s no real benefit to introducing this extra layer of synchronization. It would just make the code more complicated and less performant without actually solving any problem.

The use-case for having a non-mutating export method would be in reentrant log processors, where LogProcessor::emit() -> LogProcessor::export() could be invoked concurrently from multiple user threads. In such scenarios, interior mutability would be necessary if the exporter implementation needs to manage internal state. However, this is not relevant for the current simple and batch processors, which operate in a strictly sequential manner.

Actually, there are 4 different exporter scenarios to think about -

sync/concurrent:

fn export<'a>(&'a self, batch: &'a LogBatch<'a>) -> LogResult<()>;

sync/sequential,

fn export<'a>(&'a mut self, batch: &'a LogBatch<'a>) -> LogResult<()>;

async/concurrent

fn export<'a>(&'a self, batch: &'a LogBatch<'a>) -> impl std::future::Future<Output = LogResult<()>> + Send + 'a;

async/sequential

fn export<'a>(&'a mut self, batch: &'a LogBatch<'a>) -> impl std::future::Future<Output = LogResult<()>> + Send + 'a;

Currently:

The SimpleLogProcessor and BatchLogProcessor invoke the exporter in async/sequential mode.

The ReentrantProcessor, used by ETW and user_events in the contrib repository, operates the exporter in sync/concurrent mode.

To ensure clarity and extensibility, we need to properly model these scenarios within the design.

Just to add, we don't currently have specific use-case for sync/sequential, and async/concurrent, they are mentioned above for completeness of all the scenarios.

cijothomas · 2024-12-03T04:02:50Z

opentelemetry-appender-tracing/src/layer.rs

@@ -247,11 +247,16 @@ mod tests {

    #[async_trait]


we don't need async-trait anymore?

yes, this can be safely removed.

unresolving to remove async_trait

codecov · 2024-12-06T23:51:18Z

Codecov Report

Attention: Patch coverage is 28.26087% with 132 lines in your changes missing coverage. Please review.

Project coverage is 79.3%. Comparing base (c726c4d) to head (8c34380).

Files with missing lines	Patch %	Lines
opentelemetry-otlp/src/exporter/http/logs.rs	0.0%	36 Missing ⚠️
opentelemetry-otlp/src/exporter/tonic/logs.rs	0.0%	28 Missing ⚠️
opentelemetry-stdout/src/logs/exporter.rs	0.0%	25 Missing ⚠️
opentelemetry-otlp/src/logs.rs	0.0%	19 Missing ⚠️
opentelemetry-sdk/src/logs/log_processor.rs	76.0%	11 Missing ⚠️
opentelemetry-appender-tracing/src/layer.rs	0.0%	10 Missing ⚠️
opentelemetry-otlp/src/exporter/http/mod.rs	0.0%	2 Missing ⚠️
opentelemetry-otlp/src/exporter/tonic/mod.rs	0.0%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            main   #2374     +/-   ##
=======================================
- Coverage   79.4%   79.3%   -0.2%     
=======================================
  Files        122     122             
  Lines      21708   21736     +28     
=======================================
- Hits       17255   17250      -5     
- Misses      4453    4486     +33

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

lalitb · 2024-12-07T00:05:49Z

@lalitb I think it maybe better to first check how to avoid requiring mut reference for exporting, before evaluating the results of this perf test, as that requirement has forced the need of mutex and introduced contention and hence stress tests shows way low throughput than before.

OK, have updated this PR after removing the mut self in #2380. This eliminates the need of mutex for exporter in stress test. The stress result are now similar to earlier i.e., without the inclusion of exporter. Which means, adding futures::block_on() on native async-method doesn't introduce much of latency.

Before this PR (the regression introduced by #2380 because of inclusion of exporter ~44 -> ~40):

Number of threads: 16
Throughput: 40,468,186 iterations/sec
Throughput: 40,792,480 iterations/sec
Throughput: 40,456,227 iterations/sec
Throughput: 40,455,461 iterations/sec

After this PR:

Throughput: 44,793,174 iterations/sec
Throughput: 45,321,168 iterations/sec
Throughput: 45,387,969 iterations/sec
Throughput: 45,629,648 iterations/sec
Throughput: 45,607,326 iterations/sec
Throughput: 45,303,331 iterations/sec
Throughput: 45,634,108 iterations/sec

The cons of introducing the native-async in trait would be

Bump the msrv to 1.75
The exporter interface is no longer object-safe. Which means, it can't be used in dynamic dispatch (e.g., via dyn Trait). This introduced slight complexity in otel-otlp, where dyn dispatch was used for http and tonic exporters.

Current export method could be simplified a bit for lifetime specifiers. I will look into that. But in general, open for discussion if we should bump the msrv.

cijothomas · 2024-12-10T17:28:39Z

Bump the msrv to 1.75
The exporter interface is no longer object-safe. Which means, it can't be used in dynamic dispatch (e.g., via dyn Trait). This introduced slight complexity in otel-otlp, where dyn dispatch was used for http and tonic exporters.

Based on today's SIG discussion:
1.75 bump is acceptable.
the exporter interface not being object safe is an easy to fix issue as this PR already demonstrates fixing it for OTLP.

Awaiting more feedback.

lalitb · 2024-12-13T07:33:48Z

This is ready for review now, with #2417 to bump msrv is merged

lalitb · 2024-12-13T07:40:32Z

opentelemetry-appender-tracing/benches/logs.rs

@@ -34,8 +34,12 @@ struct NoopExporter {

 #[async_trait]
 impl LogExporter for NoopExporter {
-    async fn export(&self, _: LogBatch<'_>) -> LogResult<()> {
-        LogResult::Ok(())
+    #[allow(clippy::manual_async_fn)]


This annotation is required for false positive clippy warning - rust-lang/rust-clippy#12664

Maybe have it on the project level until #12664 is addressed?

Adding #[allow(clippy::manual_async_fn)] at the package level in Cargo.toml is not supported—it only works as an attribute in Rust source files. To suppress this lint effectively, it needs to be added in lib.rs or mod.rs for the main crate and separately for benches (and likely tests as well). It seems, adding at method level seems cleaner as of now. We can revisit this if similar implementation is done for Spans, and the issue is not fixed by then.

cijothomas · 2024-12-13T19:57:15Z

opentelemetry-otlp/src/exporter/tonic/logs.rs

@@ -58,35 +58,40 @@ impl TonicLogsClient {

 #[async_trait]


we can remove async_trait from everywhere now.

This is done, thanks.

TommyCpp · 2024-12-13T22:40:28Z

opentelemetry-otlp/src/logs.rs

@@ -108,7 +108,16 @@ impl HasHttpConfig for LogExporterBuilder<HttpExporterBuilderSet> {
 /// OTLP exporter that sends log data
 #[derive(Debug)]
 pub struct LogExporter {
-    client: Box<dyn opentelemetry_sdk::export::logs::LogExporter>,
+    //client: Box<dyn opentelemetry_sdk::export::logs::LogExporter>,


Can remove this now

Yes, removed. thanks

TommyCpp · 2024-12-13T22:42:51Z

opentelemetry-otlp/src/logs.rs

+}
+
+#[derive(Debug)]
+enum LogExporterInner {


The name can be better IMO. Maybe SupportedTransportClient. Inner just don't convey this is a enum clearly

Not a blocking issue though

Good point. Have renamed it to SupportedTransportClient.

lalitb added 6 commits December 1, 2024 02:26

initial commit

d454536

futher changes..

3ed482d

changes..

a2aa648

initial change

8ba1173

stdout exporter

37823e7

update stress

3b4cd57

lalitb requested a review from a team as a code owner December 2, 2024 22:34

cijothomas reviewed Dec 2, 2024

View reviewed changes

cijothomas reviewed Dec 3, 2024

View reviewed changes

Merge branch 'main' into log-async-trait-impl

858cd87

lalitb mentioned this pull request Dec 4, 2024

Remove mut self reference from LogExporter::export() method. #2380

Merged

4 tasks

lalitb added 2 commits December 6, 2024 11:44

Merge branch 'main' into log-async-trait-impl

e6472bf

fix otlp

8cacf52

cijothomas added this to the 0.28.0 milestone Dec 10, 2024

lalitb mentioned this pull request Dec 12, 2024

bump MSRV to 1.75.0 for all crates #2417

Merged

4 tasks

lalitb added 2 commits December 12, 2024 21:17

Merge branch 'main' into log-async-trait-impl

dfac978

add comment

f752d50

lalitb changed the title ~~Draft to Discuss: Use native Rust support for async traits in LogExporter::export() method~~ Use native Rust support for async traits in LogExporter::export() method Dec 13, 2024

lalitb commented Dec 13, 2024

View reviewed changes

lalitb added the integration tests Run integration tests label Dec 13, 2024

Merge branch 'main' into log-async-trait-impl

6ac8aa0

cijothomas reviewed Dec 13, 2024

View reviewed changes

Merge branch 'main' into log-async-trait-impl

4924e3b

TommyCpp reviewed Dec 13, 2024

View reviewed changes

review comment

8db475c

lalitb added 3 commits December 15, 2024 19:55

Merge branch 'main' into log-async-trait-impl

4a6e36f

Merge branch 'main' into log-async-trait-impl

3af54c3

Merge branch 'main' into log-async-trait-impl

8c34380

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use native Rust support for async traits in LogExporter::export() method #2374

Use native Rust support for async traits in LogExporter::export() method #2374

lalitb commented Dec 2, 2024 •

edited

Loading

cijothomas Dec 2, 2024

lalitb Dec 3, 2024 •

edited

Loading

lalitb Dec 3, 2024 •

edited

Loading

cijothomas Dec 3, 2024

lalitb Dec 3, 2024

cijothomas Dec 13, 2024

codecov bot commented Dec 6, 2024 •

edited

Loading

lalitb commented Dec 7, 2024 •

edited

Loading

cijothomas commented Dec 10, 2024

lalitb commented Dec 13, 2024

lalitb Dec 13, 2024 •

edited

Loading

TommyCpp Dec 13, 2024

lalitb Dec 15, 2024

cijothomas Dec 13, 2024

lalitb Dec 15, 2024

TommyCpp Dec 13, 2024

lalitb Dec 15, 2024

TommyCpp Dec 13, 2024

TommyCpp Dec 13, 2024

lalitb Dec 15, 2024

Use native Rust support for async traits in LogExporter::export() method #2374

Are you sure you want to change the base?

Use native Rust support for async traits in LogExporter::export() method #2374

Conversation

lalitb commented Dec 2, 2024 • edited Loading

Changes

Merge requirement checklist

Choose a reason for hiding this comment

lalitb Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

lalitb Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 6, 2024 • edited Loading

Codecov Report

lalitb commented Dec 7, 2024 • edited Loading

cijothomas commented Dec 10, 2024

lalitb commented Dec 13, 2024

lalitb Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lalitb commented Dec 2, 2024 •

edited

Loading

lalitb Dec 3, 2024 •

edited

Loading

lalitb Dec 3, 2024 •

edited

Loading

codecov bot commented Dec 6, 2024 •

edited

Loading

lalitb commented Dec 7, 2024 •

edited

Loading

lalitb Dec 13, 2024 •

edited

Loading