Tokio client

[bitemyapp]

trafficstars

I'm grateful you wrote this crate, thank you! Spared me having to learn how statsd works.

You're probably not going to love this PR. Some issues that come to mind:

• The way the code for Client is written forced some redundancy between the sync and tokio clients.
• The tokio dependency is unconditional. I tried to make it conditional on an enable_tokio feature but couldn't get it to work.
• I am aware the doc tests don't test the actual Tokio client. I don't know how to make async code work in doc tests. I don't like or use doc tests so this isn't worth my time to learn.
• I had to enable 2018 edition because I don't remember how to use the old module syntax.
• I deleted the weird benchmarks from the original client in the client/tokio module. Couldn't figure out how to get it to work with async. If I was going to write async benchmarks I would probably use the benchmark facilities that don't require using a nightly compiler.
• To be honest, I'm not sure how much making this asynchronous really matters given the protocol uses UDP and the benchmarks show the usual runtime is in single-digit microseconds. I think I spend more time parsing JSON in some of my applications but I'd rather be cautious w/ blocking when it comes to I/O.
• I changed the module structure rather than leaving everything in lib.rs

Let me whether you want to work towards incorporating this PR or not. Either way, I'll know what my next steps are.

Hope you have a good weekend!

[mcasper]

Hey @bitemyapp, thank you for all this work! I like the direction this is headed and would love to work with you to get it in

Regarding the redundancy between client implementations, I agree that it's unfortunate, but it may be necessary to support sync and async. I suppose we could try to change the API away from "convenience function for every stat" to something like "build the stat struct yourself and then use the correct send", but that's probably too big of a change right now.

I think the most reasonable way forward here would be to add a tokio feature to this crate, and keep the same API between sync and async clients, which is what you've already done here for the most part. To make the tokio dependency optional and add the feature to this crate, just throw optional = true on to tokio in the Cargo.toml, and that will take care of both (see https://doc.rust-lang.org/cargo/reference/features.html#optional-dependencies). With the feature in place you'll just need to wrap the appropriate places in #[cfg(feature = "tokio")] and #[cfg(not(feature = "tokio"))] to expose the right client mod, and we'll be all set.

[bitemyapp]

@mcasper I'll try to make the cfg flagging behave along the lines you've suggested here. I don't know if your description was inclusive of this or not but I'd like separate modules for sync and async so that people have a choice. It's mostly down to including or not including tokio and the attendant dependencies that come with that.

[bitemyapp]

I suppose we could try to change the API away from "convenience function for every stat" to something like "build the stat struct yourself and then use the correct send", but that's probably too big of a change right now.

That's the direction I'd be inclined toward since the datatypes are common to both APIs anyway.

FWIW I combed the Cargo docs and examples I could find around optional dependencies when I originally attempted this extensively and I couldn't get it to stop pulling in the async module when I didn't have the async feature enabled. I'm about to take another stab at it.

[bitemyapp]

First note, I'm using stable so I can't have a feature named the same thing as a crate (tokio) because only nightly has namespacing like dep:tokio. I named the feature async for now.

Anyhoodle seems to work now, lol.

[rex-remind101]

any movement here?

[bitemyapp]

I've been using this library in production to emit about 30 billion events per month, however, I've been doing so with a synchronous client because the UDP interface ended up not blocking much. I use the dogstatsd metrics in a Rust Kafka client library that gets embedded in Node.js and Python applications. I might try flipping it over to the async interface and see if it behaves.

[timglabisch]

any news?

[bitemyapp]

I don't have any updates in this moment but I could take a pass at getting something in a mergeable state. You're lucky you pinged me at the beginning of a two week vacation. I think it's a little hard to get people to care about async UDP for small quantities of data like dogstatsd because blocking is usually rare. It can happen (ARP, buffer is full) but pretty exceptional. Not a reason to neglect it entirely but most programmers have more urgent fires to put out or prevent.

[bitemyapp]

@mcasper I updated this MR with the conditional compilation that was needed for the tokio module but I see extensive conflicts with lib.rs. I wasn't involved in the changes that have happened since then so it could take me quite a bit to pick it all apart. It might be better if I restart from scratch but I'm a bit queasy about doing work that won't get merged.

I'm evaluating whether I can rip out async support in our internal library built on this crate now that we're aggressively aggregating our metrics.

What would you like me to do?

[bitemyapp]

Closing as I don't think I'll pursue the async API any further.