From da6e10c2ba9f568831ac03200622390ac3f3dbf7 Mon Sep 17 00:00:00 2001
From: Oliver Gould <ver@buoyant.io>
Date: Tue, 19 Nov 2024 03:13:36 +0000
Subject: [PATCH] Update ResponseBody metrics to use a histogram

Signed-off-by: Oliver Gould <ver@buoyant.io>
---
 linkerd/http/prom/src/body_data/body.rs    | 12 +---
 linkerd/http/prom/src/body_data/metrics.rs | 71 ++++++++--------------
 linkerd/metrics/src/lib.rs                 |  7 ++-
 3 files changed, 35 insertions(+), 55 deletions(-)

diff --git a/linkerd/http/prom/src/body_data/body.rs b/linkerd/http/prom/src/body_data/body.rs
index c67c72ef5d..d14a895610 100644
--- a/linkerd/http/prom/src/body_data/body.rs
+++ b/linkerd/http/prom/src/body_data/body.rs
@@ -41,10 +41,7 @@ where
     ) -> Poll<Option<Result<Self::Data, Self::Error>>> {
         let this = self.project();
         let inner = this.inner;
-        let BodyDataMetrics {
-            frames_total,
-            frames_bytes,
-        } = this.metrics;
+        let BodyDataMetrics { frame_size } = this.metrics;
 
         let data = std::task::ready!(inner.poll_data(cx));
 
@@ -53,11 +50,8 @@ where
             //
             // NB: We're careful to call `remaining()` rather than `chunk()`, which
             // "can return a shorter slice (this allows non-continuous internal representation)."
-            let bytes = <B::Data as bytes::Buf>::remaining(data)
-                .try_into()
-                .unwrap_or(u64::MAX);
-            frames_bytes.inc_by(bytes);
-            frames_total.inc();
+            let bytes = bytes::Buf::remaining(data);
+            frame_size.observe(linkerd_metrics::to_f64(bytes as u64));
         }
 
         Poll::Ready(data)
diff --git a/linkerd/http/prom/src/body_data/metrics.rs b/linkerd/http/prom/src/body_data/metrics.rs
index b71bb4092e..3c74fc819c 100644
--- a/linkerd/http/prom/src/body_data/metrics.rs
+++ b/linkerd/http/prom/src/body_data/metrics.rs
@@ -1,23 +1,30 @@
 //! Prometheus counters for request and response bodies.
 
-use linkerd_metrics::prom::{self, Counter, Family, Registry};
+use linkerd_metrics::prom::{
+    self, metrics::family::MetricConstructor, Family, Histogram, Registry, Unit,
+};
 
 /// Counters for response body frames.
 #[derive(Clone, Debug)]
 pub struct ResponseBodyFamilies<L> {
-    /// Counts the number of response body frames.
-    resp_body_frames_total: Family<L, Counter>,
-    /// Counts the total number of bytes in response body frames.
-    resp_body_frames_bytes: Family<L, Counter>,
+    /// Counts the number of response body frames by size.
+    frame_sizes: Family<L, Histogram, NewHisto>,
 }
 
 /// Counters to instrument a request or response body.
-#[derive(Clone, Debug, Default)]
+#[derive(Clone, Debug)]
 pub struct BodyDataMetrics {
     /// Counts the number of request body frames.
-    pub frames_total: Counter,
-    /// Counts the total number of bytes in request body frames.
-    pub frames_bytes: Counter,
+    pub frame_size: Histogram,
+}
+
+#[derive(Clone, Copy)]
+struct NewHisto;
+
+impl MetricConstructor<Histogram> for NewHisto {
+    fn new_metric(&self) -> Histogram {
+        Histogram::new([128.0, 1024.0, 10240.0].into_iter())
+    }
 }
 
 // === impl ResponseBodyFamilies ===
@@ -28,8 +35,7 @@ where
 {
     fn default() -> Self {
         Self {
-            resp_body_frames_total: Default::default(),
-            resp_body_frames_bytes: Default::default(),
+            frame_sizes: Family::new_with_constructor(NewHisto),
         }
     }
 }
@@ -45,50 +51,25 @@ where
         + Sync
         + 'static,
 {
-    const RESP_BODY_FRAMES_TOTAL_NAME: &'static str = "resp_body_frames_total";
-    const RESP_BODY_FRAMES_TOTAL_HELP: &'static str =
-        "Counts the number of frames in response bodies.";
-
-    const RESP_BODY_FRAMES_BYTES_NAME: &'static str = "resp_body_frames_bytes";
-    const RESP_BODY_FRAMES_BYTES_HELP: &'static str =
-        "Counts the total number of bytes in response bodies.";
-
     /// Registers and returns a new family of body data metrics.
     pub fn register(registry: &mut Registry) -> Self {
-        let resp_body_frames_total = Family::default();
-        registry.register(
-            Self::RESP_BODY_FRAMES_TOTAL_NAME,
-            Self::RESP_BODY_FRAMES_TOTAL_HELP,
-            resp_body_frames_total.clone(),
-        );
-
-        let resp_body_frames_bytes = Family::default();
+        let frame_sizes = Family::new_with_constructor(NewHisto);
         registry.register_with_unit(
-            Self::RESP_BODY_FRAMES_BYTES_NAME,
-            Self::RESP_BODY_FRAMES_BYTES_HELP,
-            prom::Unit::Bytes,
-            resp_body_frames_bytes.clone(),
+            "response_frame_size",
+            "Response data frame sizes",
+            Unit::Bytes,
+            frame_sizes.clone(),
         );
 
-        Self {
-            resp_body_frames_total,
-            resp_body_frames_bytes,
-        }
+        Self { frame_sizes }
     }
 
     /// Returns the [`BodyDataMetrics`] for the given label set.
     pub fn metrics(&self, labels: &L) -> BodyDataMetrics {
-        let Self {
-            resp_body_frames_total,
-            resp_body_frames_bytes,
-        } = self;
+        let Self { frame_sizes } = self;
 
-        let frames_total = resp_body_frames_total.get_or_create(labels).clone();
-        let frames_bytes = resp_body_frames_bytes.get_or_create(labels).clone();
+        let frame_size = frame_sizes.get_or_create(labels).clone();
 
-        BodyDataMetrics {
-            frames_total,
-            frames_bytes,
-        }
+        BodyDataMetrics { frame_size }
     }
 }
diff --git a/linkerd/metrics/src/lib.rs b/linkerd/metrics/src/lib.rs
index 1f1dcc3cb6..58f2259642 100644
--- a/linkerd/metrics/src/lib.rs
+++ b/linkerd/metrics/src/lib.rs
@@ -85,7 +85,12 @@ pub trait Factor {
 const MAX_PRECISE_UINT64: u64 = 0x20_0000_0000_0000;
 
 impl Factor for () {
+    #[inline]
     fn factor(n: u64) -> f64 {
-        n.wrapping_rem(MAX_PRECISE_UINT64 + 1) as f64
+        to_f64(n)
     }
 }
+
+pub fn to_f64(n: u64) -> f64 {
+    n.wrapping_rem(MAX_PRECISE_UINT64 + 1) as f64
+}