Add http reporter crate. #19
Conversation
This adds a new crate to the repository containing a http reporter. The reporter features batching of spans, reporting on a background thread and spawning on a tokio core.
|
Thanks for your interest in palantir/rust-zipkin, @hannesg! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request. |
There was a problem hiding this comment.
Thanks! I left some comments in a couple of places but this seems like a pretty good API for an HTTP reporter.
I do want to tweak Reporter to pass the Span in by value. I'll make a separate PR with that, which should let us avoid the SpanBuf type entirely by just directly serializing a Vec<Span>.
zipkin-reporter-http/src/lib.rs
Outdated
|
|
||
| impl zipkin::Report for Reporter { | ||
|
|
||
| fn report(&self, span: &zipkin::Span) { |
There was a problem hiding this comment.
Hmm, we should really make report take the span by value which would allow us to avoid all of the custom Buf weirdness.
There was a problem hiding this comment.
Cool, thank you. That really simplifies writing a reporter. I'll update the code.
zipkin-reporter-http/src/lib.rs
Outdated
| /// Starts building a new client using the supplied Uri. | ||
| pub fn new( uri : http::Uri ) -> Self { | ||
| let mut parts = uri.into_parts(); | ||
| parts.path_and_query = Some( http::uri::PathAndQuery::from_static("/api/v2/spans") ); |
There was a problem hiding this comment.
I think this will break if the configured URI has a path (e.g. if the Zipkin server is behind a reverse proxy). We'll probably want to instead concatenate /api/v2/spans to the URI's path.
zipkin-reporter-http/src/lib.rs
Outdated
| /// | ||
| /// # Panics | ||
| /// When the OS fails to create the backing thread this method panics. | ||
| pub fn start_thread( self ) -> (thread::JoinHandle<()>, Reporter) { |
There was a problem hiding this comment.
Given that this is the only method defined on BuilderWithErrorHandler, it might make more sense to not have the separate type and instead just have a start_thread method on Builder which takes the error handler and starts up the thread there?
zipkin-reporter-http/src/lib.rs
Outdated
| let handle = thread::Builder::new() | ||
| .name("zipkin-reporter-http".to_string()) | ||
| .spawn(move ||{ | ||
| hyper::rt::run(worker.then(move |r|{ |
There was a problem hiding this comment.
You can simplify this a bit by using map_error instead of then.
This is strictly more flexible wrt e.g. sending the spans to a separate thread, and we're not doing anything with the span after reporting it anyway. This makes the trait definition a bit weird but we can fix that in 0.4.
This adds a new crate to the repository containing a http reporter. The reporter features batching of spans, reporting on a background thread and spawning on a tokio core.
hannesg
force-pushed
the
http_reporter
branch
from
9a592bb
to
963c821
Compare
|
Is this being worked on? I rebased in my branch and added a few more things, and it works okay. I could lend a hand if needed. |
|
Here it is in case you'd like to cherry pick the commits: https://github.com/vavrusa/rust-zipkin/tree/http_reporter |
Hi rust-zipkin team
I noticed that there is no http reporter around ( or at least I didn't find it ). So I wrote one.
This adds a new crate to the repository containing a http reporter. The reporter features batching of spans, reporting on a background thread and spawning on a tokio core.
Are you interested in integrating this? Otherwise I'd create a dedicate repository for it in my account.