Include test suite metadata in the build metrics

src/bootstrap/metrics.rs

@@ -14,6 +14,25 @@ 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.
+//
+// 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 {
     state: RefCell<MetricsState>,
 }
@@ -57,7 +76,7 @@ impl BuildMetrics {
             duration_excluding_children_sec: Duration::ZERO,
 
             children: Vec::new(),
-            tests: Vec::new(),
+            test_suites: Vec::new(),
         });
     }
 
@@ -84,19 +103,31 @@ 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() {
             return;
         }
 
         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.begin_test_suite() first");
+        }
     }
 
     fn collect_stats(&self, state: &mut MetricsState) {
@@ -131,7 +162,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::<JsonRoot>(&contents)).invocations,
+            Ok(contents) => {
+                // 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: OnlyFormatVersion = t!(serde_json::from_slice(&contents));
+                if version.format_version == CURRENT_FORMAT_VERSION {
+                    t!(serde_json::from_slice::<JsonRoot>(&contents)).invocations
+                } else {
+                    println!(
+                        "warning: overriding existing build/metrics.json, as it's not \
+                         compatible with build metrics format version {CURRENT_FORMAT_VERSION}."
+                    );
+                    Vec::new()
+                }
+            }
             Err(err) => {
                 if err.kind() != std::io::ErrorKind::NotFound {
                     panic!("failed to open existing metrics file at {}: {err}", dest.display());
@@ -149,7 +193,7 @@ impl BuildMetrics {
             children: steps.into_iter().map(|step| self.prepare_json_step(step)).collect(),
         });
 
-        let json = JsonRoot { 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)));
@@ -159,11 +203,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(JsonNode::TestSuite));
 
         JsonNode::RustbuildStep {
             type_: step.type_,
@@ -198,17 +238,14 @@ struct StepMetrics {
     duration_excluding_children_sec: Duration,
 
     children: Vec<StepMetrics>,
-    tests: Vec<Test>,
-}
-
-struct Test {
-    name: String,
-    outcome: TestOutcome,
+    test_suites: Vec<TestSuite>,
 }
 
 #[derive(Serialize, Deserialize)]
 #[serde(rename_all = "snake_case")]
 struct JsonRoot {
+    #[serde(default)] // For version 0 the field was not present.
+    format_version: usize,
     system_stats: JsonInvocationSystemStats,
     invocations: Vec<JsonInvocation>,
 }
@@ -237,13 +274,41 @@ enum JsonNode {
 
         children: Vec<JsonNode>,
     },
-    Test {
-        name: String,
-        #[serde(flatten)]
-        outcome: TestOutcome,
+    TestSuite(TestSuite),
+}
+
+#[derive(Serialize, Deserialize)]
+struct TestSuite {
+    metadata: TestSuiteMetadata,
+    tests: Vec<Test>,
+}
+
+#[derive(Serialize, Deserialize)]
+#[serde(tag = "kind", rename_all = "snake_case")]
+pub(crate) enum TestSuiteMetadata {
+    CargoPackage {
+        crates: Vec<String>,
+        target: String,
+        host: String,
+        stage: u32,
+    },
+    Compiletest {
+        suite: String,
+        mode: String,
+        compare_mode: Option<String>,
+        target: String,
+        host: String,
+        stage: u32,
     },
 }
 
+#[derive(Serialize, Deserialize)]
+pub(crate) struct Test {
+    name: String,
+    #[serde(flatten)]
+    outcome: TestOutcome,
+}
+
 #[derive(Serialize, Deserialize)]
 #[serde(tag = "outcome", rename_all = "snake_case")]
 pub(crate) enum TestOutcome {
@@ -266,3 +331,9 @@ struct JsonInvocationSystemStats {
 struct JsonStepSystemStats {
     cpu_utilization_percent: f64,
 }
+
+#[derive(Deserialize)]
+struct OnlyFormatVersion {
+    #[serde(default)] // For version 0 the field was not present.
+    format_version: usize,
+}

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::CargoPackage {
+                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::CargoPackage {
+            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)
 }