Skip to content

Commit

Permalink
feat(resolve): improve error reporting
Browse files Browse the repository at this point in the history
  • Loading branch information
lquerel committed Feb 27, 2024
1 parent 823371e commit bf38910
Show file tree
Hide file tree
Showing 5 changed files with 100 additions and 56 deletions.
41 changes: 31 additions & 10 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions crates/weaver_resolved_schema/src/catalog.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,9 @@ pub enum Stability {
impl Catalog {
/// Returns the attribute name from an attribute ref if it exists
/// in the catalog or None if it does not exist.
pub fn attribute_name(&self, attribute_ref: &AttributeRef) -> Option<String> {
pub fn attribute_name(&self, attribute_ref: &AttributeRef) -> Option<&str> {
self.attributes
.get(attribute_ref.0 as usize)
.map(|attr| attr.name.clone())
.map(|attr| attr.name.as_ref())
}
}
10 changes: 9 additions & 1 deletion crates/weaver_resolved_schema/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ pub enum TypedGroup {
}

/// Allow to define additional requirements on the semantic convention.
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Hash, Eq)]
#[serde(deny_unknown_fields)]
pub struct Constraint {
/// any_of accepts a list of sequences. Each sequence contains a list of
Expand Down Expand Up @@ -170,4 +170,12 @@ impl Group {
c.include.is_none() || !include_to_remove.contains(c.include.as_ref().unwrap())
});
}

/// Returns the provenance of the group.
pub fn provenance(&self) -> &str {
match &self.lineage {
Some(lineage) => lineage.provenance(),
None => "unknown",
}
}
}
26 changes: 13 additions & 13 deletions crates/weaver_resolver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ use walkdir::DirEntry;

use weaver_cache::Cache;
use weaver_logger::Logger;
use weaver_resolved_schema::attribute::AttributeRef;
use weaver_resolved_schema::catalog::Catalog;
use weaver_resolved_schema::registry::Constraint;
use weaver_resolved_schema::ResolvedTelemetrySchema;
use weaver_schema::{SemConvImport, TelemetrySchema};
use weaver_semconv::{ResolverConfig, SemConvRegistry, SemConvSpec, SemConvSpecWithProvenance};
Expand Down Expand Up @@ -148,22 +148,22 @@ pub enum Error {
},

/// The `any_of` constraints are unsatisfied for a group.
#[error("The `any_of` constraints are unsatisfied for the group '{group_id}'.\nGroup attributes: {group_attributes:#?}\n`any_of` constraints: {any_of_constraints:#?}")]
UnsatisfiedAnyOfConstraint {
#[error("Some `any_of` constraints are unsatisfied for the group '{group_id}'.\nUnsatisfied `any_of` constraints: {any_of_constraints:#?}")]
UnsatisfiedAnyOfConstraints {
/// The id of the group containing the unsatisfied `any_of` constraint.
group_id: String,
/// The attributes of the group.
group_attributes: Vec<String>,
/// The `any_of` constraints of the group.
any_of_constraints: Vec<Vec<String>>,
/// The `any_of` constraints of the group that are not satisfied.
any_of_constraints: Vec<UnsatisfiedAnyOfConstraint>,
},
}

/// Attribute ref not found in the catalog.
#[error("Attribute ref `{attribute_ref:?}` not found in the catalog.")]
UnresolvedAttribute {
/// The unresolved attribute reference.
attribute_ref: AttributeRef,
},
/// A constraint that is not satisfied and its missing attributes.
#[derive(Debug)]
pub struct UnsatisfiedAnyOfConstraint {
/// The `any_of` constraint that is not satisfied.
pub any_of: Constraint,
/// The detected missing attributes.
pub missing_attributes: Vec<String>,
}

impl SchemaResolver {
Expand Down
75 changes: 45 additions & 30 deletions crates/weaver_resolver/src/registry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@

//! Functions to resolve a semantic convention registry.
use serde::Deserialize;
use std::collections::{HashMap, HashSet};

use serde::Deserialize;

use weaver_resolved_schema::attribute::UnresolvedAttribute;
use weaver_resolved_schema::lineage::{FieldId, FieldLineage, GroupLineage, ResolutionMode};
use weaver_resolved_schema::registry::{Constraint, Group, Registry, TypedGroup};
Expand All @@ -17,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};
use crate::{Error, UnresolvedReference, UnsatisfiedAnyOfConstraint};

/// A registry containing unresolved groups.
#[derive(Debug, Deserialize)]
Expand Down Expand Up @@ -161,17 +162,22 @@ pub fn check_any_of_constraints(
registry: &Registry,
attr_name_index: &[String],
) -> Result<(), Error> {
let mut unresolved_refs = 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() {
let attr_name =
attr_name_index
.get(attr_ref.0 as usize)
.ok_or(Error::UnresolvedAttribute {
attribute_ref: *attr_ref,
})?;
group_attr_names.insert(attr_name.clone());
match attr_name_index.get(attr_ref.0 as usize) {
None => unresolved_refs.push(UnresolvedReference::AttributeRef {
group_id: group.id.clone(),
attribute_ref: attr_ref.0.to_string(),
provenance: group.provenance().to_string(),
}),
Some(attr_name) => {
group_attr_names.insert(attr_name.clone());
}
}
}

check_group_any_of_constraints(
Expand All @@ -181,6 +187,12 @@ pub fn check_any_of_constraints(
)?;
}

if !unresolved_refs.is_empty() {
return Err(Error::UnresolvedReferences {
refs: unresolved_refs,
});
}

Ok(())
}

Expand All @@ -190,35 +202,38 @@ fn check_group_any_of_constraints(
group_attr_names: HashSet<String>,
constraints: &[Constraint],
) -> Result<(), Error> {
let mut any_of_unsatisfied = 0;
let mut any_of_total = 0;
let mut any_of_constraints = vec![];
'outer: for constraint in constraints.iter() {
let mut unsatisfied_any_of_constraints: HashMap<&Constraint, Vec<String>> = HashMap::new();

for constraint in constraints.iter() {
if constraint.any_of.is_empty() {
continue;
}
any_of_total += 1;

// Check if the group satisfies the `any_of` constraint.
any_of_constraints.push(constraint.any_of.clone());
for attr_name in constraint.any_of.iter() {
if !group_attr_names.contains(attr_name) {
// The any_of constraint is not satisfied.
// Continue to the next constraint.
any_of_unsatisfied += 1;
continue 'outer;
}
if let Some(attr) = constraint
.any_of
.iter()
.find(|name| !group_attr_names.contains(*name))
{
// The any_of constraint is not satisfied.
// Insert the attribute into the list of missing attributes for the
// constraint.
unsatisfied_any_of_constraints
.entry(constraint)
.or_default()
.push(attr.clone());
}
}
if any_of_total > 0 && any_of_total == any_of_unsatisfied {
let group_attributes: Vec<String> = group_attr_names
.iter()
.map(|name| name.to_string())
.collect();
return Err(Error::UnsatisfiedAnyOfConstraint {
if !unsatisfied_any_of_constraints.is_empty() {
return Err(Error::UnsatisfiedAnyOfConstraints {
group_id: group_id.to_string(),
group_attributes,
any_of_constraints,
any_of_constraints: unsatisfied_any_of_constraints
.into_iter()
.map(|(c, attrs)| UnsatisfiedAnyOfConstraint {
any_of: c.clone(),
missing_attributes: attrs,
})
.collect(),
});
}
Ok(())
Expand Down

0 comments on commit bf38910

Please sign in to comment.