From bccbf1632ba17414adf84a48d730f1249121a931 Mon Sep 17 00:00:00 2001
From: David Newell <d.newell1@outlook.com>
Date: Thu, 7 Nov 2024 11:40:17 +0100
Subject: [PATCH] chore: pre shipping review improvements (#26059)

---
 rust/cymbal/src/frames/mod.rs      |  9 ++-------
 rust/cymbal/src/frames/resolver.rs | 12 ++++++------
 rust/cymbal/src/main.rs            | 12 ++++++++----
 rust/cymbal/src/types/mod.rs       | 16 ----------------
 4 files changed, 16 insertions(+), 33 deletions(-)

diff --git a/rust/cymbal/src/frames/mod.rs b/rust/cymbal/src/frames/mod.rs
index 09b12ff625b89..01ba7d13e4e97 100644
--- a/rust/cymbal/src/frames/mod.rs
+++ b/rust/cymbal/src/frames/mod.rs
@@ -32,14 +32,9 @@ impl RawFrame {
         res
     }
 
-    pub fn needs_symbols(&self) -> bool {
-        // For now, we only support JS, so this is always true
-        true
-    }
-
-    pub fn symbol_set_ref(&self) -> String {
+    pub fn symbol_set_ref(&self) -> Option<String> {
         let RawFrame::JavaScript(raw) = self;
-        raw.source_url().map(String::from).unwrap_or_default()
+        raw.source_url().map(String::from).ok()
     }
 
     pub fn frame_id(&self) -> String {
diff --git a/rust/cymbal/src/frames/resolver.rs b/rust/cymbal/src/frames/resolver.rs
index 6a10c68c67208..b93da78b6b8ed 100644
--- a/rust/cymbal/src/frames/resolver.rs
+++ b/rust/cymbal/src/frames/resolver.rs
@@ -35,10 +35,6 @@ impl Resolver {
             return Ok(result.contents);
         }
 
-        if !frame.needs_symbols() {
-            return frame.resolve(team_id, catalog).await;
-        }
-
         if let Some(result) =
             ErrorTrackingStackFrame::load(pool, team_id, &frame.frame_id()).await?
         {
@@ -48,7 +44,11 @@ impl Resolver {
 
         let resolved = frame.resolve(team_id, catalog).await?;
 
-        let set = SymbolSetRecord::load(pool, team_id, &frame.symbol_set_ref()).await?;
+        let set = if let Some(set_ref) = frame.symbol_set_ref() {
+            SymbolSetRecord::load(pool, team_id, &set_ref).await?
+        } else {
+            None
+        };
 
         let record = ErrorTrackingStackFrame::new(
             frame.frame_id(),
@@ -212,7 +212,7 @@ mod test {
 
         // get the symbol set
         let set_ref = frame.symbol_set_ref();
-        let set = SymbolSetRecord::load(&pool, 0, &set_ref)
+        let set = SymbolSetRecord::load(&pool, 0, &set_ref.unwrap())
             .await
             .unwrap()
             .unwrap();
diff --git a/rust/cymbal/src/main.rs b/rust/cymbal/src/main.rs
index 8fca47a17f34b..fc18cfbf946b2 100644
--- a/rust/cymbal/src/main.rs
+++ b/rust/cymbal/src/main.rs
@@ -14,7 +14,7 @@ use cymbal::{
 };
 use envconfig::Envconfig;
 use tokio::task::JoinHandle;
-use tracing::{error, info};
+use tracing::{error, info, warn};
 use tracing_subscriber::{layer::SubscriberExt, util::SubscriberInitExt, EnvFilter, Layer};
 
 common_alloc::used!();
@@ -83,7 +83,7 @@ async fn main() -> Result<(), Error> {
         offset.store().unwrap();
 
         if event.event != "$exception" {
-            error!("event of type {}", event.event);
+            warn!("event of type {}", event.event);
             continue;
         }
 
@@ -96,13 +96,17 @@ async fn main() -> Result<(), Error> {
             Ok(r) => r,
             Err(err) => {
                 metrics::counter!(ERRORS, "cause" => "invalid_exception_properties").increment(1);
-                error!("Error parsing properties: {:?}", err);
+                error!(
+                    "Error parsing properties: {:?} from properties {:?}",
+                    err, properties
+                );
                 continue;
             }
         };
 
         let Some(mut exception_list) = properties.exception_list else {
             // Known issue that $exception_list didn't exist on old clients
+            metrics::counter!(ERRORS, "cause" => "no_exception_list").increment(1);
             continue;
         };
 
@@ -155,6 +159,6 @@ async fn main() -> Result<(), Error> {
         let _fingerprint = fingerprinting::generate_fingerprint(&exception_list);
 
         metrics::counter!(STACK_PROCESSED).increment(1);
-        whole_loop.label("had_frame", "true").fin();
+        whole_loop.label("finished", "true").fin();
     }
 }
diff --git a/rust/cymbal/src/types/mod.rs b/rust/cymbal/src/types/mod.rs
index 6a329c75572d2..317262c52aff0 100644
--- a/rust/cymbal/src/types/mod.rs
+++ b/rust/cymbal/src/types/mod.rs
@@ -48,17 +48,6 @@ pub struct Exception {
 pub struct ErrProps {
     #[serde(rename = "$exception_list")]
     pub exception_list: Option<Vec<Exception>>, // Required from exception producers - we will not process events without this. Optional to support older clients, should eventually be removed
-    #[serde(skip_serializing_if = "Option::is_none")]
-    #[serde(rename = "$exception_type")]
-    pub exception_type: Option<String>, // legacy, overridden by exception_list
-    #[serde(skip_serializing_if = "Option::is_none")]
-    #[serde(rename = "$exception_message")]
-    pub exception_message: Option<String>, // legacy, overridden by exception_list
-    #[serde(skip_serializing_if = "Option::is_none")]
-    #[serde(rename = "$exception_stack_trace_raw")]
-    pub exception_stack_trace_raw: Option<String>, // Not all exceptions have a stack trace
-    #[serde(rename = "$exception_level")]
-    pub exception_level: Option<String>, // We generally don't touch this, but we break it out explicitly for users. Not all exceptions have a level
     #[serde(flatten)] // A catch-all for all the properties we don't "care" about
     pub other: HashMap<String, Value>,
 }
@@ -148,11 +137,6 @@ mod test {
         assert!(frame.in_app);
         assert_eq!(frame.line, 64);
         assert_eq!(frame.column, 15003);
-
-        assert_eq!(props.exception_type, None);
-        assert_eq!(props.exception_message, None);
-        assert_eq!(props.exception_stack_trace_raw, None);
-        assert_eq!(props.exception_level, Some("error".to_string()));
     }
 
     #[test]