Generate classes by root namespace

jack-berg commented 8 months ago

A rebase of #40 on top of #41. @lmolkova it was impractical to do a traditional merge because there were just too many conflicts, so I listed you as a co-author on the commit. Hope that's ok with you.

This PR breaks out classes for each top level namespace in semantic conventions:

For stable conventions, classes are named {{Namespace}}Attributes.java
For incubating / experimental conventions, classes are named {{Namespace}}IncubatingAttributes.java

This omits the automatic metric generation of #40. Figure we can do that in a separate PR to keep changes organized.

Leaving this as a draft because we're waiting https://github.com/open-telemetry/build-tools/pull/243, which is currently only merged to the feature/codegen-by-namespace branch, which doesn't have an image published yet. I think https://github.com/open-telemetry/build-tools/pull/256 might be trying to solve that.

jack-berg commented 8 months ago

Update: A image has been published containing the logic form the feature/codegen-by-namespace branch. Marking this as ready for review since the new capabilities are expected to eventually merge into main.

jack-berg commented 8 months ago

We can't say for sure, but this issue contains feedback from a number of language maintainers which is telling:

js maintainer seems to indicate that grouping attributes by namespace isn't very important
dotnet maintainers say grouping attributes by namespace makes sense
go maintainer says "is desirable to have the semantic conventions laid out in files that matched their groups so we can find them easier". I think this is referring to grouping by namespace?
c++ maintainer says group by namespace sounds good
erlang maintainer agrees with c++ maintainer
swift and php don't seem to have a preference on grouping

Didn't here from all maintainers, but there seems to agreement about grouping by namespace amongst maintainers who expressed an opinion.

Not related to this comment, but related to this PR: This issue to have separate properties from stability and deprecation, and not remove deprecated attributes will be something we will want to take advantage of.

lmolkova commented 8 months ago

Do we know that this is the direction other languages are going to take? I like it, my only reservation is to minimize the number of times we change the structure before marking it stable.

Thanks @jack-berg for summarizing the feedback! I was also checking with python SIG - they are fine with per-root-namespace approach https://github.com/open-telemetry/opentelemetry-python/pull/3586

@trask I only know about two possible directions:

current (file per signal or similar)
file per root namespace - this is a new feature in the build-tools

I think there could be something else, but nobody requests for it explicitly and nobody works on supporting it (build-tools changes would be necessary).

So at least for today, there is no third way.

jsuereth commented 7 months ago

FYI - This PR is broken against latest feature head. You need to merge this: https://github.com/jack-berg/semantic-conventions-java/pull/1

jack-berg commented 7 months ago

@open-telemetry/java-approvers - WDYT? Are there any other questions / concerns about moving forward with this?

trask commented 7 months ago

I didn't see any deprecated attributes (yet?), is this expected?

jack-berg commented 7 months ago

I didn't see any deprecated attributes (yet?), is this expected?

@trask good observation. I wouldn't expect to see any for 1.23.1, but I also don't see any in #46, which upgrades to 1.24.0 which splits out the deprecated and stability properties. I think I found the issue.

lmolkova commented 7 months ago

@jack-berg @trask I'm in doubt how to approach stabilization and wonder if you have some thoughts on it

E.g.

attr foo.bar goes stable
it disappears from incubating
it appears in stable

Assuming application uses semconv.incubating.FooIncubatingAttributes.FOO_BAR it will be a breaking change (or if instrumentation lib version is not aligned).

I wonder if we can do a better job if incubating artifact included all attributes - stable and experimental - then experimental-> stable transition would not be breaking. When attribute becomes stable we could also mark corresponding constant as @Deprecated - use semconv.FooAttributes.FOO_BAR instead

jack-berg commented 7 months ago

Ok, I've pushed a commit to use the latest release of the feature/codegen-by-namespace, which adds a --strict-validation. This allows it to work with 1.23.1 of semconv which didn't separate out the stability and deprecated properties.

The result is that we now see deprecated attributes, which addresses @trask comment. However, the deprecated annotations are missing the @Deprecated annotation, because 1.23.1 of semconv conflates stability and deprecated in a single property. This is resolved once we bump to 1.24.0 of semconv (#46). See the diff for properly working @Deprecated annotations.

jack-berg commented 7 months ago

I wonder if we can do a better job if incubating artifact included all attributes - stable and experimental - then experimental-> stable transition would not be breaking. When attribute becomes stable we could also mark corresponding constant as @Deprecated - use semconv.FooAttributes.FOO_BAR instead

I think that could work. Incubating is then a superset of stable.

lmolkova commented 7 months ago

I wonder if we can do a better job if incubating artifact included all attributes - stable and experimental - then experimental-> stable transition would not be breaking. When attribute becomes stable we could also mark corresponding constant as @deprecated - use semconv.FooAttributes.FOO_BAR instead

I think that could work. Incubating is then a superset of stable.

Tried it here - https://github.com/jack-berg/semantic-conventions-java/pull/2 Generation of @deprecated annotation is a bit ugly, but possible.

jack-berg commented 7 months ago

@lmolkova I've merged your PR and an additional commit that allows this to work as is. I think we should be good to merge this PR now.

@trask let me know if you have any additional thoughts.

lmolkova commented 7 months ago

@lmolkova I've merged your PR and an additional commit that allows this to work as is. I think we should be good to merge this PR now.

@trask let me know if you have any additional thoughts.

can you please revert it? It relies on unmerged changes in the build-tools that are unlikely to make it into the next build-tools release. I don't think it's blocking since I assume it should be fine to add a bunch or stable attributes into incubating artifact later on.

In the meantime, I really want to get your thoughts on how the perfect stable+unstable solution might look like.

Moved to #50 since it's not blocking this PR

jack-berg commented 7 months ago

@lmolkova I adjusted it so it only uses build-tools code merged to the feature branch which is published to docker hub

trask commented 7 months ago

I'd suggest that we wait on merging this (or at least block on releasing it) until we have final decision on #50, so that we can limit the breaking changes to a single release

jack-berg commented 7 months ago

I think we should hold the release until we resolve #50, but we've already merged substantial breaking changes to main, so I don't see the value of holding off on this specific PR.

open-telemetry / semantic-conventions-java

Generate classes by root namespace #45