From 96672f6aa20fef97b6dc2c0a05aabec14a048b7b Mon Sep 17 00:00:00 2001
From: Liudmila Molkova <limolkova@microsoft.com>
Date: Wed, 24 Jul 2024 11:29:49 -0700
Subject: [PATCH] clean up

---
 policies/attribute_name_collisions.rego | 13 +++++++----
 policies/registry.rego                  | 31 +++++++++++++++++++++----
 2 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/policies/attribute_name_collisions.rego b/policies/attribute_name_collisions.rego
index 7047fa9363..bbe2157a3b 100644
--- a/policies/attribute_name_collisions.rego
+++ b/policies/attribute_name_collisions.rego
@@ -4,20 +4,20 @@ deny[attr_registry_collision(description, name)] {
     names := attr_names_except(excluded_const_collisions)
     name := names[_]
     const_name := to_const_name(name)
-    collisions:= { n | n := attr_names_except(excluded_const_collisions)[_]; n != name; to_const_name(n) == const_name}
+    collisions:= { n | n := attr_names_except(excluded_const_collisions)[_]; n != name; to_const_name(n) == const_name }
     count(collisions) > 0
 
-    description := sprintf("Attribute '%s' has the same constant name '%s' as %s", [name, const_name, collisions])
+    description := sprintf("Attribute '%s' has the same constant name '%s' as '%s'.", [name, const_name, collisions])
 }
 
 deny[attr_registry_collision(description, name)] {
     names := attr_names_except(excluded_namespace_collisions)
     name := names[_]
 
-    collisions:= { n | n := input.groups[_].attributes[_].name; startswith(n, to_namespace_prefix(name))}
+    collisions:= { n | n := input.groups[_].attributes[_].name; startswith(n, to_namespace_prefix(name)) }
     count(collisions) > 0
 
-    description := sprintf("Attribute '%s' is used as a namespace in attributes %s", [name, collisions])
+    description := sprintf("Attribute '%s' name is used as a namespace in the following attributes '%s'.", [name, collisions])
 }
 
 attr_registry_collision(description, attr_name) = violation {
@@ -39,8 +39,11 @@ to_const_name(name) = const_name {
 }
 
 attr_names_except(excluded) = names {
-    names := {n | n := input.groups[_].attributes[_].name} - excluded
+    names := { n | n := input.groups[_].attributes[_].name } - excluded
 }
 
+# TODO - we'll need to specify how collision resolution happens in the schema -
+# see phase 2 in https://github.com/open-telemetry/semantic-conventions/issues/1118#issuecomment-2173803006
+# For now just allow current collisions.
 excluded_const_collisions := {"messaging.client_id"}
 excluded_namespace_collisions := {"messaging.operation", "db.operation", "deployment.environment"}
diff --git a/policies/registry.rego b/policies/registry.rego
index 632279015c..19de331a48 100644
--- a/policies/registry.rego
+++ b/policies/registry.rego
@@ -5,9 +5,9 @@ package before_resolution
 # used by semantic conventions.
 
 # Helper to create attribute registry violations.
-attr_registry_violation(violation_id, group_id, attr_id) = violation {
+attr_registry_violation(description, group_id, attr_id) = violation {
     violation := {
-        "id": violation_id,
+        "id": description,
         "type": "semconv_attribute",
         "category": "attribute_registry_checks",
         "group": group_id,
@@ -16,27 +16,48 @@ attr_registry_violation(violation_id, group_id, attr_id) = violation {
 }
 
 # We only allow attribute groups in the attribute registry.
-deny[attr_registry_violation("attribute_registry_can_only_contain_attribute_groups", group.id, "")] {
+deny[attr_registry_violation(description, group.id, "")] {
     group := input.groups[_]
     startswith(group.id, "registry.")
     group.type != "attribute_group"
+
+    # TODO - separate violation_id and description once weaver supports it.
+    # violation_id := "attribute_registry_can_only_contain_attribute_groups"
+    description := sprintf("Registry group '%s' has invalid type '%s'. Groups in attribute registry must have `attribute_group` type.", [group.id, group.type])
 }
 
 # Any group that is NOT in the attribute registry that has an attribute id is
 # in violation of not using the attribute registry.
-deny[attr_registry_violation("attributes_must_be_defined_in_attribute_registry", group.id, attr.id)] {
+deny[attr_registry_violation(description, group.id, attr.id)] {
     group := input.groups[_]
     not startswith(group.id, "registry.")
     attr := group.attributes[_]
     attr.id != null
+
+    attr_name := get_attribute_name(attr, group)
+
+    # TODO - separate violation_id and description once weaver supports it.
+    # violation_id := "attributes_must_be_defined_in_attribute_registry"
+    description := sprintf("Attribute '%s' is defined in the group '%s' which is not part of the attribute registy. Attributes can be defined in the registry group only.", [attr_name, group.id])
 }
 
 # A registry `attribute_group` containing at least one `ref` attribute is
 # considered invalid if it's not in the registry group.
-deny[attr_registry_violation("attributes_in_registry_cannot_reference_each_other", group.id, attr.ref)] {
+deny[attr_registry_violation(description, group.id, attr.ref)] {
     # TODO - this will need to be updated to support `embed` in the future.
     group := input.groups[_]
     startswith(group.id, "registry.")
     attr := group.attributes[_]
     attr.ref != null
+
+    # TODO - separate violation_id and description once weaver supports it.
+    # violation_id := "attributes_in_registry_cannot_reference_each_other"
+    description := sprintf("Registy group '%s' references attribute '%s'. Registry groups can only define new attributes.", [group.id, attr.ref])
+}
+
+get_attribute_name(attr, group) = name {
+    full_name = concat(".", [group.prefix, attr.id])
+
+    # if there was no prefix, we have a leading dot
+    name := trim(full_name, ".")
 }