diff --git a/Cargo.lock b/Cargo.lock index 7a9cc2b26ea..d17c401c674 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6315,12 +6315,14 @@ dependencies = [ "omicron-common", "omicron-workspace-hack", "oximeter-macro-impl", + "proc-macro2", "regex", "rstest", "schemars", "serde", "serde_json", "strum", + "syn 2.0.64", "thiserror", "toml 0.8.13", "trybuild", diff --git a/oximeter/impl/Cargo.toml b/oximeter/impl/Cargo.toml index 27f07f27430..43b8967de6a 100644 --- a/oximeter/impl/Cargo.toml +++ b/oximeter/impl/Cargo.toml @@ -16,11 +16,13 @@ heck.workspace = true num.workspace = true omicron-common.workspace = true oximeter-macro-impl.workspace = true +proc-macro2.workspace = true regex.workspace = true schemars = { workspace = true, features = [ "uuid1", "bytes", "chrono" ] } serde.workspace = true serde_json.workspace = true strum.workspace = true +syn.workspace = true toml.workspace = true thiserror.workspace = true uuid.workspace = true diff --git a/oximeter/impl/src/schema/ir.rs b/oximeter/impl/src/schema/ir.rs index d22d0571932..2e9abf8ccee 100644 --- a/oximeter/impl/src/schema/ir.rs +++ b/oximeter/impl/src/schema/ir.rs @@ -73,157 +73,160 @@ pub struct TimeseriesDefinition { pub fields: BTreeMap, } -#[derive(Clone, Copy, Debug)] -enum CurrentVersion { - Active { version: NonZeroU8 }, - Inactive { last_active_version: NonZeroU8 }, -} +impl TimeseriesDefinition { + pub fn into_schema_list( + self, + ) -> Result, MetricsError> { + if self.metrics.is_empty() { + return defn_error(String::from( + "At least one metric must be defined", + )); + } + let mut out = Vec::with_capacity(self.metrics.len()); + let target_name = &self.target.name; -impl CurrentVersion { - fn get(&self) -> NonZeroU8 { - match self { - CurrentVersion::Active { version } => *version, - CurrentVersion::Inactive { last_active_version } => { - *last_active_version + // First create a map from target version to the fields in it. + // + // This is used to do O(lg n) lookups into the set of target fields when we + // iterate through metric versions below, i.e., avoiding quadratic behavior. + let mut target_fields_by_version = BTreeMap::new(); + for (expected_version, target_fields) in + (1u8..).zip(self.target.versions.iter()) + { + if expected_version != target_fields.version.get() { + return defn_error(format!( + "Target '{}' versions should be sequential \ + and monotonically increasing (expected {}, found {})", + target_name, expected_version, target_fields.version, + )); } - } - } -} -fn defn_error(s: String) -> Result { - Err(MetricsError::SchemaDefinition(s)) -} + let fields: BTreeSet<_> = + target_fields.fields.iter().cloned().collect(); + if fields.len() != target_fields.fields.len() { + return defn_error(format!( + "Target '{}' version {} lists duplicate field names", + target_name, expected_version, + )); + } -/// Load the list of timeseries schema from a schema definition file. -pub fn load_schema( - contents: &str, -) -> Result, MetricsError> { - let def = toml::from_str::(contents) - .map_err(|e| MetricsError::Toml(e.to_string()))?; - let mut out = Vec::with_capacity(def.metrics.len()); - let target_name = &def.target.name; - - // First create a map from target version to the fields in it. - // - // This is used to do O(lg n) lookups into the set of target fields when we - // iterate through metric versions below, i.e., avoiding quadratic behavior. - let mut target_fields_by_version = BTreeMap::new(); - for (expected_version, target_fields) in - (1u8..).zip(def.target.versions.iter()) - { - if expected_version != target_fields.version.get() { - return defn_error(format!( - "Target '{}' versions should be sequential \ - and monotonically increasing (expected {}, found {})", - target_name, expected_version, target_fields.version, - )); + if target_fields_by_version + .insert(expected_version, fields) + .is_some() + { + return defn_error(format!( + "Target '{}' version {} is duplicated", + target_name, expected_version, + )); + } } - let fields: BTreeSet<_> = target_fields.fields.iter().collect(); - if fields.len() != target_fields.fields.len() { - return defn_error(format!( - "Target '{}' version {} lists duplicate field names", - target_name, expected_version, - )); - } + // Start by looping over all the metrics in the definition. + // + // As we do so, we'll attach the target definition at the corresponding + // version, along with running some basic lints and checks. + for metric in self.metrics.iter() { + let metric_name = &metric.name; - if target_fields_by_version.insert(expected_version, fields).is_some() { - return defn_error(format!( - "Target '{}' version {} is duplicated", - target_name, expected_version, - )); - } - } + // Store the current version of the metric. This doesn't need to be + // sequential, but they do need to be monotonic and have a matching + // target version. We'll fill in any gaps with the last active version + // of the metric (if any). + let mut current_version: Option = None; - // Start by looping over all the metrics in the definition. - // - // As we do so, we'll attach the target definition at the corresponding - // version, along with running some basic lints and checks. - for metric in def.metrics.iter() { - let metric_name = &metric.name; - - // Store the current version of the metric. This doesn't need to be - // sequential, but they do need to be monotonic and have a matching - // target version. We'll fill in any gaps with the last active version - // of the metric (if any). - let mut current_version: Option = None; - - // Also store the last used version of the target. This lets users omit - // an unchanged metric, and we use this value to fill in the implied - // version of the metric. - let mut last_target_version: u8 = 1; - - // Iterate through each version of this metric. - // - // In general, we expect metrics to be addded in the first version; - // modified by adding / removing fields; and possibly removed at the - // end. However, they can be added / removed multiple times, and not - // added until a later version of the target. - for metric_fields in metric.versions.iter() { - // Extract the fields named in this version, checking that they're - // compatible with the last known version, if any. - let (new_version, maybe_new_fields) = extract_metric_fields( - metric_name, - metric_fields, - current_version, - )?; - let _ = current_version.insert(new_version); - let Some(fields) = maybe_new_fields else { - continue; - }; + // Also store the last used version of the target. This lets users omit + // an unchanged metric, and we use this value to fill in the implied + // version of the metric. + let mut last_target_version: u8 = 0; - let metric_field_names: BTreeSet<_> = fields.iter().collect(); - if metric_field_names.len() != fields.len() { - return defn_error(format!( - "Target '{}' metric '{}' version {} \ - contains duplicated field names", - target_name, - metric_name, - new_version.get(), - )); - } + // Iterate through each version of this metric. + // + // In general, we expect metrics to be addded in the first version; + // modified by adding / removing fields; and possibly removed at the + // end. However, they can be added / removed multiple times, and not + // added until a later version of the target. + for metric_fields in metric.versions.iter() { + // Fill in any gaps from the last target version to this next + // metric version. This only works once we've filled in at least + // one version of the metric, and stored the current version / + // fields. + if let Some(current) = current_version.as_ref() { + let current_fields = current.fields().expect("Should have some fields if we have any previous version"); + while last_target_version <= current.version().get() { + last_target_version += 1; + let Some(target_fields) = + target_fields_by_version.get(&last_target_version) + else { + return defn_error(format!( + "Metric '{}' version {} does not have \ + a matching version in the target '{}'", + metric_name, last_target_version, target_name, + )); + }; + let field_schema = construct_field_schema( + &self.fields, + target_name, + target_fields, + metric_name, + current_fields, + )?; + let _authz_scope = extract_authz_scope( + metric_name, + self.target.authz_scope, + &field_schema, + )?; + let timeseries_name = TimeseriesName::try_from( + format!("{}:{}", target_name, metric_name), + )?; + out.push(TimeseriesSchema { + timeseries_name, + field_schema, + datum_type: metric.datum_type, + version: NonZeroU8::new(last_target_version) + .unwrap(), + /* TODO(ben): Add these fields. + units: metric.units, + authz_scope, + */ + created: Utc::now(), + }); + } + } - // At this point, we're guaranteed to have a meaningul active - // version and field names, so we can generate a valid timeseries - // schema. Append the schema with the matching target version, after - // some final sanity checks. - let CurrentVersion::Active { version } = &new_version else { - unreachable!(); - }; + // Extract the fields named in this version, checking that they're + // compatible with the last known version, if any. + let new_version = extract_metric_fields( + metric_name, + metric_fields, + ¤t_version, + )?; + let version = current_version.insert(new_version); + let Some(metric_fields) = version.fields() else { + continue; + }; - // To support elided metric versions that don't change, we "fill in" - // from the last known target version up to the current active - // metric version as well. - while last_target_version <= version.get() { + // Now, insert the _next_ version of the metric with the + // validated fields we've collected for it. + last_target_version += 1; let Some(target_fields) = target_fields_by_version.get(&last_target_version) else { return defn_error(format!( "Metric '{}' version {} does not have \ - a matching ersion in the target '{}'", + a matching version in the target '{}'", metric_name, last_target_version, target_name, )); }; - - if let Some(dup) = - target_fields.intersection(&metric_field_names).next() - { - return defn_error(format!( - "Field '{}' is duplicated between target \ - '{}' and metric '{}'", - dup, target_name, metric_name, - )); - } let field_schema = construct_field_schema( - &def.fields, + &self.fields, target_name, target_fields, metric_name, - &metric_field_names, + metric_fields, )?; let _authz_scope = extract_authz_scope( metric_name, - def.target.authz_scope, + self.target.authz_scope, &field_schema, )?; let timeseries_name = TimeseriesName::try_from(format!( @@ -241,70 +244,112 @@ pub fn load_schema( */ created: Utc::now(), }); - last_target_version = - last_target_version.checked_add(1).expect("version < 256"); } - } - // We also allow omitting later versions of metrics if they are - // unchanged. A target has to specify every version, even if it's the - // same, but the metrics need only specify differents. - // - // Here, look for any target version strictly later than the last metric - // version, and create a corresponding target / metric pair for it. - if let Some(last_metric_fields) = metric.versions.last() { - match last_metric_fields { - MetricFields::Removed { .. } => {} - MetricFields::Added { - added_in: last_metric_version, - fields, - } - | MetricFields::Versioned(VersionedFields { - version: last_metric_version, - fields, - }) => { - let metric_field_names: BTreeSet<_> = - fields.iter().collect(); - let next_version = last_metric_version - .get() - .checked_add(1) - .expect("version < 256"); - for (version, target_fields) in - target_fields_by_version.range(next_version..) - { - let field_schema = construct_field_schema( - &def.fields, - target_name, - target_fields, - metric_name, - &metric_field_names, - )?; - let _authz_scope = extract_authz_scope( - metric_name, - def.target.authz_scope, - &field_schema, - )?; - let timeseries_name = TimeseriesName::try_from( - format!("{}:{}", target_name, metric_name), - )?; - out.push(TimeseriesSchema { - timeseries_name, - field_schema, - datum_type: metric.datum_type, - version: NonZeroU8::new(*version).unwrap(), - /* TODO(ben): Add these fields. - units: metric.units, - authz_scope, - */ - created: Utc::now(), - }); + /* + println!("{}", last_target_version); + println!("{:#?}", current_version); + */ + + // We also allow omitting later versions of metrics if they are + // unchanged. A target has to specify every version, even if it's the + // same, but the metrics need only specify differences. + // + // Here, look for any target version strictly later than the last metric + // version, and create a corresponding target / metric pair for it. + if let Some(last_metric_fields) = metric.versions.last() { + match last_metric_fields { + MetricFields::Removed { .. } => {} + MetricFields::Added { + added_in: last_metric_version, + fields, + } + | MetricFields::Versioned(VersionedFields { + version: last_metric_version, + fields, + }) => { + let metric_field_names: BTreeSet<_> = + fields.iter().cloned().collect(); + let next_version = last_metric_version + .get() + .checked_add(1) + .expect("version < 256"); + for (version, target_fields) in + target_fields_by_version.range(next_version..) + { + let field_schema = construct_field_schema( + &self.fields, + target_name, + target_fields, + metric_name, + &metric_field_names, + )?; + let _authz_scope = extract_authz_scope( + metric_name, + self.target.authz_scope, + &field_schema, + )?; + let timeseries_name = TimeseriesName::try_from( + format!("{}:{}", target_name, metric_name), + )?; + out.push(TimeseriesSchema { + timeseries_name, + field_schema, + datum_type: metric.datum_type, + version: NonZeroU8::new(*version).unwrap(), + /* TODO(ben): Add these fields. + units: metric.units, + authz_scope, + */ + created: Utc::now(), + }); + } } } } } + out.sort_by(|a, b| { + a.timeseries_name + .cmp(&b.timeseries_name) + .then_with(|| a.version.cmp(&b.version)) + }); + Ok(out) } +} - Ok(out) +#[derive(Clone, Debug)] +enum CurrentVersion { + Active { version: NonZeroU8, fields: BTreeSet }, + Inactive { removed_in: NonZeroU8 }, +} + +impl CurrentVersion { + fn version(&self) -> NonZeroU8 { + match self { + CurrentVersion::Active { version, .. } => *version, + CurrentVersion::Inactive { removed_in } => *removed_in, + } + } + + fn fields(&self) -> Option<&BTreeSet> { + match self { + CurrentVersion::Active { fields, .. } => Some(fields), + CurrentVersion::Inactive { .. } => None, + } + } +} + +fn defn_error(s: String) -> Result { + Err(MetricsError::SchemaDefinition(s)) +} + +/// Load the list of timeseries schema from a schema definition in TOML format. +pub fn load_schema( + contents: &str, +) -> Result, MetricsError> { + toml::from_str::(contents) + .map_err(|e| MetricsError::Toml(e.to_string())) + .and_then(TimeseriesDefinition::into_schema_list) } fn extract_authz_scope( @@ -314,12 +359,14 @@ fn extract_authz_scope( ) -> Result { let check_for_key = |scope: &str| { let key = format!("{scope}_id"); - if field_schema.iter().any(|field| field.name == key) { + if field_schema.iter().any(|field| { + field.name == key && field.field_type == FieldType::Uuid + }) { Ok(()) } else { defn_error(format!( "Metric '{}' has '{}' authorization scope, and so must \ - contain a field '{}'", + contain a field '{}' of UUID type", metric_name, scope, key, )) } @@ -335,10 +382,18 @@ fn extract_authz_scope( fn construct_field_schema( all_fields: &BTreeMap, target_name: &str, - target_fields: &BTreeSet<&String>, + target_fields: &BTreeSet, metric_name: &str, - metric_field_names: &BTreeSet<&String>, + metric_field_names: &BTreeSet, ) -> Result, MetricsError> { + if let Some(dup) = target_fields.intersection(&metric_field_names).next() { + return defn_error(format!( + "Field '{}' is duplicated between target \ + '{}' and metric '{}'", + dup, target_name, metric_name, + )); + } + let mut field_schema = BTreeSet::new(); for (field_name, source) in target_fields.iter().zip(std::iter::repeat(FieldSource::Target)).chain( @@ -373,19 +428,25 @@ fn is_snake_case(s: &str) -> bool { s == format!("{}", heck::AsSnakeCase(s)) } +fn is_valid_ident_name(s: &str) -> bool { + syn::parse_str::(s).is_ok() && is_snake_case(s) +} + fn validate_field_name( field_name: &str, metadata: &FieldMetadata, ) -> Result<(), MetricsError> { - if !is_snake_case(field_name) { + if !is_valid_ident_name(field_name) { return defn_error(format!( - "Field name '{}' should be snake_case", + "Field name '{}' should be a valid identifier in snake_case", field_name, )); } - if metadata.type_ == FieldType::Uuid && !field_name.ends_with("_id") { + if metadata.type_ == FieldType::Uuid + && !(field_name.ends_with("_id") || field_name == "id") + { return defn_error(format!( - "Uuid field '{}' should end with '_id'", + "Uuid field '{}' should end with '_id' or equal 'id'", field_name, )); } @@ -395,16 +456,16 @@ fn validate_field_name( fn extract_metric_fields<'a>( metric_name: &'a str, metric_fields: &'a MetricFields, - current_version: Option, -) -> Result<(CurrentVersion, Option<&'a Vec>), MetricsError> { - let (new_version, new_fields) = match metric_fields { + current_version: &Option, +) -> Result { + let new_version = match metric_fields { MetricFields::Removed { removed_in } => { match current_version { - Some(CurrentVersion::Active { version }) => { + Some(CurrentVersion::Active { version, .. }) => { // This metric was active, and is now being // removed. Bump the version and mark it active, // but there are no fields to return here. - if *removed_in <= version { + if removed_in <= version { return defn_error(format!( "Metric '{}' is removed in version \ {}, which is not strictly after the \ @@ -412,18 +473,13 @@ fn extract_metric_fields<'a>( metric_name, removed_in, version, )); } - ( - CurrentVersion::Inactive { - last_active_version: *removed_in, - }, - None, - ) + CurrentVersion::Inactive { removed_in: *removed_in } } - Some(CurrentVersion::Inactive { last_active_version }) => { + Some(CurrentVersion::Inactive { removed_in }) => { return defn_error(format!( "Metric '{}' was already removed in \ version {}, it cannot be removed again", - metric_name, last_active_version, + metric_name, removed_in, )); } None => { @@ -444,37 +500,37 @@ fn extract_metric_fields<'a>( metric_name, )); } - Some(CurrentVersion::Inactive { last_active_version }) => { + Some(CurrentVersion::Inactive { removed_in }) => { // The metric is currently inactive, just check // that the newly-active version is greater. - if *added_in <= last_active_version { + if added_in <= removed_in { return defn_error(format!( "Re-added metric '{}' must appear in a later \ - version than the last active version ({})", - metric_name, last_active_version, + version than the one in which it was removed ({})", + metric_name, removed_in, )); } - ( - CurrentVersion::Active { version: *added_in }, - Some(fields), - ) + CurrentVersion::Active { + version: *added_in, + fields: to_unique_field_names(metric_name, fields)?, + } } None => { // There was no previous version for this // metric, just add it. - ( - CurrentVersion::Active { version: *added_in }, - Some(fields), - ) + CurrentVersion::Active { + version: *added_in, + fields: to_unique_field_names(metric_name, fields)?, + } } } } MetricFields::Versioned(new_fields) => { match current_version { - Some(CurrentVersion::Active { version }) => { + Some(CurrentVersion::Active { version, .. }) => { // The happy-path, we're stepping the version // and possibly modifying the fields. - if new_fields.version <= version { + if new_fields.version <= *version { return defn_error(format!( "Metric '{}' version should increment, \ expected at least {}, found {}", @@ -483,18 +539,21 @@ fn extract_metric_fields<'a>( new_fields.version, )); } - ( - CurrentVersion::Active { version: new_fields.version }, - Some(&new_fields.fields), - ) + CurrentVersion::Active { + version: new_fields.version, + fields: to_unique_field_names( + metric_name, + &new_fields.fields, + )?, + } } - Some(CurrentVersion::Inactive { last_active_version }) => { + Some(CurrentVersion::Inactive { removed_in }) => { // The metric has been removed in the past, it // needs to be added again first. return defn_error(format!( - "Metric '{}' was last active at version {}, \ + "Metric '{}' was removed in version {}, \ it must be added again first", - metric_name, last_active_version, + metric_name, removed_in, )); } None => { @@ -509,5 +568,582 @@ fn extract_metric_fields<'a>( } } }; - Ok((new_version, new_fields)) + Ok(new_version) +} + +fn to_unique_field_names( + name: &str, + fields: &[String], +) -> Result, MetricsError> { + let set: BTreeSet<_> = fields.iter().cloned().collect(); + if set.len() != fields.len() { + return defn_error(format!("Object '{name}' has duplicate fields")); + } + Ok(set) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn extract_authz_scope_requires_relevant_field() { + assert!( + extract_authz_scope("foo", AuthzScope::Project, &BTreeSet::new()) + .is_err(), + "Project-scoped auth without a project_id field should be an error" + ); + + let schema = std::iter::once(FieldSchema { + name: "project_id".to_string(), + field_type: FieldType::Uuid, + source: FieldSource::Target, + }) + .collect(); + extract_authz_scope("foo", AuthzScope::Project, &schema).expect( + "Project-scoped auth with a project_id field should succeed", + ); + + let schema = std::iter::once(FieldSchema { + name: "project_id".to_string(), + field_type: FieldType::String, + source: FieldSource::Target, + }) + .collect(); + assert!( + extract_authz_scope("foo", AuthzScope::Project, &schema).is_err(), + "Project-scoped auth with a project_id field \ + that's not a UUID should be an error", + ); + } + + fn all_fields() -> BTreeMap { + let mut out = BTreeMap::new(); + for name in ["foo", "bar"] { + out.insert( + String::from(name), + FieldMetadata { + type_: FieldType::U8, + description: String::new(), + }, + ); + } + out + } + + #[test] + fn construct_field_schema_fails_with_reference_to_unknown_field() { + let all = all_fields(); + let target_fields = BTreeSet::new(); + let bad_name = String::from("baz"); + let metric_fields = std::iter::once(bad_name).collect(); + assert!( + construct_field_schema( + &all, + "target", + &target_fields, + "metric", + &metric_fields, + ) + .is_err(), + "Should return an error when the metric references a field \ + that doesn't exist in the global field list" + ); + } + + #[test] + fn construct_field_schema_fails_with_duplicate_field_names() { + let all = all_fields(); + let name = String::from("bar"); + let target_fields = std::iter::once(name.clone()).collect(); + let metric_fields = std::iter::once(name).collect(); + assert!(construct_field_schema( + &all, + "target", + &target_fields, + "metric", + &metric_fields, + ).is_err(), + "Should return an error when the target and metric share a field name" + ); + } + + #[test] + fn construct_field_schema_picks_up_correct_fields() { + let all = all_fields(); + let all_schema = all + .iter() + .zip([FieldSource::Metric, FieldSource::Target]) + .map(|((name, md), source)| FieldSchema { + name: name.clone(), + field_type: md.type_, + source, + }) + .collect(); + let foo = String::from("foo"); + let target_fields = std::iter::once(foo).collect(); + let bar = String::from("bar"); + let metric_fields = std::iter::once(bar).collect(); + assert_eq!( + construct_field_schema( + &all, + "target", + &target_fields, + "metric", + &metric_fields, + ) + .unwrap(), + all_schema, + "Each field is referenced exactly once, so we should return \ + the entire input set of fields" + ); + } + + #[test] + fn validate_field_name_disallows_bad_names() { + for name in ["PascalCase", "with spaces", "12345", "💖"] { + assert!( + validate_field_name( + name, + &FieldMetadata { + type_: FieldType::U8, + description: String::new() + } + ) + .is_err(), + "Field named {name} should be invalid" + ); + } + } + + #[test] + fn validate_field_name_verifies_uuid_field_names() { + assert!( + validate_field_name( + "projectid", + &FieldMetadata { + type_: FieldType::Uuid, + description: String::new() + } + ) + .is_err(), + "A Uuid field should be required to end in `_id`", + ); + for name in ["project_id", "id"] { + assert!( + validate_field_name(name, + &FieldMetadata { + type_: FieldType::Uuid, + description: String::new() + } + ).is_ok(), + "A Uuid field should be required to end in '_id' or exactly 'id'", + ); + } + } + + #[test] + fn extract_metric_fields_succeeds_with_gaps_in_versions() { + let metric_fields = MetricFields::Versioned(VersionedFields { + version: NonZeroU8::new(10).unwrap(), + fields: vec![], + }); + let current_version = Some(CurrentVersion::Active { + version: NonZeroU8::new(1).unwrap(), + fields: BTreeSet::new(), + }); + extract_metric_fields("foo", &metric_fields, ¤t_version).expect( + "Extracting metric fields should work with non-sequential \ + but increasing version numbers", + ); + } + + #[test] + fn extract_metric_fields_fails_with_non_increasing_versions() { + let metric_fields = MetricFields::Versioned(VersionedFields { + version: NonZeroU8::new(1).unwrap(), + fields: vec![], + }); + let current_version = Some(CurrentVersion::Active { + version: NonZeroU8::new(1).unwrap(), + fields: BTreeSet::new(), + }); + let res = + extract_metric_fields("foo", &metric_fields, ¤t_version); + let Err(MetricsError::SchemaDefinition(msg)) = &res else { + panic!("Expected schema definition error, found: {res:#?}"); + }; + assert!( + msg.contains("should increment"), + "Should fail when version numbers are non-increasing", + ); + } + + #[test] + fn extract_metric_fields_requires_adding_first() { + let metric_fields = MetricFields::Versioned(VersionedFields { + version: NonZeroU8::new(1).unwrap(), + fields: vec![], + }); + let current_version = None; + let res = + extract_metric_fields("foo", &metric_fields, ¤t_version); + let Err(MetricsError::SchemaDefinition(msg)) = &res else { + panic!("Expected schema definition error, found: {res:#?}"); + }; + assert!( + msg.contains("must be added in at its first version"), + "Should require that the first version of a metric explicitly \ + adds it in, before modification", + ); + + let metric_fields = MetricFields::Added { + added_in: NonZeroU8::new(1).unwrap(), + fields: vec![], + }; + let current_version = None; + extract_metric_fields("foo", &metric_fields, ¤t_version).expect( + "Should succeed when fields are added_in for their first version", + ); + } + + #[test] + fn extract_metric_fields_fails_to_add_existing_metric() { + let metric_fields = MetricFields::Added { + added_in: NonZeroU8::new(2).unwrap(), + fields: vec![], + }; + let current_version = Some(CurrentVersion::Active { + version: NonZeroU8::new(1).unwrap(), + fields: BTreeSet::new(), + }); + let res = + extract_metric_fields("foo", &metric_fields, ¤t_version); + let Err(MetricsError::SchemaDefinition(msg)) = &res else { + panic!("Expected schema definition error, found: {res:#?}"); + }; + assert!( + msg.contains("is already active"), + "Should fail when adding a metric that's already active", + ); + } + + #[test] + fn extract_metric_fields_fails_to_remove_non_existent_metric() { + let metric_fields = + MetricFields::Removed { removed_in: NonZeroU8::new(3).unwrap() }; + let current_version = Some(CurrentVersion::Inactive { + removed_in: NonZeroU8::new(1).unwrap(), + }); + let res = + extract_metric_fields("foo", &metric_fields, ¤t_version); + let Err(MetricsError::SchemaDefinition(msg)) = &res else { + panic!("Expected schema definition error, found: {res:#?}"); + }; + assert!( + msg.contains("was already removed"), + "Should fail when removing a metric that's already gone", + ); + } + + #[test] + fn load_schema_catches_metric_versions_not_added_in() { + let contents = r#" + name = "target" + description = "some target" + authz_scope = "fleet" + versions = [ + { version = 1, fields = [ "foo" ] } + ] + + [[metrics]] + name = "metric" + description = "some metric" + datum_type = "u8" + units = "count" + versions = [ + { version = 1, fields = [] } + ] + + [fields.foo] + type = "string" + description = "a field" + "#; + let res = load_schema(contents); + let Err(MetricsError::SchemaDefinition(msg)) = &res else { + panic!("Should fail when metrics are not added in, but found: {res:#?}"); + }; + assert!( + msg.contains("must be added in at its first"), + "Error message should indicate that metrics need to be \ + added_in first, then modified" + ); + } + + #[test] + fn into_schema_list_fails_with_zero_metrics() { + let contents = r#" + name = "target" + description = "some target" + authz_scope = "fleet" + versions = [ + { version = 1, fields = [ "foo" ] } + ] + + [[metrics]] + name = "metric" + description = "some metric" + datum_type = "u8" + units = "count" + versions = [ + { added_in = 1, fields = [] } + ] + + [fields.foo] + type = "string" + description = "a field" + "#; + let mut def: TimeseriesDefinition = toml::from_str(contents).unwrap(); + def.metrics.clear(); + let res = def.into_schema_list(); + let Err(MetricsError::SchemaDefinition(msg)) = &res else { + panic!("Should fail with zero metrics, but found: {res:#?}"); + }; + assert!( + msg.contains("At least one metric must"), + "Error message should indicate that metrics need to be \ + added_in first, then modified" + ); + } + + #[test] + fn load_schema_fails_with_nonexistent_target_version() { + let contents = r#" + name = "target" + description = "some target" + authz_scope = "fleet" + versions = [ + { version = 1, fields = [ "foo" ] }, + ] + + [[metrics]] + name = "metric" + description = "some metric" + datum_type = "u8" + units = "count" + versions = [ + { added_in = 1, fields = [] }, + { version = 2, fields = [] } + ] + + [fields.foo] + type = "string" + description = "a field" + "#; + let res = load_schema(contents); + let Err(MetricsError::SchemaDefinition(msg)) = &res else { + panic!( + "Should fail when a metric version refers \ + to a non-existent target version, but found: {res:#?}", + ); + }; + assert!( + msg.contains("does not have a matching version in the target"), + "Error message should indicate that the metric \ + refers to a nonexistent version in the target, found: {msg}", + ); + } + + fn assert_sequential_versions( + first: &TimeseriesSchema, + second: &TimeseriesSchema, + ) { + assert_eq!(first.timeseries_name, second.timeseries_name); + assert_eq!( + first.version.get(), + second.version.get().checked_sub(1).unwrap() + ); + assert_eq!(first.datum_type, second.datum_type); + assert_eq!(first.field_schema, second.field_schema); + } + + #[test] + fn load_schema_fills_in_late_implied_metric_versions() { + let contents = r#" + name = "target" + description = "some target" + authz_scope = "fleet" + versions = [ + { version = 1, fields = [ "foo" ] }, + { version = 2, fields = [ "foo" ] }, + ] + + [[metrics]] + name = "metric" + description = "some metric" + datum_type = "u8" + units = "count" + versions = [ + { added_in = 1, fields = [] } + ] + + [fields.foo] + type = "string" + description = "a field" + "#; + let schema = load_schema(contents).unwrap(); + assert_eq!( + schema.len(), + 2, + "Should have filled in version 2 of the metric using the \ + corresponding target version", + ); + assert_sequential_versions(&schema[0], &schema[1]); + } + + #[test] + fn load_schema_fills_in_implied_metric_versions_when_last_is_modified() { + let contents = r#" + name = "target" + description = "some target" + authz_scope = "fleet" + versions = [ + { version = 1, fields = [ "foo" ] }, + { version = 2, fields = [ "foo" ] }, + { version = 3, fields = [ "foo" ] }, + ] + + [[metrics]] + name = "metric" + description = "some metric" + datum_type = "u8" + units = "count" + versions = [ + { added_in = 1, fields = [] }, + { version = 3, fields = [] }, + ] + + [fields.foo] + type = "string" + description = "a field" + "#; + let schema = load_schema(contents).unwrap(); + assert_eq!( + schema.len(), + 3, + "Should have filled in version 2 of the metric using the \ + corresponding target version", + ); + assert_sequential_versions(&schema[0], &schema[1]); + assert_sequential_versions(&schema[1], &schema[2]); + } + + #[test] + fn load_schema_fills_in_implied_metric_versions() { + let contents = r#" + name = "target" + description = "some target" + authz_scope = "fleet" + versions = [ + { version = 1, fields = [ "foo" ] }, + { version = 2, fields = [ "foo" ] }, + { version = 3, fields = [ "foo" ] }, + ] + + [[metrics]] + name = "metric" + description = "some metric" + datum_type = "u8" + units = "count" + versions = [ + { added_in = 1, fields = [] }, + ] + + [fields.foo] + type = "string" + description = "a field" + "#; + let schema = load_schema(contents).unwrap(); + assert_eq!( + schema.len(), + 3, + "Should have filled in versions 2 and 3 of the metric using the \ + corresponding target version", + ); + assert_sequential_versions(&schema[0], &schema[1]); + assert_sequential_versions(&schema[1], &schema[2]); + } + + #[test] + fn load_schema_fills_in_implied_metric_versions_when_last_version_is_removed( + ) { + let contents = r#" + name = "target" + description = "some target" + authz_scope = "fleet" + versions = [ + { version = 1, fields = [ "foo" ] }, + { version = 2, fields = [ "foo" ] }, + { version = 3, fields = [ "foo" ] }, + ] + + [[metrics]] + name = "metric" + description = "some metric" + datum_type = "u8" + units = "count" + versions = [ + { added_in = 1, fields = [] }, + { removed_in = 3 }, + ] + + [fields.foo] + type = "string" + description = "a field" + "#; + let schema = load_schema(contents).unwrap(); + dbg!(&schema); + assert_eq!( + schema.len(), + 2, + "Should have filled in version 2 of the metric using the \ + corresponding target version", + ); + assert_sequential_versions(&schema[0], &schema[1]); + } + + #[test] + fn load_schema_skips_versions_until_metric_is_added( + ) { + let contents = r#" + name = "target" + description = "some target" + authz_scope = "fleet" + versions = [ + { version = 1, fields = [ "foo" ] }, + { version = 2, fields = [ "foo" ] }, + { version = 3, fields = [ "foo" ] }, + ] + + [[metrics]] + name = "metric" + description = "some metric" + datum_type = "u8" + units = "count" + versions = [ + { added_in = 3, fields = [] }, + ] + + [fields.foo] + type = "string" + description = "a field" + "#; + let schema = load_schema(contents).unwrap(); + dbg!(&schema); + assert_eq!( + schema.len(), + 1, + "Should have only created the last version of the timeseries" + ); + } } diff --git a/oximeter/impl/src/schema/mod.rs b/oximeter/impl/src/schema/mod.rs index 1b96faa6bd2..9573036f15d 100644 --- a/oximeter/impl/src/schema/mod.rs +++ b/oximeter/impl/src/schema/mod.rs @@ -268,6 +268,7 @@ impl TimeseriesSchema { impl PartialEq for TimeseriesSchema { fn eq(&self, other: &TimeseriesSchema) -> bool { self.timeseries_name == other.timeseries_name + && self.version == other.version && self.datum_type == other.datum_type && self.field_schema == other.field_schema }