Merge pull request #1287 from rylev/extract-deserves-attention

Extract function for determining whether an artifact comparison deserves attention

Author: rylev

.gitignore

@@ -4,3 +4,4 @@ target
 /cache/
 /rust.git/
 /rust/
+/results

site/src/comparison.rs

@@ -158,15 +158,7 @@ async fn populate_report(
         (None, None) => return,
     };
 
-    let include_in_triage = match (primary.largest_change(), secondary.largest_change()) {
-        (Some(c), _) if c.magnitude() >= Magnitude::Medium => true,
-        (_, Some(c)) if c.magnitude() >= Magnitude::Medium => true,
-        _ => {
-            let primary_n = primary.num_changes();
-            let secondary_n = secondary.num_changes();
-            (primary_n * 2 + secondary_n) >= 6
-        }
-    };
+    let include_in_triage = deserves_attention(&primary, &secondary);
 
     if include_in_triage {
         let entry = report.entry(direction).or_default();
@@ -358,6 +350,27 @@ impl ArtifactComparisonSummary {
     }
 }
 
+/// Whether we are confident enough that an artifact comparison represents a real change and thus deserves to be looked at.
+///
+/// For example, this can be used to determine if artifact comparisons with regressions should be labeled with the
+/// `perf-regression` GitHub label or should be shown in the perf triage report.
+pub(crate) fn deserves_attention(
+    primary: &ArtifactComparisonSummary,
+    secondary: &ArtifactComparisonSummary,
+) -> bool {
+    match (primary.largest_change(), secondary.largest_change()) {
+        (Some(c), _) if c.magnitude() >= Magnitude::Medium => true,
+        (_, Some(c)) if c.magnitude() >= Magnitude::Medium => true,
+        _ => {
+            // How we determine whether a group of small changes deserves attention is and always will be arbitrary,
+            // but this feels good enough for now. We may choose in the future to become more sophisticated about it.
+            let primary_n = primary.num_changes();
+            let secondary_n = secondary.num_changes();
+            (primary_n * 2 + secondary_n) >= 6
+        }
+    }
+}
+
 async fn write_triage_summary(
     comparison: &ArtifactComparison,
     primary: &ArtifactComparisonSummary,

site/src/github.rs

@@ -1,5 +1,7 @@
 use crate::api::github::Issue;
-use crate::comparison::{write_summary_table, ArtifactComparisonSummary, Direction, Magnitude};
+use crate::comparison::{
+    deserves_attention, write_summary_table, ArtifactComparisonSummary, Direction,
+};
 use crate::load::{Config, SiteCtxt, TryCommit};
 
 use anyhow::Context as _;
@@ -637,16 +639,7 @@ fn next_steps(
     direction: Option<Direction>,
     is_master_commit: bool,
 ) -> String {
-    // whether we are confident enough this is a real change and it deserves to be looked at
-    let deserves_attention = match (primary.largest_change(), secondary.largest_change()) {
-        (Some(c), _) if c.magnitude() >= Magnitude::Medium => true,
-        (_, Some(c)) if c.magnitude() >= Magnitude::Medium => true,
-        _ => {
-            let primary_n = primary.num_changes();
-            let secondary_n = secondary.num_changes();
-            (primary_n * 2 + secondary_n) >= 6
-        }
-    };
+    let deserves_attention = deserves_attention(&primary, &secondary);
     let label = match (deserves_attention, direction) {
         (true, Some(Direction::Regression | Direction::Mixed)) => "+perf-regression",
         _ => "-perf-regression",