open-telemetry / opentelemetry-rust

The Rust OpenTelemetry implementation
https://opentelemetry.io
Apache License 2.0
1.82k stars 423 forks source link

Should Exporter force async? #2031

Open cijothomas opened 1 month ago

cijothomas commented 1 month ago

Exporter traits are now written as async. This works great if the exporter actually needs to use async. But when exporter is not needing async (like when exporting to kernel tracing systems like Windows ETW, Linux user_events), the async is causing unnecessary overhead, as shown in the benchmarks.

This issue is to track addressing this. One way could be offer multiple methods. Another would be model etw/user-events exporter as a processor itself,

demurgos commented 1 week ago

I'm coming from #2071. My opinion is that block_on should be avoided at all cost because of bad interactions with single-threaded runtimes and of the hangs that it causes.

If performance is a concern then multiple methods are a good approach I think.

demurgos commented 1 week ago

the async is causing unnecessary overhead, as shown in the benchmarks.

I ran the benchmarks myself to compare, and I agree that there's an overhead when using the async_trait crate. Here are my results:

LogExporterWithFuture
                        time:   [59.786 ns 59.883 ns 59.987 ns]
LogExporterWithoutFuture
                        time:   [50.156 ns 50.266 ns 50.392 ns]

However I also added an impl using native async trait support now that Rust supports it for a while:

LogExporterWithNativeFuture
                         time:   [51.791 ns 51.936 ns 52.102 ns

This suggests that there is no real overhead when using native async traits. This means that there may be no need to require multiple methods if async is good enough.

cijothomas commented 1 week ago

@demurgos can you send a PR to modify the benchmark to use the native async trait part?

demurgos commented 1 week ago

@cijothomas Here you go: https://github.com/open-telemetry/opentelemetry-rust/pull/2143