From 4d3e0abd41edbdbde8dd3e748ccae21bc917fd5c Mon Sep 17 00:00:00 2001
From: Oliver Gould <ver@buoyant.io>
Date: Mon, 12 Nov 2018 18:50:36 -0800
Subject: [PATCH] Ensure metrics are not evicted for active routes (#124)

It was possible for a metrics scope to be deregistered for active
routes. This could cause metrics to disappear and never be recorded in
some situations.

This change ensure that metrics are only evicted for scopes that are not
active (i.e. in a router, load balancer, etc).
---
 src/proxy/http/metrics/mod.rs | 79 ++++++++++++++++++++++++++++++++++-
 1 file changed, 78 insertions(+), 1 deletion(-)

diff --git a/src/proxy/http/metrics/mod.rs b/src/proxy/http/metrics/mod.rs
index 51775c77a..13986a1fd 100644
--- a/src/proxy/http/metrics/mod.rs
+++ b/src/proxy/http/metrics/mod.rs
@@ -74,8 +74,12 @@ where
     T: Hash + Eq,
     C: Hash + Eq,
 {
+    /// Retains metrics for all targets that (1) no longer have an active
+    /// reference to the `Metrics` structure and (2) have not been updated since `epoch`.
     fn retain_since(&mut self, epoch: Instant) {
-        self.by_target.retain(|_, m| m.lock().map(|m| m.last_update >= epoch).unwrap_or(false))
+        self.by_target.retain(|_, m| {
+            Arc::strong_count(&m) > 1 || m.lock().map(|m| m.last_update >= epoch).unwrap_or(false)
+        })
     }
 }
 
@@ -103,3 +107,76 @@ where
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    #[test]
+    fn expiry() {
+        use std::fmt;
+        use std::time::Duration;
+        use tokio_timer::clock;
+
+        use metrics::FmtLabels;
+
+        #[derive(Clone, Debug, Hash, Eq, PartialEq)]
+        struct Target(usize);
+        impl FmtLabels for Target {
+            fn fmt_labels(&self, f: &mut fmt::Formatter) -> fmt::Result {
+                write!(f, "n=\"{}\"", self.0)
+            }
+        }
+
+        #[allow(dead_code)]
+        #[derive(Clone, Debug, Hash, Eq, PartialEq)]
+        enum Class {
+            Good,
+            Bad,
+        };
+        impl FmtLabels for Class {
+            fn fmt_labels(&self, f: &mut fmt::Formatter) -> fmt::Result {
+                use std::fmt::Display;
+                match self {
+                    Class::Good => "class=\"good\"".fmt(f),
+                    Class::Bad => "class=\"bad\"".fmt(f),
+                }
+            }
+        }
+
+        let retain_idle_for = Duration::from_secs(1);
+        let (r, report) = super::new::<Target, Class>(retain_idle_for);
+        let mut registry = r.lock().unwrap();
+
+        let before_update = clock::now();
+        let metrics = registry
+            .by_target
+            .entry(Target(123))
+            .or_insert_with(|| Default::default())
+            .clone();
+        assert_eq!(registry.by_target.len(), 1, "target should be registered");
+        let after_update = clock::now();
+
+        registry.retain_since(after_update);
+        assert_eq!(
+            registry.by_target.len(),
+            1,
+            "target should not be evicted by time alone"
+        );
+
+        drop(metrics);
+        registry.retain_since(before_update);
+        assert_eq!(
+            registry.by_target.len(),
+            1,
+            "target should not be evicted by availability alone"
+        );
+
+        registry.retain_since(after_update);
+        assert_eq!(
+            registry.by_target.len(),
+            0,
+            "target should be evicted by time once the handle is dropped"
+        );
+
+        drop((registry, report));
+    }
+}