open-telemetry / opentelemetry-java-instrumentation

OpenTelemetry auto-instrumentation and instrumentation libraries for Java
https://opentelemetry.io
Apache License 2.0
1.96k stars 860 forks source link

Provide Java 9 Module names to all instrumentation projects #9124

Open rjbaucells opened 1 year ago

rjbaucells commented 1 year ago

Modern Java applications (JDK 9+) should use modules for encapsulation and easy dependency management. The first step libraries should take (without breaking compatibility with old JDK versions) is providing a module name in the META-INF\MANIFEST.MF file. The level of effort is low and will remove unnecessary warning messages in applications consuming it. e.g.:

[WARNING] ***************************************************************************************************************************************************************************
[WARNING] * Required filename-based automodules detected: [opentelemetry-logback-appender-1.0-1.28.0-alpha.jar]. Please don't publish this project to a public artifact repository! *
[WARNING] ***************************************************************************************************************************************************************************

Using the MANIFEST.MF Automatic-Module-Name header will remove the above warning from consumer applications: e.g.:

Automatic-Module-Name: io.opentelemetry.instrumentation.logback

OpenTelemetry API and SDK already provide this configuration in the MANIFEST.MF files.

I can provide PR for adding such names once the naming convention is agreed upon.

trask commented 1 year ago

hi @rjbaucells! this sounds good, can you review https://github.com/open-telemetry/opentelemetry-java/pull/781 and propose something similar in this repo?

rjbaucells commented 1 year ago

Sure, I can do a similar PR in this repo. Now, what should be the module naming convention?

trask commented 1 year ago

this looks good to me 👍

trask commented 1 year ago

we might want to include the "base version" as well, since we include that in the artifact and package names, so that we can change the supported base version in the future without introducing breaking changes

rjbaucells commented 1 year ago

The module name should not contain version, it should follow same guidelines as package name and must be the same from version to version. Two modules cannot export the same package name (JVM will not load the application).

trask commented 1 year ago

we use "base version" to refer to the lowest version of the instrumented library that the instrumenation supports, e.g.

https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/23213fdf30893b63ec0f0ed2482ad301a6dbb79d/instrumentation/logback/logback-appender-1.0/library/src/main/java/io/opentelemetry/instrumentation/logback/appender/v1_0/OpenTelemetryAppender.java#L6

this "base version" is also encoded in the artifact name, e.g.

there's also some more context in https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/932#issuecomment-1209349496

rjbaucells commented 1 year ago

Adding specific examples:

Could share the same module name io.opentelemetry.instrumentation.log4j.appender since they cannot be loaded together in the same JVM process (both log4j versions export the same package name org.apache.logging.log4j.*). Java module system enforces it.

I can add the same naming convention as the package name... but I think it will bring more problems in the future. Changing a dependency version requires the update of the module-info.java files.

mateuszrzeszutek commented 1 year ago

The module name should not contain version, it should follow same guidelines as package name and must be the same from version to version. Two modules cannot export the same package name (JVM will not load the application).

The "base version" is not the version of the instrumentation library; it's the minimum supported version of the instrumented library, and we normally encode it as part of the package/artifact name precisely to avoid conflicts. I strongly think that the Automatic-Module-Name should correspond with the gradle module name (or, instrumentation name) and include the base version.

rjbaucells commented 1 year ago

OK, I will use the same convention as in the package name.