56quarters / cadence

An extensible Statsd client for Rust
https://docs.rs/cadence/
Apache License 2.0
84 stars 27 forks source link

Add support for sampling metrics #182

Closed ianks closed 3 weeks ago

ianks commented 1 year ago

This PR adds some initial support for sampling individual metrics. I made it an opt-in feature in Cargo so folks won't pull in the rand dependency by default.

Since I'm new to the codebase, I probably did some things sub-optimally. Let me know I can improve it!

resolves #179

56quarters commented 1 year ago

Thanks! I'll try to take a first pass over this in a bit.

ianks commented 1 year ago

I messed around with a slight refactor of this PR which tries a slightly different approach by moving the sampling behavior to a new type. LMK which you prefer:

diff --git i/cadence/src/types.rs w/cadence/src/types.rs
index 3ececec..ee4c227 100644
--- i/cadence/src/types.rs
+++ w/cadence/src/types.rs
@@ -8,6 +8,7 @@
 // option. This file may not be copied, modified, or distributed
 // except according to those terms.

+use crate::builder::SampleRate;
 use crate::builder::{MetricFormatter, MetricValue};
 use std::error;
 use std::fmt;
@@ -19,6 +20,76 @@ use std::io;
 /// types of metrics as defined in the [Statsd spec](https://github.com/b/statsd_spec).
 pub trait Metric {
     fn as_metric_str(&self) -> &str;
+
+    fn is_sampled(&self) -> bool {
+        true
+    }
+}
+
+/// Marker trait for metrics that can be sampled.
+///
+/// > A float between 0 and 1, inclusive. Only works with COUNT, HISTOGRAM,
+/// > DISTRIBUTION, and TIMER metrics. The default is 1, which samples 100% of the
+/// > time.
+/// > - via [DataDog](https://docs.datadoghq.com/developers/dogstatsd/datagram_shell)
+pub trait Sampleable: Metric {
+    /// Returns a new metric that indicates this metric was sampled.
+    fn sampled(self, sample_rate: f32) -> Result<Sampled<Self>, MetricError>
+    where
+        Self: Sized,
+    {
+        Sampled::new(self, sample_rate)
+    }
+}
+
+impl Sampleable for Counter {}
+impl Sampleable for Timer {}
+impl Sampleable for Gauge {}
+impl Sampleable for Histogram {}
+
+/// Wraps a that indicates a metric was sampled.
+pub struct Sampled<T: Sampleable> {
+    is_sampled: bool,
+    repr: String,
+    _marker: std::marker::PhantomData<T>,
+}
+
+impl<T: Sampleable> Sampled<T> {
+    pub fn new<S: TryInto<SampleRate>>(
+        metric: T,
+        sample_rate: S,
+    ) -> Result<Self, <S as std::convert::TryInto<SampleRate>>::Error> {
+        let sample_rate = sample_rate.try_into()?;
+        let mut repr = String::with_capacity(metric.as_metric_str().len() + sample_rate.kv_size());
+        repr.push_str(metric.as_metric_str());
+        repr.push('|');
+        repr.push_str(sample_rate.as_str());
+
+        #[cfg(feature = "sample-rate")]
+        use rand::Rng;
+
+        #[cfg(feature = "sample-rate")]
+        let is_sampled = rand::thread_rng().gen_bool(sample_rate.value() as f64);
+
+        #[cfg(not(feature = "sample-rate"))]
+        let is_sampled = true;
+
+        Ok(Sampled {
+            repr,
+            is_sampled,
+            _marker: std::marker::PhantomData,
+        })
+    }
+}
+
+impl<T: Sampleable> Metric for Sampled<T> {
+    fn as_metric_str(&self) -> &str {
+        &self.repr
+    }
+
+    fn is_sampled(&self) -> bool {
+        self.is_sampled
+    }
 }

 /// Counters are simple values incremented or decremented by a client.
@@ -307,7 +378,7 @@ pub type MetricResult<T> = Result<T, MetricError>;
 mod tests {
     #![allow(deprecated, deprecated_in_future)]

-    use super::{Counter, ErrorKind, Gauge, Histogram, Meter, Metric, MetricError, Set, Timer};
+    use super::{Counter, ErrorKind, Gauge, Histogram, Meter, Metric, MetricError, Sampleable, Set, Timer};
     use std::error::Error;
     use std::io;

@@ -421,4 +492,17 @@ mod tests {
         let our_err = MetricError::from((ErrorKind::InvalidInput, "Nope!"));
         assert!(our_err.source().is_none());
     }
+
+    #[test]
+    fn test_metrics_can_be_wrapped_as_sampled() {
+        let counter = Counter::new("my.app.", "test.counter", 4).sampled(1.0 / 3.0).unwrap();
+        let gauge = Gauge::new("my.app.", "test.gauge", 2).sampled(0.5).unwrap();
+        let histogram = Histogram::new("my.app.", "test.histogram", 45).sampled(0.5).unwrap();
+        let timer = Timer::new("my.app.", "test.timer", 34).sampled(0.5).unwrap();
+
+        assert_eq!("my.app.test.counter:4|c|@0.33333", counter.as_metric_str());
+        assert_eq!("my.app.test.timer:34|ms|@0.5", timer.as_metric_str());
+        assert_eq!("my.app.test.gauge:2|g|@0.5", gauge.as_metric_str());
+        assert_eq!("my.app.test.histogram:45|h|@0.5", histogram.as_metric_str());
+    }
 }
56quarters commented 1 year ago

Before I take a more thorough look, I've come around to unconditionally including the dependency on rand since it would allow us to get rid of all the conditional compilation. WDYT?

ianks commented 1 year ago

100% agree. Most folks will have it in their dep tree already, anyway I imagine.

56quarters commented 1 year ago

Unrelated to your changes, but make sure you're running tests and linting locally since it seems like I haven't correctly set CI to run for forks.

56quarters commented 1 year ago

I messed around with a slight refactor of this PR which tries a slightly different approach by moving the sampling behavior to a new type. LMK which you prefer:

My preference would be for the original implementation, adding a .with_sample_rate() method to MetricBuilder. It feels more natural to me that you can only add sampling when you're "building" a metric to send, rather than applying a sample after a Metric (Counter, Gauge, etc) has already been constructed.

56quarters commented 3 weeks ago

Closing this in favor of #211 which adds sampling but requires that it happens outside of calls to Cadence.