perf(instrumentation): [Generated metadata 1] Generate class availability - #1404
Conversation
5b50fc6 to
01c33f8
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 01c33f8. Configure here.
01c33f8 to
d6cc1ee
Compare
runningcode
left a comment
runningcode
left a comment
There was a problem hiding this comment.
looks good! main concern would be the configuration cache issue if it is an issue!
| AVAILABILITY_FIELD, | ||
| MAP_DESCRIPTOR, | ||
| ) | ||
| visitor.visitLdcInsn(className) |
There was a problem hiding this comment.
what did lines 114-131 do and why don't we need them anymore?
There was a problem hiding this comment.
It used to instrument like this:
static Map<String, Object> classAvailability;
static {
classAvailability = new HashMap();
classAvailability.put(..., ...)
...
}now it's just this
static Map<String, Object> classAvailability = /* this is injected by SAGP */ SentryGeneratedBuildTimeOptions.getClassAvailability(); after we've switched to generating SentryGeneratedBuildTimeOptions in the plugin.
| .toSet() | ||
| val availability = resolveClassAvailability(modules) | ||
| val sourceFile = File(output.get().asFile, GENERATED_CLASS_PATH) | ||
| sourceFile.parentFile.mkdirs() |
There was a problem hiding this comment.
if this is properly marked as an output directory we don't need to do this.
There was a problem hiding this comment.
i tested this - it's marked as @OutputDirectory, however, that one only creates the root dir automatically but not the subdirs, so we still need mkdirs() here
There was a problem hiding this comment.
ah so only output is created but not sourceFile ? that makes sense.
| GenerateSentryBuildTimeOptionsTask::class.java, | ||
| ) { task -> | ||
| task.moduleIds.set( | ||
| configurationProvider.map { configuration -> |
There was a problem hiding this comment.
i can't tell if this eagerly resolves the configuration. if so, we should avoid that as resolving the configuration would mean resolving all the dependencies at configuration time.
also we should check if this is configuration cache compatible since I know that the configuration object can't be serialized in to the configuration cache.
i think itd be worth adding an integration test for this!
There was a problem hiding this comment.
configurations.named() and the following map remain lazy, and our unit test verifies registration does not resolve the runtime classpath. I’ve also made the configuration-cache integration test explicitly assert this task runs and is up-to-date when the cache is reused.
| @@ -0,0 +1,7 @@ | |||
| package io.sentry.android.core | |||
|
|
|||
| object SentryGeneratedBuildTimeOptions { | |||
There was a problem hiding this comment.
since we're generating java, should this file be in java as well?
There was a problem hiding this comment.
i think it's fine to keep it in Kotlin since the generated bytecode matches that of java anyway (thanks to @JvmStatic)
|
|
||
| companion object { | ||
| private const val GENERATED_CLASS_PATH = | ||
| "io/sentry/android/core/SentryGeneratedBuildTimeOptions.java" |
There was a problem hiding this comment.
just a note: mixing kotlin and java sources in the same module drastically slows down compilation speed. not saying we should do anything about it but just noting it here.
There was a problem hiding this comment.
that's a great point I haven't thought of! I guess there's no an easy enough way for us to identify if the module contains only kotlin or only java files and generate respectively. I guess i'd keep it in mind until someone raises a real build time overhead concern
Move runtime classpath resolution into a cacheable generated-source task and keep the SDK ASM transform parameterless. This avoids configuration-time artifact resolution while preserving reflection fallback for unknown classes. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Add Compose UI to the real SDK fixture so replay's optional Compose references do not obscure the release-build coverage. Co-Authored-By: OpenAI Codex <noreply@openai.com>
Ignore unmatched project variants while collecting external module coordinates for generated SDK availability metadata. Co-Authored-By: Codex <noreply@openai.com>
Avoid uploading a source-context bundle containing only Sentry's generated build-time options class. Co-Authored-By: Codex <noreply@openai.com>
Use Gradle's directory property API for the generated source and compare normalized paths when excluding it from source bundles. Explicitly cover the generation task in the configuration-cache integration test. Co-Authored-By: Codex <noreply@openai.com>
a965436 to
e193d84
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
2 participants
Generate an app-owned Java source containing SDK integration class availability from the variant runtime classpath. The ASM transform now emits a stable symbolic call to that generated class and no longer carries variant-specific inputs.
This moves dependency resolution into a cacheable source-generation task, avoiding runtime classpath resolution during Gradle configuration and keeping the transform isolated and configuration-cache compatible.
The SDK-side availability field shipped in sentry-java#5875. Older SDK versions are detected and left unchanged, retaining reflection as the fallback.