Automatically generate Semantic Convention attributes #1846

thisthat · 2020-10-21T13:34:34Z

Fixes #1841. I am suffixing this class with V2 because EXCEPTION_EVENT_NAME is not part of the YAML definitions. I will follow up by refactoring the code and moving that constant somewhere.

Does this solve your issue @trask? Or do you prefer having a new class for just the enum values?
For your example, the enum is available here: https://github.com/open-telemetry/opentelemetry-java/pull/1846/files#diff-50fb78fbdb13d122ada839fd124b2beffec95fe51107c23cffc9a5e48ca3e4ceR714

As future work, I am leaving the definition of the proper workflow to follow. ATM, I have the following in mind:

Manually run the generate.sh script provided within this PR
Use git submodules as we do for the protobuf definitions and define a gradle task
Create a GitHub action that opens a PR in otel-java with the newly generated java class(es) every time master in otel-spec updates a YAML file

thisthat · 2020-10-21T13:50:51Z

There is no event in YAML right now. The only event is defined by Exception SemConv and it is just a text line. If it would have been defined in the YAML defs it would appear in the generated class!

EDIT: event attributes are defined as span attributes though

Oberon00 · 2020-10-21T13:49:20Z

api/src/main/java/io/opentelemetry/trace/attributes/SemanticAttributesV2.java

+
+import io.opentelemetry.common.AttributeKey;
+
+// DO NOT EDIT, this is an Auto-generated file from /templates/SemanticAttributesV2.java.j2


I think it should be relatively straightforward to generate this in gradle using e.g. https://docs.gradle.org/current/dsl/org.gradle.api.tasks.Exec.html. Then we would not have to check in this file.

On the minus side, we would then require contributors to have a recent Python 3 installed (actually Docker currently which makes it un-buildable on Windows 🤔).

If we don't auto-generate the file, we should check during build that the file matches the generated output (does the tool support that for any generator currently or only markdown?)

Oberon00 · 2020-10-21T13:51:58Z

semantic_convention/generate.sh

+DIR="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
+
+rm -rf opentelemetry-specification || true
+git clone https://github.com/open-telemetry/opentelemetry-specification.git --depth 1


We should pin this to a certain commit, or check out a release branch to have a latest 0.x version at least.

codecov · 2020-10-21T13:56:10Z

Codecov Report

Merging #1846 into master will decrease coverage by 0.04%.
The diff coverage is 75.83%.

@@             Coverage Diff              @@
##             master    #1846      +/-   ##
============================================
- Coverage     85.69%   85.64%   -0.05%     
- Complexity     1434     1532      +98     
============================================
  Files           174      175       +1     
  Lines          5556     6066     +510     
  Branches        579      678      +99     
============================================
+ Hits           4761     5195     +434     
- Misses          597      658      +61     
- Partials        198      213      +15

