Gate any metric of any grid point, and let every matching threshold fire

A threshold gated one thing: the `value` scalar of every grid point of its
measure. A benchmark now reports as many grid points as it has parameter sets and
as many named scalars as its harness measured, and neither was addressable. A
project that wanted `p99` watched on the one configuration where it matters had
nowhere to say so.

A threshold gains two optional fields. `metric` is the name it gates, and a
threshold that names none gates the conventional `value` name: a threshold always
gates exactly one name and a bare one never gates all of them. `parameters` is a
filter over grid points, a list of parameter sets that is an OR across the list
and a subset match within each set, and a threshold with no filter gates every
grid point. Both defaults are what every existing threshold already does, so no
existing row moves and no project's alert volume changes.

Every threshold that matches a metric row runs. There is no winner: not in
gating, not in display. A grid point that a bare threshold and a filtered
threshold both match earns a boundary from each and, on a regression, an alert
from each. That is the design and it is pinned by a test, because a row that two
people asked to be watched is a row two people hear about.

Identity is the three dimensions plus the two new fields, under the null
semantics they carry. SQLite treats nulls as distinct in a unique index, so two
bare thresholds on one branch, testbed, and measure would no longer collide under
a plain unique key over the five columns. The key is declared over the effective
values instead, `COALESCE(metric, 'value')` and `COALESCE(parameters, x'')`, and
the wire canonicalizes into them: an explicit `value` and an absent name are one
threshold, an empty filter and an absent one are one threshold, and a filter has
one spelling because its sets sort by their RFC 8785 canonical bytes and
duplicates collapse.

A threshold gates the sample it names. The historical query behind detection
filters on the threshold's metric name, so a threshold on `p99` is tested against
`p99` rows and never against the `value` rows beside them, and the per grid point
separation stays exactly as it was.

`boundary` keys on `(metric_id, threshold_id)` rather than on `metric_id` alone,
because a metric row may now carry a boundary per threshold that gated it. Both
tables are rebuilt with their unique keys built after the copy, which is what
keeps the rebuild at the cost of the scan.

`JsonAlert` gains `value`, the scalar the alert fired on, and its `metric` triple
becomes optional: the triple is a convention over the `value` name, so an alert
on any other name has none. Every alert that a threshold could raise before this
carries the triple exactly as it did. The gated name is readable at
`alert.threshold.metric`.

The deprecated singular `threshold`, `boundary`, and `alert` fields carry the
bare threshold's gate and no other's, everywhere they appear. That is precisely
what a caller from before named gating has always been shown: a row that only a
named or filtered threshold gates reports no gate in them at all. Where a list of
boundaries is returned, it is ordered by threshold creation time, oldest first,
with the UUID breaking a tie.

The in-report `thresholds.models` map is unchanged and still addresses the bare
threshold: a map that names a measure and a model says nothing about a name or a
grid point, so it neither creates nor resets anything narrower.

Author: epompeii

lib/api_projects/src/metrics.rs

@@ -27,9 +27,9 @@ use bencher_schema::{
     schema,
 };
 use diesel::{
-    ExpressionMethods as _, JoinOnDsl as _, NullableExpressionMethods as _, QueryDsl as _,
-    RunQueryDsl as _, SelectableHelper as _, query_builder::QueryFragment,
-    query_dsl::methods::LoadQuery, sqlite::Sqlite,
+    BoolExpressionMethods as _, ExpressionMethods as _, JoinOnDsl as _,
+    NullableExpressionMethods as _, QueryDsl as _, RunQueryDsl as _, SelectableHelper as _,
+    query_builder::QueryFragment, query_dsl::methods::LoadQuery, sqlite::Sqlite,
 };
 use dropshot::{HttpError, Path, RequestContext, endpoint};
 use schemars::JsonSchema;
