From 26aba4500c33de41552e6d99befc8babb315f21f Mon Sep 17 00:00:00 2001
From: John Gallagher <john@oxidecomputer.com>
Date: Wed, 15 May 2024 12:08:43 -0400
Subject: [PATCH] Enable the oximeter producer garbage collection RPW (#5764)

Closes #5284.
---
 dev-tools/omdb/tests/successes.out            |  2 +-
 .../src/app/background/metrics_producer_gc.rs | 41 +++----------------
 test-utils/src/dev/test_cmds.rs               | 25 +++++++++--
 3 files changed, 27 insertions(+), 41 deletions(-)

diff --git a/dev-tools/omdb/tests/successes.out b/dev-tools/omdb/tests/successes.out
index 0aa47f2712..8c68b0f431 100644
--- a/dev-tools/omdb/tests/successes.out
+++ b/dev-tools/omdb/tests/successes.out
@@ -426,7 +426,7 @@ task: "metrics_producer_gc"
   currently executing: no
   last completed activation: <REDACTED ITERATIONS>, triggered by an explicit signal
     started at <REDACTED     TIMESTAMP> (<REDACTED DURATION>s ago) and ran for <REDACTED DURATION>ms
-    last completion reported error: metric producer gc disabled (omicron#5284)
+warning: unknown background task: "metrics_producer_gc" (don't know how to interpret details: Object {"expiration": String("<REDACTED           TIMESTAMP>"), "pruned": Array []})
 
 task: "phantom_disks"
   configured period: every 30s
diff --git a/nexus/src/app/background/metrics_producer_gc.rs b/nexus/src/app/background/metrics_producer_gc.rs
index 1e3b070249..a3e5aaab32 100644
--- a/nexus/src/app/background/metrics_producer_gc.rs
+++ b/nexus/src/app/background/metrics_producer_gc.rs
@@ -22,33 +22,14 @@ use std::time::Duration;
 pub struct MetricProducerGc {
     datastore: Arc<DataStore>,
     lease_duration: Duration,
-    disabled: bool,
 }
 
 impl MetricProducerGc {
     pub fn new(datastore: Arc<DataStore>, lease_duration: Duration) -> Self {
-        Self {
-            datastore,
-            lease_duration,
-            // TODO We should turn this task on as a part of landing the rest of
-            // the move to metric producer leases. For now, we leave it disabled
-            // to avoid pruning producers that don't know to renew leases, but
-            // make this a boolean so our unit test can enable it.
-            disabled: true,
-        }
+        Self { datastore, lease_duration }
     }
 
     async fn activate(&mut self, opctx: &OpContext) -> serde_json::Value {
-        if self.disabled {
-            warn!(
-                opctx.log,
-                "Metric producer GC: statically disabled pending omicron#5284"
-            );
-            return json!({
-                "error": "metric producer gc disabled (omicron#5284)",
-            });
-        }
-
         let Some(expiration) = TimeDelta::from_std(self.lease_duration)
             .ok()
             .and_then(|delta| Utc::now().checked_sub_signed(delta))
@@ -219,25 +200,13 @@ mod tests {
             .await
             .expect("failed to insert producer");
 
-        // Activate the task. It should immediately return because our GC is
-        // currently statically disabled (remove this check once that is no
-        // longer true!).
+        // Create the task and activate it. Technically this is racy in that it
+        // could prune the producer we just added, but if it's been an hour
+        // since then, we have bigger problems. This should _not_ prune the
+        // producer, since it's been active within the last hour.
         let mut gc =
             MetricProducerGc::new(datastore.clone(), Duration::from_secs(3600));
         let value = gc.activate(&opctx).await;
-        assert_eq!(
-            value,
-            json!({
-                "error": "metric producer gc disabled (omicron#5284)",
-            })
-        );
-
-        // Enable the task and activate it. Technically this is racy, but if
-        // it's been an hour since we inserted the producer in the previous
-        // statement, we have bigger problems. This should _not_ prune the
-        // producer, since it's been active within the last hour.
-        gc.disabled = false;
-        let value = gc.activate(&opctx).await;
         let value = value.as_object().expect("non-object");
         assert!(!value.contains_key("failures"));
         assert!(value.contains_key("expiration"));
diff --git a/test-utils/src/dev/test_cmds.rs b/test-utils/src/dev/test_cmds.rs
index 51ade208f8..3c675ddfd9 100644
--- a/test-utils/src/dev/test_cmds.rs
+++ b/test-utils/src/dev/test_cmds.rs
@@ -160,10 +160,27 @@ pub fn redact_variable(input: &str) -> String {
         .replace_all(&s, "<REDACTED_TIMESTAMP>")
         .to_string();
 
-    let s = regex::Regex::new(r"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z")
-        .unwrap()
-        .replace_all(&s, "<REDACTED     TIMESTAMP>")
-        .to_string();
+    let s = {
+        let mut new_s = String::with_capacity(s.len());
+        let mut last_match = 0;
+        for m in regex::Regex::new(r"\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d+Z")
+            .unwrap()
+            .find_iter(&s)
+        {
+            new_s.push_str(&s[last_match..m.start()]);
+            new_s.push_str("<REDACTED");
+            // We know from our regex that `m.len()` is at least 2 greater than
+            // the length of "<REDACTEDTIMESTAMP>", so this subtraction can't
+            // underflow. Insert spaces to match widths.
+            for _ in 0..(m.len() - "<REDACTEDTIMESTAMP>".len()) {
+                new_s.push(' ');
+            }
+            new_s.push_str("TIMESTAMP>");
+            last_match = m.end();
+        }
+        new_s.push_str(&s[last_match..]);
+        new_s
+    };
 
     // Replace formatted durations.  These are pretty specific to the background
     // task output.