Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions docs/components/backend/analytics/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,20 @@
"null"
]
},
"subject": {
"description": "The single topic this metric belongs to within its family, so a surface\nlisting a family can partition it into topics rather than only sorting\nby name. Exactly one per metric; absent only for metrics that declare\nnone.",
"type": [
"string",
"null"
]
},
"tags": {
"description": "Cross-cutting labels a surface can filter or search by; many per metric,\nunlike the singular `subject`. Empty when the metric declares none.",
"items": {
"type": "string"
},
"type": "array"
},
"unit": {
"type": [
"string",
Expand All @@ -506,6 +520,7 @@
"format",
"direction",
"dimensions",
"tags",
"is_enabled",
"origin",
"schema_status"
Expand Down
31 changes: 28 additions & 3 deletions docs/domain/metrics/specs/DESIGN.md
Original file line number Diff line number Diff line change
Expand Up @@ -325,6 +325,7 @@ metric_source_dimensions
metric_definitions
metric_definition_inputs
metric_definition_dimensions
metric_definition_tags
```

`metric_sources` stores typed source refs.
Expand All @@ -338,6 +339,8 @@ metric_definition_dimensions
```text
metric_key
label
short_label
subject
description
explanation
unit
Expand All @@ -353,6 +356,17 @@ schema_status
schema_error_code
```

`subject` is the single topic a metric belongs to within its family — the
grouping a surface listing a whole family uses to partition it into topics
(`meetings`, `messages`, `email`, `documents`) rather than only sorting by
name. Exactly one per metric, which is the partition a source key cannot
provide: `metric_definition_inputs` binds each input to its own measure, so a
ratio's numerator and denominator may come from different sources, and a
grouping derived from the source is not a partition. Every builtin declares a
subject; a builtin registry test (`every_metric_declares_a_shaped_subject`)
enforces presence and shape. The column is nullable so a custom definition may
omit it.

`unit` is a display suffix for formats that do not fully determine
presentation on their own (e.g. `"lines"`, `"days"`, `"h"`). `percent` and
`currency` are presentation-complete — the frontend renders `%` or a
Expand All @@ -374,6 +388,14 @@ denominator

`metric_definition_dimensions` maps metrics to source dimensions.

`metric_definition_tags` holds a metric's cross-cutting tags — free-form slugs
a surface can filter or search by (`rate`, `duration`, `distribution`), many
per metric, unlike the singular `subject`. Tags are not bound to a source, so
this table carries the tag string directly rather than referencing
`metric_source_dimensions`. A builtin registry test
(`metric_tags_are_shaped_and_unique_per_metric`) pins their shape and
per-metric uniqueness.

Rules:

- Product definitions have `tenant_id = NULL`.
Expand Down Expand Up @@ -638,10 +660,13 @@ derived from it.
1. Add one entry to the `metrics` list in
`src/backend/services/analytics/src/domain/metric_definitions/registry.yaml`:
metric key (`namespace.metric_name`, lowercase snake case), label,
description, unit, format, direction, entity type, computation, input role
mapping to the measure, allowed dimensions, peer cohort key.
subject (the one topic within the family this metric groups under — a
lowercase snake-case slug, required), description, unit, format, direction,
entity type, computation, input role mapping to the measure, allowed
dimensions, peer cohort key, and optionally tags (cross-cutting slugs).
2. Run `cargo test -p analytics` — the registry invariant tests validate
key shapes, input/measure references, and computation field combinations.
key shapes, subject/tag shapes, input/measure references, and computation
field combinations.

The reconciler seeds the definition on the next deploy. If every input measure
has healthy evidence metadata, the metric automatically receives drilldown,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,14 @@ pub struct MetricSeed {
/// None = the full label is already compact enough.
#[serde(default)]
pub short_label: Option<String>,
/// The single topic this metric belongs to within its family, so a surface
/// listing a family can partition it into topics. Required for builtins —
/// exactly one per metric, which is the partition a source key cannot give.
pub subject: String,
/// Cross-cutting labels a surface can filter or search by; many per metric,
/// unlike the singular `subject`.
#[serde(default)]
pub tags: Vec<String>,
Comment on lines +102 to +109

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Parse bounded metadata slugs at the registry boundary.

subject and tags remain raw strings. The current tests accept values longer than the VARCHAR(64) columns. A future registry entry can pass cargo test and then fail or truncate during reconciliation.

  • src/backend/services/analytics/src/domain/metric_definitions/builtin.rs#L102-L109: Replace raw metadata strings with parsed MetricSubject and MetricTag boundary types that enforce lowercase snake case and a 64-character maximum.
  • src/backend/services/analytics/src/domain/metric_definitions/builtin.rs#L325-L354: Test the maximum length and retain per-metric tag uniqueness validation.
  • docs/domain/metrics/specs/DESIGN.md#L663-L669: State the 64-character maximum in the metric-authoring instructions.

As per coding guidelines, “Parse, don't validate: introduce boundary newtypes … and do not carry raw String values through layers.”

📍 Affects 2 files
  • src/backend/services/analytics/src/domain/metric_definitions/builtin.rs#L102-L109 (this comment)
  • src/backend/services/analytics/src/domain/metric_definitions/builtin.rs#L325-L354
  • docs/domain/metrics/specs/DESIGN.md#L663-L669
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/backend/services/analytics/src/domain/metric_definitions/builtin.rs`
around lines 102 - 109, The builtin metric metadata must use parsed bounded
slugs instead of raw strings. In builtin.rs lines 102-109, introduce and use
MetricSubject and MetricTag boundary types enforcing lowercase snake case and a
maximum of 64 characters; update lines 325-354 to test the length limit while
preserving per-metric tag uniqueness validation. In
docs/domain/metrics/specs/DESIGN.md lines 663-669, document the 64-character
maximum in the metric-authoring guidance.

Source: Coding guidelines

#[serde(default)]
pub description: Option<String>,
#[serde(default)]
Expand Down Expand Up @@ -314,6 +322,53 @@ mod tests {
}
}

// Width of metric_definitions.subject and metric_definition_tags.tag. A
// longer authored value would pass shape checks but fail or truncate at
// reconcile time, so the bound is enforced here at build time.
const METADATA_MAX_LEN: usize = 64;

#[test]
fn every_metric_declares_a_shaped_subject() {
for metric in builtin_metrics() {
assert!(
is_snake_case(&metric.subject),
"{} declares an unshaped subject {:?}",
metric.metric_key,
metric.subject
);
assert!(
metric.subject.len() <= METADATA_MAX_LEN,
"{} subject {:?} exceeds {METADATA_MAX_LEN} chars",
metric.metric_key,
metric.subject
);
}
}

#[test]
fn metric_tags_are_shaped_and_unique_per_metric() {
for metric in builtin_metrics() {
let mut seen = BTreeSet::new();
for tag in &metric.tags {
assert!(
is_snake_case(tag),
"{} declares an unshaped tag {tag:?}",
metric.metric_key
);
assert!(
tag.len() <= METADATA_MAX_LEN,
"{} tag {tag:?} exceeds {METADATA_MAX_LEN} chars",
metric.metric_key
);
assert!(
seen.insert(tag.as_str()),
"{} declares duplicate tag {tag:?}",
metric.metric_key
);
}
}
}

#[test]
fn ratio_metrics_have_numerator_and_denominator_roles() {
for metric in builtin_metrics() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ use uuid::Uuid;

use crate::domain::metric_definitions::definition::{MetricDirection, MetricFormat, MetricOrigin};
use crate::domain::metric_definitions::error_code::{MetricSchemaErrorCode, SchemaStatus};
use crate::domain::metric_definitions::repository::fetch_dimensions;
use crate::domain::metric_definitions::repository::{fetch_dimensions, fetch_tags};
use crate::domain::metric_drilldown::{MetricDrilldownCapability, load_capabilities};

/// Response body for `GET /v1/metric-definitions`. Metrics are sorted by
Expand All @@ -37,12 +37,20 @@ pub struct MetricDefinitionView {
/// Compact label for dense surfaces; absent when the full label is
/// already compact enough.
pub short_label: Option<String>,
/// The single topic this metric belongs to within its family, so a surface
/// listing a family can partition it into topics rather than only sorting
/// by name. Exactly one per metric; absent only for metrics that declare
/// none.
pub subject: Option<String>,
pub description: Option<String>,
pub explanation: Option<String>,
pub unit: Option<String>,
pub format: MetricFormat,
pub direction: MetricDirection,
pub dimensions: Vec<String>,
/// Cross-cutting labels a surface can filter or search by; many per metric,
/// unlike the singular `subject`. Empty when the metric declares none.
pub tags: Vec<String>,
pub is_enabled: bool,
/// `builtin` metrics read managed observation relations; `custom` metrics
/// execute inline SQL at query time. The validator stamps `schema_status`
Expand Down Expand Up @@ -72,6 +80,7 @@ struct ListingRow {
metric_key: String,
label: String,
short_label: Option<String>,
subject: Option<String>,
description: Option<String>,
explanation: Option<String>,
unit: Option<String>,
Expand Down Expand Up @@ -111,8 +120,11 @@ pub async fn list_definition_views(
let dimensions = fetch_dimensions(db, &definition_ids)
.await
.map_err(|error| db_error(&error))?;
let tags = fetch_tags(db, &definition_ids)
.await
.map_err(|error| db_error(&error))?;

let mut metrics = build_views(selected, dimensions)?;
let mut metrics = build_views(selected, dimensions, tags)?;
for metric in &mut metrics {
metric.drilldown = capabilities.remove(&metric.metric_key);
}
Expand Down Expand Up @@ -142,6 +154,7 @@ fn select_rows(rows: Vec<ListingRow>) -> Vec<ListingRow> {
fn build_views(
selected: Vec<ListingRow>,
mut dimensions: HashMap<Uuid, Vec<String>>,
mut tags: HashMap<Uuid, Vec<String>>,
) -> Result<Vec<MetricDefinitionView>, CanonicalError> {
let mut metrics = Vec::with_capacity(selected.len());
for row in selected {
Expand All @@ -165,12 +178,14 @@ fn build_views(
metric_key: row.metric_key,
label: row.label,
short_label: row.short_label,
subject: row.subject,
description: row.description,
explanation: row.explanation,
unit: row.unit,
format,
direction,
dimensions: dimensions.remove(&row.definition_id).unwrap_or_default(),
tags: tags.remove(&row.definition_id).unwrap_or_default(),
is_enabled: row.is_enabled,
origin,
schema_status,
Expand All @@ -194,6 +209,7 @@ async fn fetch_listing_rows(
d.metric_key AS metric_key, \
d.label AS label, \
d.short_label AS short_label, \
d.subject AS subject, \
d.description AS description, \
d.explanation AS explanation, \
d.unit AS unit, \
Expand Down Expand Up @@ -239,6 +255,7 @@ mod tests {
metric_key: metric_key.to_owned(),
label: label.to_owned(),
short_label: None,
subject: None,
description: None,
explanation: None,
unit: None,
Expand Down Expand Up @@ -277,12 +294,14 @@ mod tests {
#[test]
fn build_views_decodes_columns_and_attaches_dimensions() {
let mut r = row("git.commits", None, "Commits");
r.subject = Some("commits".to_owned());
r.schema_status = "error".to_owned();
r.schema_error_code = Some("table_not_found".to_owned());
let id = r.definition_id;
let dims = HashMap::from([(id, vec!["repo".to_owned()])]);
let tags = HashMap::from([(id, vec!["rate".to_owned()])]);

let Ok(views) = build_views(vec![r], dims) else {
let Ok(views) = build_views(vec![r], dims, tags) else {
panic!("canonical rows must map");
};
assert_eq!(views.len(), 1);
Expand All @@ -298,14 +317,16 @@ mod tests {
Some(MetricSchemaErrorCode::TableNotFound)
);
assert_eq!(view.dimensions, vec!["repo".to_owned()]);
assert_eq!(view.subject.as_deref(), Some("commits"));
assert_eq!(view.tags, vec!["rate".to_owned()]);
}

#[test]
fn build_views_decodes_custom_origin() {
let mut r = row("team.velocity", None, "Velocity");
r.origin = "custom".to_owned();

let Ok(views) = build_views(vec![r], HashMap::new()) else {
let Ok(views) = build_views(vec![r], HashMap::new(), HashMap::new()) else {
panic!("canonical rows must map");
};
let Some(view) = views.first() else {
Expand All @@ -315,16 +336,18 @@ mod tests {
assert_eq!(view.schema_status, SchemaStatus::Unchecked);
assert_eq!(view.schema_error_code, None);
assert_eq!(view.last_observed_date, None);
assert_eq!(view.subject, None);
assert!(view.tags.is_empty());
}

#[test]
fn build_views_rejects_a_noncanonical_enum_value() {
let mut r = row("git.commits", None, "Commits");
r.format = "not-a-format".to_owned();
assert!(build_views(vec![r], HashMap::new()).is_err());
assert!(build_views(vec![r], HashMap::new(), HashMap::new()).is_err());

let mut r = row("git.commits", None, "Commits");
r.origin = "not-an-origin".to_owned();
assert!(build_views(vec![r], HashMap::new()).is_err());
assert!(build_views(vec![r], HashMap::new(), HashMap::new()).is_err());
}
}
Loading
Loading