From 894c5ff1a64cc2de78f2fd7374b8547c963305fb Mon Sep 17 00:00:00 2001 From: CongkaiTan Date: Fri, 5 Jun 2026 13:19:34 -0700 Subject: [PATCH] Include run name after copied to report to handle multi runs --- Cargo.lock | 2 +- Cargo.toml | 2 +- src/data/common/utils.rs | 11 ++- src/data/java_profile.rs | 1 + src/data/perf_profile.rs | 1 + src/server/mcp/tools.rs | 140 ++++++++++++++++++++++++++++----------- 6 files changed, 115 insertions(+), 42 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 18a42cf7..1cfa135b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -97,7 +97,7 @@ checksum = "7f202df86484c868dbad7eaa557ef785d5c66295e41b460ef922eca0723b842c" [[package]] name = "aperf" -version = "1.2.2" +version = "1.2.3" dependencies = [ "anyhow", "aws-config", diff --git a/Cargo.toml b/Cargo.toml index f0e499e9..38a8a36d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "aperf" -version = "1.2.2" +version = "1.2.3" edition = "2021" publish = false diff --git a/src/data/common/utils.rs b/src/data/common/utils.rs index 52e2ecbe..3a72bb0a 100644 --- a/src/data/common/utils.rs +++ b/src/data/common/utils.rs @@ -84,10 +84,13 @@ pub fn no_tar_gz_file_name(path: &PathBuf) -> Option { } /// Copy a graph file to the report data dir and update the GraphData with its info. +/// The destination file is prefixed with the (deduplicated, hence unique) `run_name` so that +/// graphs from different runs do not collide in the flat `data/js/` report directory. pub fn copy_graph_and_update_graph_data( source_dir: &PathBuf, dest_dir: &PathBuf, filename: &str, + run_name: &str, graph_group_name: &str, graph_key: &str, graph_name: String, @@ -97,7 +100,13 @@ pub fn copy_graph_and_update_graph_data( if !source_graph_path.exists() { return; } - let relative_graph_path = PathBuf::from("data").join("js").join(&filename); + let run_prefix = format!("{run_name}-"); + let dest_filename = if filename.starts_with(&run_prefix) { + filename.to_string() + } else { + format!("{run_prefix}{filename}") + }; + let relative_graph_path = PathBuf::from("data").join("js").join(&dest_filename); let dest_graph_path = dest_dir.join(&relative_graph_path); if let Err(e) = std::fs::copy(&source_graph_path, &dest_graph_path) { diff --git a/src/data/java_profile.rs b/src/data/java_profile.rs index 64c5cacf..89916dd7 100644 --- a/src/data/java_profile.rs +++ b/src/data/java_profile.rs @@ -458,6 +458,7 @@ impl ProcessData for JavaProfile { ¶ms.data_dir, ¶ms.report_dir, &filename, + ¶ms.run_name, metric, &deduped_name, format!("({}) JVM: {}", metric, deduped_name), diff --git a/src/data/perf_profile.rs b/src/data/perf_profile.rs index 840391d1..8998a6d5 100644 --- a/src/data/perf_profile.rs +++ b/src/data/perf_profile.rs @@ -312,6 +312,7 @@ impl ProcessData for PerfProfile { ¶ms.data_dir, ¶ms.report_dir, &filename, + ¶ms.run_name, if is_reverse { "reverse" } else { "default" }, "cpu", "Perf CPU Profile".to_string(), diff --git a/src/server/mcp/tools.rs b/src/server/mcp/tools.rs index 870b5d5a..45b291ce 100644 --- a/src/server/mcp/tools.rs +++ b/src/server/mcp/tools.rs @@ -954,7 +954,10 @@ impl AperfMcpServer { if is_diff { let min_delta = req.min_pct.unwrap_or(0.5); - // Resolve the two run IDs for diff + // Resolve the two run IDs for diff. When omitted, default to the first two runs that + // have flamegraph SVGs (passing None to find_flamegraph_svg would resolve both sides + // to the same first-alphabetical SVG). + let default_ids = find_all_flamegraph_run_ids(loaded, svg_type); let (rid1, rid2) = match &req.run_id { Some(ids) if ids.len() == 2 => (Some(ids[0].as_str()), Some(ids[1].as_str())), Some(ids) if ids.len() > 2 => { @@ -967,7 +970,14 @@ impl AperfMcpServer { json!({"status": "error", "message": "Error: diff/reverse-diff mode requires exactly 2 run IDs in run_id array. Got 1."}), ); } - _ => (None, None), // Use defaults (first and second run) + _ => { + if default_ids.len() < 2 { + return ok_json( + json!({"status": "error", "message": format!("Error: diff/reverse-diff mode needs at least 2 runs with flamegraphs; found {}.", default_ids.len())}), + ); + } + (Some(default_ids[0].as_str()), Some(default_ids[1].as_str())) + } }; // Find both SVGs @@ -1471,6 +1481,23 @@ struct AnnotatedFrame { parent: String, } +/// Whether an SVG `filename` is the flamegraph of `run_id` for the given `fg_type` +/// ("reverse" => `-reverse-flamegraph.svg`, else forward `-flamegraph.svg`). +/// Anchored on the `-` prefix so run ids that are substrings of one another don't +/// cross-match. +fn svg_matches_run_and_type(filename: &str, run_id: &str, fg_type: &str) -> bool { + let prefix = format!("{run_id}-"); + if !filename.starts_with(&prefix) { + return false; + } + let is_reverse = filename.ends_with("reverse-flamegraph.svg"); + if fg_type == "reverse" { + is_reverse + } else { + filename.ends_with("flamegraph.svg") && !is_reverse + } +} + /// Find the flamegraph SVG file for a given run and type. fn find_flamegraph_svg( loaded: &LoadedReport, @@ -1481,19 +1508,14 @@ fn find_flamegraph_svg( let search_dir = &loaded.data_dir; let not_reverse = "reverse-flamegraph.svg"; - // If run_id specified, look for files containing run_id and matching the type + // If run_id specified, look for the run's SVG of the requested type. The report names graphs + // `-flamegraph.svg` / `-reverse-flamegraph.svg`, so anchor on the run-id + // prefix rather than a loose substring match (which would mis-match e.g. `run` vs `run_1`). if let Some(rid) = run_id { if let Ok(entries) = std::fs::read_dir(search_dir) { for entry in entries.flatten() { let name = entry.file_name().to_string_lossy().to_string(); - let matches = if fg_type == "reverse" { - name.contains(rid) && name.ends_with(not_reverse) - } else { - name.contains(rid) - && name.ends_with("flamegraph.svg") - && !name.ends_with(not_reverse) - }; - if matches { + if svg_matches_run_and_type(&name, rid, fg_type) { return Ok(entry.path()); } } @@ -1537,54 +1559,50 @@ fn find_flamegraph_svg( )) } -/// Find all run IDs that have flamegraph SVGs available. -/// Extracts run IDs from SVG filenames by matching against known run IDs from metadata. +/// Find all run IDs that have a flamegraph SVG of the given type, in a deterministic order. +/// The report names graphs `-flamegraph.svg` / `-reverse-flamegraph.svg`. fn find_all_flamegraph_run_ids(loaded: &LoadedReport, fg_type: &str) -> Vec { let search_dir = &loaded.data_dir; - let not_reverse = "reverse-flamegraph.svg"; - // Get known run IDs from metadata - let known_run_ids: Vec = loaded.metadata.run_ids.clone().unwrap_or_default(); - - // Find all matching SVG files - let svgs: Vec = match std::fs::read_dir(search_dir) { + // SVG filenames of the requested type present in the report. + let mut svgs: Vec = match std::fs::read_dir(search_dir) { Ok(entries) => entries .filter_map(|e| e.ok()) .map(|e| e.file_name().to_string_lossy().to_string()) .filter(|name| { + let is_reverse = name.ends_with("reverse-flamegraph.svg"); if fg_type == "reverse" { - name.ends_with(not_reverse) + is_reverse } else { - name.ends_with("flamegraph.svg") && !name.ends_with(not_reverse) + name.ends_with("flamegraph.svg") && !is_reverse } }) .collect(), Err(_) => return Vec::new(), }; + svgs.sort(); - // Match SVG filenames to known run IDs + // Prefer known run IDs from metadata (preserves their order), matched precisely against the + // `-` prefix. Fall back to deriving the id from the filename for any SVG not matched + // (e.g. a report whose metadata is incomplete). + let known_run_ids: Vec = loaded.metadata.run_ids.clone().unwrap_or_default(); let mut matched_ids: Vec = Vec::new(); for rid in &known_run_ids { - if svgs.iter().any(|svg| svg.contains(rid.as_str())) { + if svgs + .iter() + .any(|svg| svg_matches_run_and_type(svg, rid, fg_type)) + { matched_ids.push(rid.clone()); } } - - // If no known run IDs matched (e.g., single-run report), fall back to returning - // a synthetic ID based on the SVG filename - if matched_ids.is_empty() && !svgs.is_empty() { - // Use the SVG filenames themselves as identifiers — extract the part before "-flamegraph.svg" - let mut sorted_svgs = svgs; - sorted_svgs.sort(); - for svg_name in &sorted_svgs { - let stem = if fg_type == "reverse" { - svg_name.trim_end_matches("-reverse-flamegraph.svg") - } else { - svg_name.trim_end_matches("-flamegraph.svg") - }; - if !stem.is_empty() && stem != svg_name { - matched_ids.push(stem.to_string()); - } + for svg_name in &svgs { + let stem = if fg_type == "reverse" { + svg_name.trim_end_matches("-reverse-flamegraph.svg") + } else { + svg_name.trim_end_matches("-flamegraph.svg") + }; + if !stem.is_empty() && stem != svg_name.as_str() && !matched_ids.iter().any(|m| m == stem) { + matched_ids.push(stem.to_string()); } } @@ -2742,4 +2760,48 @@ mod tests { assert!(!top.parent.is_empty()); assert!(top.pct > 10.0); } + + #[test] + fn test_svg_matches_run_and_type() { + // Reports name graphs `-flamegraph.svg` / `-reverse-flamegraph.svg`. + assert!(svg_matches_run_and_type( + "run1-flamegraph.svg", + "run1", + "normal" + )); + assert!(svg_matches_run_and_type( + "run1-reverse-flamegraph.svg", + "run1", + "reverse" + )); + // Forward must not match the reverse file, and vice-versa. + assert!(!svg_matches_run_and_type( + "run1-reverse-flamegraph.svg", + "run1", + "normal" + )); + assert!(!svg_matches_run_and_type( + "run1-flamegraph.svg", + "run1", + "reverse" + )); + // Anchored on the `-` prefix: a run id that is a substring/prefix of another + // must not cross-match. + assert!(!svg_matches_run_and_type( + "run1-flamegraph.svg", + "run", + "normal" + )); + assert!(!svg_matches_run_and_type( + "run11-flamegraph.svg", + "run1", + "normal" + )); + // Wrong run id entirely. + assert!(!svg_matches_run_and_type( + "run2-flamegraph.svg", + "run1", + "normal" + )); + } }