Skip to content

Use weaver for semantic convention codegen#70

Merged
jack-berg merged 38 commits into
open-telemetry:mainfrom
jsuereth:wip-use-weaver
Sep 6, 2024
Merged

Use weaver for semantic convention codegen#70
jack-berg merged 38 commits into
open-telemetry:mainfrom
jsuereth:wip-use-weaver

Conversation

@jsuereth

@jsuereth jsuereth commented May 15, 2024

Copy link
Copy Markdown
Contributor

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

  • Import markdown / notes -> javadoc filter to the Jinja weaver uses.
  • Finish supporting all types of template keys or throw an exception if an unsupported one shows up.
  • Remove previous jinja template
  • filter unwanted semconv groups in JQ expression in weaver config.
  • figure out what to do w/ K8S convention camel case.
  • Use type_mapping in weaver.yaml instead of macros.
  • Add stability: deprecated attributes back to incubating classes.

Possible Improvements thanks to weaver

  • We could generate all the code (for incubating + stable) in the same weaver config, with the same shared set of templates. I kept it as-is for now so you can more easily compare what's happening.
  • We can register name conversions for method/class/etc. for easier to understand case conversions.
  • We can register acronymns, for consistent capitalization.

Comment thread buildscripts/templates/registry/java/SemanticAttributes.java.j2 Outdated

@jack-berg jack-berg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between build tools and weaver? Is the community trying to transition away from build tools?

Comment thread semconv/src/main/java/io/opentelemetry/semconv/SchemaUrls.java
@jsuereth

jsuereth commented Aug 5, 2024

Copy link
Copy Markdown
Contributor Author

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
jsuereth marked this pull request as ready for review August 7, 2024 16:45
@jsuereth
jsuereth requested a review from a team August 7, 2024 16:45
@jsuereth jsuereth changed the title [prototype] Use weaver for semantic convention codegen Use weaver for semantic convention codegen Aug 7, 2024
@jsuereth

Copy link
Copy Markdown
Contributor Author

@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.

/** The version of the artifact. */
public static final AttributeKey<String> ARTIFACT_VERSION = stringKey("artifact.version");

// Enum definitions

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI - I can remove this in the template when there aren't enum definiitons if you'd like to see that.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think its a big deal. Your choice.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's leave as-is for now.

Once we're on weaver (the important piece of this PR) there's incremental improvements we can begin making - but I'd like to focus on progress.

/** test */
public static final String TEST = "test";

/** deploy. */

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI - there's a conversation on weaver about whether we need a feature to force ending . in comments, see: https://cloud-native.slack.com/archives/C0697EXNTL3/p1725530857893809

I have concerns fixing this may just cause more problems in comment generation, and these one-word descriptions should be fixed upstream, but as an FYI - I wasn't able to safely fix the . vs. "no dot" here.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The GH issue for tracking open-telemetry/weaver#353

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @lquerel !

@jack-berg

Copy link
Copy Markdown
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants