From ff4ec0aeb49f9c61570f67ca4caf62bfcd1a9ce5 Mon Sep 17 00:00:00 2001 From: Oliver Browne Date: Tue, 19 Nov 2024 13:16:47 +0200 Subject: [PATCH] fix(err): structured context, liveness often (#26275) --- rust/cymbal/src/frames/mod.rs | 24 ++++++++++++++++- rust/cymbal/src/frames/records.rs | 21 ++++++++++----- rust/cymbal/src/langs/js.rs | 43 ++++++++++++++++++------------- rust/cymbal/src/lib.rs | 7 +++-- 4 files changed, 67 insertions(+), 28 deletions(-) diff --git a/rust/cymbal/src/frames/mod.rs b/rust/cymbal/src/frames/mod.rs index 5b8d8fdfadb..89fce9d8cad 100644 --- a/rust/cymbal/src/frames/mod.rs +++ b/rust/cymbal/src/frames/mod.rs @@ -68,7 +68,20 @@ pub struct Frame { // it should never go in clickhouse / be queried over, but we do store it in PG for // use in the frontend #[serde(skip)] - pub context: Option, + pub context: Option, +} + +#[derive(Debug, Clone, Serialize, Deserialize, Eq, PartialEq)] +pub struct Context { + pub before: Vec, + pub line: ContextLine, + pub after: Vec, +} + +#[derive(Debug, Clone, Serialize, Deserialize, Eq, PartialEq)] +pub struct ContextLine { + number: u32, + line: String, } impl Frame { @@ -98,3 +111,12 @@ impl Frame { h.update(self.lang.as_bytes()); } } + +impl ContextLine { + pub fn new(number: u32, line: impl ToString) -> Self { + Self { + number, + line: line.to_string(), + } + } +} diff --git a/rust/cymbal/src/frames/records.rs b/rust/cymbal/src/frames/records.rs index 2e00e5fccd5..157fd9d85ce 100644 --- a/rust/cymbal/src/frames/records.rs +++ b/rust/cymbal/src/frames/records.rs @@ -6,7 +6,7 @@ use uuid::Uuid; use crate::error::UnhandledError; -use super::Frame; +use super::{Context, Frame}; #[derive(Debug, Serialize, Deserialize, Clone)] pub struct ErrorTrackingStackFrame { @@ -16,7 +16,7 @@ pub struct ErrorTrackingStackFrame { pub symbol_set_id: Option, pub contents: Frame, pub resolved: bool, - pub context: Option, + pub context: Option, } impl ErrorTrackingStackFrame { @@ -26,7 +26,7 @@ impl ErrorTrackingStackFrame { symbol_set_id: Option, contents: Frame, resolved: bool, - context: Option, + context: Option, ) -> Self { Self { raw_id, @@ -61,7 +61,7 @@ impl ErrorTrackingStackFrame { serde_json::to_value(&self.contents)?, self.resolved, Uuid::now_v7(), - self.context + serde_json::to_string(&self.context)? ).execute(e).await?; Ok(()) } @@ -103,9 +103,16 @@ impl ErrorTrackingStackFrame { // We don't serialise frame contexts on the Frame itself, but save it on the frame record, // and so when we load a frame record we need to patch back up the context onto the frame, // since we dropped it when we serialised the frame during saving. - let mut frame: Frame = serde_json::from_value(found.contents)?; - frame.context = found.context.clone(); + let context = if let Some(context) = found.context.as_ref() { + // We serialise the frame context as a json string, but it's a structure we have to manually + // deserialise back into the frame. + Some(serde_json::from_str(context)?) + } else { + None + }; + + frame.context = context.clone(); Ok(Some(Self { raw_id: found.raw_id, @@ -114,7 +121,7 @@ impl ErrorTrackingStackFrame { symbol_set_id: found.symbol_set_id, contents: frame, resolved: found.resolved, - context: found.context, + context, })) } } diff --git a/rust/cymbal/src/langs/js.rs b/rust/cymbal/src/langs/js.rs index 77e5b269783..cca8ec15160 100644 --- a/rust/cymbal/src/langs/js.rs +++ b/rust/cymbal/src/langs/js.rs @@ -1,5 +1,3 @@ -use std::cmp::min; - use reqwest::Url; use serde::{Deserialize, Serialize}; use sha2::{Digest, Sha512}; @@ -7,7 +5,7 @@ use sourcemap::{SourceMap, Token}; use crate::{ error::{Error, FrameError, JsResolveErr, UnhandledError}, - frames::Frame, + frames::{Context, ContextLine, Frame}, metric_consts::{FRAME_NOT_RESOLVED, FRAME_RESOLVED}, symbol_store::SymbolCatalog, }; @@ -160,28 +158,37 @@ impl From<(&RawJSFrame, JsResolveErr)> for Frame { } } -fn get_context(token: &Token) -> Option { +fn get_context(token: &Token) -> Option { let sv = token.get_source_view()?; - let token_line = token.get_src_line(); - let start_line = token_line.saturating_sub(5); - let end_line = min(token_line.saturating_add(5) as usize, sv.line_count()) as u32; + let token_line_num = token.get_src_line(); - // Rough guess on capacity here - let mut context = String::with_capacity(((end_line - start_line) * 100) as usize); + let token_line = sv.get_line(token_line_num)?; - for line in start_line..end_line { - if let Some(l) = sv.get_line(line) { - context.push_str(l); - context.push('\n'); + let mut before = Vec::new(); + let mut i = token_line_num; + while before.len() < 5 && i > 0 { + i -= 1; + if let Some(line) = sv.get_line(i) { + before.push(ContextLine::new(i, line)); + } + } + before.reverse(); + + let mut after = Vec::new(); + let mut i = token_line_num; + while after.len() < 5 && i < sv.line_count() as u32 { + i += 1; + if let Some(line) = sv.get_line(i) { + after.push(ContextLine::new(i, line)); } } - if !context.is_empty() { - Some(context) - } else { - None - } + Some(Context { + before, + line: ContextLine::new(token_line_num, token_line), + after, + }) } #[cfg(test)] diff --git a/rust/cymbal/src/lib.rs b/rust/cymbal/src/lib.rs index 6ab91b33c95..471df70c0fe 100644 --- a/rust/cymbal/src/lib.rs +++ b/rust/cymbal/src/lib.rs @@ -108,10 +108,13 @@ async fn process_exception( // process those groups in-order (but the individual frames in them can still be // thrown at the wall), with some cross-group concurrency. handles.push(tokio::spawn(async move { - context + context.worker_liveness.report_healthy().await; + let res = context .resolver .resolve(&frame, team_id, &context.pool, &context.catalog) - .await + .await; + context.worker_liveness.report_healthy().await; + res })); }