add flush_sink() to StatsdClient

[56quarters]

@zh-jq Hi!

Can you please explain why this is required? I'm hesitant to expose client internals via new methods on StatsdClient.

[zh-jq]

StatsdClient take the buffered sink, and we need a way to flush the buffered sink after batch send of metrics.

[56quarters]

StatsdClient take the buffered sink, and we need a way to flush the buffered sink after batch send of metrics.

Assuming you know the buffered sink will flush when it's full, I'd rather handle the case where it needs to be flushed explicitly or periodically with a custom MetricSink of some sort instead of exposing a method on the StatsdClient that doesn't make sense for some kinds of sinks.

Something like

#[derive(Clone)]
struct CloneableSink {
    wrapped: Arc<dyn MetricSink + Send + Sync>
}

impl MetricSink for CloneableSink {
    fn emit(&self, metric: &str) -> io::Result<usize> {
        self.wrapped.emit(metric)
    }

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

let real_sink = BufferedUdpMetricSink::new();
let wrapped1 = CloneableSink{ wrapped: Arc::new(real_sink) };
let wrapped2 = wrapped1.clone();

let client = StatsdClient::from_sink("prefix", wrapped1);

// do a bunch of work that emits some metrics

wrapped2.flush();

[zh-jq]

exposing a method on the StatsdClient that doesn't make sense for some kinds of sinks

A wrapper is working. But just like flush in std::io::Write or tokio::io::AsyncWrite trait, the flush method doesn't make sense for all of the implementation types, but it does make sense for the trait and for the code that use the trait.

[56quarters]

I'm going to close this PR. My reasoning is:

• I don't want to add another method to the API of StatsdClient unless necessary.
• It's possible to achieve what this does using a wrapper MetricSink as demonstrated above.
• This wouldn't work in the most common use of Cadence (QueuingMetricSink wrapping BufferedUdpMetricSink) since the QueuingMetricSink::flush() method is a no-op. This would require QueuingMetricSink to be modified to retain a reference to any sink it wrapped. I'm not sure if QueuingMetricSink keeping a reference to the wrapped sink is wrong, but it feels fragile. I could be wrong about this though.

If there is enough of a use case for flushing buffered sinks when they aren't full, I would accept a PR that adds a FlushingMetricSink that periodically calls the flush() method of a wrapped sink.

[zh-jq]

• I don't want to add another method to the API of StatsdClient unless necessary.

It's more clean to use just one StatsdClient in APIs like we have done here: https://github.com/bytedance/g3/blob/master/g3proxy/src/stat/mod.rs#L58-L78

• It's possible to achieve what this does using a wrapper MetricSink as demonstrated above.
• This wouldn't work in the most common use of Cadence (QueuingMetricSink wrapping BufferedUdpMetricSink) since the QueuingMetricSink::flush() method is a no-op. This would require QueuingMetricSink to be modified to retain a reference to any sink it wrapped. I'm not sure if QueuingMetricSink keeping a reference to the wrapped sink is wrong, but it feels fragile. I could be wrong about this though.

If there is enough of a use case for flushing buffered sinks when they aren't full, I would accept a PR that adds a FlushingMetricSink that periodically calls the flush() method of a wrapped sink.

Use a Arc\<dyn MetricSink> or Arc\<FlushingMetricSink> will somehow hurt the performance which can be avoided.