Skip to content

Extract function for determining whether an artifact comparison deserves attention #1287

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 8, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ target
/cache/
/rust.git/
/rust/
/results
31 changes: 22 additions & 9 deletions site/src/comparison.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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,
Expand Down
15 changes: 4 additions & 11 deletions site/src/github.rs
Original file line number Diff line number Diff line change
@@ -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 _;
Expand Down Expand Up @@ -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",
Expand Down