From e57aa0513eacc679a080bd57eab324cc6e32552b Mon Sep 17 00:00:00 2001 From: Everett Pompeii Date: Wed, 26 Aug 2026 11:27:12 +0000 Subject: [PATCH] Declare named and filtered thresholds in the v1 report payload A report has always carried its thresholds as a map of measure to model, and a map key names a measure and nothing else. Every threshold a report could declare was therefore the bare one: the conventional `value` name of every variant. A pipeline that wanted `p99` watched, or wanted only some variants watched, had to reach for the thresholds endpoint and then keep it in step with the run by hand. At BMF version 1 `thresholds.models` is a list. An entry is `parameters`, `measure`, `metric`, and `model`: the dimensions a threshold hangs off that the report does not already state, in their canonical order, and the model to check with. `measure` is the same name, slug, or UUID the map key is today, created if the project has never seen it. An absent `metric` is the conventional `value` name and an absent `parameters` checks every variant, so an entry naming only a measure and a model is the map pair written out longhand. One measure may carry several entries, and each one creates or updates the threshold with that identity under the report's branch and testbed, through the same null collapse and canonical filter storage every other writer goes through. A threshold the report declares checks the very report that declared it. Nothing moved to make that true: thresholds are resolved before results are parsed, which is where they already were. It is worth saying out loud because it is what makes the list worth having, and it is pinned by a test whose history is five unchecked reports and whose alerts all belong to thresholds that did not exist when the request arrived. The report's BMF version, the one it declares or else the project's default, says which shape to expect, and the shape is checked rather than guessed at. A list at version 0 and a map at version 1 are both a 400 naming that version and the shape it calls for, and the check runs before anything is created for the report. `reset` reaches as far as the report's version can address. A version 0 map can only name bare thresholds, so it takes a model away from bare thresholds and nothing else, which is what it already did. At version 0, `reset` cannot strip a named or filtered threshold the payload has no way to spell. A version 1 list can name every identity, so `reset` reaches every threshold on the branch and testbed that the entries did not name, including a payload that names none at all. One identity declared twice in one payload is one threshold: the position is where it was first written and the model is what it was last told, resolved without an error. Two spellings of one filter are one identity, because a filter canonicalizes before it is compared. Two shapes behind one key is a place where error quality quietly dies. The obvious spelling, an untagged enum, buffers the input, tries each variant, and on failure says only that nothing matched, so a misspelled model test in a version 0 map would come back as "data did not match any variant" rather than as the field and the variants it could have been. The shape is known from the first token, so it is decided by looking rather than by trying: every error a version 0 client got before this layer it gets after it, byte for byte, and a malformed version 1 entry is named by its position and its field. The one message that moves is the one that has to, where `models` is neither shape and what it expects now names both. Deleting a variant learns about thresholds. A threshold whose filter names a variant is a reference to that row, so the variant cannot go out from under it, and the refusal says which threshold to delete first. Naming is canonical equality and nothing looser: a filter of `{"a":1}` matches the variant `{"a":1,"b":2}` without naming it, because a filter names only the keys it cares about and is a predicate over values rather than a pointer at a row. That variant can be deleted and the filter still says what it said. The comparison runs in Rust over the project's filtered thresholds, because canonical equality is what the canonical form defines and that form is written in Rust. Nothing caps that read: it is small because a filtered threshold is a rare thing to write and deleting a variant is a rare thing to ask for, not because a limit says so. The delete and the check share one transaction with the delete first, so a variant that a report still references is refused for that reason and the client is sent to the results rather than to a threshold it would have to delete anyway. The CLI is unchanged: it declares bare thresholds, which is what the version 0 map spells, and naming a metric or a variant from the command line is a separate piece of work. --- lib/api_projects/src/variants.rs | 88 +- lib/api_projects/tests/metrics.rs | 28 +- lib/api_projects/tests/report_thresholds.rs | 988 ++++++++++++++++++ lib/api_projects/tests/variants.rs | 307 +++++- lib/bencher_json/src/project/report.rs | 174 ++- .../src/model/project/report/mod.rs | 16 +- .../src/model/project/threshold/mod.rs | 279 ++++- services/api/openapi.json | 76 +- .../sub/project/report/create/thresholds.rs | 6 +- 9 files changed, 1845 insertions(+), 117 deletions(-) create mode 100644 lib/api_projects/tests/report_thresholds.rs diff --git a/lib/api_projects/src/variants.rs b/lib/api_projects/src/variants.rs index 2fce432777..90c52c6d6f 100644 --- a/lib/api_projects/src/variants.rs +++ b/lib/api_projects/src/variants.rs @@ -3,21 +3,21 @@ use bencher_endpoint::{ TotalCount, }; use bencher_json::{ - BenchmarkResourceId, JsonDirection, JsonPagination, JsonVariant, JsonVariants, - ProjectResourceId, VariantUuid, + BenchmarkResourceId, JsonDirection, JsonPagination, JsonVariant, JsonVariants, ParameterFilter, + ParameterSet, ProjectResourceId, ThresholdUuid, VariantUuid, project::variant::{JsonNewVariant, JsonUpdateVariant}, }; use bencher_rbac::project::Permission; use bencher_schema::{ actor_conn, auth_conn, - context::ApiContext, + context::{ApiContext, DbConnection}, error::{ conflict_error, resource_conflict_err, resource_not_found_err, with_auth_hint, with_token_hint, }, model::{ project::{ - QueryProject, + ProjectId, QueryProject, benchmark::QueryBenchmark, variant::{QueryVariant, UpdateVariant}, }, @@ -26,7 +26,7 @@ use bencher_schema::{ auth::{AuthUser, BearerToken}, }, }, - schema, write_conn, + schema, write_conn, write_transaction, }; use diesel::{BelongingToDsl as _, ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _}; use dropshot::{HttpError, Path, Query, RequestContext, TypedBody, endpoint}; @@ -402,6 +402,12 @@ pub async fn patch_inner( /// Delete a variant for a benchmark. /// The user must have `delete` permissions for the project. /// All reports that use this variant must be deleted first! +/// All thresholds that use this variant must be deleted first! +/// +/// A threshold uses a variant when its `parameters` filter names that exact set of +/// parameters. A filter that merely matches the variant, because the variant pins +/// every key the filter names and more besides, is a predicate over values rather +/// than a reference to this row, and it does not stand in the way. /// /// A benchmark's empty variant cannot be deleted. /// The empty variant is structural: every benchmark is born with exactly one, and @@ -457,9 +463,73 @@ async fn delete_inner( ))); } - diesel::delete(schema::variant::table.filter(schema::variant::id.eq(query_variant.id))) - .execute(write_conn!(context)) - .map_err(resource_conflict_err!(Variant, &query_variant))?; + // The delete goes first and the threshold check goes second, both inside one + // transaction, so a variant that a report still references is refused for that + // reason: the foreign key fires on the delete itself and the report refusal is + // the one the client reads. A variant nothing reports is deleted and then put + // back if a threshold names it, which is what keeps the two refusals in that + // order without either of them growing a query the other already does. + let mut blocking_threshold = None; + let deleted = write_transaction!(context, |conn| { + diesel::delete(schema::variant::table.filter(schema::variant::id.eq(query_variant.id))) + .execute(conn)?; + + blocking_threshold = + threshold_naming_parameters(conn, query_project.id, &query_variant.parameters)?; + if blocking_threshold.is_some() { + return Err(diesel::result::Error::RollbackTransaction); + } + diesel::QueryResult::Ok(()) + }); + + match (deleted, blocking_threshold) { + (Ok(()), _) => Ok(()), + (Err(diesel::result::Error::RollbackTransaction), Some(threshold)) => { + Err(conflict_error(format!( + "All thresholds that use this variant must be deleted first! Threshold ({threshold}) checks the variant ({variant}) of benchmark ({benchmark}).", + variant = query_variant.parameters, + benchmark = query_benchmark.uuid, + ))) + }, + (Err(e), _) => Err(resource_conflict_err!(Variant, &query_variant)(e)), + } +} - Ok(()) +/// The first threshold in the project whose filter names this exact set of +/// parameters, if there is one. +/// +/// A filter names parameters by canonical equality and only by canonical equality. A +/// filter that merely matches them, say `{"a":1}` against the variant +/// `{"a":1,"b":2}`, is a predicate over values rather than a reference to a row, and +/// deleting the row it happens to match takes nothing out from under it. +/// +/// The comparison runs here rather than in SQL because canonical equality is what +/// the canonical form defines and that form is written in Rust. Only the thresholds +/// carrying a filter at all are read, which is a small share of a project's +/// thresholds and empty for every project that has never written one, and deleting a +/// variant is a rare administrative request rather than anything on the ingest +/// path. Nothing here caps the read, so a project that one day holds a great many +/// filtered thresholds is what would move this into SQL. +fn threshold_naming_parameters( + conn: &mut DbConnection, + project_id: ProjectId, + parameters: &ParameterSet, +) -> diesel::QueryResult> { + let canonical = parameters.canonical(); + Ok(schema::threshold::table + .filter(schema::threshold::project_id.eq(project_id)) + .filter(schema::threshold::parameters.is_not_null()) + .order(schema::threshold::id.asc()) + .select((schema::threshold::uuid, schema::threshold::parameters)) + .load::<(ThresholdUuid, Option)>(conn)? + .into_iter() + .find(|(_, parameters)| { + parameters.as_ref().is_some_and(|parameters| { + parameters + .sets() + .iter() + .any(|set| set.canonical() == canonical) + }) + }) + .map(|(uuid, _)| uuid)) } diff --git a/lib/api_projects/tests/metrics.rs b/lib/api_projects/tests/metrics.rs index f149cc3cc8..7101e550d9 100644 --- a/lib/api_projects/tests/metrics.rs +++ b/lib/api_projects/tests/metrics.rs @@ -478,19 +478,23 @@ async fn fixture(server: &TestServer, label: &str) -> Fixture { } } -/// A threshold model loose enough to compute a boundary from a short history and -/// tight enough that a tenfold jump is an outlier. -fn threshold_models() -> serde_json::Value { +/// The bare threshold, the conventional `value` name of every variant, with a model +/// loose enough to compute a boundary from a short history and tight enough that a +/// tenfold jump is an outlier. A version 1 payload declares its thresholds as a list. +fn threshold_entries() -> serde_json::Value { serde_json::json!({ - "models": { - "latency": { - "test": "t_test", - "min_sample_size": 2, - "max_sample_size": 64, - "lower_boundary": 0.98, - "upper_boundary": 0.98, + "models": [ + { + "measure": "latency", + "model": { + "test": "t_test", + "min_sample_size": 2, + "max_sample_size": 64, + "lower_boundary": 0.98, + "upper_boundary": 0.98, + } } - } + ] }) } @@ -833,7 +837,7 @@ async fn metrics_get_checked_value_row() { &serde_json::json!({ "latency": { "value": value, "p99": value * 2.0 } }), )], )], - Some(threshold_models()), + Some(threshold_entries()), Some(1), ) .await; diff --git a/lib/api_projects/tests/report_thresholds.rs b/lib/api_projects/tests/report_thresholds.rs new file mode 100644 index 0000000000..53501a4098 --- /dev/null +++ b/lib/api_projects/tests/report_thresholds.rs @@ -0,0 +1,988 @@ +#![expect( + unused_crate_dependencies, + clippy::expect_used, + clippy::tests_outside_test_module, + reason = "integration test file" +)] +//! The thresholds a report payload declares, end to end through ingest. +//! +//! A report has always carried a map of measure to model, and a map key names a +//! measure and nothing else. BMF version 1 carries a list instead, and a list entry +//! names everything a threshold checks: the variants, the measure, and the metric +//! name. The version the payload declares is what says which shape it is written +//! in, so the two shapes never have to be told apart by looking at them. +//! +//! Two things follow from that and are what most of this file is about. A threshold +//! the report creates checks the very report that created it, because thresholds are +//! resolved before results are. And `reset` reaches exactly as far as the shape can +//! address: a version 0 map can only name bare thresholds, so a legacy run cannot +//! strip a filtered threshold it cannot even spell. + +use bencher_api_tests::{TestServer, helpers::get_project_id}; +use bencher_json::{MetricName, ParameterFilter, ParameterSet, ProjectSlug, ThresholdUuid}; +use bencher_schema::{context::DbConnection, schema}; +use diesel::{ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _}; +use http::StatusCode; + +/// A threshold model loose enough to compute a boundary from a short history and +/// tight enough that a tenfold jump is an outlier. +fn model() -> serde_json::Value { + serde_json::json!({ + "test": "t_test", + "min_sample_size": 2, + "max_sample_size": 64, + "lower_boundary": 0.98, + "upper_boundary": 0.98, + }) +} + +/// A signed up user with an organization and a project to report into. +struct Fixture { + project_slug: ProjectSlug, + token: String, +} + +/// A signed up user with an organization and a project to report into. +async fn fixture(server: &TestServer, label: &str) -> Fixture { + let user = server + .signup("Test User", &format!("rt{label}@example.com")) + .await; + let org = server.create_org(&user, &format!("RT Org {label}")).await; + let project = server + .create_project(&user, &org, &format!("RT Project {label}")) + .await; + Fixture { + project_slug: project.slug, + token: user.token, + } +} + +/// One report request. +/// +/// `day` only has to be distinct and increasing, so reports order the way they were +/// submitted. +struct Post { + day: usize, + results: Vec, + bmf_version: u8, + thresholds: Option, + branch: &'static str, + testbed: &'static str, +} + +impl Post { + fn new(day: usize, results: Vec) -> Self { + Self { + day, + results, + bmf_version: 1, + thresholds: None, + branch: "main", + testbed: "localhost", + } + } + + fn v0(mut self) -> Self { + self.bmf_version = 0; + self + } + + fn thresholds(mut self, thresholds: serde_json::Value) -> Self { + self.thresholds = Some(thresholds); + self + } + + /// Report onto a branch and testbed the project does not have yet, so that + /// whether they exist afterwards says whether the request got as far as + /// creating anything. + fn onto(mut self, branch: &'static str, testbed: &'static str) -> Self { + self.branch = branch; + self.testbed = testbed; + self + } +} + +/// Post one report and return its status and body, whatever they are. +async fn try_report(server: &TestServer, fixture: &Fixture, post: Post) -> (StatusCode, String) { + let Post { + day, + results, + bmf_version, + thresholds, + branch, + testbed, + } = post; + let body = serde_json::json!({ + "branch": branch, + "testbed": testbed, + "start_time": format!("2024-01-{day:02}T00:00:00Z"), + "end_time": format!("2024-01-{day:02}T00:01:00Z"), + "results": results, + "bmf_version": bmf_version, + "thresholds": thresholds, + }); + + let resp = server + .client + .post(server.api_url(&format!("/v0/projects/{}/reports", fixture.project_slug))) + .header( + bencher_json::AUTHORIZATION, + bencher_json::bearer_header(&fixture.token), + ) + .json(&body) + .send() + .await + .expect("Request failed"); + let status = resp.status(); + let body = resp.text().await.expect("Failed to read the response"); + (status, body) +} + +/// Post one report and require it to be created. +async fn report(server: &TestServer, fixture: &Fixture, post: Post) { + let day = post.day; + let (status, body) = try_report(server, fixture, post).await; + assert_eq!(status, StatusCode::CREATED, "POST report {day}: {body}"); +} + +/// One BMF v1 payload for a single benchmark's variants. +fn v1(entries: &[serde_json::Value]) -> String { + serde_json::to_string(&serde_json::json!({ "bench": entries })).expect("the results serialize") +} + +fn entry(parameters: &serde_json::Value, measures: &serde_json::Value) -> serde_json::Value { + serde_json::json!({ "parameters": parameters, "measures": measures }) +} + +/// One BMF v0 payload for a single benchmark. +fn v0(value: f64) -> String { + serde_json::json!({ "bench": { "latency": { "value": value } } }).to_string() +} + +/// What a threshold checks, spelled the way the wire spells it. +/// +/// The metric name of a threshold that checks the conventional name is `value`, and +/// the variants of a threshold that checks every one of them is `*`, so the two +/// canonical absences read as what they mean rather than as a hole. +fn identity(metric: Option, parameters: Option) -> (String, String) { + ( + metric.map_or_else(|| "value".to_owned(), |metric| metric.to_string()), + parameters.map_or_else(|| "*".to_owned(), |parameters| parameters.canonical()), + ) +} + +/// Every threshold in a project: what it checks and whether it still carries a model. +fn thresholds(conn: &mut DbConnection, project_id: i32) -> Vec<(String, String, bool)> { + schema::threshold::table + .filter(schema::threshold::project_id.eq(project_id)) + .order(schema::threshold::id.asc()) + .select(( + schema::threshold::metric, + schema::threshold::parameters, + schema::threshold::model_id, + )) + .load::<(Option, Option, Option)>(conn) + .expect("Failed to load the thresholds") + .into_iter() + .map(|(metric, parameters, model_id)| { + let (metric, parameters) = identity(metric, parameters); + (metric, parameters, model_id.is_some()) + }) + .collect() +} + +/// How many branches of a project carry this name. +fn branch_count(conn: &mut DbConnection, project_id: i32, name: &str) -> i64 { + schema::branch::table + .filter(schema::branch::project_id.eq(project_id)) + .filter(schema::branch::name.eq(name)) + .count() + .get_result(conn) + .expect("Failed to count the branches") +} + +/// How many testbeds of a project carry this name. +fn testbed_count(conn: &mut DbConnection, project_id: i32, name: &str) -> i64 { + schema::testbed::table + .filter(schema::testbed::project_id.eq(project_id)) + .filter(schema::testbed::name.eq(name)) + .count() + .get_result(conn) + .expect("Failed to count the testbeds") +} + +/// Every threshold row id in a project, in creation order. +fn threshold_ids(conn: &mut DbConnection, project_id: i32) -> Vec { + schema::threshold::table + .filter(schema::threshold::project_id.eq(project_id)) + .order(schema::threshold::id.asc()) + .select(schema::threshold::id) + .load::(conn) + .expect("Failed to load the threshold ids") +} + +/// Every threshold UUID in a project, in creation order. +fn threshold_uuids(conn: &mut DbConnection, project_id: i32) -> Vec { + schema::threshold::table + .filter(schema::threshold::project_id.eq(project_id)) + .order(schema::threshold::id.asc()) + .select(schema::threshold::uuid) + .load::(conn) + .expect("Failed to load the threshold uuids") +} + +/// Every alert in a project, as the variant and metric name it fired on and the +/// identity of the threshold that fired it, sorted so the assertion does not depend +/// on detection order. +fn alerts(conn: &mut DbConnection, project_id: i32) -> Vec<(String, String, String, String)> { + let mut alerts = schema::alert::table + .inner_join( + schema::boundary::table + .inner_join(schema::threshold::table) + .inner_join(schema::metric::table.inner_join( + schema::report_benchmark::table.inner_join(schema::variant::table), + )), + ) + .filter(schema::threshold::project_id.eq(project_id)) + .select(( + schema::variant::parameters, + schema::metric::name, + schema::threshold::metric, + schema::threshold::parameters, + )) + .load::<( + ParameterSet, + MetricName, + Option, + Option, + )>(conn) + .expect("Failed to load the alerts") + .into_iter() + .map(|(set, name, metric, parameters)| { + let (metric, parameters) = identity(metric, parameters); + (set.canonical(), name.to_string(), metric, parameters) + }) + .collect::>(); + alerts.sort(); + alerts +} + +/// The point estimates each variant reports, run after run. +const SMALL: [f64; 5] = [10.0, 11.0, 12.0, 13.0, 14.0]; +const LARGE: [f64; 5] = [100.0, 101.0, 102.0, 103.0, 104.0]; + +/// One steady day of the two variants, each carrying a point estimate and a +/// `p99` beside it. +fn steady(day: usize) -> String { + let (small, large) = SMALL + .into_iter() + .zip(LARGE) + .nth(day % SMALL.len()) + .expect("the day is one of the steady ones"); + variants(small, large) +} + +fn variants(small: f64, large: f64) -> String { + v1(&[ + entry( + &serde_json::json!({ "size_mb": 16 }), + &serde_json::json!({ "latency": { "value": small, "p99": small + 1.0 } }), + ), + entry( + &serde_json::json!({ "size_mb": 32 }), + &serde_json::json!({ "latency": { "value": large, "p99": large + 1.0 } }), + ), + ]) +} + +/// The threshold entries a version 1 payload declares: one that checks `p99` on +/// every variant, and one that checks the point estimate of a single variant. +fn entries() -> serde_json::Value { + serde_json::json!({ + "models": [ + { "measure": "latency", "metric": "p99", "model": model() }, + { + "parameters": [{ "size_mb": 16 }], + "measure": "latency", + "model": model(), + }, + ] + }) +} + +// A version 1 entry list creates the thresholds it names and the very report that +// created them is checked by them. +// +// The history is five reports that declared no threshold at all, so nothing was +// checked until the sixth report both declared the entries and carried the outliers. +// Every alert here belongs to a threshold that did not exist when the request +// arrived, which is what "thresholds are resolved before results" buys. +#[tokio::test] +async fn v1_entries_create_thresholds_that_check_the_same_report() { + let server = TestServer::new().await; + let fixture = fixture(&server, "entries").await; + + for day in 0..SMALL.len() { + report(&server, &fixture, Post::new(day + 1, vec![steady(day)])).await; + } + + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + assert!( + thresholds(&mut conn, project_id).is_empty(), + "nothing checked the history" + ); + drop(conn); + + // Both variants jump tenfold on both names. + report( + &server, + &fixture, + Post::new(6, vec![variants(1_000.0, 10_000.0)]).thresholds(entries()), + ) + .await; + + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![ + ("p99".to_owned(), "*".to_owned(), true), + ("value".to_owned(), r#"[{"size_mb":16}]"#.to_owned(), true), + ], + "each entry created the threshold it named" + ); + + assert_eq!( + alerts(&mut conn, project_id), + vec![ + ( + r#"{"size_mb":16}"#.to_owned(), + "p99".to_owned(), + "p99".to_owned(), + "*".to_owned(), + ), + ( + r#"{"size_mb":16}"#.to_owned(), + "value".to_owned(), + "value".to_owned(), + r#"[{"size_mb":16}]"#.to_owned(), + ), + ( + r#"{"size_mb":32}"#.to_owned(), + "p99".to_owned(), + "p99".to_owned(), + "*".to_owned(), + ), + ], + "the named threshold fires on both variants, and the filtered one fires on its own: the large variant's point estimate jumped just as far and nobody checks it" + ); +} + +// The filtered threshold checks its variant and no other. The large variant's +// point estimate is an outlier against its own history and raises nothing, because +// no threshold checks it. +#[tokio::test] +async fn a_filtered_threshold_checks_only_its_variants() { + let server = TestServer::new().await; + let fixture = fixture(&server, "filtered").await; + + let thresholds_json = serde_json::json!({ + "models": [{ + "parameters": [{ "size_mb": 16 }], + "measure": "latency", + "model": model(), + }] + }); + for day in 0..SMALL.len() { + report( + &server, + &fixture, + Post::new(day + 1, vec![steady(day)]).thresholds(thresholds_json.clone()), + ) + .await; + } + report( + &server, + &fixture, + Post::new(6, vec![variants(1_000.0, 10_000.0)]).thresholds(thresholds_json), + ) + .await; + + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + assert_eq!( + alerts(&mut conn, project_id), + vec![( + r#"{"size_mb":16}"#.to_owned(), + "value".to_owned(), + "value".to_owned(), + r#"[{"size_mb":16}]"#.to_owned(), + )], + "the other variant's tenfold jump is nobody's business" + ); +} + +// The same identity declared by two reports updates the one threshold rather than +// creating a second. +#[tokio::test] +async fn one_identity_across_two_reports_is_one_threshold() { + let server = TestServer::new().await; + let fixture = fixture(&server, "update").await; + + let first = serde_json::json!({ + "models": [{ + "parameters": [{ "size_mb": 16 }], + "measure": "latency", + "metric": "p99", + "model": model(), + }] + }); + // The same identity spelled differently: the filter's sets are canonical, so + // this is the same threshold with a different model. + let second = serde_json::json!({ + "models": [{ + "parameters": [{ "size_mb": 16.0 }], + "measure": "latency", + "metric": "p99", + "model": { + "test": "percentage", + "upper_boundary": 0.25, + }, + }] + }); + + report( + &server, + &fixture, + Post::new(1, vec![steady(0)]).thresholds(first), + ) + .await; + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + let created = threshold_uuids(&mut conn, project_id); + let created_ids = threshold_ids(&mut conn, project_id); + assert_eq!(created.len(), 1, "the entry created one threshold"); + drop(conn); + + report( + &server, + &fixture, + Post::new(2, vec![steady(1)]).thresholds(second), + ) + .await; + + let mut conn = server.db_conn(); + assert_eq!( + threshold_uuids(&mut conn, project_id), + created, + "the second report updated the threshold the first created" + ); + let models = schema::model::table + .filter(schema::model::threshold_id.eq_any(&created_ids)) + .count() + .get_result::(&mut conn) + .expect("Failed to count the models"); + assert_eq!(models, 2, "the new model sits beside the replaced one"); +} + +// One payload may name one identity twice, and the last entry is the one that +// counts. Two spellings of one filter are one identity, so this is a duplicate even +// though the two entries do not read alike. +#[tokio::test] +async fn a_duplicate_identity_in_one_payload_keeps_the_last() { + let server = TestServer::new().await; + let fixture = fixture(&server, "duplicate").await; + + report( + &server, + &fixture, + Post::new(1, vec![steady(0)]).thresholds(serde_json::json!({ + "models": [ + { + "parameters": [{ "size_mb": 16 }, { "size_mb": 16 }], + "measure": "latency", + "model": model(), + }, + { + "parameters": [{ "size_mb": 16 }], + "measure": "latency", + "model": { "test": "percentage", "upper_boundary": 0.25 }, + }, + ] + })), + ) + .await; + + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![("value".to_owned(), r#"[{"size_mb":16}]"#.to_owned(), true)], + "one identity is one threshold" + ); + let boundaries = schema::model::table + .filter(schema::model::threshold_id.eq_any(threshold_ids(&mut conn, project_id))) + .select(schema::model::upper_boundary) + .load::>(&mut conn) + .expect("Failed to load the models"); + assert_eq!( + boundaries, + vec![Some(0.25)], + "the last entry is the model that was written" + ); +} + +// A version 0 payload still carries the map, and the map still addresses the bare +// threshold: the conventional name of every variant. +#[tokio::test] +async fn v0_map_creates_a_bare_threshold() { + let server = TestServer::new().await; + let fixture = fixture(&server, "v0map").await; + + report( + &server, + &fixture, + Post::new(1, vec![v0(10.0)]) + .v0() + .thresholds(serde_json::json!({ "models": { "latency": model() } })), + ) + .await; + + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![("value".to_owned(), "*".to_owned(), true)], + "a map key addresses the bare threshold" + ); +} + +// The shape is not guessed at. A payload that declares a version and then sends the +// other version's shape is refused, and the refusal names the version it declared. +#[tokio::test] +async fn a_list_at_version_0_is_a_bad_request() { + let server = TestServer::new().await; + let fixture = fixture(&server, "listv0").await; + + let (status, body) = try_report( + &server, + &fixture, + Post::new(1, vec![v0(10.0)]) + .v0() + .onto("refused-branch", "refused-testbed") + .thresholds(entries()), + ) + .await; + assert_eq!( + status, + StatusCode::BAD_REQUEST, + "a list at version 0: {body}" + ); + assert!(body.contains('0'), "the refusal names the version: {body}"); + + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + assert!( + thresholds(&mut conn, project_id).is_empty(), + "the refused payload created no threshold" + ); + assert_eq!( + ( + branch_count(&mut conn, project_id, "refused-branch"), + testbed_count(&mut conn, project_id, "refused-testbed"), + ), + (0, 0), + "the shape is refused before the report's dimensions are created" + ); +} + +#[tokio::test] +async fn a_map_at_version_1_is_a_bad_request() { + let server = TestServer::new().await; + let fixture = fixture(&server, "mapv1").await; + + let (status, body) = try_report( + &server, + &fixture, + Post::new(1, vec![steady(0)]) + .onto("refused-branch", "refused-testbed") + .thresholds(serde_json::json!({ "models": { "latency": model() } })), + ) + .await; + assert_eq!( + status, + StatusCode::BAD_REQUEST, + "a map at version 1: {body}" + ); + assert!(body.contains('1'), "the refusal names the version: {body}"); + + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + assert_eq!( + ( + branch_count(&mut conn, project_id, "refused-branch"), + testbed_count(&mut conn, project_id, "refused-testbed"), + ), + (0, 0), + "the shape is refused before the report's dimensions are created" + ); +} + +// An empty map is still a map, so it is refused at version 1 for the same reason a +// full one is. The shape is checked before anything the payload says is acted on. +#[tokio::test] +async fn an_empty_map_at_version_1_is_a_bad_request() { + let server = TestServer::new().await; + let fixture = fixture(&server, "emptymap").await; + + let (status, body) = try_report( + &server, + &fixture, + Post::new(1, vec![steady(0)]) + .thresholds(serde_json::json!({ "models": {}, "reset": true })), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "an empty map: {body}"); +} + +/// A project holding three thresholds: a bare one, a named one, and a filtered one, +/// each with a model. +async fn three_thresholds(server: &TestServer, label: &str) -> (Fixture, i32) { + let fixture = fixture(server, label).await; + report( + server, + &fixture, + Post::new(1, vec![steady(0)]).thresholds(serde_json::json!({ + "models": [ + { "measure": "latency", "model": model() }, + { "measure": "latency", "metric": "p99", "model": model() }, + { + "parameters": [{ "size_mb": 16 }], + "measure": "latency", + "model": model(), + }, + ] + })), + ) + .await; + let project_id = get_project_id(server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![ + ("value".to_owned(), "*".to_owned(), true), + ("p99".to_owned(), "*".to_owned(), true), + ("value".to_owned(), r#"[{"size_mb":16}]"#.to_owned(), true), + ], + "the fixture holds one threshold of each kind" + ); + drop(conn); + (fixture, project_id) +} + +// `reset` reaches as far as the payload's shape can address and no further. A +// version 0 map can only name the bare threshold, so that is the only model it +// takes away: a legacy pipeline cannot strip a threshold it has no way to spell. +#[tokio::test] +async fn v0_reset_leaves_the_named_and_filtered_thresholds_standing() { + let server = TestServer::new().await; + let (fixture, project_id) = three_thresholds(&server, "v0reset").await; + + report( + &server, + &fixture, + Post::new(2, vec![v0(11.0)]) + .v0() + .thresholds(serde_json::json!({ "reset": true })), + ) + .await; + + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![ + ("value".to_owned(), "*".to_owned(), false), + ("p99".to_owned(), "*".to_owned(), true), + ("value".to_owned(), r#"[{"size_mb":16}]"#.to_owned(), true), + ], + "only the bare threshold is addressable at version 0" + ); +} + +// A version 1 list addresses every identity, so `reset` reaches every threshold it +// did not name. +#[tokio::test] +async fn v1_reset_strips_what_the_entries_did_not_name() { + let server = TestServer::new().await; + let (fixture, project_id) = three_thresholds(&server, "v1reset").await; + + report( + &server, + &fixture, + Post::new(2, vec![steady(1)]).thresholds(serde_json::json!({ + "models": [{ "measure": "latency", "metric": "p99", "model": model() }], + "reset": true, + })), + ) + .await; + + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![ + ("value".to_owned(), "*".to_owned(), false), + ("p99".to_owned(), "*".to_owned(), true), + ("value".to_owned(), r#"[{"size_mb":16}]"#.to_owned(), false), + ], + "the named threshold keeps its model and the rest lose theirs" + ); +} + +// A version 1 payload that names nothing at all still addresses everything, so a +// bare `reset` strips every threshold on the branch and testbed. +#[tokio::test] +async fn v1_reset_without_entries_strips_every_threshold() { + let server = TestServer::new().await; + let (fixture, project_id) = three_thresholds(&server, "v1resetall").await; + + report( + &server, + &fixture, + Post::new(2, vec![steady(1)]).thresholds(serde_json::json!({ "reset": true })), + ) + .await; + + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![ + ("value".to_owned(), "*".to_owned(), false), + ("p99".to_owned(), "*".to_owned(), false), + ("value".to_owned(), r#"[{"size_mb":16}]"#.to_owned(), false), + ], + "a list that names nothing still addresses everything" + ); +} + +// A threshold on another branch or testbed is not on this report's, so `reset` at +// version 1 does not reach it either. +#[tokio::test] +async fn v1_reset_stays_on_the_report_branch_and_testbed() { + let server = TestServer::new().await; + let fixture = fixture(&server, "resetscope").await; + + let body = serde_json::json!({ + "branch": "other", + "testbed": "localhost", + "start_time": "2024-01-01T00:00:00Z", + "end_time": "2024-01-01T00:01:00Z", + "results": [steady(0)], + "bmf_version": 1, + "thresholds": { "models": [{ "measure": "latency", "model": model() }] }, + }); + let resp = server + .client + .post(server.api_url(&format!("/v0/projects/{}/reports", fixture.project_slug))) + .header( + bencher_json::AUTHORIZATION, + bencher_json::bearer_header(&fixture.token), + ) + .json(&body) + .send() + .await + .expect("Request failed"); + assert_eq!( + resp.status(), + StatusCode::CREATED, + "POST the other branch's report" + ); + + report( + &server, + &fixture, + Post::new(2, vec![steady(1)]).thresholds(serde_json::json!({ "reset": true })), + ) + .await; + + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![("value".to_owned(), "*".to_owned(), true)], + "the other branch's threshold keeps its model" + ); +} + +// A version 0 map and `reset` in one payload, which is what a pipeline running +// `bencher run --thresholds-reset` sends today. The map updates the bare threshold +// of the measure it names, `reset` takes the model away from the bare threshold it +// did not name, and the named and filtered thresholds it cannot address are left +// exactly as they were. +#[tokio::test] +async fn v0_map_with_reset_updates_what_it_names_and_strips_the_rest() { + let server = TestServer::new().await; + let fixture = fixture(&server, "v0mapreset").await; + + report( + &server, + &fixture, + Post::new(1, vec![steady(0)]).thresholds(serde_json::json!({ + "models": [ + { "measure": "latency", "model": model() }, + { "measure": "throughput", "model": model() }, + { "measure": "latency", "metric": "p99", "model": model() }, + { + "parameters": [{ "size_mb": 16 }], + "measure": "latency", + "model": model(), + }, + ] + })), + ) + .await; + + let project_id = get_project_id(&server, fixture.project_slug.as_ref()); + let mut conn = server.db_conn(); + let latency = threshold_ids(&mut conn, project_id) + .first() + .copied() + .expect("the bare latency threshold"); + drop(conn); + + report( + &server, + &fixture, + Post::new(2, vec![v0(11.0)]) + .v0() + .thresholds(serde_json::json!({ + "models": { "latency": { "test": "percentage", "upper_boundary": 0.25 } }, + "reset": true, + })), + ) + .await; + + let mut conn = server.db_conn(); + assert_eq!( + thresholds(&mut conn, project_id), + vec![ + ("value".to_owned(), "*".to_owned(), true), + ("value".to_owned(), "*".to_owned(), false), + ("p99".to_owned(), "*".to_owned(), true), + ("value".to_owned(), r#"[{"size_mb":16}]"#.to_owned(), true), + ], + "the map keeps the measure it named, reset strips the bare threshold it did not, and the rest are out of reach" + ); + + let models = schema::model::table + .filter(schema::model::threshold_id.eq(latency)) + .count() + .get_result::(&mut conn) + .expect("Failed to count the models"); + assert_eq!(models, 2, "the map updated the model it named"); +} + +// What a client is told when the thresholds it sent are malformed rather than the +// wrong shape for its version. +// +// Two shapes behind one key is a place where error quality quietly dies: the obvious +// spelling, an untagged enum, buffers the input, tries each variant, and reports only +// that nothing matched, so a misspelled model test comes back as "data did not match +// any variant" instead of naming the field and the variants it could have been. The +// shape is known from the first token, so it is decided by looking rather than by +// trying, and the map's own errors and the list's own errors are what a client reads. +// +// These pin the two halves of that: the version 0 map's message is exactly the one it +// was before this layer existed, and the version 1 list's message points at the entry +// and the field inside it. +#[tokio::test] +async fn a_malformed_v0_map_names_the_field_that_is_wrong() { + let server = TestServer::new().await; + let fixture = fixture(&server, "badmap").await; + + let (status, body) = try_report( + &server, + &fixture, + Post::new(1, vec![v0(10.0)]) + .v0() + .thresholds(serde_json::json!({ "models": { "latency": { "test": "bogus" } } })), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "a bogus test: {body}"); + assert!( + body.contains("thresholds.models.latency.test"), + "the refusal names the field, not just the payload: {body}" + ); + assert!( + body.contains("unknown variant `bogus`"), + "the refusal names what was wrong with it: {body}" + ); + assert!( + body.contains("`t_test`"), + "the refusal lists what it could have been: {body}" + ); +} + +#[tokio::test] +async fn a_malformed_v1_entry_names_the_entry_and_the_field() { + let server = TestServer::new().await; + let fixture = fixture(&server, "badentry").await; + + let (status, body) = try_report( + &server, + &fixture, + Post::new(1, vec![steady(0)]).thresholds(serde_json::json!({ + "models": [{ "measure": "latency", "model": { "test": "bogus" } }] + })), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "a bogus test: {body}"); + assert!( + body.contains("thresholds.models[0].model.test"), + "the refusal names the entry and the field inside it: {body}" + ); + assert!( + body.contains("unknown variant `bogus`"), + "the refusal names what was wrong with it: {body}" + ); + + // A missing field is named the same way, by the entry it is missing from. + let (status, body) = try_report( + &server, + &fixture, + Post::new(2, vec![steady(1)]).thresholds(serde_json::json!({ + "models": [{ "model": { "test": "static" } }] + })), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "a missing measure: {body}"); + assert!( + body.contains("thresholds.models[0]") && body.contains("missing field `measure`"), + "the refusal names the entry and the field it wants: {body}" + ); +} + +// A `models` that is neither shape is refused by naming both, which is the one +// message this layer changes: the field used to accept only a map and now accepts +// either, so what it expects has to say so. +#[tokio::test] +async fn a_models_field_that_is_neither_shape_names_both() { + let server = TestServer::new().await; + let fixture = fixture(&server, "neither").await; + + let (status, body) = try_report( + &server, + &fixture, + Post::new(1, vec![steady(0)]).thresholds(serde_json::json!({ "models": 7 })), + ) + .await; + assert_eq!(status, StatusCode::BAD_REQUEST, "neither shape: {body}"); + assert!( + body.contains("thresholds.models") && body.contains("invalid type: integer `7`"), + "the refusal names the field and what arrived: {body}" + ); + assert!( + body.contains("a map of measure to threshold model") + && body.contains("a list of threshold entries"), + "the refusal names both shapes it would have taken: {body}" + ); +} diff --git a/lib/api_projects/tests/variants.rs b/lib/api_projects/tests/variants.rs index c9cd784c5c..97810fe9a7 100644 --- a/lib/api_projects/tests/variants.rs +++ b/lib/api_projects/tests/variants.rs @@ -45,6 +45,26 @@ fn threshold_models() -> serde_json::Value { }) } +/// The version 1 spelling of [`threshold_models`]: one entry naming the same measure +/// and the same model, which is the bare threshold, the conventional `value` name of +/// every variant. A version 1 payload has to declare its thresholds as a list. +fn threshold_entries() -> serde_json::Value { + serde_json::json!({ + "models": [ + { + "measure": "latency", + "model": { + "test": "t_test", + "min_sample_size": 2, + "max_sample_size": 64, + "lower_boundary": 0.98, + "upper_boundary": 0.98, + } + } + ] + }) +} + /// A signed up user with an organization and a project to report into. struct Fixture { project_slug: String, @@ -131,6 +151,12 @@ async fn try_report( (status, body) } +/// One BMF v0 payload: a benchmark, a measure, and a point estimate. +fn v0(benchmark: &str, value: f64) -> String { + serde_json::to_string(&serde_json::json!({ benchmark: { "latency": { "value": value } } })) + .expect("the results serialize") +} + /// One BMF v1 payload for a single benchmark's variants. fn v1(benchmark: &str, entries: &[serde_json::Value]) -> String { serde_json::to_string(&serde_json::json!({ benchmark: entries })) @@ -369,7 +395,7 @@ async fn ingest_variants(server: &TestServer) -> (Fixture, i32) { ), ], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -459,7 +485,7 @@ async fn baselines_separate_by_variant() { ), ], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -496,7 +522,7 @@ async fn bare_threshold_checks_only_the_value_name() { }), )], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -944,7 +970,7 @@ async fn report_response_echoes_metrics_and_separates_variants() { ), ], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -974,7 +1000,7 @@ async fn report_response_echoes_metrics_and_separates_variants() { ), ], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) .await; @@ -1449,7 +1475,7 @@ async fn alert_json_carries_the_boundary_the_metric_exceeded() { &serde_json::json!({ "latency": { "value": value, "p99": value * 2.0 } }), )], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -1676,7 +1702,7 @@ async fn ingest_two_variants( ), ], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -2009,7 +2035,7 @@ async fn alert_for_bounds( &measures(value), )], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -3327,7 +3353,7 @@ async fn every_matching_threshold_fires() { &serde_json::json!({ "latency": { "value": FILTERED[0] } }), )], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -3358,7 +3384,7 @@ async fn every_matching_threshold_fires() { &serde_json::json!({ "latency": { "value": value } }), )], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -3657,7 +3683,7 @@ async fn metric_row_singular_check_is_the_bare_one() { &serde_json::json!({ "latency": { "value": value } }), )], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -3832,6 +3858,9 @@ fn threshold_with( // threshold it addresses is the bare one. `reset` takes a model away from the bare // thresholds it did not name, and from no others: a threshold that checks only some // variants is addressed through the thresholds endpoint, so a report cannot reset it. +// +// The payload is version 0, which is the shape a map is: it is the map's reach that +// is under test, not the results. #[tokio::test] async fn reset_leaves_a_filtered_threshold_alone() { let server = TestServer::new().await; @@ -3842,16 +3871,10 @@ async fn reset_leaves_a_filtered_threshold_alone() { &server, &fixture, 1, - vec![v1( - "bench", - &[entry( - &serde_json::json!({ "size": 512 }), - &serde_json::json!({ "latency": { "value": FILTERED[0] } }), - )], - )], + vec![v0("bench", FILTERED[0])], Some(threshold_models()), None, - Some(1), + None, ) .await; create_threshold( @@ -3874,16 +3897,10 @@ async fn reset_leaves_a_filtered_threshold_alone() { &server, &fixture, 2, - vec![v1( - "bench", - &[entry( - &serde_json::json!({ "size": 512 }), - &serde_json::json!({ "latency": { "value": FILTERED[1] } }), - )], - )], + vec![v0("bench", FILTERED[1])], Some(serde_json::json!({ "reset": true })), None, - Some(1), + None, ) .await; @@ -3948,7 +3965,7 @@ async fn start_point_clone_carries_what_each_threshold_checks() { &serde_json::json!({ "latency": { "value": FILTERED[0] } }), )], )], - Some(threshold_models()), + Some(threshold_entries()), None, Some(1), ) @@ -3988,3 +4005,237 @@ async fn start_point_clone_carries_what_each_threshold_checks() { let source = list_thresholds(&server, &fixture, Some("main")).await; assert_eq!(source.len(), 2, "the start point is unchanged: {source:?}"); } + +/// Delete a threshold through the thresholds endpoint. +async fn delete_threshold( + server: &TestServer, + fixture: &Fixture, + threshold: &str, +) -> (StatusCode, String) { + let resp = server + .client + .delete(server.api_url(&format!( + "/v0/projects/{}/thresholds/{threshold}", + fixture.project_slug + ))) + .header( + bencher_json::AUTHORIZATION, + bencher_json::bearer_header(&fixture.token), + ) + .send() + .await + .expect("Request failed"); + let status = resp.status(); + let body = resp.text().await.expect("Failed to read the response"); + (status, body) +} + +/// The UUID a threshold response carries. +fn threshold_uuid(threshold: &serde_json::Value) -> String { + threshold + .get("uuid") + .and_then(serde_json::Value::as_str) + .expect("the threshold carries its uuid") + .to_owned() +} + +/// One report of one variant, taken back out again. +/// +/// A variant is only ever minted by a report, and a report that still references +/// it refuses the delete on its own. Deleting the report leaves the variant behind +/// with nothing pointing at it, which is the state where a threshold's claim on it +/// is the only thing left to see. +async fn unreferenced_variant( + server: &TestServer, + fixture: &Fixture, + parameters_value: &serde_json::Value, +) -> (String, JsonVariant) { + let json_report = report( + server, + fixture, + 1, + vec![v1( + "bench", + &[entry( + parameters_value, + &serde_json::json!({ "latency": { "value": 1.0 } }), + )], + )], + None, + None, + Some(1), + ) + .await; + let report_uuid = json_report + .get("uuid") + .and_then(serde_json::Value::as_str) + .expect("the report carries its uuid") + .to_owned(); + + let benchmark = only_benchmark(server, fixture).await; + let wanted = + parameters(&serde_json::to_string(parameters_value).expect("the parameters serialize")); + let variant = variant_list(server, fixture, &benchmark, "") + .await + .into_iter() + .find(|variant| variant.parameters == wanted) + .expect("the reported variant"); + + let (status, body) = delete_report(server, fixture, &report_uuid).await; + assert_eq!(status, StatusCode::NO_CONTENT, "DELETE report: {body}"); + + (benchmark, variant) +} + +// A threshold that names a variant in its filter is a reference to it, so the +// variant cannot be deleted out from under it. Deleting the threshold is what makes +// the variant deletable, exactly as deleting a report is one level up. +#[tokio::test] +async fn variant_delete_refuses_while_a_threshold_names_it() { + let server = TestServer::new().await; + let fixture = fixture(&server, "delete-threshold").await; + let (benchmark, variant) = + unreferenced_variant(&server, &fixture, &serde_json::json!({ "size_mb": 16 })).await; + + let threshold = create_threshold( + &server, + &fixture, + None, + Some(serde_json::json!([{ "size_mb": 16 }])), + ) + .await; + + let (status, body) = + delete_variant(&server, &fixture, &benchmark, &fixture.token, &variant.uuid).await; + assert_eq!( + status, + StatusCode::CONFLICT, + "a threshold names the variant: {body}" + ); + assert!( + body.contains("All thresholds that use this variant must be deleted first!"), + "the refusal says what to delete first: {body}" + ); + + let mut conn = server.db_conn(); + assert!( + variant_row_id(&mut conn, &variant.uuid).is_some(), + "the refused delete put the variant back" + ); + drop(conn); + + let (status, body) = delete_threshold(&server, &fixture, &threshold_uuid(&threshold)).await; + assert_eq!(status, StatusCode::NO_CONTENT, "DELETE threshold: {body}"); + + let (status, body) = + delete_variant(&server, &fixture, &benchmark, &fixture.token, &variant.uuid).await; + assert_eq!( + status, + StatusCode::NO_CONTENT, + "nothing names the variant any more: {body}" + ); + + let mut conn = server.db_conn(); + assert!( + variant_row_id(&mut conn, &variant.uuid).is_none(), + "the variant is gone" + ); +} + +// Matching a variant is not naming it. A filter of `{"size_mb": 16}` matches the variant +// `{"size_mb": 16, "os": "linux"}` because a filter names only the keys it +// cares about, but it is a predicate over values rather than a reference to that +// row: the variant can go and the filter still says what it said. +#[tokio::test] +async fn variant_delete_allows_a_filter_that_only_matches_it() { + let server = TestServer::new().await; + let fixture = fixture(&server, "delete-subset").await; + let (benchmark, variant) = unreferenced_variant( + &server, + &fixture, + &serde_json::json!({ "os": "linux", "size_mb": 16 }), + ) + .await; + + create_threshold( + &server, + &fixture, + None, + Some(serde_json::json!([{ "size_mb": 16 }])), + ) + .await; + + let (status, body) = + delete_variant(&server, &fixture, &benchmark, &fixture.token, &variant.uuid).await; + assert_eq!( + status, + StatusCode::NO_CONTENT, + "a filter that merely matches does not stand in the way: {body}" + ); + + let mut conn = server.db_conn(); + assert!( + variant_row_id(&mut conn, &variant.uuid).is_none(), + "the variant is gone" + ); +} + +// When a report and a threshold both point at a variant, the report is the one the +// refusal names. The results have to go first either way, and telling a client +// about the threshold while its reports still reference the variant would send it +// to the wrong place. +#[tokio::test] +async fn variant_delete_reports_the_report_reference_first() { + let server = TestServer::new().await; + let fixture = fixture(&server, "delete-precedence").await; + + report( + &server, + &fixture, + 1, + vec![v1( + "bench", + &[entry( + &serde_json::json!({ "size_mb": 16 }), + &serde_json::json!({ "latency": { "value": 1.0 } }), + )], + )], + None, + None, + Some(1), + ) + .await; + + let benchmark = only_benchmark(&server, &fixture).await; + let variant = variant_list(&server, &fixture, &benchmark, "") + .await + .into_iter() + .find(|variant| variant.parameters == parameters(r#"{"size_mb":16}"#)) + .expect("the reported variant"); + + create_threshold( + &server, + &fixture, + None, + Some(serde_json::json!([{ "size_mb": 16 }])), + ) + .await; + + let (status, body) = + delete_variant(&server, &fixture, &benchmark, &fixture.token, &variant.uuid).await; + assert_eq!( + status, + StatusCode::CONFLICT, + "both point at the variant: {body}" + ); + assert!( + !body.contains("All thresholds that use this variant must be deleted first!"), + "the report reference is the one that fires: {body}" + ); + + let mut conn = server.db_conn(); + assert!( + variant_row_id(&mut conn, &variant.uuid).is_some(), + "the variant is still there" + ); +} diff --git a/lib/bencher_json/src/project/report.rs b/lib/bencher_json/src/project/report.rs index fcaafce885..ecc292e856 100644 --- a/lib/bencher_json/src/project/report.rs +++ b/lib/bencher_json/src/project/report.rs @@ -4,14 +4,14 @@ use bencher_valid::{BmfVersion, DateTime, DateTimeMillis, GitHash, MetricName, M use ordered_float::OrderedFloat; #[cfg(feature = "schema")] use schemars::JsonSchema; -use serde::{Deserialize, Serialize}; +use serde::{Deserialize, Deserializer, Serialize, de, de::Visitor}; #[cfg(feature = "plus")] use crate::runner::job::JobUuid; use crate::{ BranchNameId, JsonAlert, JsonBenchmark, JsonBoundary, JsonBranch, JsonMeasure, JsonMetricTriple, JsonProject, JsonPubUser, JsonTestbed, MeasureNameId, MetricUuid, - ParameterSet, TestbedNameId, VariantUuid, + ParameterFilter, ParameterSet, TestbedNameId, VariantUuid, urlencoded::{UrlEncodedError, from_urlencoded, to_urlencoded}, }; @@ -65,16 +65,180 @@ pub struct JsonNewReport { #[derive(Debug, Clone, Serialize, Deserialize)] #[cfg_attr(feature = "schema", derive(JsonSchema))] pub struct JsonReportThresholds { - /// Map of measure UUID, slug, or name to the threshold model to use. + /// The thresholds to create or update for the report's branch and testbed. + /// At BMF version 0 this is a map of measure UUID, slug, or name to the threshold model to use. + /// At BMF version 1 this is a list of threshold entries. /// If a measure name or slug is provided, the measure will be created if it does not exist. - pub models: Option>, + pub models: Option, /// Reset all thresholds for the branch and testbed. - /// Any models present in the `models` field will still be updated accordingly. + /// Any thresholds present in the `models` field will still be updated accordingly. /// If a threshold already exists and is not present in the `models` field, /// its current model will be removed. + /// The payload's BMF version decides which thresholds this can reach: + /// a version 0 map can only address bare thresholds, so only bare thresholds are reset, + /// while a version 1 list can address every threshold, so every threshold is reset. pub reset: Option, } +/// The thresholds a report declares, in the shape its BMF version spells them. +/// +/// The two shapes are not interchangeable: the version the payload declares is what +/// says which one is expected, and the other one is a bad request. +#[derive(Debug, Clone, Serialize)] +#[serde(untagged)] +pub enum JsonReportThresholdModels { + /// BMF version 0: a map of measure to model. + /// + /// A map key names a measure and nothing else, so the threshold it addresses is + /// the bare one: the conventional `value` name of every variant. + Map(HashMap), + /// BMF version 1: a list of entries. + /// + /// An entry names everything a threshold checks, so one measure may carry several + /// of them. + List(Vec), +} + +impl JsonReportThresholdModels { + /// Whether this declares no threshold at all. + #[must_use] + pub fn is_empty(&self) -> bool { + match self { + Self::Map(models) => models.is_empty(), + Self::List(entries) => entries.is_empty(), + } + } +} + +/// The shape is known from the first token, `{` or `[`, so the two are told apart +/// by looking once rather than by trying one and then the other. +/// +/// This is written out instead of derived because `#[serde(untagged)]` swallows +/// what went wrong inside the shape it chose. An untagged enum buffers the input, +/// tries each variant, and on failure reports only that nothing matched, so a +/// misspelled model test in a version 0 map comes back as "data did not match any +/// variant" rather than as the field and the variants it could have been. Every +/// error a client used to get is an error it still gets, because the map's own +/// deserialize and the list's own deserialize are what run. +impl<'de> Deserialize<'de> for JsonReportThresholdModels { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + deserializer.deserialize_any(JsonReportThresholdModelsVisitor) + } +} + +struct JsonReportThresholdModelsVisitor; + +impl<'de> Visitor<'de> for JsonReportThresholdModelsVisitor { + type Value = JsonReportThresholdModels; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str( + "a map of measure to threshold model (BMF version 0) or a list of threshold entries (BMF version 1)", + ) + } + + fn visit_map(self, map: A) -> Result + where + A: de::MapAccess<'de>, + { + HashMap::deserialize(de::value::MapAccessDeserializer::new(map)) + .map(JsonReportThresholdModels::Map) + } + + fn visit_seq(self, seq: A) -> Result + where + A: de::SeqAccess<'de>, + { + Vec::deserialize(de::value::SeqAccessDeserializer::new(seq)) + .map(JsonReportThresholdModels::List) + } +} + +/// One threshold a BMF version 1 report declares. +/// +/// The fields are the dimensions a threshold hangs off that the report does not +/// already state, in their canonical order, and the model to check with. +#[derive(Debug, Clone, Serialize, Deserialize)] +#[cfg_attr(feature = "schema", derive(JsonSchema))] +pub struct JsonReportThresholdEntry { + /// The variants this threshold checks, as a parameters filter. + /// A variant matches when any entry in the filter is a subset of its parameters. + /// If not set, or set to an empty list, the threshold checks every variant. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub parameters: Option, + /// Measure UUID, slug, or name. + /// If a measure name or slug is provided, the measure will be created if it does not exist. + pub measure: MeasureNameId, + /// The name of the metric this threshold checks. + /// If not set, the threshold checks the conventional `value` name. + /// A threshold always checks exactly one name. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub metric: Option, + /// The threshold model to use. + pub model: Model, +} + +/// The description a derived schema would have taken from the doc comment, spelled +/// out because a hand written schema takes nothing from it. +#[cfg(feature = "schema")] +const MODELS_DESCRIPTION: &str = "The thresholds a report declares, in the shape its BMF version spells them.\n\nThe two shapes are not interchangeable: the version the payload declares is what says which one is expected, and the other one is a bad request."; + +/// The two shapes are mutually exclusive, so they are a `oneOf` rather than the +/// `anyOf` a derived untagged enum would emit. A client generator turns an `anyOf` +/// of two objects into one struct of flattened optional members, and a list cannot +/// be flattened into a struct. +#[cfg(feature = "schema")] +impl JsonSchema for JsonReportThresholdModels { + fn schema_name() -> String { + "JsonReportThresholdModels".to_owned() + } + + fn json_schema(generator: &mut schemars::r#gen::SchemaGenerator) -> schemars::schema::Schema { + use schemars::schema::{Metadata, SchemaObject, SubschemaValidation}; + + fn described( + title: &str, + description: &str, + schema: schemars::schema::Schema, + ) -> schemars::schema::Schema { + let mut schema = SchemaObject::from(schema); + schema.metadata = Some(Box::new(Metadata { + title: Some(title.to_owned()), + description: Some(description.to_owned()), + ..Default::default() + })); + schema.into() + } + + SchemaObject { + metadata: Some(Box::new(Metadata { + description: Some(MODELS_DESCRIPTION.to_owned()), + ..Default::default() + })), + subschemas: Some(Box::new(SubschemaValidation { + one_of: Some(vec![ + described( + "Map", + "BMF version 0: a map of measure to model. A map key names a measure and nothing else, so the threshold it addresses is the bare one: the conventional `value` name of every variant.", + >::json_schema(generator), + ), + described( + "List", + "BMF version 1: a list of entries. An entry names everything a threshold checks, so one measure may carry several of them.", + >::json_schema(generator), + ), + ]), + ..Default::default() + })), + ..Default::default() + } + .into() + } +} + #[derive(Debug, Clone, Copy, Default, Serialize, Deserialize)] #[cfg_attr(feature = "schema", derive(JsonSchema))] pub struct JsonReportSettings { diff --git a/lib/bencher_schema/src/model/project/report/mod.rs b/lib/bencher_schema/src/model/project/report/mod.rs index 01b6abc472..e0b5908ceb 100644 --- a/lib/bencher_schema/src/model/project/report/mod.rs +++ b/lib/bencher_schema/src/model/project/report/mod.rs @@ -99,7 +99,7 @@ impl NewRunJob { use super::{ branch::{BranchId, QueryBranch, head::HeadId, version::VersionId}, - threshold::{InsertThreshold, boundary::QueryBoundary}, + threshold::{InsertThreshold, boundary::QueryBoundary, check_report_thresholds_shape}, }; pub mod report_benchmark; @@ -181,6 +181,13 @@ impl QueryReport { return existing.into_json(log, actor_conn!(context, api_actor), ReportMode::Full); } + // The report's BMF version, declared or else the project's default, says + // which shape the payload's thresholds are written in, and that is checked + // before anything at all is created for the report, so a payload turned away + // leaves nothing behind on its way out. + let bmf_version = json_report.bmf_version.unwrap_or(query_project.bmf_version); + check_report_thresholds_shape(bmf_version, json_report.thresholds.as_ref())?; + #[cfg(all(feature = "plus", not(feature = "otel")))] let _ = is_claimed; #[cfg(all(feature = "plus", feature = "otel"))] @@ -219,20 +226,23 @@ impl QueryReport { ) .await?; - // Insert the thresholds for the report + // Insert the thresholds for the report. + // + // The report's BMF version says which shape the thresholds are written in, + // and the shape has already been checked above. InsertThreshold::from_report_json( log, context, project_id, branch_id, testbed_id, + bmf_version, json_report.thresholds.take(), ) .await?; let json_settings = json_report.settings.take().unwrap_or_default(); let adapter = json_settings.adapter.unwrap_or_default().normalize(); - let bmf_version = json_report.bmf_version.unwrap_or(query_project.bmf_version); // Validate job before inserting report so that report + job creation is atomic: // if OCI resolution fails, neither the report nor the job is created. diff --git a/lib/bencher_schema/src/model/project/threshold/mod.rs b/lib/bencher_schema/src/model/project/threshold/mod.rs index aa13bc0a17..36817ef8f3 100644 --- a/lib/bencher_schema/src/model/project/threshold/mod.rs +++ b/lib/bencher_schema/src/model/project/threshold/mod.rs @@ -1,9 +1,9 @@ use std::collections::HashMap; use bencher_json::{ - DateTime, MetricName, Model, ParameterFilter, ParameterSet, ThresholdUuid, + BmfVersion, DateTime, MetricName, Model, ParameterFilter, ParameterSet, ThresholdUuid, project::{ - report::JsonReportThresholds, + report::{JsonReportThresholdEntry, JsonReportThresholdModels, JsonReportThresholds}, threshold::{JsonThreshold, JsonThresholdModel}, }, }; @@ -26,8 +26,8 @@ use crate::{ auth_conn, context::{ApiContext, DbConnection}, error::{ - BencherResource, assert_parentage, assert_siblings, resource_conflict_error, - resource_not_found_err, + BencherResource, assert_parentage, assert_siblings, bad_request_error, + resource_conflict_error, resource_not_found_err, }, macros::{ fn_get::{fn_get, fn_get_id, fn_get_uuid}, @@ -443,7 +443,7 @@ enum StartPointAction { } enum ThresholdAction { - Create(MeasureId, Model), + Create(MeasureId, ThresholdIdentity, Model), Update(QueryThreshold, Model), NoChange, } @@ -761,6 +761,12 @@ impl InsertThreshold { Ok((actions, orphans)) } + /// The thresholds a report declares, created or updated for its branch and its + /// testbed. + /// + /// The report's BMF version, declared or taken from the project, says which + /// shape the thresholds are written in, and it is the version rather than the + /// shape that decides: a payload in the other version's shape is refused. #[expect( clippy::too_many_lines, reason = "Batch threshold processing with rate limiting" @@ -771,6 +777,7 @@ impl InsertThreshold { project_id: ProjectId, branch_id: BranchId, testbed_id: TestbedId, + bmf_version: BmfVersion, json_thresholds: Option, ) -> Result<(), HttpError> { #[cfg(feature = "plus")] @@ -780,73 +787,87 @@ impl InsertThreshold { slog::debug!(log, "No thresholds in report"); return Ok(()); }; - let no_models = json_thresholds - .models + let JsonReportThresholds { models, reset } = json_thresholds; + // Ingest already refused a mismatched shape before it created anything. + // This keeps the check with the code that relies on it, + // so the function is sound on its own and an empty map at version 1 is + // refused for the same reason a full one is. + check_models_shape(bmf_version, models.as_ref())?; + + let reset_thresholds = reset.unwrap_or_default(); + if models .as_ref() - .is_none_or(HashMap::is_empty); - let reset_thresholds = json_thresholds.reset.unwrap_or_default(); - if no_models && !reset_thresholds { + .is_none_or(JsonReportThresholdModels::is_empty) + && !reset_thresholds + { slog::debug!(log, "No threshold models or reset in report"); return Ok(()); } // Get all thresholds for the report branch and testbed (read phase). // - // The map a report carries names a measure and a model and nothing else, so - // the threshold it addresses is the bare one: the `value` name of every - // variant. A threshold that checks a name or only some variants is addressed - // through the thresholds endpoint, so it is not what this updates and not - // what `reset` takes a model away from. - let mut current_thresholds = schema::threshold::table + // What the payload can address is what `reset` can reach. A version 0 map + // key names a measure and nothing else, so the threshold it addresses is the + // bare one: the `value` name of every variant. A threshold that checks a + // name or only some variants is not expressible in that shape, so a legacy + // run cannot strip one it cannot even name. A version 1 entry names + // everything a threshold checks, so the list reaches every threshold on the + // branch and testbed and `reset` reaches them all with it. + let mut current_query = schema::threshold::table .filter(schema::threshold::project_id.eq(project_id)) .filter(schema::threshold::branch_id.eq(branch_id)) .filter(schema::threshold::testbed_id.eq(testbed_id)) - .filter(schema::threshold::metric.is_null()) - .filter(schema::threshold::parameters.is_null()) + .into_boxed(); + if bmf_version == BmfVersion::V0 { + current_query = current_query + .filter(schema::threshold::metric.is_null()) + .filter(schema::threshold::parameters.is_null()); + } + let mut current_thresholds = current_query .load::(auth_conn!(context)) .map_err(resource_not_found_err!(Threshold, (branch_id, testbed_id)))? .into_iter() - .map(|threshold| (threshold.measure_id, threshold)) + .map(|threshold| ((threshold.measure_id, threshold.identity()), threshold)) .collect::>(); slog::debug!(log, "Current thresholds: {current_thresholds:?}"); // Phase 1: Pre-resolve all measure IDs (may trigger get_or_create writes) // and read current model state. + let declared = Self::declared_thresholds(log, context, project_id, models).await?; let auth_conn = auth_conn!(context); let mut actions = Vec::new(); - if let Some(models) = json_thresholds.models { - for (measure, model) in models { - let measure_id = QueryMeasure::get_or_create(context, project_id, &measure).await?; - slog::debug!(log, "Processing threshold for measure {measure_id}"); - if let Some(current_threshold) = current_thresholds.remove(&measure_id) { - match QueryThreshold::compute_model_action( - auth_conn, - current_threshold.model_id, - Some(model), - )? { - ThresholdModelAction::Update(model) => { - #[cfg(feature = "plus")] - InsertModel::rate_limit(context, ¤t_threshold).await?; - slog::debug!(log, "Updating threshold for measure {measure_id}"); - actions.push(ThresholdAction::Update(current_threshold, model)); - }, - ThresholdModelAction::NoChange => { - slog::debug!(log, "Model unchanged for measure {measure_id}"); - actions.push(ThresholdAction::NoChange); - }, - // Cannot happen: we always pass Some(model) as new_model. - ThresholdModelAction::Remove => { - return Err(crate::error::issue_error( - "Unexpected threshold model removal", - "compute_model_action returned Remove with Some(model) input for measure:", - measure_id, - )); - }, - } - } else { - slog::debug!(log, "Creating threshold for measure {measure_id}"); - actions.push(ThresholdAction::Create(measure_id, model)); + for ((measure_id, identity), model) in declared { + slog::debug!(log, "Processing threshold for measure {measure_id}"); + if let Some(current_threshold) = + current_thresholds.remove(&(measure_id, identity.clone())) + { + match QueryThreshold::compute_model_action( + auth_conn, + current_threshold.model_id, + Some(model), + )? { + ThresholdModelAction::Update(model) => { + #[cfg(feature = "plus")] + InsertModel::rate_limit(context, ¤t_threshold).await?; + slog::debug!(log, "Updating threshold for measure {measure_id}"); + actions.push(ThresholdAction::Update(current_threshold, model)); + }, + ThresholdModelAction::NoChange => { + slog::debug!(log, "Model unchanged for measure {measure_id}"); + actions.push(ThresholdAction::NoChange); + }, + // Cannot happen: we always pass Some(model) as new_model. + ThresholdModelAction::Remove => { + return Err(crate::error::issue_error( + "Unexpected threshold model removal", + "compute_model_action returned Remove with Some(model) input for measure:", + measure_id, + )); + }, } + } else { + slog::debug!(log, "Creating threshold for measure {measure_id}"); + actions.push(ThresholdAction::Create(measure_id, identity, model)); } } @@ -868,7 +889,7 @@ impl InsertThreshold { write_transaction!(context, |conn| { for action in actions { match action { - ThresholdAction::Create(measure_id, model) => { + ThresholdAction::Create(measure_id, identity, model) => { InsertThreshold::from_model_inner( conn, project_id, @@ -877,7 +898,7 @@ impl InsertThreshold { testbed_id, measure_id, }, - ThresholdIdentity::default(), + identity, model, )?; }, @@ -904,6 +925,121 @@ impl InsertThreshold { Ok(()) } + + /// The thresholds the payload declares, each resolved to the measure and the + /// identity it addresses. + /// + /// A measure name or slug the project has never seen is created here, which is + /// what makes this the one phase that writes before the batch. + /// + /// A version 1 list may name one identity twice, which the last entry wins: + /// declaring a threshold is a statement of what the model should be, and the + /// payload's last word on it is the one it meant. A version 0 map is left + /// exactly as it was, duplicates and all, because a JSON object cannot repeat a + /// key and two spellings of one measure in one map is not a shape this layer + /// changes. + async fn declared_thresholds( + log: &Logger, + context: &ApiContext, + project_id: ProjectId, + models: Option, + ) -> Result, HttpError> { + let mut declared = Vec::new(); + match models { + None => {}, + Some(JsonReportThresholdModels::Map(models)) => { + for (measure, model) in models { + let measure_id = + QueryMeasure::get_or_create(context, project_id, &measure).await?; + declared.push(((measure_id, ThresholdIdentity::default()), model)); + } + }, + Some(JsonReportThresholdModels::List(entries)) => { + // The model is the last entry's and the position is the first + // entry's, so a payload that names one identity twice reads as one + // entry where it was first written, carrying what it was last told. + let mut models = HashMap::new(); + let mut order = Vec::new(); + for entry in entries { + let JsonReportThresholdEntry { + parameters, + measure, + metric, + model, + } = entry; + let measure_id = + QueryMeasure::get_or_create(context, project_id, &measure).await?; + let identity = ThresholdIdentity::new(metric, parameters); + let key = (measure_id, identity); + if let Some(replaced) = models.insert(key.clone(), model) { + slog::debug!( + log, + "Threshold declared more than once in one report for measure {measure_id}, replacing model {replaced:?}" + ); + } else { + order.push(key); + } + } + for key in order { + if let Some(model) = models.remove(&key) { + declared.push((key, model)); + } + } + }, + } + Ok(declared) + } +} + +/// Refuse a report whose thresholds are not in the shape its BMF version, declared +/// or taken from the project, spells. +/// +/// Called before anything at all is created for the report: a payload that is going +/// to be turned away should not leave a branch, a testbed, or a measure behind on +/// its way out. +pub fn check_report_thresholds_shape( + bmf_version: BmfVersion, + json_thresholds: Option<&JsonReportThresholds>, +) -> Result<(), HttpError> { + check_models_shape( + bmf_version, + json_thresholds.and_then(|thresholds| thresholds.models.as_ref()), + ) +} + +/// Refuse a thresholds shape that is not the one the payload's BMF version, +/// declared or taken from the project, spells. +/// +/// The version is what discriminates, not the shape that arrived: a version 0 map +/// read at version 1 is as wrong as the reverse, and both refusals name the version +/// the report is read as and the shape it expects. +fn check_models_shape( + bmf_version: BmfVersion, + models: Option<&JsonReportThresholdModels>, +) -> Result<(), HttpError> { + let expected = if bmf_version == BmfVersion::V0 { + "a map of measure to threshold model" + } else { + "a list of threshold entries" + }; + let sent = match models { + None => return Ok(()), + Some(JsonReportThresholdModels::Map(_)) => { + if bmf_version == BmfVersion::V0 { + return Ok(()); + } + "a map" + }, + Some(JsonReportThresholdModels::List(_)) => { + if bmf_version == BmfVersion::V1 { + return Ok(()); + } + "a list" + }, + }; + Err(bad_request_error(format!( + "The report is read as BMF version {bmf_version}, so `thresholds.models` must be {expected}, but {sent} was sent." + ))) } #[derive(Debug, Clone, diesel::AsChangeset)] @@ -2427,3 +2563,40 @@ mod tests { ); } } + +#[cfg(test)] +mod shape_tests { + use bencher_json::{BmfVersion, project::report::JsonReportThresholdModels}; + + use super::check_models_shape; + + /// A payload that declares nothing about its thresholds is neither shape. + #[test] + fn absent_models_are_every_version() { + check_models_shape(BmfVersion::V0, None).expect("no models at version 0"); + check_models_shape(BmfVersion::V1, None).expect("no models at version 1"); + } + + /// Each version accepts its own shape and refuses the other's, and both + /// refusals name the version the report is read as and the shape it expects. + #[test] + fn each_version_takes_its_own_shape() { + let map = JsonReportThresholdModels::Map(std::collections::HashMap::new()); + let list = JsonReportThresholdModels::List(Vec::new()); + + check_models_shape(BmfVersion::V0, Some(&map)).expect("a map at version 0"); + check_models_shape(BmfVersion::V1, Some(&list)).expect("a list at version 1"); + + let map_at_v1 = check_models_shape(BmfVersion::V1, Some(&map)) + .expect_err("a map at version 1") + .external_message; + assert!(map_at_v1.contains('1'), "{map_at_v1}"); + assert!(map_at_v1.contains("list"), "{map_at_v1}"); + + let list_at_v0 = check_models_shape(BmfVersion::V0, Some(&list)) + .expect_err("a list at version 0") + .external_message; + assert!(list_at_v0.contains('0'), "{list_at_v0}"); + assert!(list_at_v0.contains("map"), "{list_at_v0}"); + } +} diff --git a/services/api/openapi.json b/services/api/openapi.json index 297913387a..150e390a9a 100644 --- a/services/api/openapi.json +++ b/services/api/openapi.json @@ -4508,7 +4508,7 @@ "variants" ], "summary": "Delete a variant", - "description": "Delete a variant for a benchmark. The user must have `delete` permissions for the project. All reports that use this variant must be deleted first!\n\nA benchmark's empty variant cannot be deleted. The empty variant is structural: every benchmark is born with exactly one, and report ingest treats a missing empty variant as data corruption rather than a variant to mint. Deleting it would manufacture exactly that corruption, so the request is refused. Archiving the empty variant stays allowed, because a later report revives it the same way it revives any other archived variant.", + "description": "Delete a variant for a benchmark. The user must have `delete` permissions for the project. All reports that use this variant must be deleted first! All thresholds that use this variant must be deleted first!\n\nA threshold uses a variant when its `parameters` filter names that exact set of parameters. A filter that merely matches the variant, because the variant pins every key the filter names and more besides, is a predicate over values rather than a reference to this row, and it does not stand in the way.\n\nA benchmark's empty variant cannot be deleted. The empty variant is structural: every benchmark is born with exactly one, and report ingest treats a missing empty variant as data corruption rather than a variant to mint. Deleting it would manufacture exactly that corruption, so the request is refused. Archiving the empty variant stays allowed, because a later report revives it the same way it revives any other archived variant.", "operationId": "proj_variant_delete", "parameters": [ { @@ -17066,20 +17066,86 @@ } } }, - "JsonReportThresholds": { + "JsonReportThresholdEntry": { + "description": "One threshold a BMF version 1 report declares.\n\nThe fields are the dimensions a threshold hangs off that the report does not already state, in their canonical order, and the model to check with.", "type": "object", "properties": { - "models": { + "measure": { + "description": "Measure UUID, slug, or name. If a measure name or slug is provided, the measure will be created if it does not exist.", + "allOf": [ + { + "$ref": "#/components/schemas/NameId" + } + ] + }, + "metric": { + "nullable": true, + "description": "The name of the metric this threshold checks. If not set, the threshold checks the conventional `value` name. A threshold always checks exactly one name.", + "allOf": [ + { + "$ref": "#/components/schemas/MetricName" + } + ] + }, + "model": { + "description": "The threshold model to use.", + "allOf": [ + { + "$ref": "#/components/schemas/Model" + } + ] + }, + "parameters": { "nullable": true, - "description": "Map of measure UUID, slug, or name to the threshold model to use. If a measure name or slug is provided, the measure will be created if it does not exist.", + "description": "The variants this threshold checks, as a parameters filter. A variant matches when any entry in the filter is a subset of its parameters. If not set, or set to an empty list, the threshold checks every variant.", + "allOf": [ + { + "$ref": "#/components/schemas/ParameterFilter" + } + ] + } + }, + "required": [ + "measure", + "model" + ] + }, + "JsonReportThresholdModels": { + "description": "The thresholds a report declares, in the shape its BMF version spells them.\n\nThe two shapes are not interchangeable: the version the payload declares is what says which one is expected, and the other one is a bad request.", + "oneOf": [ + { + "title": "Map", + "description": "BMF version 0: a map of measure to model. A map key names a measure and nothing else, so the threshold it addresses is the bare one: the conventional `value` name of every variant.", "type": "object", "additionalProperties": { "$ref": "#/components/schemas/Model" } }, + { + "title": "List", + "description": "BMF version 1: a list of entries. An entry names everything a threshold checks, so one measure may carry several of them.", + "type": "array", + "items": { + "$ref": "#/components/schemas/JsonReportThresholdEntry" + } + } + ] + }, + "JsonReportThresholds": { + "type": "object", + "properties": { + "models": { + "nullable": true, + "description": "The thresholds to create or update for the report's branch and testbed. At BMF version 0 this is a map of measure UUID, slug, or name to the threshold model to use. At BMF version 1 this is a list of threshold entries. If a measure name or slug is provided, the measure will be created if it does not exist.", + "allOf": [ + { + "$ref": "#/components/schemas/JsonReportThresholdModels" + } + ] + }, "reset": { "nullable": true, - "description": "Reset all thresholds for the branch and testbed. Any models present in the `models` field will still be updated accordingly. If a threshold already exists and is not present in the `models` field, its current model will be removed.", + "description": "Reset all thresholds for the branch and testbed. Any thresholds present in the `models` field will still be updated accordingly. If a threshold already exists and is not present in the `models` field, its current model will be removed. The payload's BMF version decides which thresholds this can reach: a version 0 map can only address bare thresholds, so only bare thresholds are reset, while a version 1 list can address every threshold, so every threshold is reset.", "type": "boolean" } } diff --git a/services/cli/src/bencher/sub/project/report/create/thresholds.rs b/services/cli/src/bencher/sub/project/report/create/thresholds.rs index aad25e064b..a8792ccfd8 100644 --- a/services/cli/src/bencher/sub/project/report/create/thresholds.rs +++ b/services/cli/src/bencher/sub/project/report/create/thresholds.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; -use bencher_client::types::JsonReportThresholds; +use bencher_client::types::{JsonReportThresholdModels, JsonReportThresholds}; use bencher_json::{Boundary, MeasureNameId, SampleSize, Window}; use crate::{ @@ -148,7 +148,9 @@ impl From for Option { None } else { Some(JsonReportThresholds { - models, + // The CLI declares bare thresholds, which is the map the BMF + // version 0 payload spells them in. + models: models.map(JsonReportThresholdModels::Map), reset: reset.then_some(reset), }) }