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

Should Exporter force async? #2031

Open
cijothomas opened this issue Aug 16, 2024 · 4 comments
Open

Should Exporter force async? #2031

cijothomas opened this issue Aug 16, 2024 · 4 comments
Milestone

Comments

@cijothomas
Copy link
Member

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member Author

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

@demurgos
Copy link
Contributor

@cijothomas Here you go: #2143

@lalitb lalitb added this to the Logging SDK Stable milestone Nov 26, 2024
@cijothomas cijothomas modified the milestones: Logging SDK Stable, 0.28.0 Dec 10, 2024
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

No branches or pull requests

3 participants