Make StatsdClient more convenient to use

[robinst]

I'm using this crate to add metrics to a small server application that uses the Iron framework.

For a request handler, I have a struct that implements Iron's Handler trait (note that it needs to be Send + Sync because it's used for handling requests in different threads).

In the handler struct, I keep things that are needed when handling the requests. One of these things is a slog Logger. The nice thing about it is that I can construct it when starting up the application and clone() it when I put it in the struct. And because it's Send + Sync, it works nicely.

For metrics, I started out with putting a StatsdClient<QueuingMetricSink> into the handler struct.
But then ideally, for testing, I'd just want to use a StatsdClient<NopMetricSink> instead.

So what I ended up with in the end is a struct like this, with some methods that delegate to the client:

#[derive(Clone)]
pub struct Metrics {
    client: Arc<MetricClient + Send + Sync>,
}

Using that, I can easily construct a client that uses NopMetricSink if I want to disable metrics.

So I'm wondering if it would be generally more useful if StatsdClient itself didn't come with generics but instead wrapped its sink in an Arc (see the implementation of Logger).

That would also make it nicer when code needs to pass a StatsdClient around as an argument/field (which happens in my code base and I'd expect in most real-world uses of this).

What do you think?

[56quarters]

Ha, funny you should mention this. I was just thinking about how to add some sort of MetricFormatter trait + impl to StatsdClient for #41 and it seemed a little ugly to have to start passing another generic param around.

Getting rid of the generic param and wrapping the sink in Arc (as well as making the client Clone) makes sense to me. Since this is a breaking change it would probably end up in 0.10.0 or later.

[56quarters]

The only problem I see with wrapping the sink in an Arc<MetricSink + Sync + Send> is that the AsyncMetricSink, which uses a thread pool, is not Sync. Since it uses a std::sync::mpsc::Sender instance, it relies on being .cloned() between threads.

So, AsyncMetricSink is not compatible with using an Arc<MetricSink + Sync + Send> in the client struct. But, I've been wanting to get rid of AsyncMetricSink anyway.

So my plan is to deprecate AsyncMetricSink (per #34) and release version 0.10.0. After that I'll remove it and make this change in version 0.11.0.

[robinst]

Yay, excellent!

[56quarters]

Hey @robinst,

I submitted a PR to remove the generic param from the client (#52) if you like to check it out. The biggest change was adding some docs to the client on the ways that it could be shared between threads, based on the use cases you've described here. Let me know what you think. Thanks!

[robinst]

Looks good! Just had a single comment. Like the docs as well, very nice.