Expose async exporter interface about opentelemetry-rust HOT 19 CLOSED

@LukeMathWalker happy to accept a PR for any of those as long as they are a working replacement for the sync behavior. The specific http client is an implementation detail that we can iterate on so seems more important to get a working exporter first 👍

from opentelemetry-rust.

Oh sure, I was just suggesting a temporary workaround to avoid blocking the core thread to ship traces 😁

from opentelemetry-rust.

@glademiller it looks like your spawn_blocking closure creates a future (the async block) that is never awaited, so task is never polled

from opentelemetry-rust.

Resolved in #232

from opentelemetry-rust.

@samschlegel did you have any preliminary thoughts on what you'd like to see in this interface?

from opentelemetry-rust.

Something I've been thinking about is how a lot of the same abstractions we have in tower would be nice for building exporters. I'm not sure that you'd want to add a tower dep though, but I rather like that interface. Would make batching and buffering and retries really easy. Right now my work on Rust tracing has been put on hold for a bit, but I was planning on writing a ServiceExporter that would basically do that.

from opentelemetry-rust.

I'd be happy to help here as well - from a quick look I am not a 100% sure of how extensive the changes would have to be though. Am I mistaken in believing that it might be necessary to touch the thrift crate?

from opentelemetry-rust.

@LukeMathWalker Seems like there are really two changes here:

1. Finding an alternative sync http client to replace reqwest so it does not panic when used in async apps.
2. SpanExporter trait would ideally have a method that returns a future (or Pin<Box<dyn Future>>), then the jaeger/zipkin exporters could use that to avoid using blocking APIs entirely.

from opentelemetry-rust.

For 1., we could look into

• ureq;
• isahc;
• attohttpc.

A good starting point could be this analysis.

from opentelemetry-rust.

I haven't actually tested this code but it is pretty straightforward if someone wants to run it an make sure everything is in order. It compiles all right #116

from opentelemetry-rust.

@jtescher quick question that I haven't been able to answer quickly myself by looking at the code. Using ureq shouldn't interfere directly with the tokio event loop or runtime but where is the collector code being called? Can I put it on its own thread or is it on its own thread already so it won't interfere with my event loop? Or is this something that I will have to wait for an async exporter to have? Thanks for all your help.

from opentelemetry-rust.

@glademiller So currently you pass in a function pointer basicaly fn(Future) -> T to however you want the batch span processor to be run, e.g. https://github.com/open-telemetry/opentelemetry-rust/blob/master/examples/async/src/main.rs#L65 passes tokio::spawn so the batch span processor work is spawned on the current executor.

You could pass a pointer to a function that would spin up a new thread and do the work there if you wanted to customize this behavior beyond what tokio or async_std do by default.

Possibly something along the lines of:

fn spawn<T>(task: T) -> JoinHandle where
    T: Future + Send + 'static,
{
  thread::spawn(|| {
    // do other setup work and spawn task
  })
}

from opentelemetry-rust.

@glademiller the async exporter interface would likely add a similar trait as https://github.com/open-telemetry/opentelemetry-rust/blob/master/src/exporter/trace/mod.rs#L34-L57, but allow the result to be a future, which would let async http clients not have to block to accommodate the current sync methods.

from opentelemetry-rust.

Would passing tokio::spawn_blocking there solve the issue?

from opentelemetry-rust.

@LukeMathWalker Well this issue specifically is allowing the exporters to be entirely async (updated issue description to reflect #121 being merged)

from opentelemetry-rust.

@LukeMathWalker yeah good point, haven't tried it but that may be a good workaround for now 👍

from opentelemetry-rust.

@LukeMathWalker @jtescher I was attempting to use the work around described but it appears something about it causes traces to never get sent. This is how I have defined my spawn function as follows

fn spawn<T>(task: T) -> tokio::task::JoinHandle<impl futures::Future> where T: futures::Future + Send + 'static {
    tokio::task::spawn_blocking(|| async {
        task.await
    })
}

This might not be the best place to discuss a workaround but I'm hoping to move forward with something that works in the near term.

Any thoughts as to why traces would not be sent with my implementation of spawn?

from opentelemetry-rust.

Pretty obvious in hindsight. Thank you @hawkw

from opentelemetry-rust.

In case someone is looking for the workaround this is what I landed on

fn spawn<T>(task: T) -> std::thread::JoinHandle<()> where T: futures::Future + Send + 'static {
    std::thread::spawn(|| {
        futures::executor::block_on(async { task.await });
    })
}

from opentelemetry-rust.