diff --git a/Cargo.lock b/Cargo.lock index 47b7d862570..b139d1de4d1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6330,6 +6330,7 @@ dependencies = [ "schemars", "serde", "serde_json", + "slog-error-chain", "strum", "syn 2.0.66", "thiserror", diff --git a/oximeter/impl/Cargo.toml b/oximeter/impl/Cargo.toml index 9cbc8d3a4fb..05ab9fa41f1 100644 --- a/oximeter/impl/Cargo.toml +++ b/oximeter/impl/Cargo.toml @@ -22,6 +22,7 @@ regex.workspace = true schemars = { workspace = true, features = [ "uuid1", "bytes", "chrono" ] } serde.workspace = true serde_json.workspace = true +slog-error-chain.workspace = true strum.workspace = true syn.workspace = true toml.workspace = true diff --git a/oximeter/impl/src/schema/ir.rs b/oximeter/impl/src/schema/ir.rs index 748593df4f5..f143b3fbf4a 100644 --- a/oximeter/impl/src/schema/ir.rs +++ b/oximeter/impl/src/schema/ir.rs @@ -78,14 +78,14 @@ impl TimeseriesDefinition { self, ) -> Result, MetricsError> { if self.target.versions.is_empty() { - return defn_error(String::from( + return Err(MetricsError::SchemaDefinition(String::from( "At least one target version must be defined", - )); + ))); } if self.metrics.is_empty() { - return defn_error(String::from( + return Err(MetricsError::SchemaDefinition(String::from( "At least one metric must be defined", - )); + ))); } let mut timeseries = BTreeMap::new(); let target_name = &self.target.name; @@ -105,11 +105,11 @@ impl TimeseriesDefinition { .iter() .any(|fields| fields.version.get() > 1) { - return defn_error(String::from( + return Err(MetricsError::SchemaDefinition(String::from( "Exactly one timeseries version, with version number 1, \ may currently be specified. Updates will be supported \ in future releases.", - )); + ))); } // First create a map from target version to the fields in it. @@ -121,30 +121,30 @@ impl TimeseriesDefinition { (1u8..).zip(self.target.versions.iter()) { if expected_version != target_fields.version.get() { - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(format!( "Target '{}' versions should be sequential \ and monotonically increasing (expected {}, found {})", target_name, expected_version, target_fields.version, - )); + ))); } let fields: BTreeSet<_> = target_fields.fields.iter().cloned().collect(); if fields.len() != target_fields.fields.len() { - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(format!( "Target '{}' version {} lists duplicate field names", target_name, expected_version, - )); + ))); } if target_fields_by_version .insert(expected_version, fields) .is_some() { - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(format!( "Target '{}' version {} is duplicated", target_name, expected_version, - )); + ))); } } @@ -186,10 +186,14 @@ impl TimeseriesDefinition { let Some(target_fields) = target_fields_by_version.get(&last_target_version) else { - return defn_error(format!( - "Metric '{}' version {} does not have \ + return Err(MetricsError::SchemaDefinition( + format!( + "Metric '{}' version {} does not have \ a matching version in the target '{}'", - metric_name, last_target_version, target_name, + metric_name, + last_target_version, + target_name, + ), )); }; let field_schema = construct_field_schema( @@ -226,9 +230,11 @@ impl TimeseriesDefinition { if let Some(old) = timeseries .insert((timeseries_name, version), schema) { - return defn_error(format!( - "Timeseries '{}' version {} is duplicated", - old.timeseries_name, old.version, + return Err(MetricsError::SchemaDefinition( + format!( + "Timeseries '{}' version {} is duplicated", + old.timeseries_name, old.version, + ), )); } } @@ -252,11 +258,11 @@ impl TimeseriesDefinition { let Some(target_fields) = target_fields_by_version.get(&last_target_version) else { - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(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, @@ -292,10 +298,10 @@ impl TimeseriesDefinition { if let Some(old) = timeseries.insert((timeseries_name, version), schema) { - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(format!( "Timeseries '{}' version {} is duplicated", old.timeseries_name, old.version, - )); + ))); } } @@ -358,9 +364,11 @@ impl TimeseriesDefinition { if let Some(old) = timeseries .insert((timeseries_name, version), schema) { - return defn_error(format!( + return Err(MetricsError::SchemaDefinition( + format!( "Timeseries '{}' version {} is duplicated", old.timeseries_name, old.version, + ), )); } } @@ -394,16 +402,16 @@ impl CurrentVersion { } } -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())) + .map_err(|e| { + MetricsError::Toml( + slog_error_chain::InlineErrorChain::new(&e).to_string(), + ) + }) .and_then(TimeseriesDefinition::into_schema_list) } @@ -447,11 +455,11 @@ fn extract_authz_scope( }) { Ok(()) } else { - defn_error(format!( + Err(MetricsError::SchemaDefinition(format!( "Metric '{}' has '{}' authorization scope, and so must \ contain a field '{}' of UUID type", metric_name, scope, key, - )) + ))) } }; match authz_scope { @@ -470,11 +478,11 @@ fn construct_field_schema( metric_field_names: &BTreeSet, ) -> Result, MetricsError> { if let Some(dup) = target_fields.intersection(&metric_field_names).next() { - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(format!( "Field '{}' is duplicated between target \ '{}' and metric '{}'", dup, target_name, metric_name, - )); + ))); } let mut field_schema = BTreeSet::new(); @@ -491,11 +499,11 @@ fn construct_field_schema( } else { ("metric", metric_name) }; - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(format!( "Field '{}' is referenced in the {} '{}', but it \ does not appear in the set of all fields.", field_name, kind, name, - )); + ))); }; validate_field_name(field_name, metadata)?; field_schema.insert(FieldSchema { @@ -521,18 +529,18 @@ fn validate_field_name( metadata: &FieldMetadata, ) -> Result<(), MetricsError> { if !is_valid_ident_name(field_name) { - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(format!( "Field name '{}' should be a valid identifier in snake_case", field_name, - )); + ))); } if metadata.type_ == FieldType::Uuid && !(field_name.ends_with("_id") || field_name == "id") { - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(format!( "Uuid field '{}' should end with '_id' or equal 'id'", field_name, - )); + ))); } Ok(()) } @@ -550,49 +558,49 @@ fn extract_metric_fields<'a>( // removed. Bump the version and mark it active, // but there are no fields to return here. if removed_in <= version { - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(format!( "Metric '{}' is removed in version \ {}, which is not strictly after the \ current active version, {}", metric_name, removed_in, version, - )); + ))); } CurrentVersion::Inactive { removed_in: *removed_in } } Some(CurrentVersion::Inactive { removed_in }) => { - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(format!( "Metric '{}' was already removed in \ version {}, it cannot be removed again", metric_name, removed_in, - )); + ))); } None => { - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(format!( "Metric {} has no previous version, \ it cannot be removed.", metric_name, - )); + ))); } } } MetricFields::Added { added_in, fields } => { match current_version { Some(CurrentVersion::Active { .. }) => { - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(format!( "Metric '{}' is already active, it \ cannot be added again until it is removed.", metric_name, - )); + ))); } Some(CurrentVersion::Inactive { removed_in }) => { // The metric is currently inactive, just check // that the newly-active version is greater. if added_in <= removed_in { - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(format!( "Re-added metric '{}' must appear in a later \ version than the one in which it was removed ({})", metric_name, removed_in, - )); + ))); } CurrentVersion::Active { version: *added_in, @@ -615,13 +623,13 @@ fn extract_metric_fields<'a>( // The happy-path, we're stepping the version // and possibly modifying the fields. if new_fields.version <= *version { - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(format!( "Metric '{}' version should increment, \ expected at least {}, found {}", metric_name, version.checked_add(1).expect("version < 256"), new_fields.version, - )); + ))); } CurrentVersion::Active { version: new_fields.version, @@ -634,20 +642,20 @@ fn extract_metric_fields<'a>( Some(CurrentVersion::Inactive { removed_in }) => { // The metric has been removed in the past, it // needs to be added again first. - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(format!( "Metric '{}' was removed in version {}, \ it must be added again first", metric_name, removed_in, - )); + ))); } None => { // The metric never existed, it must be added // first. - return defn_error(format!( + return Err(MetricsError::SchemaDefinition(format!( "Metric '{}' must be added in at its first \ version, and can then be modified", metric_name, - )); + ))); } } } @@ -661,7 +669,9 @@ fn to_unique_field_names( ) -> Result, MetricsError> { let set: BTreeSet<_> = fields.iter().cloned().collect(); if set.len() != fields.len() { - return defn_error(format!("Object '{name}' has duplicate fields")); + return Err(MetricsError::SchemaDefinition(format!( + "Object '{name}' has duplicate fields" + ))); } Ok(set) } diff --git a/oximeter/impl/src/schema/mod.rs b/oximeter/impl/src/schema/mod.rs index 035d05c911b..23b9e547f1a 100644 --- a/oximeter/impl/src/schema/mod.rs +++ b/oximeter/impl/src/schema/mod.rs @@ -355,7 +355,7 @@ pub enum AuthzScope { /// Timeseries data is limited to the authorized projects for a user. Project, /// The timeseries is viewable to all without limitation. - None, + ViewableToAll, } impl AuthzScope {