Adds required stability property to enum members (#267)

Co-authored-by: Joao Grassi <5938087+joaopgrassi@users.noreply.github.com>

semantic-conventions/CHANGELOG.md

@@ -4,6 +4,8 @@ Please update the changelog as part of any significant pull request.
 
 ## Unreleased
 
+- BREAKING: Add `stability` (required) and `deprecated` (optional) properties to `EnumMember`
+  ([#267](https://github.com/open-telemetry/build-tools/pull/267))
 - Change minimum support python version to 3.10 in setup.cfg and Dockerfile
   ([#285](https://github.com/open-telemetry/build-tools/pull/285))
 - BREAKING: Make stability required (also: fix ref and extends, render badges on metrics).

semantic-conventions/README.md

@@ -133,13 +133,18 @@ The `{semconv version}` (e.g. `1.24.0`) is the previously released version of se
 Following checks are performed
 
 - On all attributes and metrics (experimental and stable):
-  - attributes and metrics must not be removed.
+  - attributes and metrics must not be removed
+  - enum attribute members must not be removed
 
 - On stable attributes and attribute templates:
   - stability must not be changed
   - the type of attribute must not be changed
   - enum attribute: type of value must not be changed
-  - enum attribute: members must not be removed (changing `id` field is allowed, as long as `value` does not change)
+
+- On stable enum attribute members:
+  - stability must not be changed
+  - `id` and `value` must not be changed
+
 - On stable metrics:
   - stability must not be changed
   - instrument and unit must not be changed

semantic-conventions/semconv.schema.json

@@ -259,6 +259,9 @@
 							"note": {
 								"type": "string",
 								"description": "longer description. It defaults to an empty string."
+							},
+							"stability": {
+								"allOf": [{ "$ref": "#/definitions/StabilityLevel" }]
 							}
 						}
 					}
@@ -343,6 +346,14 @@
 				}
 			]
 		},
+		"StabilityLevel": {
+			"description": "specifies the stability level. Can be 'stable' or 'experimental' (the default).",
+			"type": "string",
+			"enum": [
+				"stable",
+				"experimental"
+			]
+		},
 		"Attribute": {
 			"type": "object",
 			"allOf": [
@@ -416,6 +427,9 @@
 						"deprecated": {
 							"type": "string",
 							"description": "specifies if the attribute is deprecated. The string provided as <description> MUST specify why it's deprecated and/or what to use instead."
+						},
+						"stability": {
+							"allOf": [{ "$ref": "#/definitions/StabilityLevel" }]
 						}
 					}
 				},

semantic-conventions/src/opentelemetry/semconv/model/semantic_attribute.py

@@ -187,6 +187,17 @@ def parse(
             stability = SemanticAttribute.parse_stability(
                 attribute.get("stability"), position_data, strict_validation
             )
+            if stability == StabilityLevel.EXPERIMENTAL and isinstance(
+                attr_type, EnumAttributeType
+            ):
+                for member in attr_type.members:
+                    if member.stability == StabilityLevel.STABLE:
+                        msg = (
+                            f"Member '{member.member_id}' is marked as stable "
+                            + "but it is not allowed on experimental attribute!"
+                        )
+                        raise ValidationError.from_yaml_pos(position_data["type"], msg)
+
             deprecated = SemanticAttribute.parse_deprecated(
                 attribute.get("deprecated"), position_data
             )
@@ -482,7 +493,7 @@ def parse(attribute_type):
                 attribute_type.lc.data["members"], "Enumeration without members!"
             )
 
-        allowed_keys = ["id", "value", "brief", "note"]
+        allowed_keys = ["id", "value", "brief", "note", "stability", "deprecated"]
         mandatory_keys = ["id", "value"]
         for member in attribute_type["members"]:
             validate_values(member, allowed_keys, mandatory_keys)
@@ -492,12 +503,26 @@ def parse(attribute_type):
                     f"Invalid value used in enum: <{member['value']}>",
                 )
             validate_id(member["id"], member.lc.data["id"])
+
+            stability_str = member.get("stability")
+            if not stability_str:
+                raise ValidationError.from_yaml_pos(
+                    member.lc.data["id"],
+                    f"Enumeration member '{member['value']}' must have a stability level",
+                )
+
+            stability = SemanticAttribute.parse_stability(stability_str, member.lc.data)
+            deprecated = SemanticAttribute.parse_deprecated(
+                member.get("deprecated"), member.lc.data
+            )
             members.append(
                 EnumMember(
                     member_id=member["id"],
                     value=member["value"],
                     brief=member.get("brief", member["id"]).strip(),
                     note=member.get("note", "").strip(),
+                    stability=stability,
+                    deprecated=deprecated,
                 )
             )
         enum_type = AttributeType.get_type(members[0].value)
