From 3c9b07643a11fe4049826a45382b08a6eaf722fd Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Thu, 25 May 2023 09:39:19 +0200 Subject: [PATCH 1/5] include test suite metadata in build metrics --- src/bootstrap/metrics.rs | 77 ++++++++++++++++++++++++++++------------ src/bootstrap/test.rs | 49 +++++++++++++++++++++++++ 2 files changed, 104 insertions(+), 22 deletions(-) diff --git a/src/bootstrap/metrics.rs b/src/bootstrap/metrics.rs index e19d56ccd6a..b119cf2bc95 100644 --- a/src/bootstrap/metrics.rs +++ b/src/bootstrap/metrics.rs @@ -57,7 +57,7 @@ impl BuildMetrics { duration_excluding_children_sec: Duration::ZERO, children: Vec::new(), - tests: Vec::new(), + test_suites: Vec::new(), }); } @@ -84,6 +84,17 @@ impl BuildMetrics { } } + pub(crate) fn begin_test_suite(&self, metadata: TestSuiteMetadata, builder: &Builder<'_>) { + // Do not record dry runs, as they'd be duplicates of the actual steps. + if builder.config.dry_run() { + return; + } + + let mut state = self.state.borrow_mut(); + let step = state.running_steps.last_mut().unwrap(); + step.test_suites.push(TestSuite { metadata, tests: Vec::new() }); + } + pub(crate) fn record_test(&self, name: &str, outcome: TestOutcome, builder: &Builder<'_>) { // Do not record dry runs, as they'd be duplicates of the actual steps. if builder.config.dry_run() { @@ -91,12 +102,15 @@ impl BuildMetrics { } let mut state = self.state.borrow_mut(); - state - .running_steps - .last_mut() - .unwrap() - .tests - .push(Test { name: name.to_string(), outcome }); + let step = state.running_steps.last_mut().unwrap(); + + if let Some(test_suite) = step.test_suites.last_mut() { + test_suite.tests.push(Test { name: name.to_string(), outcome }); + } else { + panic!( + "metrics.record_test() called without calling metrics.record_test_suite() first" + ); + } } fn collect_stats(&self, state: &mut MetricsState) { @@ -159,11 +173,7 @@ impl BuildMetrics { fn prepare_json_step(&self, step: StepMetrics) -> JsonNode { let mut children = Vec::new(); children.extend(step.children.into_iter().map(|child| self.prepare_json_step(child))); - children.extend( - step.tests - .into_iter() - .map(|test| JsonNode::Test { name: test.name, outcome: test.outcome }), - ); + children.extend(step.test_suites.into_iter().map(|suite| JsonNode::TestSuite(suite))); JsonNode::RustbuildStep { type_: step.type_, @@ -198,12 +208,7 @@ struct StepMetrics { duration_excluding_children_sec: Duration, children: Vec, - tests: Vec, -} - -struct Test { - name: String, - outcome: TestOutcome, + test_suites: Vec, } #[derive(Serialize, Deserialize)] @@ -237,11 +242,39 @@ enum JsonNode { children: Vec, }, - Test { - name: String, - #[serde(flatten)] - outcome: TestOutcome, + TestSuite(TestSuite), +} + +#[derive(Serialize, Deserialize)] +struct TestSuite { + metadata: TestSuiteMetadata, + tests: Vec, +} + +#[derive(Serialize, Deserialize)] +#[serde(tag = "kind", rename_all = "snake_case")] +pub(crate) enum TestSuiteMetadata { + Crate { + crates: Vec, + target: String, + host: String, + stage: u32, }, + Compiletest { + suite: String, + mode: String, + compare_mode: Option, + target: String, + host: String, + stage: u32, + }, +} + +#[derive(Serialize, Deserialize)] +pub(crate) struct Test { + name: String, + #[serde(flatten)] + outcome: TestOutcome, } #[derive(Serialize, Deserialize)] diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index 2b72d6c48eb..f64b5f96523 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -317,6 +317,17 @@ impl Step for Cargo { cargo.env("CARGO_TEST_DISABLE_NIGHTLY", "1"); cargo.env("PATH", &path_for_cargo(builder, compiler)); + #[cfg(feature = "build-metrics")] + builder.metrics.begin_test_suite( + crate::metrics::TestSuiteMetadata::Crate { + crates: vec!["cargo".into()], + target: self.host.triple.to_string(), + host: self.host.triple.to_string(), + stage: self.stage, + }, + builder, + ); + let _time = util::timeit(&builder); add_flags_and_try_run_tests(builder, &mut cargo); } @@ -1759,6 +1770,19 @@ note: if you're sure you want to do this, please open an issue as to why. In the builder.ci_env.force_coloring_in_ci(&mut cmd); + #[cfg(feature = "build-metrics")] + builder.metrics.begin_test_suite( + crate::metrics::TestSuiteMetadata::Compiletest { + suite: suite.into(), + mode: mode.into(), + compare_mode: None, + target: self.target.triple.to_string(), + host: self.compiler.host.triple.to_string(), + stage: self.compiler.stage, + }, + builder, + ); + builder.info(&format!( "Check compiletest suite={} mode={} ({} -> {})", suite, mode, &compiler.host, target @@ -1768,6 +1792,20 @@ note: if you're sure you want to do this, please open an issue as to why. In the if let Some(compare_mode) = compare_mode { cmd.arg("--compare-mode").arg(compare_mode); + + #[cfg(feature = "build-metrics")] + builder.metrics.begin_test_suite( + crate::metrics::TestSuiteMetadata::Compiletest { + suite: suite.into(), + mode: mode.into(), + compare_mode: Some(compare_mode.into()), + target: self.target.triple.to_string(), + host: self.compiler.host.triple.to_string(), + stage: self.compiler.stage, + }, + builder, + ); + builder.info(&format!( "Check compiletest suite={} mode={} compare_mode={} ({} -> {})", suite, mode, compare_mode, &compiler.host, target @@ -2094,6 +2132,17 @@ fn run_cargo_test( let mut cargo = prepare_cargo_test(cargo, libtest_args, crates, primary_crate, compiler, target, builder); let _time = util::timeit(&builder); + + #[cfg(feature = "build-metrics")] + builder.metrics.begin_test_suite( + crate::metrics::TestSuiteMetadata::Crate { + crates: crates.iter().map(|c| c.to_string()).collect(), + target: target.triple.to_string(), + host: compiler.host.triple.to_string(), + stage: compiler.stage, + }, + builder, + ); add_flags_and_try_run_tests(builder, &mut cargo) } From 0553f71b1b27bd75102284711c84aede3001943b Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Thu, 25 May 2023 09:49:01 +0200 Subject: [PATCH 2/5] introduce build metrics version numbers to handle breaking changes --- src/bootstrap/metrics.rs | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/src/bootstrap/metrics.rs b/src/bootstrap/metrics.rs index b119cf2bc95..9f68eea9a5e 100644 --- a/src/bootstrap/metrics.rs +++ b/src/bootstrap/metrics.rs @@ -14,6 +14,13 @@ use std::io::BufWriter; use std::time::{Duration, Instant, SystemTime}; use sysinfo::{CpuExt, System, SystemExt}; +// Update this number whenever a breaking change is made to the build metrics. +// +// Versions: +// 0: initial version +// 1: replaced JsonNode::Test with JsonNode::TestSuite +const CURRENT_METADATA_VERSION: usize = 1; + pub(crate) struct BuildMetrics { state: RefCell, } @@ -145,7 +152,20 @@ impl BuildMetrics { // Some of our CI builds consist of multiple independent CI invocations. Ensure all the // previous invocations are still present in the resulting file. let mut invocations = match std::fs::read(&dest) { - Ok(contents) => t!(serde_json::from_slice::(&contents)).invocations, + Ok(contents) => { + // We first parse just the metadata_version field to have the check succeed even if + // the rest of the contents are not valid anymore. + let version: OnlyMetadataVersion = t!(serde_json::from_slice(&contents)); + if version.metadata_version == CURRENT_METADATA_VERSION { + t!(serde_json::from_slice::(&contents)).invocations + } else { + println!( + "warning: overriding existing build/metrics.json, as it's not \ + compatible with build metrics format version {CURRENT_METADATA_VERSION}." + ); + Vec::new() + } + } Err(err) => { if err.kind() != std::io::ErrorKind::NotFound { panic!("failed to open existing metrics file at {}: {err}", dest.display()); @@ -163,7 +183,8 @@ impl BuildMetrics { children: steps.into_iter().map(|step| self.prepare_json_step(step)).collect(), }); - let json = JsonRoot { system_stats, invocations }; + let json = + JsonRoot { metadata_version: CURRENT_METADATA_VERSION, system_stats, invocations }; t!(std::fs::create_dir_all(dest.parent().unwrap())); let mut file = BufWriter::new(t!(File::create(&dest))); @@ -214,6 +235,8 @@ struct StepMetrics { #[derive(Serialize, Deserialize)] #[serde(rename_all = "snake_case")] struct JsonRoot { + #[serde(default)] // For version 0 the field was not present. + metadata_version: usize, system_stats: JsonInvocationSystemStats, invocations: Vec, } @@ -299,3 +322,9 @@ struct JsonInvocationSystemStats { struct JsonStepSystemStats { cpu_utilization_percent: f64, } + +#[derive(Deserialize)] +struct OnlyMetadataVersion { + #[serde(default)] // For version 0 the field was not present. + metadata_version: usize, +} From cb68c051511fe0583ecf3f39147b8537a26c1e1f Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Fri, 26 May 2023 09:47:21 +0200 Subject: [PATCH 3/5] address review feedback --- src/bootstrap/metrics.rs | 8 +++----- src/bootstrap/test.rs | 4 ++-- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/src/bootstrap/metrics.rs b/src/bootstrap/metrics.rs index 9f68eea9a5e..8aa821a3afc 100644 --- a/src/bootstrap/metrics.rs +++ b/src/bootstrap/metrics.rs @@ -114,9 +114,7 @@ impl BuildMetrics { if let Some(test_suite) = step.test_suites.last_mut() { test_suite.tests.push(Test { name: name.to_string(), outcome }); } else { - panic!( - "metrics.record_test() called without calling metrics.record_test_suite() first" - ); + panic!("metrics.record_test() called without calling metrics.begin_test_suite() first"); } } @@ -194,7 +192,7 @@ impl BuildMetrics { fn prepare_json_step(&self, step: StepMetrics) -> JsonNode { let mut children = Vec::new(); children.extend(step.children.into_iter().map(|child| self.prepare_json_step(child))); - children.extend(step.test_suites.into_iter().map(|suite| JsonNode::TestSuite(suite))); + children.extend(step.test_suites.into_iter().map(JsonNode::TestSuite)); JsonNode::RustbuildStep { type_: step.type_, @@ -277,7 +275,7 @@ struct TestSuite { #[derive(Serialize, Deserialize)] #[serde(tag = "kind", rename_all = "snake_case")] pub(crate) enum TestSuiteMetadata { - Crate { + CargoPackage { crates: Vec, target: String, host: String, diff --git a/src/bootstrap/test.rs b/src/bootstrap/test.rs index f64b5f96523..29edbe5ae41 100644 --- a/src/bootstrap/test.rs +++ b/src/bootstrap/test.rs @@ -319,7 +319,7 @@ impl Step for Cargo { #[cfg(feature = "build-metrics")] builder.metrics.begin_test_suite( - crate::metrics::TestSuiteMetadata::Crate { + crate::metrics::TestSuiteMetadata::CargoPackage { crates: vec!["cargo".into()], target: self.host.triple.to_string(), host: self.host.triple.to_string(), @@ -2135,7 +2135,7 @@ fn run_cargo_test( #[cfg(feature = "build-metrics")] builder.metrics.begin_test_suite( - crate::metrics::TestSuiteMetadata::Crate { + crate::metrics::TestSuiteMetadata::CargoPackage { crates: crates.iter().map(|c| c.to_string()).collect(), target: target.triple.to_string(), host: compiler.host.triple.to_string(), From 7040d4102f0f3915944369e8d25ebf91f6a99640 Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Fri, 26 May 2023 15:21:21 +0200 Subject: [PATCH 4/5] rename metadata_version to format_version The new name is more accurate. --- src/bootstrap/metrics.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/bootstrap/metrics.rs b/src/bootstrap/metrics.rs index 8aa821a3afc..405a88d7568 100644 --- a/src/bootstrap/metrics.rs +++ b/src/bootstrap/metrics.rs @@ -19,7 +19,7 @@ use sysinfo::{CpuExt, System, SystemExt}; // Versions: // 0: initial version // 1: replaced JsonNode::Test with JsonNode::TestSuite -const CURRENT_METADATA_VERSION: usize = 1; +const CURRENT_FORMAT_VERSION: usize = 1; pub(crate) struct BuildMetrics { state: RefCell, @@ -151,15 +151,15 @@ impl BuildMetrics { // previous invocations are still present in the resulting file. let mut invocations = match std::fs::read(&dest) { Ok(contents) => { - // We first parse just the metadata_version field to have the check succeed even if + // We first parse just the format_version field to have the check succeed even if // the rest of the contents are not valid anymore. - let version: OnlyMetadataVersion = t!(serde_json::from_slice(&contents)); - if version.metadata_version == CURRENT_METADATA_VERSION { + let version: OnlyFormatVersion = t!(serde_json::from_slice(&contents)); + if version.format_version == CURRENT_FORMAT_VERSION { t!(serde_json::from_slice::(&contents)).invocations } else { println!( "warning: overriding existing build/metrics.json, as it's not \ - compatible with build metrics format version {CURRENT_METADATA_VERSION}." + compatible with build metrics format version {CURRENT_FORMAT_VERSION}." ); Vec::new() } @@ -181,8 +181,7 @@ impl BuildMetrics { children: steps.into_iter().map(|step| self.prepare_json_step(step)).collect(), }); - let json = - JsonRoot { metadata_version: CURRENT_METADATA_VERSION, system_stats, invocations }; + let json = JsonRoot { format_version: CURRENT_FORMAT_VERSION, system_stats, invocations }; t!(std::fs::create_dir_all(dest.parent().unwrap())); let mut file = BufWriter::new(t!(File::create(&dest))); @@ -234,7 +233,7 @@ struct StepMetrics { #[serde(rename_all = "snake_case")] struct JsonRoot { #[serde(default)] // For version 0 the field was not present. - metadata_version: usize, + format_version: usize, system_stats: JsonInvocationSystemStats, invocations: Vec, } @@ -322,7 +321,7 @@ struct JsonStepSystemStats { } #[derive(Deserialize)] -struct OnlyMetadataVersion { +struct OnlyFormatVersion { #[serde(default)] // For version 0 the field was not present. - metadata_version: usize, + format_version: usize, } From c5139b9136c25c71f4c5f71335da9aedb8088cbc Mon Sep 17 00:00:00 2001 From: Pietro Albini Date: Fri, 26 May 2023 15:25:21 +0200 Subject: [PATCH 5/5] add reasoning for introducing a metrics format version --- src/bootstrap/metrics.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src/bootstrap/metrics.rs b/src/bootstrap/metrics.rs index 405a88d7568..5990f33b9bc 100644 --- a/src/bootstrap/metrics.rs +++ b/src/bootstrap/metrics.rs @@ -16,9 +16,21 @@ use sysinfo::{CpuExt, System, SystemExt}; // Update this number whenever a breaking change is made to the build metrics. // -// Versions: -// 0: initial version -// 1: replaced JsonNode::Test with JsonNode::TestSuite +// The output format is versioned for two reasons: +// +// - The metadata is intended to be consumed by external tooling, and exposing a format version +// helps the tools determine whether they're compatible with a metrics file. +// +// - If a developer enables build metrics in their local checkout, making a breaking change to the +// metrics format would result in a hard-to-diagnose error message when an existing metrics file +// is not compatible with the new changes. With a format version number, bootstrap can discard +// incompatible metrics files instead of appending metrics to them. +// +// Version changelog: +// +// - v0: initial version +// - v1: replaced JsonNode::Test with JsonNode::TestSuite +// const CURRENT_FORMAT_VERSION: usize = 1; pub(crate) struct BuildMetrics {