Impacted Files	Coverage Δ	Complexity Δ
...telemetry/trace/attributes/SemanticAttributes.java	`80.43% <75.67%> (-19.57%)`	`1.00 <1.00> (ø)`
...ntelemetry/sdk/trace/RecordEventsReadableSpan.java	`80.81% <100.00%> (-0.23%)`	`75.00 <0.00> (+8.00)`	⬇️
...tensions/trace/propagation/OtTracerPropagator.java	`79.06% <0.00%> (-3.69%)`	`14.00% <0.00%> (+4.00%)`	⬇️
...opentelemetry/opentracingshim/SpanContextShim.java	`87.50% <0.00%> (-1.08%)`	`15.00% <0.00%> (+6.00%)`	⬇️
api/src/main/java/io/opentelemetry/trace/Span.java	`100.00% <0.00%> (ø)`	`14.00% <0.00%> (+8.00%)`
...rc/main/java/io/opentelemetry/baggage/Baggage.java	`100.00% <0.00%> (ø)`	`5.00% <0.00%> (+3.00%)`
...in/java/io/opentelemetry/sdk/OpenTelemetrySdk.java	`100.00% <0.00%> (ø)`	`3.00% <0.00%> (+1.00%)`
...va/io/opentelemetry/opentracingshim/TraceShim.java	`100.00% <0.00%> (ø)`	`5.00% <0.00%> (+2.00%)`
.../io/opentelemetry/opentracingshim/Propagation.java	`100.00% <0.00%> (ø)`	`5.00% <0.00%> (ø%)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0827915...7d1df65. Read the comment docs.

thisthat · 2020-10-21T14:05:41Z

I think this is outside the scope of this PR. I have opened open-telemetry/build-tools#17 for tracking this.

Oberon00 · 2020-10-21T14:09:25Z

Of course this is outside the scope of this PR 😃 But since you mentioned that you will work around this in the description, I brought it up.

jkwatson · 2020-10-21T17:57:14Z

I really don't like the V2 suffix. It makes it sound like we're on version 2 of the semantic conventions, which we definitely are not. I'd rather this PR rename the existing class, if we intend the yaml-generated ones to the be true source of truth.
I'd prefer checking in this file, rather than generating it at build time. Yet another build step that can go wrong doesn't seem like an improvement.
I'd prefer not to introduce another git submodule, if we can possibly help it. I'd rather that the code that generates the file pulled down a specific version from GH if you wanted to regenerate the file, rather than using a submodule for it.

trask · 2020-10-21T21:06:14Z

Thanks @thisthat!

Do you think enum is useful for "db.system" values (over string constants), since we can't enforce enum usage since it's a non-exhaustive list?

thisthat · 2020-10-22T05:38:41Z

@trask I opted for an enum for simplicity of the template. The strings can be retrieved via getValue(). Would you prefer for open enums to have public constants? If so, I can adapt the template and differentiate between the two cases :)

@jkwatson I tend to agree with you. I'll refactor the code and remove the V2 as part of this PR then :)

trask · 2020-10-22T06:13:04Z

Would you prefer for open enums to have public constants? If so, I can adapt the template and differentiate between the two cases :)

Ya, the enum makes it look like an exhaustive list, so I'd probably opt not to use enum in this case 👍

Oberon00 · 2020-10-21T21:16:13Z

semantic_convention/templates/SemanticAttributesV2.java.j2

+  {%- elif type == "boolean" -%}
+    boolean
+  {%- elif type == "number" -%}
+    long


Please see open-telemetry/build-tools#13

Oberon00 · 2020-10-22T06:56:38Z

BTW, regarding EXCEPTION_EVENT_NAME: We could write that literally in the template for now.

jkwatson · 2020-10-22T15:12:45Z

BTW, regarding EXCEPTION_EVENT_NAME: We could write that literally in the template for now.

yeah, adding a couple of custom pre-specced items to the template itself seems like a fine approach to me.

jkwatson

Thanks!

anuraaga

This is great, thanks

Automatically generate Semantic Convention attributes

a1e7fba

thisthat requested review from anuraaga, arminru, bogdandrutu, carlosalberto, jkwatson, pavolloffay and tylerbenson as code owners October 21, 2020 13:34

arminru requested a review from trask October 21, 2020 13:36

Fix checkstyle

8b23010

This comment has been minimized.

Sign in to view

Oberon00 reviewed Oct 21, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

Oberon00 reviewed Oct 22, 2020

View reviewed changes

thisthat added 2 commits October 22, 2020 09:34

Transition to automatically generated class

1339da4

Pin to specific commit

586a4ee

Move EXCEPTION_EVENT_NAME inside the template

7d1df65

Oberon00 mentioned this pull request Oct 23, 2020

Remove Makefile #1872

Merged

jkwatson approved these changes Oct 23, 2020

View reviewed changes

anuraaga approved these changes Oct 23, 2020

View reviewed changes

jkwatson merged commit d0785ff into open-telemetry:master Oct 23, 2020

arminru deleted the generate-constants branch October 23, 2020 16:02

thisthat mentioned this pull request Oct 27, 2020

Replace string values with constants. open-telemetry/opentelemetry-java-instrumentation#1468

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Automatically generate Semantic Convention attributes #1846

Automatically generate Semantic Convention attributes #1846

thisthat commented Oct 21, 2020 •

edited

Loading

This comment has been minimized.

thisthat commented Oct 21, 2020 •

edited

Loading

Oberon00 Oct 21, 2020

Oberon00 Oct 21, 2020

This comment has been minimized.

codecov bot commented Oct 21, 2020 •

edited

Loading

thisthat commented Oct 21, 2020

Oberon00 commented Oct 21, 2020

jkwatson commented Oct 21, 2020

trask commented Oct 21, 2020

thisthat commented Oct 22, 2020

trask commented Oct 22, 2020 •

edited

Loading

Oberon00 Oct 21, 2020

thisthat Oct 22, 2020

Oberon00 commented Oct 22, 2020

jkwatson commented Oct 22, 2020

jkwatson left a comment

anuraaga left a comment


		import io.opentelemetry.common.AttributeKey;

		// DO NOT EDIT, this is an Auto-generated file from /templates/SemanticAttributesV2.java.j2

Automatically generate Semantic Convention attributes #1846

Automatically generate Semantic Convention attributes #1846

Conversation

thisthat commented Oct 21, 2020 • edited Loading

This comment has been minimized.

thisthat commented Oct 21, 2020 • edited Loading

Oberon00 Oct 21, 2020

Choose a reason for hiding this comment

Oberon00 Oct 21, 2020

Choose a reason for hiding this comment

This comment has been minimized.

codecov bot commented Oct 21, 2020 • edited Loading

Codecov Report

thisthat commented Oct 21, 2020

Oberon00 commented Oct 21, 2020

jkwatson commented Oct 21, 2020

trask commented Oct 21, 2020

thisthat commented Oct 22, 2020

trask commented Oct 22, 2020 • edited Loading

Oberon00 Oct 21, 2020

Choose a reason for hiding this comment

thisthat Oct 22, 2020

Choose a reason for hiding this comment

Oberon00 commented Oct 22, 2020

jkwatson commented Oct 22, 2020

jkwatson left a comment

Choose a reason for hiding this comment

anuraaga left a comment

Choose a reason for hiding this comment

thisthat commented Oct 21, 2020 •

edited

Loading

thisthat commented Oct 21, 2020 •

edited

Loading

codecov bot commented Oct 21, 2020 •

edited

Loading

trask commented Oct 22, 2020 •

edited

Loading