@@ -516,6 +541,8 @@ class EnumMember:
     value: str
     brief: str
     note: str
+    stability: StabilityLevel
+    deprecated: str
 
 
 class MdLink:

semantic-conventions/src/opentelemetry/semconv/templating/compatibility.py

@@ -67,49 +67,66 @@ def _check_attribute(self, prev: SemanticAttribute, problems: list[Problem]):
             problems.append(Problem("attribute", prev.fqn, "was removed"))
             return
 
+        self._check_stability(
+            prev.stability, cur.stability, "attribute", prev.fqn, problems
+        )
         if prev.stability == StabilityLevel.STABLE:
-            if cur.stability != prev.stability:
+            self._check_attribute_type(prev, cur, problems)
+
+        if (
+            isinstance(prev.attr_type, EnumAttributeType)
+            and
+            # this makes mypy happy, we already checked that type is the same for stable attributes
+            isinstance(cur.attr_type, EnumAttributeType)
+        ):
+            for member in prev.attr_type.members:
+                self._check_member(prev.fqn, member, cur.attr_type.members, problems)
+
+    def _check_stability(
+        self,
+        prev: StabilityLevel,
+        cur: StabilityLevel,
+        signal: str,
+        fqn: str,
+        problems: list[Problem],
+    ):
+        if prev == StabilityLevel.STABLE and cur != prev:
+            problems.append(
+                Problem(signal, fqn, f"stability changed from '{prev}' to '{cur}'")
+            )
+
+    def _check_attribute_type(
+        self, prev: SemanticAttribute, cur: SemanticAttribute, problems: list[Problem]
+    ):
+        if isinstance(prev.attr_type, EnumAttributeType):
+            if not isinstance(cur.attr_type, EnumAttributeType):
                 problems.append(
                     Problem(
                         "attribute",
                         prev.fqn,
-                        f"stability changed from '{prev.stability}' to '{cur.stability}'",
+                        f"type changed from '{prev.attr_type}' to '{cur.attr_type}'",
                     )
                 )
-
-            if isinstance(prev.attr_type, EnumAttributeType):
-                if not isinstance(cur.attr_type, EnumAttributeType):
+            else:
+                # enum type change inevitably causes some values to be removed
+                # which will be reported in _check_member method as well.
+                # keeping this check to provide more detailed error message
+                if cur.attr_type.enum_type != prev.attr_type.enum_type:
                     problems.append(
                         Problem(
                             "attribute",
                             prev.fqn,
-                            f"type changed from '{prev.attr_type}' to '{cur.attr_type}'",
+                            f"enum type changed from '{prev.attr_type.enum_type}' to '{cur.attr_type.enum_type}'",
                         )
                     )
