open-telemetry / opentelemetry-java-contrib

https://opentelemetry.io
Apache License 2.0
144 stars 117 forks source link

proposal to use annotation processor for incubating semconv #1322

Closed SylvainJuge closed 1 month ago

SylvainJuge commented 1 month ago

The semconv attributes in java is now split in two artifacts:

While those bindings are very convenient to use, creating a library or component that depends on the incubating artifact can create unsolvable dependency issues:

This is why we have to recommend to inline the usages of incubating semconv as written here in the README and also discussed here in a related PR.

However, from the maintainer of a component that relies on those incubating semconv it can become a non-trivial burden to maintain those duplicated attributes definitions.

However, I think that we should be able to provide an annotation processor that would check that the inlined attribute definition matches the one in the incubating semconv artifact.

Example of how an inlined attribute could be annotated for the code.stacktrace attribute

public class Attributes {
    @SemconvField(value = CodeIncubatingAttributes.class, field = "CODE_STACKTRACE")
  public static final AttributeKey<Long> CODE_STACKTRACE_ATTRIBUTE = AttributeKey.longKey("code.stacktrace");

}

The library where this duplicated attribute would be compiled with compileOnly dependency on the incubating semconv, and the annotation processor would check both match at compile time.

Adding an annotation processor is a very small change to the library build process. I also tried to use shading but the shadow plugin makes the setup very complex and tedious, also it makes the jar heavier that it should (+70kb to include all incubating semconv).

The annotation processor would check at least two things at compile-time:

I already have an almost working implementation for this and I'd like to contribute it to contrib repo, it could also be something moved directly to the java semconv repo if you think that's relevant.

jackshirazi commented 1 month ago

Some discussion at the Java SIG May 23 2024 meeting

laurit commented 1 month ago

Another alternative would be to use the incubating semconv in the code and use byte buddy gradle/maven plugin to inline the semconv constants.

SylvainJuge commented 1 month ago

After doing a PoC implementation, this approach is 90% working except that checking the attribute key matches between two attributes is harder than expected.

It should still be possible to do the same by reading the classes with another tool like bytebuddy, but this also increases the complexity a lot just to ensure we prevent copy-paste errors and when semconv attributes changes in a non-compatible manner.

As a conclusion, until we have better options I think copying the incubating attributes and rely on the semconv-incubating artifact for testing is the best compromise we can achieve for now.

SylvainJuge commented 1 month ago

Closing this as a simpler approach was taken (and merged) in #1298:

Whenever the semconv-incubating is updated, we get feedback on the breaking changes from the test execution.