feat(plugin): Auto-instrument SQLiteDriver for Room users (GRADLE-107)#1285
feat(plugin): Auto-instrument SQLiteDriver for Room users (GRADLE-107)#12850xadam-brown wants to merge 5 commits into
Conversation
|
0xadam-brown
force-pushed
the
feat/wrap-sqlite-driver
branch
2 times, most recently
from
f64ad3f to
1c821b7
Compare
… users (GRADLE-107)
Adds a new ASM bytecode method visitor that lets us auto-wrap all occurrences of SQLiteDriver with SentrySQLiteDriver whenever the driver is passed to Room.DatabaseBuilder.setDriver(...).
For instance:
val database = Room.databaseBuilder(context, MyDatabase::class.java, "dbName")
.setDriver(AndroidSQLiteDriver())
.build()
becomes:
val database = Room.databaseBuilder(context, MyDatabase::class.java, "dbName")
.setDriver(SentrySQLiteDriver.create(AndroidSQLiteDriver()))
.build()
The wrapping policy is naive in that every SQLiteDriver passed to setDriver() is wrapped. That's deliberate because SentrySQLiteDriver protects against double-wrapping internally, which lets us keep our visitor implementation simple.
Preconditions:
1. InstrumentationFeature.DATABASE is enabled
2. The owning app is using a version of sentry-android-sqlite that includes SentrySQLiteDriver
Coverage:
- Auto-wraps SQLiteDriver for all Room users (sole Room access point is via its Room.DatabaseBuilder.setDriver() method).
- SQLDelight users don't need driver auto-wrapping (they still use SupportSQLiteOpenHelper, which we already auto-wrap).
- The few developers who use SQLiteDriver directly will need to wrap it manually.
0xadam-brown
force-pushed
the
feat/wrap-sqlite-driver
branch
from
1c821b7 to
1548c9c
Compare
0xadam-brown
requested review from
adinauer,
markushi,
romtsn and
runningcode
as code owners
0xadam-brown
changed the title
0xadam-brown
changed the title
runningcode
left a comment
runningcode
left a comment
There was a problem hiding this comment.
Thanks this looks good! I just don't think I'm enough of an expert in how we instrument classes to approve this.
I left some questions for my better understanding of the patterns we are using.
| # test | ||
| mockitoKotlin = { group = "org.mockito.kotlin", name = "mockito-kotlin", version = "5.4.0" } | ||
| arscLib = { group = "io.github.reandroid", name = "ARSCLib", version = "1.1.4" } | ||
| mockitoKotlin = { group = "org.mockito.kotlin", name = "mockito-kotlin", version = "5.4.0" } |
There was a problem hiding this comment.
why was this line moved?
There was a problem hiding this comment.
Just to alphabetize (in line with the other sections in this file...mostly). I can move it back if you prefer, of course.
There was a problem hiding this comment.
If we depend on room already can we load the class files from the artifact instead of checking in the binaries?
testImplementationAar(libs.roomRuntimeAndroid)
testImplementationAar(libs.room3RuntimeAndroid)
There was a problem hiding this comment.
Yes, and good question: we could load via ClassLoader.getResourceAsStream rather than pinning bytecode. Your approach would be a win, too, because we'd keep testing against the latest as Dependabot bumps us.
That said, this matches existing instrumentation tests. Happy to switch if folks want to move generally...
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| internal const val SENTRY_CREATE_DESCRIPTOR = | ||
| "($SQLITE_DRIVER_TYPE_DESCRIPTOR)$SQLITE_DRIVER_TYPE_DESCRIPTOR" |
There was a problem hiding this comment.
Bug: The hardcoded method signature for SentrySQLiteDriver.create() in the bytecode visitor may not match the actual library's implementation, risking a NoSuchMethodError runtime crash.
Severity: HIGH
Suggested Fix
Verify that the method signature of SentrySQLiteDriver.create() in the sentry-android-sqlite library matches the descriptor (Landroidx/sqlite/SQLiteDriver;)Landroidx/sqlite/SQLiteDriver;. If they differ, update the descriptor in SetDriverMethodVisitor to match the actual implementation to prevent a runtime NoSuchMethodError.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.
Location:
plugin-build/src/main/kotlin/io/sentry/android/gradle/instrumentation/androidx/sqlite/driver/visitor/SetDriverMethodVisitor.kt#L31-L32
Potential issue: The bytecode instrumentation in `SetDriverMethodVisitor` hardcodes the
method descriptor for `SentrySQLiteDriver.create()` to be
`(Landroidx/sqlite/SQLiteDriver;)Landroidx/sqlite/SQLiteDriver;`. While tests pass using
a stub with this signature, a runtime crash will occur if the actual
`sentry-android-sqlite` library implements this method with a different signature, such
as returning the more specific `SentrySQLiteDriver` type instead of the `SQLiteDriver`
interface. This mismatch would lead to a `NoSuchMethodError` when the instrumented code
is executed, causing the application to crash during database initialization.
Did we get this right? 👍 / 👎 to inform future reviews.
| ### Dependencies | ||
|
|
||
| - Bump Android SDK from v8.43.2 to v8.45.0 ([#TODO](https://github.com/getsentry/sentry-android-gradle-plugin/pull/TODO)) | ||
| - [changelog](https://github.com/getsentry/sentry-java/blob/main/CHANGELOG.md#TODO) |
There was a problem hiding this comment.
TODO: Update URLs once we publish Android SDK 8.45.0.
![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 2ee2ad8. Configure here.
| SentryVersions.VERSION_SQLITE_DRIVER.major, | ||
| SentryVersions.VERSION_SQLITE_DRIVER.minor, | ||
| SentryVersions.VERSION_SQLITE_DRIVER.patch - 1, | ||
| ) |
There was a problem hiding this comment.
Patch-below threshold test breaks
Low Severity
The test that asserts behavior one patch below VERSION_SQLITE_DRIVER builds a SemVer with patch - 1. When VERSION_SQLITE_DRIVER is set to a release like 8.45.0 (patch 0), that subtraction yields -1, which violates SemVer's non-negative patch requirement and fails during test setup instead of exercising the gate.
Reviewed by Cursor Bugbot for commit 2ee2ad8. Configure here.
There was a problem hiding this comment.
TODO: Address once we bump the Android SDK to 8.45.0.
| ktfmt = "0.51" | ||
| sqlite = "2.1.0" | ||
| sentry = "8.43.2" | ||
| sentry = "8.44.1" |
There was a problem hiding this comment.
TODO: Bump to 8.45.0 once we publish the Android SDK.
| internal val VERSION_LOGCAT = SemVer(6, 17, 0) | ||
| internal val VERSION_APP_START = SemVer(7, 1, 0) | ||
| internal val VERSION_SQLITE = SemVer(6, 21, 0) | ||
| internal val VERSION_SQLITE_DRIVER = SemVer(8, 44, 1) |
There was a problem hiding this comment.
TODO: Bump to 8.45.0 once we publish the Android SDK.
2 participants
📜 Description
Adds a new bytecode method visitor that lets us auto-wrap all occurrences of
SQLiteDriverwithSentrySQLiteDriverwhenever the driver is passed toRoom.DatabaseBuilder.setDriver(SQLiteDriver)(Room's sole driver access point – see the "Coverage" section from #1281 for why we don't need to worry about anything else).In other words, this:
becomes:
The wrapping policy is naive in that every SQLiteDriver passed to Room.DatabaseBuilder.setDriver() is wrapped. That's deliberate because SentrySQLiteDriver protects against double-wrapping internally, which lets us keep our visitor implementation simple.
💡 Motivation and Context
Room 2.7 introduced
RoomDatabase.Builder.setDriver(SQLiteDriver)as the new way to configure an underlying SQLite implementation (link), with the intention of eventually replacingSupportSQLiteOpenHelper. (Room 3.0+ does just that.)The Sentry Android Gradle Plugin currently auto-instruments only the open helper path. Apps that adopt the driver-based API will lose automatic SQL performance instrumentation unless they manually wrap their driver with
SentrySQLiteDriver.create(...)at everysetDriver(...)call site.This PR provides auto-wrapping for the driver path too.
Addresses GRADLE-107 / #1281
Preconditions
Coverage
Impact on build times
Near zero. The new instrumentation gates on a two-entry allowlist (androidx.room.RoomDatabase$Builder / androidx.room3.RoomDatabase$Builder), which means 0-1 classes will be visited per build in virtually all scenarios. The rest of the per-class cost is a single Set.contains check.
Interactions with other db instrumentation
SupportSQLiteOpenHelper
Driver instrumentation operates alongside our existing instrumentation of SupportSQLiteOpenHelper. (It's fine to use both drivers and open helpers in the same project, so long as a given db file is associated with exactly one of them. Room's APIs help enforce that restriction.)
The Sentry Android SDK – and not the SAGP – is responsible for protecting against double-wrapping and duplicate spans. In general that's easy (just check the type being passed to the wrapper). In the case of the SupportSQLiteDriver used with Room [2.7, 3.0) things get a bit trickier, as it's both a driver and consumes an open helper. But the SDK has that covered as well (see #5514).
Room DAO methods
Driver instrumentation also operates in tandem with our existing Room DAO spans – in theory, at least. Unfortunately DAO instrumentation broke under API changes in Room 2.7, which is the same version that introduced SQLiteDriver. So for now the two are never active at the same time. Fixing that is a task for another day...
💚 How did you test it?
[1] Added unit tests + integration tests in this PR
[2] Verified that all the (for now)
@Ignoredintegration tests pass against a local sentry-java SDK artifact containing SentrySQLiteDriver.[3] Built out a local version of the Sentry Android sample app that let me manually test SAGP + sentry-java compatibility end-to-end. Verified that auto-instrumentation of
SQLiteDriverworks as expected.📝 Checklist
🔮 Next steps
VERSION_SQLITE_DRIVERto match the sentry-android version that introducesSentrySQLiteDriver(PR here) + address other inline TODOs.