From 0225e44dbc7b676d6755badc04c0db3f457f7af7 Mon Sep 17 00:00:00 2001
From: Arpad Borsos <arpad.borsos@sentry.io>
Date: Tue, 27 Feb 2024 16:53:17 +0100
Subject: [PATCH] Slight metrics improvements (#1387)

- Avoid `format!`-ing metric keys
- Use macros instead of manual invocations
- Add a metrics for the JS `file_in_bundle` cache
---
 crates/symbolicator-js/src/bundle_lookup.rs   |  3 +++
 .../symbolicator-service/src/download/mod.rs  |  8 +++----
 .../symbolicator-service/src/utils/futures.rs | 24 +++++++------------
 crates/symbolicator/src/endpoints/metrics.rs  |  5 +++-
 4 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/crates/symbolicator-js/src/bundle_lookup.rs b/crates/symbolicator-js/src/bundle_lookup.rs
index 4a4e46239..b82a5ee27 100644
--- a/crates/symbolicator-js/src/bundle_lookup.rs
+++ b/crates/symbolicator-js/src/bundle_lookup.rs
@@ -1,6 +1,7 @@
 use std::fmt;
 use std::time::Duration;
 
+use symbolicator_service::metric;
 use symbolicator_sources::RemoteFileUri;
 
 use crate::interface::ResolvedWith;
@@ -62,10 +63,12 @@ impl FileInBundleCache {
             // XXX: this is a really horrible workaround for not being able to look up things via `(&A, &B)` instead of `&(A, B)`.
             let lookup_key = (bundle_uri, key);
             if let Some((file_entry, resolved_with)) = self.cache.get(&lookup_key) {
+                metric!(counter("js.file_in_bundle.hit") += 1);
                 return Some((lookup_key.0, file_entry, resolved_with));
             }
             key = lookup_key.1;
         }
+        metric!(counter("js.file_in_bundle.miss") += 1);
         None
     }
 
diff --git a/crates/symbolicator-service/src/download/mod.rs b/crates/symbolicator-service/src/download/mod.rs
index 7d1059f80..306da99af 100644
--- a/crates/symbolicator-service/src/download/mod.rs
+++ b/crates/symbolicator-service/src/download/mod.rs
@@ -615,9 +615,9 @@ impl Drop for MeasureSourceDownloadGuard<'_> {
         };
 
         let duration = self.creation_time.elapsed();
-        let metric_name = format!("{}.duration", self.task_name);
         metric!(
-            timer(&metric_name) = duration,
+            timer("download_duration") = duration,
+            "task_name" => self.task_name,
             "status" => status,
             "source" => self.source_name,
         );
@@ -631,9 +631,9 @@ impl Drop for MeasureSourceDownloadGuard<'_> {
                 .checked_div(duration.as_millis())
                 .and_then(|t| t.try_into().ok())
                 .unwrap_or(bytes_transferred);
-            let throughput_name = format!("{}.throughput", self.task_name);
             metric!(
-                histogram(&throughput_name) = throughput,
+                histogram("download_throughput") = throughput,
+                "task_name" => self.task_name,
                 "status" => status,
                 "source" => self.source_name,
             );
diff --git a/crates/symbolicator-service/src/utils/futures.rs b/crates/symbolicator-service/src/utils/futures.rs
index 391dfe0b2..b944067c2 100644
--- a/crates/symbolicator-service/src/utils/futures.rs
+++ b/crates/symbolicator-service/src/utils/futures.rs
@@ -3,8 +3,6 @@ use std::time::Instant;
 
 use tokio::task::JoinHandle;
 
-use crate::metrics::{self, prelude::*};
-
 /// Execute a callback on dropping of the container type.
 ///
 /// The callback must not panic under any circumstance. Since it is called while dropping an item,
@@ -100,12 +98,10 @@ impl<'a> MeasureGuard<'a> {
     /// By default, the future is waiting to be polled. `start` emits the `futures.wait_time`
     /// metric.
     pub fn start(&mut self) {
-        metrics::with_client(|client| {
-            client
-                .time_with_tags("futures.wait_time", self.creation_time.elapsed())
-                .with_tag("task_name", self.task_name)
-                .send();
-        })
+        metric!(
+            timer("futures.wait_time") = self.creation_time.elapsed(),
+            "task_name" => self.task_name,
+        );
     }
 
     /// Marks the future as terminated and emits the `futures.done` metric.
@@ -120,13 +116,11 @@ impl Drop for MeasureGuard<'_> {
             MeasureState::Pending => "canceled",
             MeasureState::Done(status) => status,
         };
-        metrics::with_client(|client| {
-            client
-                .time_with_tags("futures.done", self.creation_time.elapsed())
-                .with_tag("task_name", self.task_name)
-                .with_tag("status", status)
-                .send();
-        })
+        metric!(
+            timer("futures.done") = self.creation_time.elapsed(),
+            "task_name" => self.task_name,
+            "status" => status,
+        );
     }
 }
 
diff --git a/crates/symbolicator/src/endpoints/metrics.rs b/crates/symbolicator/src/endpoints/metrics.rs
index f1e563a84..a3381c11f 100644
--- a/crates/symbolicator/src/endpoints/metrics.rs
+++ b/crates/symbolicator/src/endpoints/metrics.rs
@@ -39,7 +39,10 @@ where
                 .as_ref()
                 .map(|r| r.status())
                 .unwrap_or(StatusCode::INTERNAL_SERVER_ERROR);
-            metric!(counter(&format!("responses.status_code.{status}")) += 1);
+            metric!(
+                counter("responses.status_code") += 1,
+                "status" => status.as_str(),
+            );
         }
         poll
     }