open-telemetry / semantic-conventions-java

Java generated classes for semantic conventions
Apache License 2.0
15 stars 19 forks source link

Use weaver for semantic convention codegen #70

Closed jsuereth closed 1 month ago

jsuereth commented 5 months ago

I wasn't sure what you might want to change/fix about codegen when moving to weaver, so this attempts to keep the status quo as best as possible.

TODOs

Possible Improvements thanks to weaver

jsuereth commented 2 months ago

Update this, but we're now getting issues where weaver can't appropriately "javadoc-friendly" a comment, e.g.:

[ant:checkstyle] [ERROR] /home/joshuasuereth/src/open-telemetry/semantic-conventions-java/semconv/src/main/java/io/opentelemetry/semconv/ExceptionAttributes.java:26: <p> tag should be preceded with an empty line. [JavadocParagraph]
jsuereth commented 1 month ago

@jack-berg This is ready for review now. We have the javadoc/comment fixes in place for codegen.

I haven't consolidated down to calling weaver ONCE in gradle yet, but I can do that in a follow up PR.

jsuereth commented 1 month ago

Here are two minor suggestions:

  1. Attributes are sorted by default, so most of the sort filters can be removed.

Fixed

  1. The file name definition can be simplified, as detailed below.

I recommend using the new, simpler method to specify file names. For example, in IncubatingSemanticAttributes.java.j2, instead of:

{%- set my_file_name = ctx.root_namespace | pascal_case ~ "IncubatingAttributes.java" -%}
{{- template.set_file_name(my_file_name) -}}

you could define the file name directly in weaver.yaml within each template object like this:

templates:
  - template: "incubating_java/IncubatingSemanticAttributes.java.j2"
    filter: ...
    application_mode: ...
    file_name: ctx.root_namespace | pascal_case ~ "IncubatingAttributes.java"

FYI - the file_name is actually a JINJA template, not an expression, so it's:

templates:
  - template: "incubating_java/IncubatingSemanticAttributes.java.j2"
    filter: ...
    application_mode: ...
    file_name: "{{ctx.root_namespace | pascal_case}}IncubatingAttributes.java"
jack-berg commented 1 month ago

@jsuereth, @lmolkova, @lquerel, and @open-telemetry/java-approvers - I'll merge this tomorrow if there are no additional comments.