Generate class with instrumentation version#18600
Conversation
laurit
changed the title
trask
left a comment
trask
left a comment
There was a problem hiding this comment.
Not completely convinced that this is worth it.
I'm ok with it, I don't have a perspective on how important the issue is to solve, maybe @open-telemetry/android-approvers can chime in?
AI findings below...
|
@trask from an Android perspective it's certainly preferable to generate POJOs with constants at buildtime. Reading a META-INF file at runtime results in disk access which has the potential to block the UI thread, and therefore freeze the screen. |
| .replace(".", "_") | ||
| val packageName = moduleName + if (baseVersion.isNotEmpty()) ".v$baseVersion" else "" | ||
|
|
||
| val classDir = layout.buildDirectory.dir("generated/instrumentationVersionClass/${packageName.replace('.', '/')}/internal") |
There was a problem hiding this comment.
note that the package name will be something like io.opentelemetry.okhttp.v3_0 which doesn't get shaded
|
@trask have a look at https://medium.com/@NimbleDroid/why-is-classloader-getresourceasstream-so-slow-in-android-5f7c8583b63 it is an old article so idk whether anything has changed. |
LikeTheSalad
left a comment
LikeTheSalad
left a comment
There was a problem hiding this comment.
LGTM. I only added a recommendation for using a Gradle plugin to avoid doing all the Gradle task setup ourselves, which should help keep our code simpler.
| outputs.dir(classDir) | ||
|
|
||
| doLast { | ||
| File(classDir.get().asFile, "InstrumentationVersion.java").writeText(""" |
There was a problem hiding this comment.
I'd recommend using this plugin for creating this class: https://github.com/gmazzo/gradle-buildconfig-plugin - It's meant for these use cases.
breedx-splk
left a comment
breedx-splk
left a comment
There was a problem hiding this comment.
I like this. I think I understand the desire to use a gradle plugin, but I am constantly paranoid these days about any new dependency...especially one in the build chain. 😱
I have been skeptical about these kinds of changes having any real-world impact. After all, a load is a load, and why would loading and interpreting some class bytes necessarily be better/faster than loading and interpreting a tiny property file? I have some vague theories, but I can't quite reason that out...sooooo.....
I needed to see it myself, so I build this thing to do some very low fi benchmarking, and I was pleasantly surprised. The results are considerably faster when loading classes over loading resource properties files.
https://github.com/breedx-splk/android_resource_loading_bench
If I understand https://medium.com/@NimbleDroid/why-is-classloader-getresourceasstream-so-slow-in-android-5f7c8583b63 correctly to test it you should load the resource only once. Only the very first resource access via class loader should be slow, the rest should be significantly faster. You should make your test app large by including a ton of dependencies. The impact should be higher the larger your app is. |
There was a problem hiding this comment.
Pull request overview
This PR addresses Android StrictMode disk-read violations caused by loading instrumentation version from META-INF/...properties by introducing a generated InstrumentationVersion class per instrumentation and updating version resolution to prefer the class-based lookup (with a properties fallback).
Changes:
- Add
EmbeddedInstrumentationProperties.loadVersionFromClass(...)and update version resolution to prefer the generated class, falling back to the existing properties file. - Introduce a new Gradle convention plugin (
otel.instrumentation-version) to generate both the version properties file and theInstrumentationVersionsource for instrumentations. - Adjust javaagent/shaded packaging to avoid duplicate
InstrumentationVersion.classconflicts; add a basic unit test for both class- and properties-based loading.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| javaagent/build.gradle.kts | Excludes duplicate InstrumentationVersion.class entries when assembling the agent shadow jar. |
| instrumentation/r2dbc-1.0/library-instrumentation-shaded/build.gradle.kts | Excludes InstrumentationVersion.class from extracted shaded contents to prevent duplication. |
| instrumentation/jdbc/library/build.gradle.kts | Excludes InstrumentationVersion.class from extracted shaded contents to prevent duplication. |
| instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/internal/EmbeddedInstrumentationPropertiesTest.java | Adds tests for loading version via generated class and via properties file. |
| instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/EmbeddedInstrumentationProperties.java | Adds class-based version lookup (reflection) and makes version loading try class first, then properties. |
| instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java | Defers instrumentation version lookup until tracer construction. |
| instrumentation-api/build.gradle.kts | Adds an instrumentation module dependency for tests (to provide a real generated version class). |
| conventions/src/main/kotlin/otel.library-instrumentation.gradle.kts | Applies the new otel.instrumentation-version generation plugin to library instrumentations. |
| conventions/src/main/kotlin/otel.javaagent-instrumentation.gradle.kts | Applies the new otel.instrumentation-version generation plugin to javaagent instrumentations. |
| conventions/src/main/kotlin/otel.instrumentation-version.gradle.kts | New plugin that generates the version properties file and an InstrumentationVersion Java source file. |
| conventions/src/main/kotlin/io.opentelemetry.instrumentation.base.gradle.kts | Removes the older “generate version properties” logic from the base conventions plugin. |
| testImplementation("io.opentelemetry.javaagent:opentelemetry-testing-common") | ||
| testImplementation("io.opentelemetry:opentelemetry-sdk-testing") | ||
| testImplementation("io.opentelemetry:opentelemetry-exporter-common") | ||
| testImplementation(project(":instrumentation:okhttp:okhttp-3.0:library")) |
Yeah, old article, but I wonder if that's all still the case. I bet it is. There sure didn't seem to a huge or linear difference between first resource load and subsequent loads, so that makes sense. As the article suggests, having a huge APK should make it worse for the resource loading case. For the classloading case, are you suggesting that having a ton of dependencies would make the classpath much slower to search via I think that sounds reasonable. I tried the simple/small case, but it would be interesting to compare a fat classpath with a minimal resource set. |
No, only the first resource loading via the class loader should be slow. |
Resolves #18495
Instrumenter sets instrumentation version based on the content of a properties files such as
META-INF/io/opentelemetry/instrumentation/io.opentelemetry.okhttp-3.0.properties. Apparently on android reading the resource file is undesirable. This PR introduces generatedInstrumentationVersionclass files that also contain the instrumentation version.InstrumentationVersionclass name is created based on the instrumentation name. We can do this since we verify that gradle module name matches instrumentation name, the existing version properties files also rely on this. To transform the instrumentation name into package name we apply some transformations, since the instrumentation name isn't necessarily a valid java package name. These transformations aren't nice. Initially I thought that perhaps we could replace the properties files with the class files but I think that for anybody who needs to add the version to their instrumentations properties files are easier to use.Not completely convinced that this is worth it.