Check attribute/metric/event name format (#1302)

.chloggen/1302.yaml

@@ -0,0 +1,4 @@
+change_type: enhancement
+component: docs
+note: Add note on tooling limitations for attribute names and enforce name format.
+issues: [1124]

Makefile

@@ -226,6 +226,7 @@ check-policies:
 test-policies:
 	docker run --rm -v $(PWD)/policies:/policies -v $(PWD)/policies_test:/policies_test \
 	${OPA_CONTAINER} test \
+    --var-values \
 	--explain fails \
 	/policies \
 	/policies_test

docs/general/attribute-naming.md

@@ -116,6 +116,12 @@ denote old attribute names in rename operations).
   the following Unicode code points: Latin alphabet, Numeric, Underscore, Dot
   (as namespace delimiter).
 
+> Note:
+> Semantic Conventions tooling limits names to lowercase
+> Latin alphabet, Numeric, Underscore, Dot (as namespace delimiter).
+> Names must start with a letter, end with an alphanumeric character, and must not
+> contain two or more consecutive delimiters (Underscore or Dot).
+
 ## Recommendations for Application Developers
 
 As an application developer when you need to record an attribute first consult

policies/attribute_name_collisions.rego

@@ -4,12 +4,12 @@ import rego.v1
 
 # Data structures to make checking things faster.
 attribute_names := { obj |
-  group := input.groups[_];
-  attr := group.attributes[_];
-  obj := { "name": attr.name, "const_name": to_const_name(attr.name), "namespace_prefix": to_namespace_prefix(attr.name) }
+    group := input.groups[_];
+    attr := group.attributes[_];
+    obj := { "name": attr.name, "const_name": to_const_name(attr.name), "namespace_prefix": to_namespace_prefix(attr.name) }
 }
 
