Skip to content

Commit 33cfa4a

Browse files
authored
Rename the metric triple and the parameter set field (#1009)
## The renames Three renames and the migration one of them needs. No behavior change. ### `JsonMetric` -> `JsonMetricTriple` A `JsonMetric` is a value with its two bounds: the triple a metric row used to be before named values existed. The plain name read like the whole of what a metric is, which it is not any more. This is a type rename only. Every serde field that carries one keeps its own name (`JsonAlert.metric`, `JsonPerfMetric.metric`, and the rest), so not one response byte changes. The OpenAPI component and the TypeScript type follow. ### `JsonReportParameter.parameters` -> `set` The object is already named `parameter` where a report result carries it, so the field spelled its parent again. It now spells what it holds: ```json "parameter": { "uuid": "...", "set": { "size_mb": 16 } } ``` This one does change report response bytes. It is free right now: no released CLI deserializes the report `parameter` object, so nothing in the wild reads the old spelling. Once the next release ships a client that does, the same rename becomes a break. The type name `JsonReportParameter` is unchanged. The BMF v1 **input** format is untouched. A report still submits `"parameters"` on each entry, and every adapter fixture stays as it was. ### `JsonParameters` -> `ParameterSet` The type has been called a parameter set in its own prose since it was written. It is an internal newtype with a hand written `JsonSchema` impl, so the component name is renamed there as well, and the typeshare mapping follows. ## The migration `2026-08-23-120000_parameter_set` renames the `parameter.parameters` column to `parameter.set`, and the model fields follow it (`QueryParameter.set`, `InsertParameter.set`, `UpdateParameter.set`). `set` is an SQL keyword, so the migration and the raw SQL that reads the column quote it. Diesel quotes identifiers of its own accord, so the DSL needs nothing. The migration is metadata only: ```sql ALTER TABLE parameter RENAME COLUMN parameters TO "set"; ``` SQLite rewrites the stored DDL in place, `UNIQUE(benchmark_id, "set")` and the index that backs it included, without reading a row. `down.sql` is the reverse, and just as cheap. Migrations already written keep the spelling that was true when they ran: the migrations that create and read `parameter.parameters` are history and are left alone. The one seed in the migration tests that hand writes a `parameter` row below this layer now does so in raw SQL under the old column name, which is what the schema at that point in the chain has; the chain renames it on the way back up. ## Gates - `cargo fmt -- --check` - `cargo clippy --no-deps --all-targets --all-features -- -Dwarnings` - `cargo nextest run --all-features --profile ci`, `cargo test --doc --all-features` - `cargo check --no-default-features` - `cargo gen-types`, with the regenerated spec and TypeScript types committed
1 parent 37abefb commit 33cfa4a

29 files changed

Lines changed: 248 additions & 203 deletions

File tree

lib/api_projects/tests/benchmarks.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
//! Integration tests for project benchmark endpoints.
88
99
use bencher_api_tests::{TestServer, helpers::create_empty_parameter};
10-
use bencher_json::{JsonBenchmark, JsonBenchmarks, JsonParameters};
10+
use bencher_json::{JsonBenchmark, JsonBenchmarks, ParameterSet};
1111
use bencher_schema::schema;
1212
use diesel::{
1313
ExpressionMethods as _, QueryDsl as _, RunQueryDsl as _, connection::SimpleConnection as _,
@@ -149,14 +149,14 @@ fn assert_benchmark_birth_invariant(server: &TestServer) {
149149
);
150150

151151
for benchmark_id in benchmark_ids {
152-
let parameters: Vec<JsonParameters> = schema::parameter::table
152+
let parameters: Vec<ParameterSet> = schema::parameter::table
153153
.filter(schema::parameter::benchmark_id.eq(benchmark_id))
154-
.select(schema::parameter::parameters)
154+
.select(schema::parameter::set)
155155
.load(&mut conn)
156156
.expect("Failed to load parameters");
157157
assert_eq!(
158158
parameters,
159-
vec![JsonParameters::default()],
159+
vec![ParameterSet::default()],
160160
"benchmark {benchmark_id} must have exactly one empty parameter set"
161161
);
162162
}
@@ -196,12 +196,12 @@ async fn benchmarks_create_empty_parameter_set() {
196196
.select(schema::benchmark::id)
197197
.first(&mut conn)
198198
.expect("Failed to get benchmark ID");
199-
let parameters: Vec<JsonParameters> = schema::parameter::table
199+
let parameters: Vec<ParameterSet> = schema::parameter::table
200200
.filter(schema::parameter::benchmark_id.eq(benchmark_id))
201-
.select(schema::parameter::parameters)
201+
.select(schema::parameter::set)
202202
.load(&mut conn)
203203
.expect("Failed to load parameters");
204-
assert_eq!(parameters, vec![JsonParameters::default()]);
204+
assert_eq!(parameters, vec![ParameterSet::default()]);
205205

206206
assert_benchmark_birth_invariant(&server);
207207
}
@@ -221,7 +221,7 @@ async fn benchmarks_create_rolls_back_with_parameter_set() {
221221

222222
// Poison the empty parameter set that the next benchmark will be born with.
223223
// SQLite hands an `INTEGER PRIMARY KEY` the next rowid after the largest in
224-
// use, so the row below collides on `UNIQUE(benchmark_id, parameters)` with
224+
// use, so the row below collides on `UNIQUE(benchmark_id, "set")` with
225225
// the set created inside the benchmark's own transaction. Foreign keys are
226226
// off on this connection, so it may point at a benchmark that does not exist yet.
227227
let mut conn = server.db_conn();

lib/api_projects/tests/metric_migration.rs

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,12 +19,12 @@
1919
2020
use bencher_api_tests::{
2121
TestServer,
22-
helpers::{base_timestamp, create_empty_parameter, get_project_id},
22+
helpers::{base_timestamp, get_project_id},
2323
};
2424
use bencher_json::{
2525
AlertUuid, BenchmarkUuid, BoundaryUuid, BranchUuid, HeadUuid, MeasureUuid, MetricName,
26-
MetricUuid, ModelUuid, ReportBenchmarkUuid, ReportUuid, TestbedUuid, ThresholdUuid,
27-
VersionUuid, project::alert::AlertStatus, project::boundary::BoundaryLimit,
26+
MetricUuid, ModelUuid, ParameterUuid, ReportBenchmarkUuid, ReportUuid, TestbedUuid,
27+
ThresholdUuid, VersionUuid, project::alert::AlertStatus, project::boundary::BoundaryLimit,
2828
};
2929
use bencher_schema::{MIGRATIONS, context::DbConnection, schema};
3030
use diesel::{
@@ -710,6 +710,36 @@ fn revert_migration(conn: &mut DbConnection) {
710710
.expect("Failed to enable foreign keys");
711711
}
712712

713+
/// Seed a benchmark's empty parameter set at the reverted schema point.
714+
///
715+
/// Benchmarks inserted directly into the database bypass `QueryBenchmark::create`,
716+
/// so they need the birth invariant applied by hand. The migrations are reverted
717+
/// when this runs, so the column is still spelled `parameters`: the rename to `set`
718+
/// is a layer above, and the chain puts it back on the way up. That is why this is
719+
/// raw SQL rather than `create_empty_parameter`, for the same reason the metric rows
720+
/// below are.
721+
fn seed_empty_parameter(conn: &mut DbConnection, benchmark_id: i32) -> i32 {
722+
let now = base_timestamp();
723+
724+
let parameter_uuid = ParameterUuid::new();
725+
diesel::sql_query(
726+
"INSERT INTO parameter (uuid, benchmark_id, parameters, created, modified)
727+
VALUES (?, ?, jsonb('{}'), ?, ?)",
728+
)
729+
.bind::<Text, _>(parameter_uuid.to_string())
730+
.bind::<Integer, _>(benchmark_id)
731+
.bind::<BigInt, _>(now)
732+
.bind::<BigInt, _>(now)
733+
.execute(&mut *conn)
734+
.expect("Failed to seed the empty parameter set");
735+
736+
schema::parameter::table
737+
.filter(schema::parameter::uuid.eq(&parameter_uuid))
738+
.select(schema::parameter::id)
739+
.first(&mut *conn)
740+
.expect("Failed to get the parameter id")
741+
}
742+
713743
/// The `metric_boundary` column list, in the order `view.rs` declares it.
714744
const VIEW_COLUMNS: [&str; 14] = [
715745
"metric_id",
@@ -904,7 +934,7 @@ async fn seed_legacy_project(server: &TestServer, label: &str) -> Fixture {
904934
.select(schema::benchmark::id)
905935
.first(&mut conn)
906936
.expect("Failed to get the benchmark id");
907-
let parameter_id = create_empty_parameter(&mut conn, benchmark_id);
937+
let parameter_id = seed_empty_parameter(&mut conn, benchmark_id);
908938

909939
// Two measures, so a pivot that joins on the report benchmark alone pulls a
910940
// bound across measures and fails.

lib/api_projects/tests/parameters.rs

Lines changed: 21 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ use bencher_api_tests::{
1717
TestServer,
1818
helpers::{base_timestamp, get_project_id},
1919
};
20-
use bencher_json::{DateTime, JsonParameters, MetricName, Slug};
20+
use bencher_json::{DateTime, MetricName, ParameterSet, Slug};
2121
use bencher_schema::{
2222
context::DbConnection,
2323
model::{organization::OrganizationId, project::metric::QueryMetric},
@@ -140,12 +140,12 @@ fn organization_id(conn: &mut DbConnection, project_id: i32) -> OrganizationId {
140140

141141
/// Every parameter set stored under a project, with the number of `report_benchmark`
142142
/// rows pointing at it.
143-
fn parameter_sets(conn: &mut DbConnection, project_id: i32) -> Vec<(JsonParameters, i64)> {
144-
let parameters: Vec<(i32, JsonParameters)> = schema::parameter::table
143+
fn parameter_sets(conn: &mut DbConnection, project_id: i32) -> Vec<(ParameterSet, i64)> {
144+
let parameters: Vec<(i32, ParameterSet)> = schema::parameter::table
145145
.inner_join(schema::benchmark::table)
146146
.filter(schema::benchmark::project_id.eq(project_id))
147147
.order(schema::parameter::id.asc())
148-
.select((schema::parameter::id, schema::parameter::parameters))
148+
.select((schema::parameter::id, schema::parameter::set))
149149
.load(&mut *conn)
150150
.expect("Failed to load the parameter sets");
151151

@@ -163,7 +163,7 @@ fn parameter_sets(conn: &mut DbConnection, project_id: i32) -> Vec<(JsonParamete
163163
}
164164

165165
/// Every metric name stored for a project, with its value, keyed by parameter set.
166-
fn named_values(conn: &mut DbConnection, project_id: i32) -> Vec<(JsonParameters, String, f64)> {
166+
fn named_values(conn: &mut DbConnection, project_id: i32) -> Vec<(ParameterSet, String, f64)> {
167167
schema::metric::table
168168
.inner_join(
169169
schema::report_benchmark::table
@@ -173,19 +173,19 @@ fn named_values(conn: &mut DbConnection, project_id: i32) -> Vec<(JsonParameters
173173
.filter(schema::benchmark::project_id.eq(project_id))
174174
.order((schema::parameter::id.asc(), schema::metric::name.asc()))
175175
.select((
176-
schema::parameter::parameters,
176+
schema::parameter::set,
177177
schema::metric::name,
178178
schema::metric::value,
179179
))
180-
.load::<(JsonParameters, MetricName, f64)>(&mut *conn)
180+
.load::<(ParameterSet, MetricName, f64)>(&mut *conn)
181181
.expect("Failed to load the metric rows")
182182
.into_iter()
183183
.map(|(parameters, name, value)| (parameters, name.to_string(), value))
184184
.collect()
185185
}
186186

187187
/// The parameter set and metric name of every alert in a project.
188-
fn alerts(conn: &mut DbConnection, project_id: i32) -> Vec<(JsonParameters, String)> {
188+
fn alerts(conn: &mut DbConnection, project_id: i32) -> Vec<(ParameterSet, String)> {
189189
schema::alert::table
190190
.inner_join(
191191
schema::boundary::table.inner_join(
@@ -198,8 +198,8 @@ fn alerts(conn: &mut DbConnection, project_id: i32) -> Vec<(JsonParameters, Stri
198198
)
199199
.filter(schema::benchmark::project_id.eq(project_id))
200200
.order(schema::alert::id.asc())
201-
.select((schema::parameter::parameters, schema::metric::name))
202-
.load::<(JsonParameters, MetricName)>(&mut *conn)
201+
.select((schema::parameter::set, schema::metric::name))
202+
.load::<(ParameterSet, MetricName)>(&mut *conn)
203203
.expect("Failed to load the alerts")
204204
.into_iter()
205205
.map(|(parameters, name)| (parameters, name.to_string()))
@@ -239,7 +239,7 @@ fn series_measures(conn: &mut DbConnection, project_id: i32) -> Vec<String> {
239239
.collect()
240240
}
241241

242-
fn parameters(canonical: &str) -> JsonParameters {
242+
fn parameters(canonical: &str) -> ParameterSet {
243243
canonical.parse().expect("Failed to parse the parameters")
244244
}
245245

@@ -279,7 +279,7 @@ async fn v1_report_lands_grid_points_and_named_values() {
279279
assert_eq!(
280280
parameter_sets(&mut conn, project_id),
281281
vec![
282-
(JsonParameters::default(), 0),
282+
(ParameterSet::default(), 0),
283283
(parameters(r#"{"size_mb": 16}"#), 1),
284284
(parameters(r#"{"size_mb": 32}"#), 1),
285285
],
@@ -499,7 +499,7 @@ async fn bare_threshold_gates_only_the_value_name() {
499499
let alerts = alerts(&mut conn, project_id);
500500
assert_eq!(
501501
alerts,
502-
vec![(JsonParameters::default(), "value".to_owned())],
502+
vec![(ParameterSet::default(), "value".to_owned())],
503503
"the alert is on the point estimate"
504504
);
505505
}
@@ -534,7 +534,7 @@ async fn absent_parameters_land_on_the_empty_set() {
534534

535535
assert_eq!(
536536
parameter_sets(&mut conn, project_id),
537-
vec![(JsonParameters::default(), 1)],
537+
vec![(ParameterSet::default(), 1)],
538538
"an absent parameter set and an explicit empty one are one grid point"
539539
);
540540
}
@@ -595,7 +595,7 @@ async fn v1_entry_without_measures_mints_nothing() {
595595
assert_eq!(benchmark_count(&mut conn, project_id), 1);
596596
assert_eq!(
597597
parameter_sets(&mut conn, project_id),
598-
vec![(JsonParameters::default(), 0)],
598+
vec![(ParameterSet::default(), 0)],
599599
"only the empty set the benchmark was born with, pointed at by nothing"
600600
);
601601
assert_eq!(named_values(&mut conn, project_id), vec![]);
@@ -623,7 +623,7 @@ async fn v1_benchmark_without_entries_mints_nothing() {
623623
assert_eq!(benchmark_count(&mut conn, project_id), 1);
624624
assert_eq!(
625625
parameter_sets(&mut conn, project_id),
626-
vec![(JsonParameters::default(), 0)],
626+
vec![(ParameterSet::default(), 0)],
627627
);
628628
assert_eq!(named_values(&mut conn, project_id), vec![]);
629629
assert_eq!(series_measures(&mut conn, project_id), Vec::<String>::new());
@@ -655,7 +655,7 @@ async fn v0_benchmark_without_measures_is_unchanged() {
655655
assert_eq!(benchmark_count(&mut conn, project_id), 1);
656656
assert_eq!(
657657
parameter_sets(&mut conn, project_id),
658-
vec![(JsonParameters::default(), 1)],
658+
vec![(ParameterSet::default(), 1)],
659659
"the v0 row still lands on the empty parameter set"
660660
);
661661
assert_eq!(named_values(&mut conn, project_id), vec![]);
@@ -684,7 +684,7 @@ async fn archived_parameter_set_is_unarchived_on_report() {
684684
let mut conn = server.db_conn();
685685
let updated = diesel::update(
686686
schema::parameter::table
687-
.filter(schema::parameter::parameters.eq(parameters(r#"{"size_mb": 16}"#))),
687+
.filter(schema::parameter::set.eq(parameters(r#"{"size_mb": 16}"#))),
688688
)
689689
.set(schema::parameter::archived.eq(Some(1_000_000_000i64)))
690690
.execute(&mut conn)
@@ -698,7 +698,7 @@ async fn archived_parameter_set_is_unarchived_on_report() {
698698
let archived: Vec<Option<i64>> = schema::parameter::table
699699
.inner_join(schema::benchmark::table)
700700
.filter(schema::benchmark::project_id.eq(project_id))
701-
.filter(schema::parameter::parameters.eq(parameters(r#"{"size_mb": 16}"#)))
701+
.filter(schema::parameter::set.eq(parameters(r#"{"size_mb": 16}"#)))
702702
.select(schema::parameter::archived)
703703
.load(&mut conn)
704704
.expect("Failed to load the parameter set");
@@ -978,12 +978,12 @@ async fn report_response_echoes_named_values_and_separates_grid_points() {
978978
Some(&serde_json::json!({ "benchmarks": 2, "measures": 2 })),
979979
);
980980

981-
let grid_points: Vec<JsonParameters> = results
981+
let grid_points: Vec<ParameterSet> = results
982982
.iter()
983983
.map(|result| {
984984
serde_json::from_value(
985985
result
986-
.pointer("/parameter/parameters")
986+
.pointer("/parameter/set")
987987
.expect("each result names its parameter set")
988988
.clone(),
989989
)

lib/api_projects/tests/reports.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,8 @@ use bencher_api_tests::{
1616
},
1717
};
1818
use bencher_json::{
19-
BenchmarkUuid, BoundaryUuid, JsonParameters, JsonReport, JsonReports, MeasureUuid, MetricName,
20-
MetricUuid, ModelUuid, ReportBenchmarkUuid, ThresholdUuid,
19+
BenchmarkUuid, BoundaryUuid, JsonReport, JsonReports, MeasureUuid, MetricName, MetricUuid,
20+
ModelUuid, ParameterSet, ReportBenchmarkUuid, ThresholdUuid,
2121
};
2222
use bencher_schema::{
2323
context::DbConnection,
@@ -626,14 +626,14 @@ async fn reports_ingest_empty_parameter_sets() {
626626
assert_eq!(benchmark_ids.len(), 2, "two benchmarks were ingested");
627627

628628
for benchmark_id in benchmark_ids {
629-
let parameters: Vec<JsonParameters> = schema::parameter::table
629+
let parameters: Vec<ParameterSet> = schema::parameter::table
630630
.filter(schema::parameter::benchmark_id.eq(benchmark_id))
631-
.select(schema::parameter::parameters)
631+
.select(schema::parameter::set)
632632
.load(&mut conn)
633633
.expect("Failed to load parameters");
634634
assert_eq!(
635635
parameters,
636-
vec![JsonParameters::default()],
636+
vec![ParameterSet::default()],
637637
"benchmark {benchmark_id} must have exactly one empty parameter set"
638638
);
639639
}

lib/bencher_adapter/src/adapters/json/v0.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use std::collections::HashMap;
22

3-
use bencher_json::{BenchmarkNameId, JsonNewMetric, JsonParameters, MeasureNameId};
3+
use bencher_json::{BenchmarkNameId, JsonNewMetric, MeasureNameId, ParameterSet};
44

55
use crate::{
66
Adaptable, Settings,
@@ -39,7 +39,7 @@ fn from_wire(results: JsonV0Results) -> AdapterResults {
3939
.collect::<HashMap<_, _>>();
4040
(
4141
benchmark,
42-
std::iter::once((JsonParameters::default(), metrics.into())).collect(),
42+
std::iter::once((ParameterSet::default(), metrics.into())).collect(),
4343
)
4444
})
4545
.collect::<ResultsMap>()

lib/bencher_adapter/src/adapters/json/v1.rs

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
use std::collections::HashMap;
22

3-
use bencher_json::{BenchmarkNameId, JsonParameters, MeasureNameId};
3+
use bencher_json::{BenchmarkNameId, MeasureNameId, ParameterSet};
44
use serde::Deserialize;
55

66
use crate::{
@@ -21,7 +21,7 @@ pub struct JsonV1Entry {
2121
/// Optional. An entry without it resolves to the benchmark's empty parameter set,
2222
/// which is exactly what an explicit `{}` resolves to.
2323
#[serde(default)]
24-
pub parameters: JsonParameters,
24+
pub parameters: ParameterSet,
2525
pub measures: HashMap<MeasureNameId, JsonV1Measure>,
2626
}
2727

@@ -86,7 +86,7 @@ fn from_wire(results: JsonV1Results) -> AdapterResults {
8686

8787
#[cfg(test)]
8888
pub(crate) mod test_json_v1 {
89-
use bencher_json::{BenchmarkNameId, JsonParameters, MAX_PARAMETER_KEYS, MetricName};
89+
use bencher_json::{BenchmarkNameId, MAX_PARAMETER_KEYS, MetricName, ParameterSet};
9090
use ordered_float::OrderedFloat;
9191
use pretty_assertions::assert_eq;
9292

@@ -125,7 +125,7 @@ pub(crate) mod test_json_v1 {
125125
.parse::<BenchmarkNameId>()
126126
.expect("Failed to parse benchmark name");
127127
let parameters = parameters
128-
.parse::<JsonParameters>()
128+
.parse::<ParameterSet>()
129129
.expect("Failed to parse parameters");
130130
results
131131
.inner
@@ -217,7 +217,7 @@ pub(crate) mod test_json_v1 {
217217
let merged = "tests::merged".parse::<BenchmarkNameId>().unwrap();
218218
let entries = &results.inner[&merged];
219219
assert_eq!(entries.len(), 1);
220-
let metrics = &entries[&JsonParameters::default()];
220+
let metrics = &entries[&ParameterSet::default()];
221221
assert_eq!(named(metrics, "latency", "value"), Some(1.0.into()));
222222
assert_eq!(named(metrics, "throughput", "value"), Some(2.0.into()));
223223
}

0 commit comments

Comments
 (0)