Option to observe write errors

[mlowicki]

Currently for the sinks like QueuingMetricSink which are passing data to the downstream sink in an async way there is no way to see if there were any errors while writing metrics - https://github.com/56quarters/cadence/blob/master/cadence/src/sinks/queuing.rs#L161.

The possible idea would be an option to register error callback which would be called each time write fails so the app could e.g. measure number of errors or report those in some way. We could extend MetricSink and provide default no-op impl to make the change backward compatible.

Opinions?

[56quarters]

The possible idea would be an option to register error callback which would be called each time write fails so the app could e.g. measure number of errors or report those in some way. We could extend MetricSink and provide default no-op impl to make the change backward compatible.

Good idea but I'd rather not add it to MetricSink since it's behavior that only really makes sense with QueuingMetricSink. We could add a new constructor to QueuingMetricSink that accepts a closure to handle the errors similar to how the client currently does when calling client.count_with_tags().send()

[mlowicki]

Right, although on our side as sinks as composable we usually have signatures using MetricSink trait not particular implementation or QueuingMetricSink might be wrapped by some other custom sink. So if that would be added only to QueuingMetricSink then it might be not that easy to use. Having something like:

fn onError(fn: Box<dyn Fn(MetricError) + Sync + Send + RefUnwindSafe>)

added to MetricSink would make it easier to use and it would allow to impl easily in the future for other cases where writes are handled asynchronously.

Already emit from MetricSink already describes the possibility that write may not be synchronous:

/// Note that implementations may return `0` bytes if the metric is not
/// immediately written (such as when it is buffered).  Callers should *NOT*
/// interpret this as an error.

so having onError would be complementary to it.

[56quarters]

I don't like the idea of adding a mutator method to the MetricSink trait. Because the problem of missing errors in Cadence itself is confined to the QueuingMetricSink, I'd rather add it only to that sink.

[mlowicki]

https://github.com/56quarters/cadence/pull/195