Skip to content

Commit

Permalink
fix(err): discard old frame results (#26495)
Browse files Browse the repository at this point in the history
  • Loading branch information
oliverb123 authored Nov 27, 2024
1 parent e5aa629 commit 5bd5d7a
Show file tree
Hide file tree
Showing 3 changed files with 25 additions and 7 deletions.
8 changes: 8 additions & 0 deletions rust/cymbal/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,14 @@ pub struct Config {
#[envconfig(default = "600")]
pub frame_cache_ttl_seconds: u64,

// When we resolve a frame, we put it in PG, so other instances of cymbal can
// use it, or so we can re-use it after a restart. This is the TTL for that,
// after this many minutes we'll discard saved resolution results and re-resolve
// TODO - 10 minutes is too short for production use, it's only twice as long as
// our in-memory caching. We should do at least an hour once we release
#[envconfig(default = "10")]
pub frame_result_ttl_minutes: u32,

// Maximum number of lines of pre and post context to get per frame
#[envconfig(default = "15")]
pub context_line_count: usize,
Expand Down
8 changes: 7 additions & 1 deletion rust/cymbal/src/frames/records.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use chrono::{DateTime, Utc};
use chrono::{DateTime, Duration, Utc};
use serde::{Deserialize, Serialize};
use serde_json::Value;
use sqlx::Executor;
Expand Down Expand Up @@ -75,6 +75,7 @@ impl ErrorTrackingStackFrame {
e: E,
team_id: i32,
raw_id: &str,
result_ttl: Duration,
) -> Result<Option<Self>, UnhandledError>
where
E: Executor<'c, Database = sqlx::Postgres>,
Expand Down Expand Up @@ -105,6 +106,11 @@ impl ErrorTrackingStackFrame {
return Ok(None);
};

// If frame results are older than an hour or so, we discard them (and overwrite them)
if found.created_at < Utc::now() - result_ttl {
return Ok(None);
}

// 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.
Expand Down
16 changes: 10 additions & 6 deletions rust/cymbal/src/frames/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ use super::{records::ErrorTrackingStackFrame, Frame, RawFrame};

pub struct Resolver {
cache: Cache<String, ErrorTrackingStackFrame>,
result_ttl: chrono::Duration,
}

impl Resolver {
Expand All @@ -21,7 +22,9 @@ impl Resolver {
.time_to_live(Duration::from_secs(config.frame_cache_ttl_seconds))
.build();

Self { cache }
let result_ttl = chrono::Duration::minutes(config.frame_result_ttl_minutes as i64);

Self { cache, result_ttl }
}

pub async fn resolve(
Expand All @@ -36,7 +39,7 @@ impl Resolver {
}

if let Some(result) =
ErrorTrackingStackFrame::load(pool, team_id, &frame.frame_id()).await?
ErrorTrackingStackFrame::load(pool, team_id, &frame.frame_id(), self.result_ttl).await?
{
self.cache.insert(frame.frame_id(), result.clone());
return Ok(result.contents);
Expand Down Expand Up @@ -237,10 +240,11 @@ mod test {

// get the frame
let frame_id = frame.frame_id();
let frame = ErrorTrackingStackFrame::load(&pool, 0, &frame_id)
.await
.unwrap()
.unwrap();
let frame =
ErrorTrackingStackFrame::load(&pool, 0, &frame_id, chrono::Duration::minutes(30))
.await
.unwrap()
.unwrap();

assert_eq!(frame.symbol_set_id.unwrap(), set.id);

Expand Down

0 comments on commit 5bd5d7a

Please sign in to comment.