v2: Support requirement levels on public v2 groups (#1584)

Author: lmolkova

CHANGELOG.md

@@ -4,6 +4,7 @@ All notable changes to this project will be documented in this file.
 
 # Unreleased
 
+- 💥 BREAKING CHANGE 💥 Preserve per-attribute `requirement_level` on attribute refs of public attribute groups in the v2 resolved and materialized schemas. Each entry in an attribute group's `attributes` is now an object (`{ base, requirement_level }`) instead of a bare `attribute_catalog` index. ([#1584](https://github.com/open-telemetry/weaver/pull/1584) by @lmolkova)
 - Use the OS-native certificate store (via ureq's `platform-verifier` feature) to validate TLS connections for remote registry downloads, instead of a fixed bundled root CA list. ([#1583](https://github.com/open-telemetry/weaver/pull/1583) by @jerbly)
 - Fix panic when a registry, policy, or template path uses a commit SHA. ([#1414](https://github.com/open-telemetry/weaver/pull/1414))
 - Add a stats dashboard with charts to the `serve` UI. ([#1570](https://github.com/open-telemetry/weaver/pull/1570) by @jerbly)

crates/weaver_forge/src/v2/attribute_group.rs

@@ -3,7 +3,10 @@
 use crate::v2::provenance::Provenance;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
-use weaver_semconv::v2::{signal_id::SignalId, CommonFields};
+use weaver_semconv::{
+    attribute::RequirementLevel,
+    v2::{signal_id::SignalId, CommonFields},
+};
 
 use crate::v2::attribute::Attribute;
 
@@ -14,7 +17,7 @@ pub struct AttributeGroup {
     /// The name of the attribute group, must be unique.
     pub id: SignalId,
     /// List of attributes.
-    pub attributes: Vec<Attribute>,
+    pub attributes: Vec<AttributeGroupAttribute>,
     /// Common fields (like brief, note, annotations).
     #[serde(flatten)]
     pub common: CommonFields,
@@ -23,3 +26,20 @@ pub struct AttributeGroup {
     #[serde(skip_serializing_if = "Provenance::is_empty")]
     pub provenance: Provenance,
 }
+
+/// An attribute belonging to a public attribute group, carrying the
+/// group-specific requirement level.
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema)]
+#[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))]
+#[serde(deny_unknown_fields)]
+pub struct AttributeGroupAttribute {
+    /// Base attribute definition.
+    #[serde(flatten)]
+    pub base: Attribute,
+    /// The requirement level of the attribute within this group. One of
+    /// "required", "conditionally_required", "recommended" or "opt_in".
+    /// When set to "conditionally_required", the string provided as
+    /// `condition` MUST specify the conditions under which the attribute
+    /// is required.
+    pub requirement_level: RequirementLevel,
+}

crates/weaver_forge/src/v2/registry.rs

@@ -10,7 +10,7 @@ use crate::{
     error::Error,
     v2::{
         attribute::Attribute,
-        attribute_group::AttributeGroup,
+        attribute_group::{AttributeGroup, AttributeGroupAttribute},
         entity::{Entity, EntityAttribute},
         event::{Event, EventAttribute, EventRefinement},
         metric::{Metric, MetricAttribute, MetricRefinement},
@@ -422,17 +422,20 @@ impl ForgeResolvedRegistry {
                 .attributes
                 .iter()
                 .filter_map(|ar| {
-                    let attr = attribute_lookup(ar).map(|a| Attribute {
-                        key: a.key.clone(),
-                        r#type: a.r#type.clone(),
-                        examples: a.examples.clone(),
-                        common: a.common.clone(),
-                        provenance: resolve_provenance(&a.provenance),
+                    let attr = attribute_lookup(&ar.base).map(|a| AttributeGroupAttribute {
+                        base: Attribute {
+                            key: a.key.clone(),
+                            r#type: a.r#type.clone(),
+                            examples: a.examples.clone(),
+                            common: a.common.clone(),
+                            provenance: resolve_provenance(&a.provenance),
+                        },
+                        requirement_level: ar.requirement_level.clone(),
                     });
                     if attr.is_none() {
                         errors.push(Error::AttributeNotFound {
                             group_id: format!("attribute_group.{}", &ag.id),
-                            attr_ref: AttributeRef(ar.0),
+                            attr_ref: AttributeRef(ar.base.0),
                         });
                     }
                     attr
@@ -567,7 +570,17 @@ mod tests {
                     common: CommonFields::default(),
                     provenance: Default::default(),
                 }],
-                attribute_groups: vec![],
+                attribute_groups: vec![v2::attribute_group::AttributeGroup {
+                    id: SignalId::from("my-group".to_owned()),
+                    attributes: vec![v2::attribute_group::AttributeGroupAttributeRef {
+                        base: attribute::AttributeRef(0),
+                        requirement_level: weaver_semconv::attribute::RequirementLevel::Basic(
+                            weaver_semconv::attribute::BasicRequirementLevelSpec::Required,
+                        ),
+                    }],
+                    common: CommonFields::default(),
+                    provenance: Default::default(),
+                }],
             },
             refinements: v2::refinements::Refinements {
                 spans: vec![span::SpanRefinement {
@@ -636,6 +649,18 @@ mod tests {
         assert_eq!(forge_registry.registry.metrics.len(), 1);
         assert_eq!(forge_registry.registry.events.len(), 1);
         assert_eq!(forge_registry.registry.entities.len(), 1);
+        assert_eq!(forge_registry.registry.attribute_groups.len(), 1);
+
+        let group = &forge_registry.registry.attribute_groups[0];
+        assert_eq!(group.id, "my-group".to_owned().into());
+        assert_eq!(group.attributes.len(), 1);
+        assert_eq!(group.attributes[0].base.key, "test.attr");
+        assert_eq!(
+            group.attributes[0].requirement_level,
+            weaver_semconv::attribute::RequirementLevel::Basic(
+                weaver_semconv::attribute::BasicRequirementLevelSpec::Required,
+            )
+        );
         assert_eq!(forge_registry.refinements.spans.len(), 1);
         assert_eq!(forge_registry.refinements.metrics.len(), 1);
         assert_eq!(forge_registry.refinements.events.len(), 1);

crates/weaver_resolved_schema/src/v2/attribute_group.rs

@@ -4,7 +4,10 @@
 
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
-use weaver_semconv::v2::{signal_id::SignalId, CommonFields};
+use weaver_semconv::{
+    attribute::RequirementLevel,
+    v2::{signal_id::SignalId, CommonFields},
+};
 
 use crate::v2::{attribute::AttributeRef, provenance::Provenance, Signal};
 
@@ -23,7 +26,7 @@ pub struct AttributeGroup {
     pub id: SignalId,
 
     /// List of attributes and group references that belong to this group
-    pub attributes: Vec<AttributeRef>,
+    pub attributes: Vec<AttributeGroupAttributeRef>,
 
     /// Common fields (like brief, note, annotations).
     #[serde(flatten)]
@@ -35,6 +38,22 @@ pub struct AttributeGroup {
     pub provenance: Provenance,
 }
 
+/// A reference to an attribute in a public attribute group that remembers the
+/// group-specific requirement level refinement.
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema)]
+#[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))]
+#[serde(deny_unknown_fields)]
+pub struct AttributeGroupAttributeRef {
+    /// Reference, by index, to the attribute catalog.
+    pub base: AttributeRef,
+    /// The requirement level of the attribute within this group. One of
+    /// "required", "conditionally_required", "recommended" or "opt_in".
+    /// When set to "conditionally_required", the string provided as
+    /// `condition` MUST specify the conditions under which the attribute
+    /// is required.
+    pub requirement_level: RequirementLevel,
+}
+
 impl Signal for AttributeGroup {
     fn id(&self) -> &str {
         &self.id

crates/weaver_resolved_schema/src/v2/mod.rs

@@ -529,7 +529,10 @@ pub fn convert_v1_to_v2(
                     // TODO - we need to check lineage and remove parent groups.
                     for attr in g.attributes.iter().filter_map(|a| c.attribute(a)) {
                         if let Some(a) = v2_catalog.convert_ref(attr) {
-                            attributes.push(a);
+                            attributes.push(attribute_group::AttributeGroupAttributeRef {
+                                base: a,
+                                requirement_level: attr.requirement_level.clone(),
+                            });
                         } else {
                             // TODO logic error!
                         }
@@ -1190,6 +1193,79 @@ mod tests {
         }
     }
 
+    #[test]
+    fn test_convert_public_attribute_group_carries_requirement_level() {
+        use weaver_semconv::attribute::{BasicRequirementLevelSpec, RequirementLevel};
+
+        let mut builder = crate::catalog::test_utils::CatalogBuilder::default();
+        // The catalog attribute carries the requirement level that was
+        // resolved from the attribute ref refinement in the public group.
+        let ref0 = builder.add(
+            Attribute {
+                name: "test.key".to_owned(),
+                r#type: weaver_semconv::attribute::AttributeType::PrimitiveOrArray(
+                    weaver_semconv::attribute::PrimitiveOrArrayTypeSpec::String,
+                ),
+                brief: "".to_owned(),
+                examples: None,
+                tag: None,
+                requirement_level: RequirementLevel::Basic(BasicRequirementLevelSpec::Required),
+                sampling_relevant: None,
+                note: "".to_owned(),
+                stability: Some(Stability::Stable),
+                deprecated: None,
+                prefix: false,
+                tags: None,
+                annotations: None,
+                value: None,
+                role: None,
+            },
+            None,
+        );
+        let v1_catalog = builder.build();
+        let v1_registry = crate::registry::Registry {
+            registry_url: "my.schema.url".to_owned(),
+            groups: vec![Group {
+                id: "test.group".to_owned(),
+                r#type: GroupType::AttributeGroup,
+                brief: "a public group".to_owned(),
+                note: "".to_owned(),
+                prefix: "".to_owned(),
+                extends: None,
+                stability: Some(Stability::Stable),
+                deprecated: None,
+                attributes: vec![ref0],
+                span_kind: None,
+                events: vec![],
+                metric_name: None,
+                instrument: None,
+                unit: None,
+                requirement_level: None,
+                name: None,
+                lineage: None,
+                display_name: None,
+                body: None,
+                annotations: None,
+                entity_associations: vec![],
+                visibility: Some(AttributeGroupVisibilitySpec::Public),
+                is_v2: true,
+                span_name: None,
+            }],
+        };
+        let (_, v2_registry, _, _) = convert_v1_to_v2(v1_catalog, v1_registry, BTreeSet::new())
+            .expect("Failed to convert v1 to v2");
+        // The public attribute group is emitted...
+        assert_eq!(v2_registry.attribute_groups.len(), 1);
+        let group = &v2_registry.attribute_groups[0];
+        assert_eq!(group.id, "test.group".to_owned().into());
+        // ...and its attribute ref carries the group-specific requirement level.
+        assert_eq!(group.attributes.len(), 1);
+        assert_eq!(
+            group.attributes[0].requirement_level,
+            RequirementLevel::Basic(BasicRequirementLevelSpec::Required)
+        );
+    }
+
     #[test]
     fn test_try_from_v1_to_v2() {
         let mut dependencies = BTreeSet::new();

crates/weaver_resolver/data/registry-test-published-2/published/resolved_schema.yaml

@@ -16,7 +16,8 @@ registry:
   attribute_groups:
     - id: imported.group.a
       attributes:
-        - 1
+        - base: 1
+          requirement_level: recommended
       brief: a fun group
       note: a group note
       stability: stable

crates/weaver_resolver/data/registry-test-v2-5-published/expected-schema.yaml

@@ -23,7 +23,8 @@ registry:
   attribute_groups:
   - id: local.group
     attributes:
-    - 1
+    - base: 1
+      requirement_level: required
     brief: local group
     stability: stable
   spans:

crates/weaver_resolver/data/registry-test-v2-5-published/published/resolved_schema.yaml

@@ -16,7 +16,8 @@ registry:
   attribute_groups:
     - id: imported.group.a
       attributes:
-        - 1
+        - base: 1
+          requirement_level: recommended
       brief: a fun group
       note: a group note
       stability: stable

crates/weaver_resolver/data/registry-test-v2-5-published/registry/main.yaml

@@ -26,6 +26,7 @@ attribute_groups:
   stability: stable
   attributes:
   - ref: local.group.attr
+    requirement_level: required
 imports:
   spans:
     - imported.*

crates/weaver_resolver/src/dependency.rs

@@ -935,15 +935,15 @@ impl ImportableDependency for V2Schema {
             }
             let mut attributes = vec![];
             for ar in ag.attributes.iter() {
-                let attr = self.attribute_catalog.attribute(ar).ok_or(
+                let attr = self.attribute_catalog.attribute(&ar.base).ok_or(
                     Error::InvalidRegistryAttributeRef {
                         registry_name: self.schema_url.name().to_owned(),
-                        attribute_ref: ar.0,
+                        attribute_ref: ar.base.0,
                     },
                 )?;
                 let source = get_attribute_source(attr);
                 attributes.push(attribute_catalog.attribute_ref_with_provenance(
-                    convert_v2_attribute(attr, RequirementLevel::default(), None),
+                    convert_v2_attribute(attr, ar.requirement_level.clone(), None),
                     source,
                     cache_lookup,
                 )?);
@@ -1394,7 +1394,18 @@ mod tests {
                 attribute_groups: vec![
                     weaver_resolved_schema::v2::attribute_group::AttributeGroup {
                         id: "attribute_group.e".to_owned().into(),
-                        attributes: vec![],
+                        // A public group whose attribute ref carries a
+                        // non-default requirement level; importing the group
+                        // must preserve it.
+                        attributes: vec![
+                            weaver_resolved_schema::v2::attribute_group::AttributeGroupAttributeRef {
+                                base: weaver_resolved_schema::v2::attribute::AttributeRef(0),
+                                requirement_level:
+                                    weaver_semconv::attribute::RequirementLevel::Basic(
+                                        weaver_semconv::attribute::BasicRequirementLevelSpec::Required,
+                                    ),
+                            },
+                        ],
                         common: Default::default(),
                         provenance: Default::default(),
                     },
@@ -1439,7 +1450,15 @@ mod tests {
                 }],
                 attributes: vec![],
             },
-            attribute_catalog: vec![],
+            attribute_catalog: vec![weaver_resolved_schema::v2::attribute::Attribute {
+                key: "attr.in.group".to_owned(),
+                r#type: weaver_semconv::attribute::AttributeType::PrimitiveOrArray(
+                    weaver_semconv::attribute::PrimitiveOrArrayTypeSpec::String,
+                ),
+                examples: None,
+                common: Default::default(),
+                provenance: Default::default(),
+            }],
             refinements: weaver_resolved_schema::v2::refinements::Refinements {
                 spans: vec![],
                 metrics: vec![],
@@ -1570,6 +1589,26 @@ mod tests {
             "Should import metric, event, entity, span and attribute_group"
         );
 
+        // The imported public attribute group must preserve the per-attribute
+        // requirement level authored on its ref (rather than resetting it to
+        // the default).
+        let group = result
+            .iter()
+            .find(|g| g.group.id == "attribute_group.e")
+            .expect("attribute_group.e should be imported")
+            .group
+            .clone();
+        assert_eq!(group.attributes.len(), 1);
+        let attr = catalog
+            .attribute(&group.attributes[0])
+            .expect("imported attribute should exist in the catalog");
+        assert_eq!(
+            attr.requirement_level,
+            weaver_semconv::attribute::RequirementLevel::Basic(
+                weaver_semconv::attribute::BasicRequirementLevelSpec::Required,
+            )
+        );
+
         Ok(())
     }