From cbb8e7aab36af7d592ec4f8956ab196a85007c6b Mon Sep 17 00:00:00 2001 From: Laurent Querel Date: Tue, 27 Feb 2024 14:30:15 -0800 Subject: [PATCH] feat(resolve): improve error reporting --- crates/weaver_resolver/src/lib.rs | 114 +++++++++++++++---------- crates/weaver_resolver/src/registry.rs | 60 ++++++------- 2 files changed, 101 insertions(+), 73 deletions(-) diff --git a/crates/weaver_resolver/src/lib.rs b/crates/weaver_resolver/src/lib.rs index 486ce3e8..6beeef0a 100644 --- a/crates/weaver_resolver/src/lib.rs +++ b/crates/weaver_resolver/src/lib.rs @@ -47,38 +47,6 @@ mod tags; /// All references to semantic conventions will be resolved. pub struct SchemaResolver {} -/// Different types of unresolved references. -#[derive(Debug)] -pub enum UnresolvedReference { - /// An unresolved attribute reference. - AttributeRef { - /// The id of the group containing the attribute reference. - group_id: String, - /// The unresolved attribute reference. - attribute_ref: String, - /// The provenance of the reference (URL or path). - provenance: String, - }, - /// An unresolved `extends` clause reference. - ExtendsRef { - /// The id of the group containing the `extends` clause reference. - group_id: String, - /// The unresolved `extends` clause reference. - extends_ref: String, - /// The provenance of the reference (URL or path). - provenance: String, - }, - /// An unresolved `include` reference. - IncludeRef { - /// The id of the group containing the `include` reference. - group_id: String, - /// The unresolved `include` reference. - include_ref: String, - /// The provenance of the reference (URL or path). - provenance: String, - }, -} - /// An error that can occur while resolving a telemetry schema. #[derive(thiserror::Error, Debug)] pub enum Error { @@ -115,13 +83,6 @@ pub enum Error { error: String, }, - /// Failed to resolve a set of references. - #[error("Failed to resolve the following references {refs:?}")] - UnresolvedReferences { - /// The list of unresolved references. - refs: Vec, - }, - /// Failed to resolve a metric. #[error("Failed to resolve the metric '{r#ref}'")] FailToResolveMetric { @@ -147,14 +108,53 @@ pub enum Error { message: String, }, - /// The `any_of` constraints are unsatisfied for a group. - #[error("Some `any_of` constraints are unsatisfied for the group '{group_id}'.\nUnsatisfied `any_of` constraints: {any_of_constraints:#?}")] - UnsatisfiedAnyOfConstraints { + /// An unresolved attribute reference. + #[error("The following attribute reference is not resolved for the group '{group_id}'.\nAttribute reference: {attribute_ref}\nProvenance: {provenance}")] + UnresolvedAttributeRef { + /// The id of the group containing the attribute reference. + group_id: String, + /// The unresolved attribute reference. + attribute_ref: String, + /// The provenance of the reference (URL or path). + provenance: String, + }, + + /// An unresolved `extends` clause reference. + #[error("The following `extends` clause reference is not resolved for the group '{group_id}'.\n`extends` clause reference: {extends_ref}\nProvenance: {provenance}")] + UnresolvedExtendsRef { + /// The id of the group containing the `extends` clause reference. + group_id: String, + /// The unresolved `extends` clause reference. + extends_ref: String, + /// The provenance of the reference (URL or path). + provenance: String, + }, + + /// An unresolved `include` reference. + #[error("The following `include` reference is not resolved for the group '{group_id}'.\n`include` reference: {include_ref}\nProvenance: {provenance}")] + UnresolvedIncludeRef { + /// The id of the group containing the `include` reference. + group_id: String, + /// The unresolved `include` reference. + include_ref: String, + /// The provenance of the reference (URL or path). + provenance: String, + }, + + /// An `any_of` constraint that is not satisfied for a group. + #[error("The following `any_of` constraint is not satisfied for the group '{group_id}'.\n`any_of` constraint: {any_of:#?}\nMissing attributes: {missing_attributes:?}")] + UnsatisfiedAnyOfConstraint { /// The id of the group containing the unsatisfied `any_of` constraint. group_id: String, - /// The `any_of` constraints of the group that are not satisfied. - any_of_constraints: Vec, + /// The `any_of` constraint that is not satisfied. + any_of: Constraint, + /// The detected missing attributes. + missing_attributes: Vec, }, + + /// A container for multiple errors. + #[error("{:?}", Error::format_errors(.0))] + CompoundError(Vec), } /// A constraint that is not satisfied and its missing attributes. @@ -166,6 +166,32 @@ pub struct UnsatisfiedAnyOfConstraint { pub missing_attributes: Vec, } +impl Error { + /// Creates a compound error from a list of errors. + /// Note: All compound errors are flattened. + pub fn compound_error(errors: Vec) -> Error { + Error::CompoundError( + errors + .into_iter() + .flat_map(|e| match e { + Error::CompoundError(errors) => errors, + e => vec![e], + }) + .collect(), + ) + } + + /// Formats the given errors into a single string. + /// This used to render compound errors. + pub fn format_errors(errors: &[Error]) -> String { + errors + .iter() + .map(|e| e.to_string()) + .collect::>() + .join("\n\n") + } +} + impl SchemaResolver { /// Loads a telemetry schema from an URL or a file and returns the resolved /// schema. diff --git a/crates/weaver_resolver/src/registry.rs b/crates/weaver_resolver/src/registry.rs index 2aa2d4f4..87601049 100644 --- a/crates/weaver_resolver/src/registry.rs +++ b/crates/weaver_resolver/src/registry.rs @@ -18,7 +18,7 @@ use crate::constraint::resolve_constraints; use crate::metrics::resolve_instrument; use crate::spans::resolve_span_kind; use crate::stability::resolve_stability; -use crate::{Error, UnresolvedReference, UnsatisfiedAnyOfConstraint}; +use crate::{Error, UnsatisfiedAnyOfConstraint}; /// A registry containing unresolved groups. #[derive(Debug, Deserialize)] @@ -84,11 +84,11 @@ pub fn resolve_semconv_registry( // If the resolution process fails, then we build an error containing all // the unresolved references. if !all_refs_resolved { - let mut unresolved_refs = vec![]; + let mut errors = vec![]; for group in ureg.groups.iter() { // Collect unresolved `extends` references. if let Some(extends) = group.group.extends.as_ref() { - unresolved_refs.push(UnresolvedReference::ExtendsRef { + errors.push(Error::UnresolvedExtendsRef { group_id: group.group.id.clone(), extends_ref: extends.clone(), provenance: group.provenance.clone(), @@ -97,7 +97,7 @@ pub fn resolve_semconv_registry( // Collect unresolved `ref` attributes. for attr in group.attributes.iter() { if let AttributeSpec::Ref { r#ref, .. } = &attr.spec { - unresolved_refs.push(UnresolvedReference::AttributeRef { + errors.push(Error::UnresolvedAttributeRef { group_id: group.group.id.clone(), attribute_ref: r#ref.clone(), provenance: group.provenance.clone(), @@ -107,7 +107,7 @@ pub fn resolve_semconv_registry( // Collect unresolved `include` constraints. for constraint in group.group.constraints.iter() { if let Some(include) = &constraint.include { - unresolved_refs.push(UnresolvedReference::IncludeRef { + errors.push(Error::UnresolvedIncludeRef { group_id: group.group.id.clone(), include_ref: include.clone(), provenance: group.provenance.clone(), @@ -115,10 +115,8 @@ pub fn resolve_semconv_registry( } } } - if !unresolved_refs.is_empty() { - return Err(Error::UnresolvedReferences { - refs: unresolved_refs, - }); + if !errors.is_empty() { + return Err(Error::CompoundError(errors)); } } @@ -162,14 +160,14 @@ pub fn check_any_of_constraints( registry: &Registry, attr_name_index: &[String], ) -> Result<(), Error> { - let mut unresolved_refs = vec![]; + let mut errors = vec![]; for group in registry.groups.iter() { // Build a list of attribute names for the group. let mut group_attr_names = HashSet::new(); for attr_ref in group.attributes.iter() { match attr_name_index.get(attr_ref.0 as usize) { - None => unresolved_refs.push(UnresolvedReference::AttributeRef { + None => errors.push(Error::UnresolvedAttributeRef { group_id: group.id.clone(), attribute_ref: attr_ref.0.to_string(), provenance: group.provenance().to_string(), @@ -180,17 +178,17 @@ pub fn check_any_of_constraints( } } - check_group_any_of_constraints( + if let Err(e) = check_group_any_of_constraints( group.id.as_ref(), group_attr_names, group.constraints.as_ref(), - )?; + ) { + errors.push(e); + } } - if !unresolved_refs.is_empty() { - return Err(Error::UnresolvedReferences { - refs: unresolved_refs, - }); + if !errors.is_empty() { + return Err(Error::CompoundError(errors)); } Ok(()) @@ -202,7 +200,8 @@ fn check_group_any_of_constraints( group_attr_names: HashSet, constraints: &[Constraint], ) -> Result<(), Error> { - let mut unsatisfied_any_of_constraints: HashMap<&Constraint, Vec> = HashMap::new(); + let mut unsatisfied_any_of_constraints: HashMap<&Constraint, UnsatisfiedAnyOfConstraint> = + HashMap::new(); for constraint in constraints.iter() { if constraint.any_of.is_empty() { @@ -220,21 +219,24 @@ fn check_group_any_of_constraints( // constraint. unsatisfied_any_of_constraints .entry(constraint) - .or_default() + .or_insert_with(|| UnsatisfiedAnyOfConstraint { + any_of: constraint.clone(), + missing_attributes: vec![], + }) + .missing_attributes .push(attr.clone()); } } if !unsatisfied_any_of_constraints.is_empty() { - return Err(Error::UnsatisfiedAnyOfConstraints { - group_id: group_id.to_string(), - any_of_constraints: unsatisfied_any_of_constraints - .into_iter() - .map(|(c, attrs)| UnsatisfiedAnyOfConstraint { - any_of: c.clone(), - missing_attributes: attrs, - }) - .collect(), - }); + let errors = unsatisfied_any_of_constraints + .into_values() + .map(|v| Error::UnsatisfiedAnyOfConstraint { + group_id: group_id.to_string(), + any_of: v.any_of, + missing_attributes: v.missing_attributes, + }) + .collect(); + return Err(Error::CompoundError(errors)); } Ok(()) }