-
+# check that attribute constant names do not collide
 deny contains attr_registry_collision(description, name) if {
     some i
     name := attribute_names[i].name
@@ -26,6 +26,7 @@ deny contains attr_registry_collision(description, name) if {
     description := sprintf("Attribute '%s' has the same constant name '%s' as '%s'.", [name, const_name, collisions])
 }
 
+# check that attribute names do not collide with namespaces
 deny contains attr_registry_collision(description, name) if {
     some i
     name := attribute_names[i].name
@@ -38,7 +39,19 @@ deny contains attr_registry_collision(description, name) if {
     ]
     count(collisions) > 0
     # TODO (https://github.com/open-telemetry/weaver/issues/279): provide other violation properties once weaver supports it.
-    description := sprintf("Attribute '%s' is used as a namespace in '%s'.", [name, collisions])
+    description := sprintf("Attribute '%s' name is used as a namespace in the following attributes '%s'.", [name, collisions])
+}
+
+# check that attribute is not defined or referenced more than once within the same group
+deny contains attr_registry_collision(description, name) if {
+    group := input.groups[_]
+    attr := group.attributes[_]
+    name := attr.name
+
+    collisions := [n | n := group.attributes[_].name; n == name ]
+    count(collisions) > 1
+
+    description := sprintf("Attribute '%s' is already defined in the group '%s'. Attributes must be unique.", [name, group.id])
 }
 
 attr_registry_collision(description, attr_name) = violation if {

policies/attribute_name_collisions_test.rego

@@ -0,0 +1,19 @@
+package after_resolution
+import future.keywords
+
+test_fails_on_const_name_collision if {
+    collision := {"groups": [
+        {"id": "test1", "attributes": [{"name": "foo.bar.baz"}]},
+        {"id": "test2", "attributes": [{"name": "foo.bar_baz"}]}
+    ]}
+    # each attribute counts as a collision, so there are 2 collisions
+    count(deny) == 2 with input as collision
+}
+
+test_fails_on_namespace_collision if {
+    collision := {"groups": [
+        {"id": "test1", "attributes": [{"name": "foo.bar.baz"}]},
+        {"id": "test2", "attributes": [{"name": "foo.bar"}]}
+    ]}
+    count(deny) == 1 with input as collision
+}

policies/yaml_schema.rego

@@ -1,5 +1,62 @@
 package before_resolution
 
+# checks attribute name format
+deny[yaml_schema_violation(description, group.id, name)] {
+    group := input.groups[_]
+    attr := group.attributes[_]
+    name := attr.id
+
+    not regex.match(name_regex, name)
+
+    description := sprintf("Attribute name '%s' is invalid. Attribute name %s", [name, invalid_name_helper])
+}
+
+# checks metric name format
+deny[yaml_schema_violation(description, group.id, name)] {
+    group := input.groups[_]
+    name := group.metric_name
+
+    name != null
+    not regex.match(name_regex, name)
+
+    description := sprintf("Metric name '%s' is invalid. Metric name %s'", [name, invalid_name_helper])
+}
+
+# checks event name format
+deny[yaml_schema_violation(description, group.id, name)] {
+    group := input.groups[_]
+    group.type == "event"
+    name := group.name
+
+    name != null
+    not regex.match(name_regex, name)
+
+    description := sprintf("Event name '%s' is invalid. Event name %s'", [name, invalid_name_helper])
+}
+
+# checks attribute member id format
+deny[yaml_schema_violation(description, group.id, attr_name)] {
+    group := input.groups[_]
+    attr := group.attributes[_]
+    attr_name := attr.id
+    name := attr.type.members[_].id
+
+    not regex.match(name_regex, name)
+
+    description := sprintf("Member id '%s' on attribute '%s' is invalid. Member id %s'", [name, attr_name, invalid_name_helper])
+}
+
+# check that attribute is fully qualified with their id, prefix is no longer supported
+deny[yaml_schema_violation(description, group.id, "")] {
+    group := input.groups[_]
+
+    group.prefix != null
+    group.prefix != ""
+
+    # TODO (https://github.com/open-telemetry/weaver/issues/279): provide other violation properties once weaver supports it.
+    description := sprintf("Group '%s' uses prefix '%s'. All attribute should be fully qualified with their id, prefix is no longer supported.", [group.id, group.prefix])
+}
+
 yaml_schema_violation(description, group, attr) = violation {
     violation := {
         "id": description,
@@ -10,12 +67,8 @@ yaml_schema_violation(description, group, attr) = violation {
     }
 }
 
-deny[yaml_schema_violation(description, group.id, "")] {
-    group := input.groups[_]
+# not valid: '1foo.bar', 'foo.bar.', 'foo.bar_', 'foo..bar', 'foo._bar' ...
+# valid: 'foo.bar', 'foo.1bar', 'foo.1_bar'
+name_regex := "^[a-z][a-z0-9]*([._][a-z0-9]+)*$"
 
-    group.prefix != null
-    group.prefix != ""
-
-    # TODO (https://github.com/open-telemetry/weaver/issues/279): provide other violation properties once weaver supports it.
-    description := sprintf("Group '%s' uses prefix '%s'. All attribute should be fully qualified with their id, prefix is no longer supported.", [group.id, group.prefix])
-}
+invalid_name_helper := "must consist of lowercase alphanumeric characters separated by '_' and '.'"

policies/yaml_schema_test.rego

@@ -0,0 +1,67 @@
+package before_resolution
+
+import future.keywords
+
+test_fails_on_invalid_attribute_name if {
+    every name in invalid_names {
+        count(deny) == 1 with input as {"groups": create_attribute_group(name)}
+    }
+}
+
+test_fails_on_invalid_metric_name if {
+    every name in invalid_names {
+        count(deny) == 1 with input as {"groups": create_metric(name)}
+    }
+}
+
+test_fails_on_invalid_event_name if {
+    every name in invalid_names {
+        count(deny) == 1 with input as {"groups": create_event(name)}
+    }
+}
+
+test_passes_on_valid_names if {
+    every name in valid_names {
+        count(deny) == 0 with input as {"groups": create_attribute_group(name)}
+        count(deny) == 0 with input as {"groups": create_metric(name)}
+        count(deny) == 0 with input as {"groups": create_event(name)}
+    }
+}
+
+test_fails_if_prefix_is_present if {
+    count(deny) == 1 with input as {"groups": [{"id": "test", "prefix": "foo"}]}
+}
+
+create_attribute_group(attr) = json {
+    json := [{"id": "yaml_schema.test", "attributes": [{"id": attr}]}]
+}
+
+create_metric(name) = json {
+    json := [{"id": "yaml_schema.test", "type": "metric", "metric_name": name}]
+}
+
+create_event(name) = json {
+    json := [{"id": "yaml_schema.test", "type": "event", "name": name}]
+}
+
+invalid_names := [
+    "1foo.bar",
+    "_foo.bar",
+    ".foo.bar",
+    "foo.bar_",
+    "foo.bar.",
+    "foo..bar",
+    "foo._bar",
+    "foo__bar",
+    "foo_.bar",
+    "foo,bar",
+    "fü.bär",
+]
+
+valid_names := [
+    "foo.bar",
+    "foo.1bar",
+    "foo_1.bar",
+    "foo.bar.baz",
+    "foo.bar_baz",
+]