From f7626226d59de2d8f17260ca4a5d8d6a8f2a529c Mon Sep 17 00:00:00 2001
From: Sebastian Zivota <loewenheim@users.noreply.github.com>
Date: Mon, 13 Jan 2025 12:42:25 +0100
Subject: [PATCH] feat: Report download metrics for internal symbol sources
 (#1589)

---
 .../symbolicator-service/src/download/mod.rs   | 18 ++++++++++++++++--
 crates/symbolicator-sources/src/remotefile.rs  | 10 +++++++---
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/crates/symbolicator-service/src/download/mod.rs b/crates/symbolicator-service/src/download/mod.rs
index 76e08de3e..48b57d633 100644
--- a/crates/symbolicator-service/src/download/mod.rs
+++ b/crates/symbolicator-service/src/download/mod.rs
@@ -329,8 +329,7 @@ impl DownloadService {
     ) -> CacheEntry {
         let host = source.host();
 
-        // Check whether `source` is an internal Sentry source. We don't ever
-        // want to put such sources on the block list.
+        let is_builtin_source = source.is_builtin();
         let source_metric_key = source.source_metric_key().to_string();
         // NOTE: This allow-lists every external non-http symbol server.
         // This includes S3, GCS, and builtin http symbol servers that might misbehave.
@@ -381,6 +380,21 @@ impl DownloadService {
             }
         }
 
+        // Emit metrics about download result for internal sources
+        if is_builtin_source {
+            let status = match result {
+                Ok(_) => "success",
+                Err(CacheError::NotFound) => "notfound",
+                Err(CacheError::PermissionDenied(_)) => "permissiondenied",
+                Err(CacheError::Timeout(_)) => "timeout",
+                Err(CacheError::DownloadError(_)) => "downloaderror",
+                Err(CacheError::Malformed(_)) => "malformed",
+                Err(CacheError::Unsupported(_)) => "unsupported",
+                Err(CacheError::InternalError) => "internalerror",
+            };
+            metric!(counter("service.builtin_source.download") += 1, "source" => &source_metric_key, "status" => status);
+        }
+
         result
     }
 
diff --git a/crates/symbolicator-sources/src/remotefile.rs b/crates/symbolicator-sources/src/remotefile.rs
index a2457b0bf..58901bc13 100644
--- a/crates/symbolicator-sources/src/remotefile.rs
+++ b/crates/symbolicator-sources/src/remotefile.rs
@@ -231,13 +231,12 @@ impl RemoteFile {
     /// If this is a built-in source the source_id is returned, otherwise this falls
     /// back to the source type name.
     pub fn source_metric_key(&self) -> &str {
-        let id = self.source_id().as_str();
         // The IDs of built-in sources (see: SENTRY_BUILTIN_SOURCES in sentry) always start with
         // "sentry:" (e.g. "sentry:electron") and are safe to use as a key. If this is a custom
         // source, then the source_id is a random string which inflates the cardinality of this
         // metric as the tag values will greatly vary.
-        if id.starts_with("sentry:") {
-            return id;
+        if self.is_builtin() {
+            return self.source_id().as_str();
         }
         match self {
             Self::Sentry(..) => "sentry",
@@ -248,6 +247,11 @@ impl RemoteFile {
         }
     }
 
+    /// Returns whether a symbol source is builtin (see `SENTRY_BUILTIN_SOURCES` in sentry).
+    pub fn is_builtin(&self) -> bool {
+        self.source_id().as_str().starts_with("sentry:")
+    }
+
     /// Returns a URI for the location of the object file.
     ///
     /// There is no guarantee about any format of this URI, for some sources it could be