Add unit semantic convention generator #21

justinfoote · 2020-11-05T00:31:32Z

This PR adds a new semantic convention type: units.
There will only be the one table of units in the spec, but because we're expecting every language to create common constants for metric instrument units (see: #1177, I'd like to make it as easy as possible to generate the constants in code.

The unit yaml looks something like this:

groups:
  - id: units
    type: units 
    brief: Specification of commonly used units.
    members:
    - id: percent
      brief: fraction of a total
      value: "%"
    - id: nanosecond
      brief: time
      value: NS
    - id: connections
      brief: connections
      value: "{connections}"

And the resulting markdown looks like this:

Name	Kind of Quantity	Unit String
percent	fraction of a total	`%`
nanosecond	time	`NS`
connections	connections	`{connections}`

There are a few concepts to check out:

I've created four new classes to define out (currently four) semantic convention types (span, resource, metric, unit). These classes share yaml validation logic, but they each define their own allowed/required keys. The SemanticConvention base class acts as a factory for the four subclasses.
I removed the @dataclass annotation from SemanticConvention, and added a custom constructor that takes a single yaml node as an argument.

I've added tests for the new functionality, and I've manually run the markdown generator against the opentelemetry-specification repo, and it doesn't have any side-effects.

justinfoote

@thisthat - I would especially your feedback on this PR. I see that you and I don't have similar styles, but I tried to match the style of the project at least a little.
I have some future changes in mind for the metrics work.

justinfoote · 2020-11-05T00:33:45Z

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

+            convention_type.validate_keys(group)
+            model = convention_type(group)
+            # Also, validate that the value of the fields is acceptable
+            model.validate_values()


These lines appear to have gotten much simpler, but the complexity is moved from here to the constructor for the class that's returned from the parse_semantic_convention_type function.

This is because the required parameters for each type of convention is different, and passing a single yaml node to the constructor allows the subclasses to own their unique implementations.

justinfoote · 2020-11-05T00:34:40Z

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

-        else:  # No parent, sort of current attributes
+            if parent_attributes or semconv.attributes:
+                semconv.attrs_by_name = parent_attributes
+        elif semconv.attributes:  # No parent, sort of current attributes


This is added so we only try to sort attributes for semantic conventions that actually have attributes (spans and resources).

justinfoote · 2020-11-05T00:39:23Z

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

@@ -48,3 +48,34 @@ def check_no_missing_keys(yaml, mandatory):
        position = yaml.lc.data[list(yaml)[0]]
        msg = "Missing keys: {}".format(missing)
        raise ValidationError.from_yaml_pos(position, msg)
+
+
+class ValidatableYamlNode:


This is the base class for SemanticConvention (and also UnitMember). In the future, I expect it to be the base class for MetricLabel.
It owns validation of keys (that all the mandatory keys are present in a yaml node, and that only allowed keys are present), and the validation of values (that the values of the constructed object are allowed).

It may be worth making SemanticAttribute derive from this class, but I didn't do that work now.

justinfoote · 2020-11-05T00:40:52Z

semantic-conventions/src/tests/semconv/templating/test_markdown.py

@@ -36,7 +35,8 @@ def testRef(self):
        renderer._render_single_file(content, md, output)
        with open(self.load_file("markdown/ref/expected.md"), "r") as markdown:
            expected = markdown.read()
-        self.assertEqualWithDiff(expected, output.getvalue())
+
+        assert output.getvalue() == expected


I almost refactored this whole class, but I settled for just using pytest style assertions, which give much more usable messages (in my opinion). Luckily, it plays well with unittest. :)

thisthat

Thank you very much for your great contribution! 🚀
I noticed that we have different styles but I believe the more we work on this tool, the faster we will converge to a common one :)

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

thisthat · 2020-11-05T09:17:44Z

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


    @property
    def attributes(self):
-        return list(self.attrs_by_name.values())
+        return []


This should never be called since we expect subclasses to overwrite this method, right?
If so, I would throw an exception to indicate that something fishy is happening.

If you intend to go this way, consider using https://docs.python.org/3/library/abc.html#abc.abstractmethod and the abc module.

This method is used downstream in the SemanticConventionSet to sort attributes. That refactor felt a little too big and rabbit-holey for this PR, and there's a proposal (#1113) in flight to standardize on "attributes" for metrics as well, so I thought we mightend up undoing the refactor anyway in the very near future.

Co-authored-by: Giovanni Liva <[email protected]>

aabmass

💯

aabmass · 2020-11-05T16:59:33Z

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

-    SPAN = 1
-    RESOURCE = 2
-    METRIC = 3


Why not keep this instead of bare strings here and in main.py?

I prefer to have this map return something that can be used (like the correct class for the semantic convention). I don't see the value in having the bare strings mapped to integers.

Posted my comment about this in the wrong place, sorry: #21 (review)

Please don't remove the SemanticConventionType enum. The integer values don't matter, but the enum is safer against typos. Even if we don't use static checking like pylint or mypy (yet), we at least would get a runtime error if we type SemanticConventionType.SAN whereas with == "san" we would just get False.

Also the enum is a nice documentation of the possible types.

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

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

aabmass · 2020-11-05T20:22:43Z

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

+            convention_type.validate_keys(group)
+            model = convention_type(group)


Should the convention class validate itself in the constructor so you don't have to remember to validate separately?

I considered this. The logical place to put this would be in ValidatableYamlNode.__init__, because that superclass owns the validation logic. ...but, the subclasses are likely adding attributes in their own init methods, so the correct place to call it would be after the superclass and subclass init methods had completed.

maybe in a factory then?

