From 899eed15adfbcda55e0eb300c037fe8d444f188d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Fri, 14 Mar 2025 22:18:04 +0100 Subject: [PATCH 01/13] Refactor metrics generation step --- .github/workflows/ci.yml | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 96c0955e871b8..47ee02ab00ed5 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -241,13 +241,17 @@ jobs: - name: postprocess metrics into the summary run: | if [ -f build/metrics.json ]; then - ./build/citool/debug/citool postprocess-metrics build/metrics.json ${GITHUB_STEP_SUMMARY} + METRICS=build/metrics.json elif [ -f obj/build/metrics.json ]; then - ./build/citool/debug/citool postprocess-metrics obj/build/metrics.json ${GITHUB_STEP_SUMMARY} + METRICS=obj/build/metrics.json else echo "No metrics.json found" + exit 0 fi + ./build/citool/debug/citool postprocess-metrics \ + ${METRICS} ${GITHUB_STEP_SUMMARY} + - name: upload job metrics to DataDog if: needs.calculate_matrix.outputs.run_type != 'pr' env: From 301c384262522d0c4b5577ffbd10642ee9bee24f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Fri, 14 Mar 2025 22:18:41 +0100 Subject: [PATCH 02/13] Do not fail the build if metrics postprocessing or DataDog upload fails --- .github/workflows/ci.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 47ee02ab00ed5..01008d70b65b6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -239,6 +239,9 @@ jobs: if: github.event_name == 'push' || env.DEPLOY == '1' || env.DEPLOY_ALT == '1' - name: postprocess metrics into the summary + # This step is not critical, and if some I/O problem happens, we don't want + # to cancel the build. + continue-on-error: true run: | if [ -f build/metrics.json ]; then METRICS=build/metrics.json @@ -253,6 +256,9 @@ jobs: ${METRICS} ${GITHUB_STEP_SUMMARY} - name: upload job metrics to DataDog + # This step is not critical, and if some I/O problem happens, we don't want + # to cancel the build. + continue-on-error: true if: needs.calculate_matrix.outputs.run_type != 'pr' env: DATADOG_API_KEY: ${{ secrets.DATADOG_API_KEY }} From 09d44a48b29fcf4c618ba38e592a1c4f365fc4e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Fri, 14 Mar 2025 22:21:41 +0100 Subject: [PATCH 03/13] Print metrics postprocessing to stdout This allows the code to be simplified a little bit. --- .github/workflows/ci.yml | 2 +- src/ci/citool/src/main.rs | 7 ++----- src/ci/citool/src/metrics.rs | 33 ++++++++++----------------------- 3 files changed, 13 insertions(+), 29 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 01008d70b65b6..ffcdc40de3a70 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -253,7 +253,7 @@ jobs: fi ./build/citool/debug/citool postprocess-metrics \ - ${METRICS} ${GITHUB_STEP_SUMMARY} + ${METRICS} >> ${GITHUB_STEP_SUMMARY} - name: upload job metrics to DataDog # This step is not critical, and if some I/O problem happens, we don't want diff --git a/src/ci/citool/src/main.rs b/src/ci/citool/src/main.rs index cd690ebeb0625..53fe75900750d 100644 --- a/src/ci/citool/src/main.rs +++ b/src/ci/citool/src/main.rs @@ -158,9 +158,6 @@ enum Args { PostprocessMetrics { /// Path to the metrics.json file metrics_path: PathBuf, - /// Path to a file where the postprocessed metrics summary will be stored. - /// Usually, this will be GITHUB_STEP_SUMMARY on CI. - summary_path: PathBuf, }, /// Upload CI metrics to Datadog. UploadBuildMetrics { @@ -211,8 +208,8 @@ fn main() -> anyhow::Result<()> { Args::UploadBuildMetrics { cpu_usage_csv } => { upload_ci_metrics(&cpu_usage_csv)?; } - Args::PostprocessMetrics { metrics_path, summary_path } => { - postprocess_metrics(&metrics_path, &summary_path)?; + Args::PostprocessMetrics { metrics_path } => { + postprocess_metrics(&metrics_path)?; } Args::PostMergeReport { current: commit, parent } => { post_merge_report(load_db(default_jobs_file)?, parent, commit)?; diff --git a/src/ci/citool/src/metrics.rs b/src/ci/citool/src/metrics.rs index 83b3d5ceed05a..8da4d4e53e64e 100644 --- a/src/ci/citool/src/metrics.rs +++ b/src/ci/citool/src/metrics.rs @@ -1,6 +1,4 @@ use std::collections::BTreeMap; -use std::fs::File; -use std::io::Write; use std::path::Path; use anyhow::Context; @@ -8,57 +6,46 @@ use build_helper::metrics::{ BuildStep, JsonNode, JsonRoot, TestOutcome, TestSuite, TestSuiteMetadata, format_build_steps, }; -pub fn postprocess_metrics(metrics_path: &Path, summary_path: &Path) -> anyhow::Result<()> { +pub fn postprocess_metrics(metrics_path: &Path) -> anyhow::Result<()> { let metrics = load_metrics(metrics_path)?; - let mut file = File::options() - .append(true) - .create(true) - .open(summary_path) - .with_context(|| format!("Cannot open summary file at {summary_path:?}"))?; - if !metrics.invocations.is_empty() { - writeln!(file, "# Bootstrap steps")?; - record_bootstrap_step_durations(&metrics, &mut file)?; - record_test_suites(&metrics, &mut file)?; + println!("# Bootstrap steps"); + record_bootstrap_step_durations(&metrics); + record_test_suites(&metrics); } Ok(()) } -fn record_bootstrap_step_durations(metrics: &JsonRoot, file: &mut File) -> anyhow::Result<()> { +fn record_bootstrap_step_durations(metrics: &JsonRoot) { for invocation in &metrics.invocations { let step = BuildStep::from_invocation(invocation); let table = format_build_steps(&step); eprintln!("Step `{}`\n{table}\n", invocation.cmdline); - writeln!( - file, + println!( r"
{} 
{table}
", invocation.cmdline - )?; + ); } eprintln!("Recorded {} bootstrap invocation(s)", metrics.invocations.len()); - - Ok(()) } -fn record_test_suites(metrics: &JsonRoot, file: &mut File) -> anyhow::Result<()> { +fn record_test_suites(metrics: &JsonRoot) { let suites = get_test_suites(&metrics); if !suites.is_empty() { let aggregated = aggregate_test_suites(&suites); let table = render_table(aggregated); - writeln!(file, "\n# Test results\n")?; - writeln!(file, "{table}")?; + println!("\n# Test results\n"); + println!("{table}"); } else { eprintln!("No test suites found in metrics"); } - - Ok(()) } fn render_table(suites: BTreeMap) -> String { From e757deab233aea31612c593773f242b6b9c6e386 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 15 Mar 2025 11:16:09 +0100 Subject: [PATCH 04/13] Refactor metrics and analysis in citool to distinguish them better --- .../src/{merge_report.rs => analysis.rs} | 204 ++++++++++-------- src/ci/citool/src/main.rs | 17 +- src/ci/citool/src/metrics.rs | 190 ++++++---------- src/ci/citool/src/utils.rs | 4 + 4 files changed, 196 insertions(+), 219 deletions(-) rename src/ci/citool/src/{merge_report.rs => analysis.rs} (62%) diff --git a/src/ci/citool/src/merge_report.rs b/src/ci/citool/src/analysis.rs similarity index 62% rename from src/ci/citool/src/merge_report.rs rename to src/ci/citool/src/analysis.rs index 62daa2e68530a..87c5708fa602b 100644 --- a/src/ci/citool/src/merge_report.rs +++ b/src/ci/citool/src/analysis.rs @@ -1,97 +1,135 @@ -use std::collections::{HashMap, HashSet}; -use std::path::PathBuf; +use crate::metrics; +use crate::metrics::{JobMetrics, JobName, get_test_suites}; +use crate::utils::pluralize; +use build_helper::metrics::{ + BuildStep, JsonRoot, TestOutcome, TestSuite, TestSuiteMetadata, format_build_steps, +}; +use std::collections::{BTreeMap, HashMap, HashSet}; + +pub fn output_bootstrap_stats(metrics: &JsonRoot) { + if !metrics.invocations.is_empty() { + println!("# Bootstrap steps"); + record_bootstrap_step_durations(&metrics); + record_test_suites(&metrics); + } +} -use anyhow::Context; -use build_helper::metrics::{JsonRoot, TestOutcome, TestSuiteMetadata}; +fn record_bootstrap_step_durations(metrics: &JsonRoot) { + for invocation in &metrics.invocations { + let step = BuildStep::from_invocation(invocation); + let table = format_build_steps(&step); + eprintln!("Step `{}`\n{table}\n", invocation.cmdline); + println!( + r"
+{} +
{table}
+
+", + invocation.cmdline + ); + } + eprintln!("Recorded {} bootstrap invocation(s)", metrics.invocations.len()); +} -use crate::jobs::JobDatabase; -use crate::metrics::get_test_suites; +fn record_test_suites(metrics: &JsonRoot) { + let suites = metrics::get_test_suites(&metrics); -type Sha = String; -type JobName = String; + if !suites.is_empty() { + let aggregated = aggregate_test_suites(&suites); + let table = render_table(aggregated); + println!("\n# Test results\n"); + println!("{table}"); + } else { + eprintln!("No test suites found in metrics"); + } +} -/// Computes a post merge CI analysis report between the `parent` and `current` commits. -pub fn post_merge_report(job_db: JobDatabase, parent: Sha, current: Sha) -> anyhow::Result<()> { - let jobs = download_all_metrics(&job_db, &parent, ¤t)?; - let aggregated_test_diffs = aggregate_test_diffs(&jobs)?; +fn render_table(suites: BTreeMap) -> String { + use std::fmt::Write; - println!("Comparing {parent} (base) -> {current} (this PR)\n"); - report_test_diffs(aggregated_test_diffs); + let mut table = "| Test suite | Passed ✅ | Ignored 🚫 | Failed ❌ |\n".to_string(); + writeln!(table, "|:------|------:|------:|------:|").unwrap(); - Ok(()) -} + fn compute_pct(value: f64, total: f64) -> f64 { + if total == 0.0 { 0.0 } else { value / total } + } -struct JobMetrics { - parent: Option, - current: JsonRoot, -} + fn write_row( + buffer: &mut String, + name: &str, + record: &TestSuiteRecord, + surround: &str, + ) -> std::fmt::Result { + let TestSuiteRecord { passed, ignored, failed } = record; + let total = (record.passed + record.ignored + record.failed) as f64; + let passed_pct = compute_pct(*passed as f64, total) * 100.0; + let ignored_pct = compute_pct(*ignored as f64, total) * 100.0; + let failed_pct = compute_pct(*failed as f64, total) * 100.0; + + write!(buffer, "| {surround}{name}{surround} |")?; + write!(buffer, " {surround}{passed} ({passed_pct:.0}%){surround} |")?; + write!(buffer, " {surround}{ignored} ({ignored_pct:.0}%){surround} |")?; + writeln!(buffer, " {surround}{failed} ({failed_pct:.0}%){surround} |")?; + + Ok(()) + } -/// Download before/after metrics for all auto jobs in the job database. -fn download_all_metrics( - job_db: &JobDatabase, - parent: &str, - current: &str, -) -> anyhow::Result> { - let mut jobs = HashMap::default(); - - for job in &job_db.auto_jobs { - eprintln!("Downloading metrics of job {}", job.name); - let metrics_parent = match download_job_metrics(&job.name, parent) { - Ok(metrics) => Some(metrics), - Err(error) => { - eprintln!( - r#"Did not find metrics for job `{}` at `{}`: {error:?}. -Maybe it was newly added?"#, - job.name, parent - ); - None - } - }; - let metrics_current = download_job_metrics(&job.name, current)?; - jobs.insert( - job.name.clone(), - JobMetrics { parent: metrics_parent, current: metrics_current }, - ); + let mut total = TestSuiteRecord::default(); + for (name, record) in suites { + write_row(&mut table, &name, &record, "").unwrap(); + total.passed += record.passed; + total.ignored += record.ignored; + total.failed += record.failed; } - Ok(jobs) + write_row(&mut table, "Total", &total, "**").unwrap(); + table } -/// Downloads job metrics of the given job for the given commit. -/// Caches the result on the local disk. -fn download_job_metrics(job_name: &str, sha: &str) -> anyhow::Result { - let cache_path = PathBuf::from(".citool-cache").join(sha).join(job_name).join("metrics.json"); - if let Some(cache_entry) = - std::fs::read_to_string(&cache_path).ok().and_then(|data| serde_json::from_str(&data).ok()) - { - return Ok(cache_entry); - } +/// Computes a post merge CI analysis report of test differences +/// between the `parent` and `current` commits. +pub fn output_test_diffs(job_metrics: HashMap) { + let aggregated_test_diffs = aggregate_test_diffs(&job_metrics); + report_test_diffs(aggregated_test_diffs); +} - let url = get_metrics_url(job_name, sha); - let mut response = ureq::get(&url).call()?; - if !response.status().is_success() { - return Err(anyhow::anyhow!( - "Cannot fetch metrics from {url}: {}\n{}", - response.status(), - response.body_mut().read_to_string()? - )); - } - let data: JsonRoot = response - .body_mut() - .read_json() - .with_context(|| anyhow::anyhow!("cannot deserialize metrics from {url}"))?; - - // Ignore errors if cache cannot be created - if std::fs::create_dir_all(cache_path.parent().unwrap()).is_ok() { - if let Ok(serialized) = serde_json::to_string(&data) { - let _ = std::fs::write(&cache_path, &serialized); +#[derive(Default)] +struct TestSuiteRecord { + passed: u64, + ignored: u64, + failed: u64, +} + +fn test_metadata_name(metadata: &TestSuiteMetadata) -> String { + match metadata { + TestSuiteMetadata::CargoPackage { crates, stage, .. } => { + format!("{} (stage {stage})", crates.join(", ")) + } + TestSuiteMetadata::Compiletest { suite, stage, .. } => { + format!("{suite} (stage {stage})") } } - Ok(data) } -fn get_metrics_url(job_name: &str, sha: &str) -> String { - let suffix = if job_name.ends_with("-alt") { "-alt" } else { "" }; - format!("https://ci-artifacts.rust-lang.org/rustc-builds{suffix}/{sha}/metrics-{job_name}.json") +fn aggregate_test_suites(suites: &[&TestSuite]) -> BTreeMap { + let mut records: BTreeMap = BTreeMap::new(); + for suite in suites { + let name = test_metadata_name(&suite.metadata); + let record = records.entry(name).or_default(); + for test in &suite.tests { + match test.outcome { + TestOutcome::Passed => { + record.passed += 1; + } + TestOutcome::Failed => { + record.failed += 1; + } + TestOutcome::Ignored { .. } => { + record.ignored += 1; + } + } + } + } + records } /// Represents a difference in the outcome of tests between a base and a current commit. @@ -101,9 +139,7 @@ struct AggregatedTestDiffs { diffs: HashMap>, } -fn aggregate_test_diffs( - jobs: &HashMap, -) -> anyhow::Result { +fn aggregate_test_diffs(jobs: &HashMap) -> AggregatedTestDiffs { let mut diffs: HashMap> = HashMap::new(); // Aggregate test suites @@ -117,7 +153,7 @@ fn aggregate_test_diffs( } } - Ok(AggregatedTestDiffs { diffs }) + AggregatedTestDiffs { diffs } } #[derive(Eq, PartialEq, Hash, Debug)] @@ -312,7 +348,3 @@ fn report_test_diffs(diff: AggregatedTestDiffs) { ); } } - -fn pluralize(text: &str, count: usize) -> String { - if count == 1 { text.to_string() } else { format!("{text}s") } -} diff --git a/src/ci/citool/src/main.rs b/src/ci/citool/src/main.rs index 53fe75900750d..5f1932854b5a0 100644 --- a/src/ci/citool/src/main.rs +++ b/src/ci/citool/src/main.rs @@ -1,7 +1,7 @@ +mod analysis; mod cpu_usage; mod datadog; mod jobs; -mod merge_report; mod metrics; mod utils; @@ -14,12 +14,13 @@ use clap::Parser; use jobs::JobDatabase; use serde_yaml::Value; +use crate::analysis::output_test_diffs; use crate::cpu_usage::load_cpu_usage; use crate::datadog::upload_datadog_metric; use crate::jobs::RunType; -use crate::merge_report::post_merge_report; -use crate::metrics::postprocess_metrics; +use crate::metrics::{download_auto_job_metrics, load_metrics}; use crate::utils::load_env_var; +use analysis::output_bootstrap_stats; const CI_DIRECTORY: &str = concat!(env!("CARGO_MANIFEST_DIR"), "/.."); const DOCKER_DIRECTORY: &str = concat!(env!("CARGO_MANIFEST_DIR"), "/../docker"); @@ -209,10 +210,14 @@ fn main() -> anyhow::Result<()> { upload_ci_metrics(&cpu_usage_csv)?; } Args::PostprocessMetrics { metrics_path } => { - postprocess_metrics(&metrics_path)?; + let metrics = load_metrics(&metrics_path)?; + output_bootstrap_stats(&metrics); } - Args::PostMergeReport { current: commit, parent } => { - post_merge_report(load_db(default_jobs_file)?, parent, commit)?; + Args::PostMergeReport { current, parent } => { + let db = load_db(default_jobs_file)?; + let metrics = download_auto_job_metrics(&db, &parent, ¤t)?; + println!("Comparing {parent} (base) -> {current} (this PR)\n"); + output_test_diffs(metrics); } } diff --git a/src/ci/citool/src/metrics.rs b/src/ci/citool/src/metrics.rs index 8da4d4e53e64e..117a4f372c4c2 100644 --- a/src/ci/citool/src/metrics.rs +++ b/src/ci/citool/src/metrics.rs @@ -1,133 +1,11 @@ -use std::collections::BTreeMap; use std::path::Path; +use crate::jobs::JobDatabase; use anyhow::Context; -use build_helper::metrics::{ - BuildStep, JsonNode, JsonRoot, TestOutcome, TestSuite, TestSuiteMetadata, format_build_steps, -}; +use build_helper::metrics::{JsonNode, JsonRoot, TestSuite}; +use std::collections::HashMap; -pub fn postprocess_metrics(metrics_path: &Path) -> anyhow::Result<()> { - let metrics = load_metrics(metrics_path)?; - - if !metrics.invocations.is_empty() { - println!("# Bootstrap steps"); - record_bootstrap_step_durations(&metrics); - record_test_suites(&metrics); - } - - Ok(()) -} - -fn record_bootstrap_step_durations(metrics: &JsonRoot) { - for invocation in &metrics.invocations { - let step = BuildStep::from_invocation(invocation); - let table = format_build_steps(&step); - eprintln!("Step `{}`\n{table}\n", invocation.cmdline); - println!( - r"
-{} -
{table}
-
-", - invocation.cmdline - ); - } - eprintln!("Recorded {} bootstrap invocation(s)", metrics.invocations.len()); -} - -fn record_test_suites(metrics: &JsonRoot) { - let suites = get_test_suites(&metrics); - - if !suites.is_empty() { - let aggregated = aggregate_test_suites(&suites); - let table = render_table(aggregated); - println!("\n# Test results\n"); - println!("{table}"); - } else { - eprintln!("No test suites found in metrics"); - } -} - -fn render_table(suites: BTreeMap) -> String { - use std::fmt::Write; - - let mut table = "| Test suite | Passed ✅ | Ignored 🚫 | Failed ❌ |\n".to_string(); - writeln!(table, "|:------|------:|------:|------:|").unwrap(); - - fn compute_pct(value: f64, total: f64) -> f64 { - if total == 0.0 { 0.0 } else { value / total } - } - - fn write_row( - buffer: &mut String, - name: &str, - record: &TestSuiteRecord, - surround: &str, - ) -> std::fmt::Result { - let TestSuiteRecord { passed, ignored, failed } = record; - let total = (record.passed + record.ignored + record.failed) as f64; - let passed_pct = compute_pct(*passed as f64, total) * 100.0; - let ignored_pct = compute_pct(*ignored as f64, total) * 100.0; - let failed_pct = compute_pct(*failed as f64, total) * 100.0; - - write!(buffer, "| {surround}{name}{surround} |")?; - write!(buffer, " {surround}{passed} ({passed_pct:.0}%){surround} |")?; - write!(buffer, " {surround}{ignored} ({ignored_pct:.0}%){surround} |")?; - writeln!(buffer, " {surround}{failed} ({failed_pct:.0}%){surround} |")?; - - Ok(()) - } - - let mut total = TestSuiteRecord::default(); - for (name, record) in suites { - write_row(&mut table, &name, &record, "").unwrap(); - total.passed += record.passed; - total.ignored += record.ignored; - total.failed += record.failed; - } - write_row(&mut table, "Total", &total, "**").unwrap(); - table -} - -#[derive(Default)] -struct TestSuiteRecord { - passed: u64, - ignored: u64, - failed: u64, -} - -fn test_metadata_name(metadata: &TestSuiteMetadata) -> String { - match metadata { - TestSuiteMetadata::CargoPackage { crates, stage, .. } => { - format!("{} (stage {stage})", crates.join(", ")) - } - TestSuiteMetadata::Compiletest { suite, stage, .. } => { - format!("{suite} (stage {stage})") - } - } -} - -fn aggregate_test_suites(suites: &[&TestSuite]) -> BTreeMap { - let mut records: BTreeMap = BTreeMap::new(); - for suite in suites { - let name = test_metadata_name(&suite.metadata); - let record = records.entry(name).or_default(); - for test in &suite.tests { - match test.outcome { - TestOutcome::Passed => { - record.passed += 1; - } - TestOutcome::Failed => { - record.failed += 1; - } - TestOutcome::Ignored { .. } => { - record.ignored += 1; - } - } - } - } - records -} +pub type JobName = String; pub fn get_test_suites(metrics: &JsonRoot) -> Vec<&TestSuite> { fn visit_test_suites<'a>(nodes: &'a [JsonNode], suites: &mut Vec<&'a TestSuite>) { @@ -150,10 +28,68 @@ pub fn get_test_suites(metrics: &JsonRoot) -> Vec<&TestSuite> { suites } -fn load_metrics(path: &Path) -> anyhow::Result { +pub fn load_metrics(path: &Path) -> anyhow::Result { let metrics = std::fs::read_to_string(path) .with_context(|| format!("Cannot read JSON metrics from {path:?}"))?; let metrics: JsonRoot = serde_json::from_str(&metrics) .with_context(|| format!("Cannot deserialize JSON metrics from {path:?}"))?; Ok(metrics) } + +pub struct JobMetrics { + pub parent: Option, + pub current: JsonRoot, +} + +/// Download before/after metrics for all auto jobs in the job database. +/// `parent` and `current` should be commit SHAs. +pub fn download_auto_job_metrics( + job_db: &JobDatabase, + parent: &str, + current: &str, +) -> anyhow::Result> { + let mut jobs = HashMap::default(); + + for job in &job_db.auto_jobs { + eprintln!("Downloading metrics of job {}", job.name); + let metrics_parent = match download_job_metrics(&job.name, parent) { + Ok(metrics) => Some(metrics), + Err(error) => { + eprintln!( + r#"Did not find metrics for job `{}` at `{}`: {error:?}. +Maybe it was newly added?"#, + job.name, parent + ); + None + } + }; + let metrics_current = download_job_metrics(&job.name, current)?; + jobs.insert( + job.name.clone(), + JobMetrics { parent: metrics_parent, current: metrics_current }, + ); + } + Ok(jobs) +} + +pub fn download_job_metrics(job_name: &str, sha: &str) -> anyhow::Result { + let url = get_metrics_url(job_name, sha); + let mut response = ureq::get(&url).call()?; + if !response.status().is_success() { + return Err(anyhow::anyhow!( + "Cannot fetch metrics from {url}: {}\n{}", + response.status(), + response.body_mut().read_to_string()? + )); + } + let data: JsonRoot = response + .body_mut() + .read_json() + .with_context(|| anyhow::anyhow!("cannot deserialize metrics from {url}"))?; + Ok(data) +} + +fn get_metrics_url(job_name: &str, sha: &str) -> String { + let suffix = if job_name.ends_with("-alt") { "-alt" } else { "" }; + format!("https://ci-artifacts.rust-lang.org/rustc-builds{suffix}/{sha}/metrics-{job_name}.json") +} diff --git a/src/ci/citool/src/utils.rs b/src/ci/citool/src/utils.rs index 9cc220987bdfd..a18af96bf3de6 100644 --- a/src/ci/citool/src/utils.rs +++ b/src/ci/citool/src/utils.rs @@ -9,3 +9,7 @@ pub fn load_env_var(name: &str) -> anyhow::Result { pub fn read_to_string>(path: P) -> anyhow::Result { std::fs::read_to_string(&path).with_context(|| format!("Cannot read file {:?}", path.as_ref())) } + +pub fn pluralize(text: &str, count: usize) -> String { + if count == 1 { text.to_string() } else { format!("{text}s") } +} From 413fd52ea98a1cd986afa3201a259304a3b58599 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 15 Mar 2025 09:32:57 +0100 Subject: [PATCH 05/13] Print number of found test diffs --- src/ci/citool/src/analysis.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/ci/citool/src/analysis.rs b/src/ci/citool/src/analysis.rs index 87c5708fa602b..8e4ab03f3b3a9 100644 --- a/src/ci/citool/src/analysis.rs +++ b/src/ci/citool/src/analysis.rs @@ -244,6 +244,7 @@ fn report_test_diffs(diff: AggregatedTestDiffs) { println!("No test diffs found"); return; } + println!("\n{} test {} found\n", diff.diffs.len(), pluralize("difference", diff.diffs.len())); fn format_outcome(outcome: &TestOutcome) -> String { match outcome { From 6c24c9c088a0eb858976781090a5b1fbb57981bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 15 Mar 2025 09:34:35 +0100 Subject: [PATCH 06/13] Use first-level heading for test differences header --- src/ci/citool/src/analysis.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ci/citool/src/analysis.rs b/src/ci/citool/src/analysis.rs index 8e4ab03f3b3a9..02199636fc798 100644 --- a/src/ci/citool/src/analysis.rs +++ b/src/ci/citool/src/analysis.rs @@ -239,7 +239,7 @@ fn normalize_test_name(name: &str) -> String { /// Prints test changes in Markdown format to stdout. fn report_test_diffs(diff: AggregatedTestDiffs) { - println!("## Test differences"); + println!("# Test differences"); if diff.diffs.is_empty() { println!("No test diffs found"); return; From 30d57576b9040a438adc2414540da3778addf34b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 15 Mar 2025 09:35:29 +0100 Subject: [PATCH 07/13] Print test diffs into GitHub summary So that we can also observe them for try builds, before merging a PR. --- .github/workflows/ci.yml | 5 +++++ src/ci/citool/src/main.rs | 41 +++++++++++++++++++++++++++++++++------ 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ffcdc40de3a70..aaae67c28bc70 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -252,7 +252,12 @@ jobs: exit 0 fi + # Get closest bors merge commit + PARENT_COMMIT=`git rev-list --author='bors ' -n1 --first-parent HEAD^1` + ./build/citool/debug/citool postprocess-metrics \ + --job-name ${CI_JOB_NAME} \ + --parent ${PARENT_COMMIT} \ ${METRICS} >> ${GITHUB_STEP_SUMMARY} - name: upload job metrics to DataDog diff --git a/src/ci/citool/src/main.rs b/src/ci/citool/src/main.rs index 5f1932854b5a0..fb0639367bd0e 100644 --- a/src/ci/citool/src/main.rs +++ b/src/ci/citool/src/main.rs @@ -5,7 +5,7 @@ mod jobs; mod metrics; mod utils; -use std::collections::BTreeMap; +use std::collections::{BTreeMap, HashMap}; use std::path::{Path, PathBuf}; use std::process::Command; @@ -18,7 +18,7 @@ use crate::analysis::output_test_diffs; use crate::cpu_usage::load_cpu_usage; use crate::datadog::upload_datadog_metric; use crate::jobs::RunType; -use crate::metrics::{download_auto_job_metrics, load_metrics}; +use crate::metrics::{JobMetrics, download_auto_job_metrics, download_job_metrics, load_metrics}; use crate::utils::load_env_var; use analysis::output_bootstrap_stats; @@ -138,6 +138,27 @@ fn upload_ci_metrics(cpu_usage_csv: &Path) -> anyhow::Result<()> { Ok(()) } +fn postprocess_metrics( + metrics_path: PathBuf, + parent: Option, + job_name: Option, +) -> anyhow::Result<()> { + let metrics = load_metrics(&metrics_path)?; + output_bootstrap_stats(&metrics); + + let (Some(parent), Some(job_name)) = (parent, job_name) else { + return Ok(()); + }; + + let parent_metrics = + download_job_metrics(&job_name, &parent).context("cannot download parent metrics")?; + let job_metrics = + HashMap::from([(job_name, JobMetrics { parent: Some(parent_metrics), current: metrics })]); + output_test_diffs(job_metrics); + + Ok(()) +} + #[derive(clap::Parser)] enum Args { /// Calculate a list of jobs that should be executed on CI. @@ -155,10 +176,19 @@ enum Args { #[clap(long = "type", default_value = "auto")] job_type: JobType, }, - /// Postprocess the metrics.json file generated by bootstrap. + /// Postprocess the metrics.json file generated by bootstrap and output + /// various statistics. + /// If `--parent` and `--job-name` are provided, also display a diff + /// against previous metrics that are downloaded from CI. PostprocessMetrics { /// Path to the metrics.json file metrics_path: PathBuf, + /// A parent SHA against which to compare. + #[clap(long, requires("job_name"))] + parent: Option, + /// The name of the current job. + #[clap(long, requires("parent"))] + job_name: Option, }, /// Upload CI metrics to Datadog. UploadBuildMetrics { @@ -209,9 +239,8 @@ fn main() -> anyhow::Result<()> { Args::UploadBuildMetrics { cpu_usage_csv } => { upload_ci_metrics(&cpu_usage_csv)?; } - Args::PostprocessMetrics { metrics_path } => { - let metrics = load_metrics(&metrics_path)?; - output_bootstrap_stats(&metrics); + Args::PostprocessMetrics { metrics_path, parent, job_name } => { + postprocess_metrics(metrics_path, parent, job_name)?; } Args::PostMergeReport { current, parent } => { let db = load_db(default_jobs_file)?; From 634a11ef4864eb3cdaefa34d320f4be4f679b542 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 15 Mar 2025 09:37:19 +0100 Subject: [PATCH 08/13] Add bootstrap stage to test names --- src/ci/citool/src/analysis.rs | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/ci/citool/src/analysis.rs b/src/ci/citool/src/analysis.rs index 02199636fc798..566b8e603fbb6 100644 --- a/src/ci/citool/src/analysis.rs +++ b/src/ci/citool/src/analysis.rs @@ -225,16 +225,23 @@ fn aggregate_tests(metrics: &JsonRoot) -> TestSuiteData { // Poor man's detection of doctests based on the "(line XYZ)" suffix let is_doctest = matches!(suite.metadata, TestSuiteMetadata::CargoPackage { .. }) && test.name.contains("(line"); - let test_entry = Test { name: normalize_test_name(&test.name), is_doctest }; + let test_entry = Test { name: generate_test_name(&test.name, &suite), is_doctest }; tests.insert(test_entry, test.outcome.clone()); } } TestSuiteData { tests } } -/// Normalizes Windows-style path delimiters to Unix-style paths. -fn normalize_test_name(name: &str) -> String { - name.replace('\\', "/") +/// Normalizes Windows-style path delimiters to Unix-style paths +/// and adds suite metadata to the test name. +fn generate_test_name(name: &str, suite: &TestSuite) -> String { + let name = name.replace('\\', "/"); + let stage = match suite.metadata { + TestSuiteMetadata::CargoPackage { stage, .. } => stage, + TestSuiteMetadata::Compiletest { stage, .. } => stage, + }; + + format!("{name} (stage {stage})") } /// Prints test changes in Markdown format to stdout. From 232be8614d8bc396f3c0917916c96ef0ee939ad3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 15 Mar 2025 09:41:00 +0100 Subject: [PATCH 09/13] Add a helper function for outputting details --- src/ci/citool/src/analysis.rs | 13 ++++--------- src/ci/citool/src/utils.rs | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/ci/citool/src/analysis.rs b/src/ci/citool/src/analysis.rs index 566b8e603fbb6..8a37544357700 100644 --- a/src/ci/citool/src/analysis.rs +++ b/src/ci/citool/src/analysis.rs @@ -1,6 +1,6 @@ use crate::metrics; use crate::metrics::{JobMetrics, JobName, get_test_suites}; -use crate::utils::pluralize; +use crate::utils::{output_details, pluralize}; use build_helper::metrics::{ BuildStep, JsonRoot, TestOutcome, TestSuite, TestSuiteMetadata, format_build_steps, }; @@ -19,14 +19,9 @@ fn record_bootstrap_step_durations(metrics: &JsonRoot) { let step = BuildStep::from_invocation(invocation); let table = format_build_steps(&step); eprintln!("Step `{}`\n{table}\n", invocation.cmdline); - println!( - r"
-{} -
{table}
-
-", - invocation.cmdline - ); + output_details(&invocation.cmdline, || { + println!("
{table}
"); + }); } eprintln!("Recorded {} bootstrap invocation(s)", metrics.invocations.len()); } diff --git a/src/ci/citool/src/utils.rs b/src/ci/citool/src/utils.rs index a18af96bf3de6..bbe24c3633bc0 100644 --- a/src/ci/citool/src/utils.rs +++ b/src/ci/citool/src/utils.rs @@ -13,3 +13,17 @@ pub fn read_to_string>(path: P) -> anyhow::Result { pub fn pluralize(text: &str, count: usize) -> String { if count == 1 { text.to_string() } else { format!("{text}s") } } + +/// Outputs a HTML
section with the provided summary. +/// Output printed by `func` will be contained within the section. +pub fn output_details(summary: &str, func: F) +where + F: FnOnce(), +{ + println!( + r"
+{summary}" + ); + func(); + println!("
\n"); +} From b4cccf01587ac672dee7a92df7e3e21728f5b7aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 15 Mar 2025 10:00:11 +0100 Subject: [PATCH 10/13] Put test differences into a `
` section and add better explanation of the post merge report --- src/ci/citool/src/analysis.rs | 63 +++++++++++++++++++---------------- src/ci/citool/src/main.rs | 23 ++++++++++--- src/ci/citool/src/utils.rs | 4 ++- 3 files changed, 56 insertions(+), 34 deletions(-) diff --git a/src/ci/citool/src/analysis.rs b/src/ci/citool/src/analysis.rs index 8a37544357700..6469bc251be66 100644 --- a/src/ci/citool/src/analysis.rs +++ b/src/ci/citool/src/analysis.rs @@ -246,7 +246,6 @@ fn report_test_diffs(diff: AggregatedTestDiffs) { println!("No test diffs found"); return; } - println!("\n{} test {} found\n", diff.diffs.len(), pluralize("difference", diff.diffs.len())); fn format_outcome(outcome: &TestOutcome) -> String { match outcome { @@ -320,34 +319,42 @@ fn report_test_diffs(diff: AggregatedTestDiffs) { // Sort diffs by job group and test name grouped_diffs.sort_by(|(d1, g1), (d2, g2)| g1.cmp(&g2).then(d1.test.name.cmp(&d2.test.name))); - for (diff, job_group) in grouped_diffs { - println!( - "- `{}`: {} ({})", - diff.test.name, - format_diff(&diff.diff), - format_job_group(job_group) - ); - } + output_details( + &format!("Show {} test {}\n", original_diff_count, pluralize("diff", original_diff_count)), + || { + for (diff, job_group) in grouped_diffs { + println!( + "- `{}`: {} ({})", + diff.test.name, + format_diff(&diff.diff), + format_job_group(job_group) + ); + } - let extra_diffs = diffs.len().saturating_sub(max_diff_count); - if extra_diffs > 0 { - println!("\n(and {extra_diffs} additional {})", pluralize("test diff", extra_diffs)); - } + let extra_diffs = diffs.len().saturating_sub(max_diff_count); + if extra_diffs > 0 { + println!( + "\n(and {extra_diffs} additional {})", + pluralize("test diff", extra_diffs) + ); + } - if doctest_count > 0 { - println!( - "\nAdditionally, {doctest_count} doctest {} were found. These are ignored, as they are noisy.", - pluralize("diff", doctest_count) - ); - } + if doctest_count > 0 { + println!( + "\nAdditionally, {doctest_count} doctest {} were found. These are ignored, as they are noisy.", + pluralize("diff", doctest_count) + ); + } - // Now print the job group index - println!("\n**Job group index**\n"); - for (group, jobs) in job_index.into_iter().enumerate() { - println!( - "- {}: {}", - format_job_group(group as u64), - jobs.iter().map(|j| format!("`{j}`")).collect::>().join(", ") - ); - } + // Now print the job group index + println!("\n**Job group index**\n"); + for (group, jobs) in job_index.into_iter().enumerate() { + println!( + "- {}: {}", + format_job_group(group as u64), + jobs.iter().map(|j| format!("`{j}`")).collect::>().join(", ") + ); + } + }, + ); } diff --git a/src/ci/citool/src/main.rs b/src/ci/citool/src/main.rs index fb0639367bd0e..01483a2633d0a 100644 --- a/src/ci/citool/src/main.rs +++ b/src/ci/citool/src/main.rs @@ -19,7 +19,7 @@ use crate::cpu_usage::load_cpu_usage; use crate::datadog::upload_datadog_metric; use crate::jobs::RunType; use crate::metrics::{JobMetrics, download_auto_job_metrics, download_job_metrics, load_metrics}; -use crate::utils::load_env_var; +use crate::utils::{load_env_var, output_details}; use analysis::output_bootstrap_stats; const CI_DIRECTORY: &str = concat!(env!("CARGO_MANIFEST_DIR"), "/.."); @@ -159,6 +159,22 @@ fn postprocess_metrics( Ok(()) } +fn post_merge_report(db: JobDatabase, current: String, parent: String) -> anyhow::Result<()> { + let metrics = download_auto_job_metrics(&db, &parent, ¤t)?; + + output_details("What is this?", || { + println!( + r#"This is an experimental post-merge analysis report that shows differences in +test outcomes between the merged PR and its parent PR."# + ); + }); + + println!("\nComparing {parent} (parent) -> {current} (this PR)\n"); + output_test_diffs(metrics); + + Ok(()) +} + #[derive(clap::Parser)] enum Args { /// Calculate a list of jobs that should be executed on CI. @@ -243,10 +259,7 @@ fn main() -> anyhow::Result<()> { postprocess_metrics(metrics_path, parent, job_name)?; } Args::PostMergeReport { current, parent } => { - let db = load_db(default_jobs_file)?; - let metrics = download_auto_job_metrics(&db, &parent, ¤t)?; - println!("Comparing {parent} (base) -> {current} (this PR)\n"); - output_test_diffs(metrics); + post_merge_report(load_db(default_jobs_file)?, current, parent)?; } } diff --git a/src/ci/citool/src/utils.rs b/src/ci/citool/src/utils.rs index bbe24c3633bc0..b9b1bf4d45502 100644 --- a/src/ci/citool/src/utils.rs +++ b/src/ci/citool/src/utils.rs @@ -22,7 +22,9 @@ where { println!( r"
-{summary}" +{summary} + +" ); func(); println!("
\n"); From e84531811160308fbdd0a8ec3e9b697bb17ccd02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 15 Mar 2025 11:11:56 +0100 Subject: [PATCH 11/13] Do not error out on missing parent metrics --- src/ci/citool/src/main.rs | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/src/ci/citool/src/main.rs b/src/ci/citool/src/main.rs index 01483a2633d0a..29f94e2be5dbd 100644 --- a/src/ci/citool/src/main.rs +++ b/src/ci/citool/src/main.rs @@ -150,11 +150,24 @@ fn postprocess_metrics( return Ok(()); }; - let parent_metrics = - download_job_metrics(&job_name, &parent).context("cannot download parent metrics")?; - let job_metrics = - HashMap::from([(job_name, JobMetrics { parent: Some(parent_metrics), current: metrics })]); - output_test_diffs(job_metrics); + // This command is executed also on PR builds, which might not have parent metrics + // available, because some PR jobs don't run on auto builds, and PR jobs do not upload metrics + // due to missing permissions. + // To avoid having to detect if this is a PR job, and to avoid having failed steps in PR jobs, + // we simply print an error if the parent metrics were not found, but otherwise exit + // successfully. + match download_job_metrics(&job_name, &parent).context("cannot download parent metrics") { + Ok(parent_metrics) => { + let job_metrics = HashMap::from([( + job_name, + JobMetrics { parent: Some(parent_metrics), current: metrics }, + )]); + output_test_diffs(job_metrics); + } + Err(error) => { + eprintln!("Metrics for job `{job_name}` and commit `{parent}` not found: {error:?}"); + } + } Ok(()) } From 4801dba9af6e6c1b3bb06959893f68ed031f6325 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Sat, 15 Mar 2025 11:34:55 +0100 Subject: [PATCH 12/13] Reformat code --- src/ci/citool/src/analysis.rs | 10 ++++++---- src/ci/citool/src/main.rs | 2 +- src/ci/citool/src/metrics.rs | 5 +++-- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/ci/citool/src/analysis.rs b/src/ci/citool/src/analysis.rs index 6469bc251be66..2088ce2962097 100644 --- a/src/ci/citool/src/analysis.rs +++ b/src/ci/citool/src/analysis.rs @@ -1,10 +1,12 @@ -use crate::metrics; -use crate::metrics::{JobMetrics, JobName, get_test_suites}; -use crate::utils::{output_details, pluralize}; +use std::collections::{BTreeMap, HashMap, HashSet}; + use build_helper::metrics::{ BuildStep, JsonRoot, TestOutcome, TestSuite, TestSuiteMetadata, format_build_steps, }; -use std::collections::{BTreeMap, HashMap, HashSet}; + +use crate::metrics; +use crate::metrics::{JobMetrics, JobName, get_test_suites}; +use crate::utils::{output_details, pluralize}; pub fn output_bootstrap_stats(metrics: &JsonRoot) { if !metrics.invocations.is_empty() { diff --git a/src/ci/citool/src/main.rs b/src/ci/citool/src/main.rs index 29f94e2be5dbd..9e4b558d77aa0 100644 --- a/src/ci/citool/src/main.rs +++ b/src/ci/citool/src/main.rs @@ -9,6 +9,7 @@ use std::collections::{BTreeMap, HashMap}; use std::path::{Path, PathBuf}; use std::process::Command; +use analysis::output_bootstrap_stats; use anyhow::Context; use clap::Parser; use jobs::JobDatabase; @@ -20,7 +21,6 @@ use crate::datadog::upload_datadog_metric; use crate::jobs::RunType; use crate::metrics::{JobMetrics, download_auto_job_metrics, download_job_metrics, load_metrics}; use crate::utils::{load_env_var, output_details}; -use analysis::output_bootstrap_stats; const CI_DIRECTORY: &str = concat!(env!("CARGO_MANIFEST_DIR"), "/.."); const DOCKER_DIRECTORY: &str = concat!(env!("CARGO_MANIFEST_DIR"), "/../docker"); diff --git a/src/ci/citool/src/metrics.rs b/src/ci/citool/src/metrics.rs index 117a4f372c4c2..263011a337089 100644 --- a/src/ci/citool/src/metrics.rs +++ b/src/ci/citool/src/metrics.rs @@ -1,9 +1,10 @@ +use std::collections::HashMap; use std::path::Path; -use crate::jobs::JobDatabase; use anyhow::Context; use build_helper::metrics::{JsonNode, JsonRoot, TestSuite}; -use std::collections::HashMap; + +use crate::jobs::JobDatabase; pub type JobName = String; From c9d314773e3ece5e2ecbca5ac6ddf9184dbfafc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakub=20Ber=C3=A1nek?= Date: Mon, 17 Mar 2025 21:48:39 +0100 Subject: [PATCH 13/13] Small review improvements --- .github/workflows/ci.yml | 4 ++-- src/ci/citool/src/metrics.rs | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index aaae67c28bc70..25397006ee23c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -244,9 +244,9 @@ jobs: continue-on-error: true run: | if [ -f build/metrics.json ]; then - METRICS=build/metrics.json + METRICS=build/metrics.json elif [ -f obj/build/metrics.json ]; then - METRICS=obj/build/metrics.json + METRICS=obj/build/metrics.json else echo "No metrics.json found" exit 0 diff --git a/src/ci/citool/src/metrics.rs b/src/ci/citool/src/metrics.rs index 263011a337089..086aa5009f341 100644 --- a/src/ci/citool/src/metrics.rs +++ b/src/ci/citool/src/metrics.rs @@ -57,9 +57,9 @@ pub fn download_auto_job_metrics( Ok(metrics) => Some(metrics), Err(error) => { eprintln!( - r#"Did not find metrics for job `{}` at `{}`: {error:?}. + r#"Did not find metrics for job `{}` at `{parent}`: {error:?}. Maybe it was newly added?"#, - job.name, parent + job.name ); None }