feat(plugin): Auto-wrap SQLiteDriver with SentrySQLiteDriver for Room users#1285
feat(plugin): Auto-wrap SQLiteDriver with SentrySQLiteDriver for Room users#12850xadam-brown wants to merge 1 commit into
Conversation
… 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 a setDriver(SQLiteDriver) method.
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. The sentry-android-sqlite version in the SAGP build must include SentrySQLiteDriver
2. InstrumentationFeature.DATABASE must be enabled
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.
Instructions and example for changelogPlease add an entry to Example: ## Unreleased
### Features
- Auto-wrap SQLiteDriver with SentrySQLiteDriver for Room users ([#1285](https://github.com/getsentry/sentry-android-gradle-plugin/pull/1285))If none of the above apply, you can opt out of this check by adding |
| // Broad activation: the setDriver call site can appear in any class. The MethodInstrumentable | ||
| // and MethodVisitor filter at the call-site level. Per-method overhead is one MethodVisitor | ||
| // construction; per-invoke overhead is one visitMethodInsn predicate evaluation. | ||
| override fun isInstrumentable(data: ClassContext): Boolean = !data.isSentryClass() |
There was a problem hiding this comment.
Some existing visitors do likewise, including logging and remapping – and logging is enabled for users by default. That makes the cost here incremental for most (db instrumentation is also enabled by default). But users who've disabled logging and enabled db instrumentation will be especially impacted.
I've run benchmarks against the DuckDuckGo app (see the PR description for details). They suggest that the impact is within reason (~1.5-2.5s extra for warm builds). We can get smarter if we hear differently, but the lift is non-trivial, so I've opted for the simple approach for now.
| ### Features | ||
|
|
||
| - Auto-wrap SQLiteDriver with SentrySQLiteDriver for Room users ([#1281](https://github.com/getsentry/sentry-android-gradle-plugin/issues/1281)) | ||
| - Gated on `sentry-android-sqlite` >= 8.44.0 and the existing `tracingInstrumentation` `DATABASE` feature |
There was a problem hiding this comment.
TODO: Update version number once we ship the SentrySQLiteDriver in sentry-android.
| } | ||
| } | ||
|
|
||
| @Ignore("Placeholder version VERSION_SQLITE_DRIVER not yet on Maven") |
There was a problem hiding this comment.
TODO: Remove @Ignore once we ship the SentrySQLiteDriver in sentry-android.
| // The open helper wrapper (>= 6.21.0) and driver wrapper (>= VERSION_SQLITE_DRIVER) are | ||
| // independent gates and can be used simultaneously; both instrumentables must appear when the | ||
| // newer SDK is on the classpath. | ||
| @Ignore("Placeholder version VERSION_SQLITE_DRIVER not yet on Maven") |
There was a problem hiding this comment.
TODO: Remove @Ignore once we ship the SentrySQLiteDriver in sentry-android.
| 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, 0) |
There was a problem hiding this comment.
TODO: Update version number once we ship the SentrySQLiteDriver in sentry-android.
1 participant
📜 Description
Adds a new bytecode method visitor that lets us auto-wrap all occurrences of
SQLiteDriverwithSentrySQLiteDriverwhenever the driver is passed to asetDriver(SQLiteDriver)method.For instance:
becomes:
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.
💡 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
💚 How did you test it?
[1] Added unit tests
[2] Performed benchmark tests against the DuckDuckGo app
tl;dr: Impact on build times looks acceptable. Auto-wrapping SQLiteDriver adds no meaningful cost on cold full builds, but roughly 1.5s- 2.5s (10–15%) compile-time overhead on warm incremental builds, as the instrumentable scans every non-Sentry class/method per compile.
Benchmark results: Click to expand
SQLiteDriver auto-wrap A/B benchmark results
Setup
13019962686.10.0from localbenchmark/sqlite-driver-*branches6.21.0+ compile stub forSentrySQLiteDriverAndroidXSQLiteDriveralways in the instrumentable chainAndroidXSQLiteDrivercommented outScenarios: cold clean build (5 iterations) and warm incremental build after ABI change to
BookmarksDao.kt(7 iterations).Note: SDK 6.21.0 + stub was used because DuckDuckGo's Kotlin 1.7 can't build with Sentry 8.43. This measures plugin instrumentation compile cost, not runtime span overhead.
Clean build (
assembleInternalDebug+clean, cold daemon)No meaningful impact / within run-to-run noise.
Incremental build (warm daemon, DAO ABI tweak)
This is the scenario that matters for day-to-day development: the driver instrumentable visits every non-Sentry class / method on each compile, even when only a Room DAO changes.
Takeaways
AndroidXSQLiteDriveris enabled, driven by broad class/method visitor activation (same pattern as Logcat).📝 Checklist
🔮 Next steps
VERSION_SQLITE_DRIVERto match the sentry-android version that introducesSentrySQLiteDriver(PR here) + address other inline TODOs.