Make HistogramType public

[GeorgeHahn]

This enables direct construction of the StatsdRecorder type for cases where a different sink configuration is desired.

[mbellani]

Thanks @GeorgeHahn for the PR. Can you describe in a little more detail what are you trying to achieve? I'm trying to understand in what cases you'd need to construct recorder directly instead of going through the StatsdBuilder.

Aren't the options exposed by the builder enough? What sink configuration are you trying to change? Currently you can configure a few different values.

[GeorgeHahn]

Totally. I'm using this in a batch service that has two additional needs. It has quiet periods where few metrics are emitted, and it needs to be able to shut down gracefully without losing metrics. I wrote a small metrics middleware to handle both of those; it flushes the underlying sink periodically and it exposes a controlled shutdown mechanism. (This is the recommendation from 56quarters/cadence#100.)

This change will also enable the use of a UDS sink.

[mbellani]

I see, thanks for the explanation. I was wondering if it would be better to modify StatsdBuilder instead to accept a fully configured sink and the build method could detect that you have configured your own sink so it wouldn't build one for you and use the provided one instead. @jorendorff or @gorzell what do you tink?

[jorendorff]

Oh, that StatsdRecorder::new method is public by accident. I think the reason we have a builder is that we didn't want to commit to StatsdRecorder having exactly those two parts forever.

@GeorgeHahn I think the best outcome would be if you can contribute the small middleware you've written. We can just make that the default behavior. Presumably most users would rather not lose metrics! But if not, please look at #23 and see if .with_sink() would work for you.

[GeorgeHahn]

The middleware I wrote is driven by tokio because I didn't want to spawn a thread just for this. It's probably not a great fit for this library as a result. The implementation is below (shared under the MIT license).

use cadence::MetricSink;
use std::{sync::Arc, time::Duration};
use tokio::task::JoinHandle;

/// A `cadence` sink middleware that flushes the inner sink periodically
pub struct FlushingSink<T> {
    inner: Arc<T>,
    handle: Arc<JoinHandle<()>>,
}

impl<T> Clone for FlushingSink<T> {
    fn clone(&self) -> Self {
        Self {
            inner: self.inner.clone(),
            handle: self.handle.clone(),
        }
    }
}

impl<T> MetricSink for FlushingSink<T>
where
    T: MetricSink,
{
    fn emit(&self, metric: &str) -> std::io::Result<usize> {
        self.inner.emit(metric)
    }

    fn flush(&self) -> std::io::Result<()> {
        self.inner.flush()
    }
}

impl<T> FlushingSink<T>
where
    T: MetricSink + Send + Sync + 'static,
{
    pub(crate) fn new(inner: T, flush_period: Duration) -> Self {
        let inner = Arc::new(inner);

        let inner2 = inner.clone();
        let handle = tokio::spawn(async move {
            loop {
                tokio::task::block_in_place(|| {
                    let _res = inner2.flush();
                });
                tokio::time::sleep(flush_period).await;
            }
        });

        Self {
            inner,
            handle: Arc::new(handle),
        }
    }

    pub fn abort(&self) {
        self.handle.abort();
        let _res = self.inner.flush();
    }
}

[GeorgeHahn]

@mbellani Just checking in on this. What do you think about this PR or @jorendorff's #23? I can go ahead and rebase and polish either if you have a preference.

[jorendorff]

@GeorgeHahn, I apologize for the inaction here. I want to discuss this with @mbellani tomorrow, hopefully merge something, and publish a release.

[jorendorff]

We merged #23. Thanks for the PR and I'm sorry again for the long delay.

[GeorgeHahn]

No worries at all about the delay; this branch has been solid in the meantime. Thanks for getting the change into a release so quickly!