Generate classes by root namespace

[jack-berg]

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 open-telemetry/build-tools#243, which is currently only merged to the feature/codegen-by-namespace branch, which doesn't have an image published yet. I think open-telemetry/build-tools#256 might be trying to solve that.

[jack-berg]

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.

[trask]

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.

[jack-berg]

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]

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 open-telemetry/opentelemetry-python#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]

FYI - This PR is broken against latest feature head. You need to merge this: jack-berg#1

[jack-berg]

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

[trask]

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

[trask]

heads up @brunobat

[jack-berg]

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]

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

E.g.

1. attr foo.bar goes stable
2. it disappears from incubating
3. 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]

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]

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]

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 - jack-berg#2
Generation of @deprecated annotation is a bit ugly, but possible.

[jack-berg]

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

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

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

[trask]

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]

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.