I moved the logic to a factory function.
This triggered some renaming (so I wouldn't have a naming collision between def SemanticConvention and class SemanticConvention), which had some cascading effects.
I removed several import SemanticConvention lines where we were only using the imported class for type declarations, and I refactored references to SemanticConvention.parse to a new parse_semantic_convention_groups function (and in the process I split the parsing concern away from the data struct concern).

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

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

…od to construct semantic conventions

Oberon00

Please don't remove the SemanticConventionType enum. The integer values don't matter, but the enum is safer against typos. Even if we don't use static checking like pylint or mypy (yet), we at least would get a runtime error if we type SemanticConventionType.SAN whereas with == "san" we would just get False.

Also the enum is a nice documentation of the possible types.

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

…PE_NAME

Oberon00 · 2020-11-30T10:29:47Z

I think this PR is too huge to properly review it. It seems to be almost a complete rewrite. If others are fine with still merging this, I won't object, personally I would be much happier with smaller pieces if possible (e.g. only have the base class refactoring in a PR, without the addition of Unit, command line interface changes, etc).

But I think we should decide on this PR soon, as having a huge refactoring looming might discourage any other changes, as they would lead to merge conflicts.

jmacd · 2020-12-02T23:42:00Z

I can't really review this either, but would be happy to see it merged presuming the output is good and others who have looked at it agree.

arminru · 2020-12-09T16:58:09Z

Same as @jmacd.

Both @aabmass and @thisthat already reviewed and approved it - thanks for that! If no one objects by tomorrow, let's merge it.

cc @open-telemetry/specs-approvers

justinfoote · 2020-12-11T20:43:23Z

@open-telemetry/specs-approvers As @arminru suggested above, let's go ahead and merge.

arminru · 2020-12-14T15:50:12Z

Thanks for your contribution and for sticking it out, @justinfoote!

justinfoote added 3 commits November 4, 2020 15:47

Use polymorphism for semantic convention typing

b0b1783

Refactor validation out of semantic convention class

1fc1733

Add UnitMember semantic convention

dd1a894

justinfoote requested a review from a team November 5, 2020 00:31

justinfoote added 2 commits November 4, 2020 16:36

Add attributes mixin for span and resource semantic conventions

4aa56a4

Add markdown unit table rendering

74d1907

justinfoote force-pushed the add_unit_semantics branch from 3e3dc66 to 74d1907 Compare November 5, 2020 00:36

justinfoote commented Nov 5, 2020

View reviewed changes

Add test for unit code generation

57c5362

thisthat approved these changes Nov 5, 2020

View reviewed changes

Strip newlines from group prefix in SemanticConvention constructor

8be824c

Co-authored-by: Giovanni Liva <[email protected]>

aabmass approved these changes Nov 5, 2020

View reviewed changes

justinfoote added 2 commits November 5, 2020 14:31

Refactor in response to PR feedback

f442bd9

Refactor parsing logic out of SemanticConvention; create factory meth…

39eb1ce

…od to construct semantic conventions

Oberon00 requested changes Nov 9, 2020

View reviewed changes

Remove comparison with bare type strings in main.py

b48c789

Oberon00 reviewed Nov 9, 2020

View reviewed changes

semantic-conventions/src/opentelemetry/semconv/model/semantic_convention.py Outdated Show resolved Hide resolved

Oberon00 reviewed Nov 9, 2020

View reviewed changes

semantic-conventions/src/opentelemetry/semconv/model/semantic_convention.py Outdated Show resolved Hide resolved

aabmass approved these changes Nov 9, 2020

View reviewed changes

Rename SemanticConvention.TYPE_VALUE with SemanticConvention.GROUP_TY…

6f4d272

…PE_NAME

justinfoote mentioned this pull request Nov 20, 2020

Add specification of metric instrument units open-telemetry/opentelemetry-specification#1177

Closed

Use immutable tuples of allowed and mandatory semantic convention keys

28d5438

jmacd approved these changes Dec 2, 2020

View reviewed changes

arminru merged commit 8535888 into open-telemetry:master Dec 14, 2020

Oberon00 mentioned this pull request Jul 23, 2021

Update syntax.md with recent feature additions, document old features #56

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add unit semantic convention generator #21

Add unit semantic convention generator #21

justinfoote commented Nov 5, 2020

justinfoote left a comment

justinfoote Nov 5, 2020

justinfoote Nov 5, 2020

justinfoote Nov 5, 2020

justinfoote Nov 5, 2020

thisthat left a comment

thisthat Nov 5, 2020

Oberon00 Nov 5, 2020

justinfoote Nov 5, 2020

aabmass left a comment

aabmass Nov 5, 2020

justinfoote Nov 5, 2020

Oberon00 Nov 9, 2020 •

edited

Loading

aabmass Nov 5, 2020

justinfoote Nov 5, 2020

aabmass Nov 6, 2020

justinfoote Nov 6, 2020

Oberon00 left a comment •

edited

Loading

Oberon00 commented Nov 30, 2020

jmacd commented Dec 2, 2020

arminru commented Dec 9, 2020

justinfoote commented Dec 11, 2020

arminru commented Dec 14, 2020

		convention_type.validate_keys(group)
		model = convention_type(group)

Add unit semantic convention generator #21

Add unit semantic convention generator #21

Conversation

justinfoote commented Nov 5, 2020

justinfoote left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thisthat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aabmass left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Oberon00 left a comment • edited Loading

Choose a reason for hiding this comment

Oberon00 commented Nov 30, 2020

jmacd commented Dec 2, 2020

arminru commented Dec 9, 2020

justinfoote commented Dec 11, 2020

arminru commented Dec 14, 2020

Oberon00 Nov 9, 2020 •

edited

Loading

Oberon00 left a comment •

edited

Loading