@@ -178,13 +178,17 @@ fn metric_query(
             schema::report::spec_id,
             QueryMetric::as_select(),
             (
+                // The column order is `QueryThreshold`'s field order, because that is
+                // what a tuple selection deserializes into, positionally.
                 (
                     schema::threshold::id,
                     schema::threshold::uuid,
                     schema::threshold::project_id,
-                    schema::threshold::measure_id,
                     schema::threshold::branch_id,
                     schema::threshold::testbed_id,
+                    schema::threshold::measure_id,
+                    schema::threshold::metric,
+                    schema::threshold::parameters,
                     schema::threshold::model_id,
                     schema::threshold::created,
                     schema::threshold::modified,
@@ -224,11 +228,31 @@ fn metric_query(
             )
                 .nullable(),
         ))
-        // The UUID is unique, so at most one row can match. The limit is what
-        // `first` renders, kept here because the query is built apart from its run.
+        // The bare threshold's row first, then one row, because the deprecated gate
+        // this response reports is the bare threshold's. See `metric_gate`.
+        .order(bare_threshold_first())
         .limit(1)
 }
 
+/// Sort the bare threshold's row first.
+///
+/// A metric row may carry a boundary per threshold that gated it, and this query
+/// keeps one row. The one it keeps is the bare threshold's, which is what the
+/// deprecated singular fields have always carried.
+type BareThresholdFirst = diesel::dsl::Desc<
+    diesel::dsl::And<
+        diesel::dsl::IsNull<schema::threshold::metric>,
+        diesel::dsl::IsNull<schema::threshold::parameters>,
+    >,
+>;
+
+fn bare_threshold_first() -> BareThresholdFirst {
+    schema::threshold::metric
+        .is_null()
+        .and(schema::threshold::parameters.is_null())
+        .desc()
+}
+
 /// The gate on the addressed row: the threshold that gated it, the model that
 /// threshold ran, the boundary it produced, and any alert that boundary raised.
 type MetricGate = (
@@ -255,6 +279,14 @@ type MetricQuery = (
     Option<MetricGate>,
 );
 
+/// The gate this response reports: the bare threshold's, and no other's.
+///
+/// A metric row may be gated by several thresholds now, so one of them has to be the
+/// one these three fields carry, and it is the bare one: the `value` name of every
+/// grid point, which is the only kind of threshold there was when these fields were
+/// the whole story. A row that only a named or filtered threshold gates reports no
+/// gate here, which is exactly what a caller from before named gating would have
+/// seen for it.
 fn metric_gate(
     project: &QueryProject,
     gate: Option<MetricGate>,
@@ -263,15 +295,16 @@ fn metric_gate(
     Option<JsonBoundary>,
     Option<JsonPerfAlert>,
 ) {
-    if let Some((query_threshold, query_model, query_boundary, query_alert)) = gate {
-        let threshold =
-            Some(query_threshold.into_threshold_model_json_for_project(project, query_model));
-        let boundary = Some(query_boundary.into_json());
-        let alert = query_alert.map(QueryAlert::into_perf_json);
-        (threshold, boundary, alert)
-    } else {
-        (None, None, None)
-    }
+    let Some((query_threshold, query_model, query_boundary, query_alert)) =
+        gate.filter(|(query_threshold, _, _, _)| query_threshold.identity().is_bare())
+    else {
+        return (None, None, None);
+    };
+    let threshold =
+        Some(query_threshold.into_threshold_model_json_for_project(project, query_model));
+    let boundary = Some(query_boundary.into_json());
+    let alert = query_alert.map(QueryAlert::into_perf_json);
+    (threshold, boundary, alert)
 }
 
 /// The metric triple, for a `value` row and for nothing else.

lib/api_projects/src/perf/mod.rs

@@ -541,13 +541,17 @@ fn perf_query(
             schema::version::hash,
             QueryMetric::as_select(),
             (
+                // The column order is `QueryThreshold`'s field order, because that is
+                // what a tuple selection deserializes into, positionally.
                 (
                     schema::threshold::id,
                     schema::threshold::uuid,
                     schema::threshold::project_id,
-                    schema::threshold::measure_id,
                     schema::threshold::branch_id,
                     schema::threshold::testbed_id,
+                    schema::threshold::measure_id,
+                    schema::threshold::metric,
+                    schema::threshold::parameters,
                     schema::threshold::model_id,
                     schema::threshold::created,
                     schema::threshold::modified,
@@ -686,6 +690,7 @@ fn into_perf_metrics(
                 },
                 value_uuid: None,
                 metrics: BTreeMap::new(),
+                bare_gate: None,
             });
         }
         let Some(pending) = line.last_mut() else {
@@ -747,6 +752,9 @@ struct PendingMetric {
     /// metric triple carries.
     value_uuid: Option<MetricUuid>,
     metrics: BTreeMap<MetricName, JsonMetricEntry>,
+    /// The bare threshold's gate on this point, if a bare threshold gated it: what
+    /// the deprecated singular `threshold`, `boundary`, and `alert` fields carry.
+    bare_gate: Option<JsonPerfBoundary>,
 }
 
 impl PendingMetric {
@@ -764,22 +772,36 @@ impl PendingMetric {
             self.value_uuid = Some(uuid);
         }
 
+        let perf_boundary = gate.map(
+            |(query_threshold, query_model, query_boundary, query_alert)| {
+                // The deprecated singular gate is the bare threshold's, and no
+                // other's: the `value` name of every grid point, which is the only
+                // kind of threshold there was when those fields were the whole story.
+                let is_bare = query_threshold.identity().is_bare();
+                let perf_boundary = JsonPerfBoundary {
+                    threshold: query_threshold
+                        .into_threshold_model_json_for_project(project, query_model),
+                    boundary: query_boundary.into_json(),
+                    alert: query_alert.map(QueryAlert::into_perf_json),
+                };
+                (is_bare, perf_boundary)
+            },
+        );
+        if let Some((true, perf_boundary)) = perf_boundary.as_ref() {
+            self.bare_gate = Some(perf_boundary.clone());
+        }
+
         // A named scalar repeats across rows only when several thresholds gated it,
         // and the boundaries list is what absorbs that.
         let entry = self.metrics.entry(name).or_insert(JsonMetricEntry {
             value: value.into(),
             boundaries: None,
         });
-        if let Some((query_threshold, query_model, query_boundary, query_alert)) = gate {
+        if let Some((_, perf_boundary)) = perf_boundary {
             entry
                 .boundaries
                 .get_or_insert_with(Vec::new)
-                .push(JsonPerfBoundary {
-                    threshold: query_threshold
-                        .into_threshold_model_json_for_project(project, query_model),
-                    boundary: query_boundary.into_json(),
-                    alert: query_alert.map(QueryAlert::into_perf_json),
-                });
+                .push(perf_boundary);
         }
     }
 
@@ -797,9 +819,18 @@ impl PendingMetric {
             end_time,
             version,
             value_uuid,
-            metrics,
+            mut metrics,
+            bare_gate,
         } = self;
 
+        // Several thresholds may gate one named scalar, so the list is put in one
+        // order: the threshold creation order, oldest first.
+        for entry in metrics.values_mut() {
+            if let Some(boundaries) = entry.boundaries.as_mut() {
+                boundaries.sort_by_key(|gate| gate.threshold.boundary_order());
+            }
+        }
+
         let value = metrics.get(&MetricName::value());
         let metric = value_uuid.zip(value).map(|(uuid, value)| JsonMetricTriple {
             uuid,
@@ -811,10 +842,7 @@ impl PendingMetric {
                 .get(&MetricName::upper_value())
                 .map(|entry| entry.value),
         });
-        // The deprecated singular gate is the one that gated the `value` row, which is
-        // the only kind of threshold there is today. A measure that names no `value`
-        // has nothing for it to have gated.
-        let (threshold, boundary, alert) = value.map_or((None, None, None), deprecated_gate);
+        let (threshold, boundary, alert) = deprecated_gate(bare_gate);
 
         JsonPerfMetric {
             report,
@@ -837,20 +865,20 @@ type DeprecatedGate = (
     Option<JsonPerfAlert>,
 );
 
-fn deprecated_gate(value: &JsonMetricEntry) -> DeprecatedGate {
-    let Some(perf_boundary) = value
-        .boundaries
-        .as_ref()
-        .and_then(|boundaries| boundaries.first())
-    else {
-        return (None, None, None);
-    };
-    let JsonPerfBoundary {
+/// The deprecated singular gate: the bare threshold's, and no other's.
+///
+/// A point that only a named or filtered threshold gates reports no gate here, which
+/// is exactly what a caller from before named gating would have seen for it.
+fn deprecated_gate(bare_gate: Option<JsonPerfBoundary>) -> DeprecatedGate {
+    let Some(JsonPerfBoundary {
         threshold,
         boundary,
         alert,
-    } = perf_boundary;
-    (Some(threshold.clone()), Some(*boundary), alert.clone())
+    }) = bare_gate
+    else {
+        return (None, None, None);
+    };
+    (Some(threshold), Some(boundary), alert)
 }
 
 #[cfg(test)]

lib/api_projects/src/thresholds.rs

@@ -23,7 +23,10 @@ use bencher_schema::{
             branch::QueryBranch,
             measure::QueryMeasure,
             testbed::QueryTestbed,
-            threshold::{InsertThreshold, QueryThreshold, ThresholdSpec, model::QueryModel},
+            threshold::{
+                InsertThreshold, QueryThreshold, ThresholdDimensions, ThresholdIdentity,
+                ThresholdSpec, model::QueryModel,
+            },
         },
         user::{
             actor::{ApiActor, PubProjectBearerToken},
@@ -247,7 +250,11 @@ type BoxedQuery<'q> = diesel::internal::table_macro::BoxedSelectStatement<
 /// Create a threshold for a project.
 /// The user must have `create` permissions for the project,
 /// or provide a valid project key for the project.
-/// There can only be one threshold for any unique combination of: branch, testbed, and measure.
+/// There can only be one threshold for any unique combination of:
+/// branch, testbed, measure, metric name, and parameters filter.
+/// A threshold that names no metric gates the conventional `value` name,
+/// and a threshold with no parameters filter gates every grid point.
+/// Every threshold that matches a metric row runs: there is no winner among them.
 #[endpoint {
     method = POST,
     path =  "/v0/projects/{project}/thresholds",
@@ -307,13 +314,22 @@ pub async fn post_inner(
     let measure_id =
         QueryMeasure::from_name_id(auth_conn!(context), project_id, &json_threshold.measure)?.id;
 
-    // Create the new threshold
+    // Create the new threshold. What it gates beyond its dimensions is canonicalized
+    // here, so an explicit `value` and an absent metric are one threshold, and so are
+    // an empty filter and an absent one.
+    let identity = ThresholdIdentity::new(
+        json_threshold.metric.clone(),
+        json_threshold.parameters.clone(),
+    );
     let threshold_id = InsertThreshold::from_model(
         context,
         project_id,
-        branch_id,
-        testbed_id,
-        measure_id,
+        ThresholdDimensions {
+            branch_id,
+            testbed_id,
+            measure_id,
+        },
+        identity,
         json_threshold.model,
     )
     .await?;

lib/api_projects/tests/metrics.rs

@@ -884,9 +884,13 @@ async fn metrics_get_gated_value_row() {
     let alert = &alerts.0[0];
     assert_eq!(metric["alert"]["uuid"], serde_json::json!(alert.uuid));
     assert_eq!(metric["boundary"], serde_json::json!(alert.boundary));
+    let alert_metric = alert
+        .metric
+        .as_ref()
+        .expect("the alert gated a `value` row, so it carries the triple");
     assert_eq!(
         metric["metric"]["uuid"],
-        serde_json::json!(alert.metric.uuid),
+        serde_json::json!(alert_metric.uuid),
         "the alert is on the addressed row",
     );