-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use weaver for semantic convention codegen #70
Conversation
buildscripts/templates/registry/incubating_java/IncubatingSemanticAttributes.java.j2
Outdated
Show resolved
Hide resolved
buildscripts/templates/registry/incubating_java/IncubatingSemanticAttributes.java.j2
Outdated
Show resolved
Hide resolved
buildscripts/templates/registry/java/SemanticAttributes.java.j2
Outdated
Show resolved
Hide resolved
buildscripts/templates/registry/incubating_java/IncubatingSemanticAttributes.java.j2
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
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?
...nv-incubating/src/main/java/io/opentelemetry/semconv/incubating/JvmIncubatingAttributes.java
Show resolved
Hide resolved
semconv/src/main/java/io/opentelemetry/semconv/SemanticAttributes.java
Outdated
Show resolved
Hide resolved
19eb631
to
f045f41
Compare
Update this, but we're now getting issues where weaver can't appropriately "javadoc-friendly" a comment, e.g.:
|
96eb361
to
580363b
Compare
@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. |
Fixed
FYI - the
|
...ubating/src/main/java/io/opentelemetry/semconv/incubating/MessagingIncubatingAttributes.java
Outdated
Show resolved
Hide resolved
...v-incubating/src/main/java/io/opentelemetry/semconv/incubating/HttpIncubatingAttributes.java
Show resolved
Hide resolved
...v-incubating/src/main/java/io/opentelemetry/semconv/incubating/HttpIncubatingAttributes.java
Show resolved
Hide resolved
...ubating/src/main/java/io/opentelemetry/semconv/incubating/ContainerIncubatingAttributes.java
Show resolved
Hide resolved
...-incubating/src/main/java/io/opentelemetry/semconv/incubating/ErrorIncubatingAttributes.java
Outdated
Show resolved
Hide resolved
...incubating/src/main/java/io/opentelemetry/semconv/incubating/SystemIncubatingAttributes.java
Show resolved
Hide resolved
@@ -80,5 +75,7 @@ public final class ArtifactIncubatingAttributes { | |||
/** The version of the artifact. */ | |||
public static final AttributeKey<String> ARTIFACT_VERSION = stringKey("artifact.version"); | |||
|
|||
// Enum definitions |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
public static final String TEST = "test"; | ||
|
||
/** deploy. */ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @lquerel !
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
stability: deprecated
attributes back to incubating classes.Possible Improvements thanks to weaver