-                else:
-                    # enum type change inevitably causes some values to be removed
-                    # which will be reported in _check_member method as well.
-                    # keeping this check to provide more detailed error message
-                    if cur.attr_type.enum_type != prev.attr_type.enum_type:
-                        problems.append(
-                            Problem(
-                                "attribute",
-                                prev.fqn,
-                                f"enum type changed from '{prev.attr_type.enum_type}' to '{cur.attr_type.enum_type}'",
-                            )
-                        )
-                    for member in prev.attr_type.members:
-                        self._check_member(
-                            prev.fqn, member, cur.attr_type.members, problems
-                        )
-            elif cur.attr_type != prev.attr_type:
-                problems.append(
-                    Problem(
-                        "attribute",
-                        prev.fqn,
-                        f"type changed from '{prev.attr_type}' to '{cur.attr_type}'",
-                    )
+        elif cur.attr_type != prev.attr_type:
+            problems.append(
+                Problem(
+                    "attribute",
+                    prev.fqn,
+                    f"type changed from '{prev.attr_type}' to '{cur.attr_type}'",
                 )
+            )
 
     def _check_member(
         self,
@@ -118,8 +135,22 @@ def _check_member(
         members: list[EnumMember],
         problems: list[Problem],
     ):
+        found = False
         for member in members:
             if prev.member_id == member.member_id:
+                found = True
+                if prev.stability != StabilityLevel.STABLE:
+                    # we allow stability and value changes for non-stable members
+                    break
+
+                self._check_stability(
+                    prev.stability,
+                    member.stability,
+                    "enum attribute member",
+                    f"{fqn}.{prev.member_id}",
+                    problems,
+                )
+
                 if prev.value != member.value:
                     member_value = (
                         f'"{member.value}"'
@@ -133,10 +164,12 @@ def _check_member(
                             f"value changed from '{prev.value}' to '{member_value}'",
                         )
                     )
-                return
-        problems.append(
-            Problem("enum attribute member", f"{fqn}.{prev.member_id}", "was removed")
-        )
+        if not found:
+            problems.append(
+                Problem(
+                    "enum attribute member", f"{fqn}.{prev.member_id}", "was removed"
+                )
+            )
 
     def _check_metric(self, prev: MetricSemanticConvention, problems: list[Problem]):
         for cur in self.current_semconv.models.values():
@@ -145,14 +178,13 @@ def _check_metric(self, prev: MetricSemanticConvention, problems: list[Problem])
                 and cur.metric_name == prev.metric_name
             ):
                 if prev.stability == StabilityLevel.STABLE:
-                    if cur.stability != prev.stability:
-                        problems.append(
-                            Problem(
-                                "metric",
-                                prev.metric_name,
-                                f"stability changed from '{prev.stability}' to '{cur.stability}'",
-                            )
-                        )
+                    self._check_stability(
+                        prev.stability,
+                        cur.stability,
+                        "metric",
+                        prev.metric_name,
+                        problems,
+                    )
                     if cur.unit != prev.unit:
                         problems.append(
                             Problem(

semantic-conventions/src/opentelemetry/semconv/templating/markdown/__init__.py

@@ -126,7 +126,10 @@ def to_markdown_attr(
             if isinstance(attribute.attr_type, EnumAttributeType)
             else AttributeType.get_instantiated_type(attribute.attr_type)
         )
-        description = self._description_with_badge(attribute) + attribute.brief
+        description = (
+            self._description_with_badge(attribute.stability, attribute.deprecated)
+            + attribute.brief
+        )
         if attribute.note:
             self.render_ctx.add_note(attribute.note)
             description += f" [{len(self.render_ctx.notes)}]"
@@ -242,7 +245,10 @@ def to_markdown_metric_table(
             "| -------- | --------------- | ----------- | -------------- |\n"
         )
 
-        description = self._description_with_badge(semconv) + semconv.brief
+        description = (
+            self._description_with_badge(semconv.stability, semconv.deprecated)
+            + semconv.brief
+        )
         if semconv.note:
             self.render_ctx.add_note(semconv.note)
             description += f" [{len(self.render_ctx.notes)}]"
@@ -327,7 +333,10 @@ def to_markdown_enum(self, output: io.StringIO):
             counter = 1
             notes = []
             for member in enum.members:
-                description = member.brief
+                description = (
+                    self._description_with_badge(member.stability, member.deprecated)
+                    + member.brief
+                )
                 if member.note:
                     description += f" [{counter}]"
                     counter += 1
@@ -528,22 +537,18 @@ def _render_group(self, semconv, parameters, output):
 
         output.write("<!-- endsemconv -->")
 
-    def _description_with_badge(
-        self, item: typing.Union[SemanticAttribute | BaseSemanticConvention]
-    ):
+    def _description_with_badge(self, stability: StabilityLevel, deprecated: str):
         description = ""
-        if item.deprecated and self.options.enable_deprecated:
-            if "deprecated" in item.deprecated.lower():
-                description = f"**{item.deprecated}**<br>"
+        if deprecated and self.options.enable_deprecated:
+            if "deprecated" in deprecated.lower():
+                description = f"**{deprecated}**<br>"
             else:
-                deprecated_msg = self.options.deprecated_md_snippet().format(
-                    item.deprecated
-                )
+                deprecated_msg = self.options.deprecated_md_snippet().format(deprecated)
                 description = f"{deprecated_msg}<br>"
-        elif item.stability == StabilityLevel.STABLE and self.options.enable_stable:
+        elif stability == StabilityLevel.STABLE and self.options.enable_stable:
             description = f"{self.options.stable_md_snippet()}<br>"
         elif (
-            item.stability == StabilityLevel.EXPERIMENTAL
+            stability == StabilityLevel.EXPERIMENTAL
             and self.options.enable_experimental
         ):
             description = f"{self.options.experimental_md_snippet()}<br>"

semantic-conventions/src/tests/data/compat/enum_member_removed/vnext.yaml

@@ -10,8 +10,9 @@ groups:
           members:
             - id: enum_two
               brief: "enum two"
+              stability: experimental
               value: "two"
         brief: "third attribute"
         note: "third attribute note"
         examples: ["two"]
-        stability: stable
+        stability: experimental

semantic-conventions/src/tests/data/compat/enum_member_removed/vprev.yaml

@@ -10,8 +10,9 @@ groups:
           members:
             - id: enum_one
               brief: "enum one"
+              stability: experimental
               value: "one"
         brief: "third attribute"
         note: "third attribute note"
         examples: ["one"]
-        stability: stable
+        stability: experimental