-
-
Notifications
You must be signed in to change notification settings - Fork 38
feat(plugin): Auto-wrap SQLiteDriver with SentrySQLiteDriver for Room users #1285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,54 @@ | ||
| package io.sentry.android.gradle.instrumentation.androidx.sqlite.driver | ||
|
|
||
| import com.android.build.api.instrumentation.ClassContext | ||
| import io.sentry.android.gradle.instrumentation.ClassInstrumentable | ||
| import io.sentry.android.gradle.instrumentation.CommonClassVisitor | ||
| import io.sentry.android.gradle.instrumentation.MethodContext | ||
| import io.sentry.android.gradle.instrumentation.MethodInstrumentable | ||
| import io.sentry.android.gradle.instrumentation.SpanAddingClassVisitorFactory | ||
| import io.sentry.android.gradle.instrumentation.androidx.sqlite.driver.visitor.SQLiteDriverCallSiteVisitor | ||
| import io.sentry.android.gradle.instrumentation.util.isSentryClass | ||
| import org.objectweb.asm.ClassVisitor | ||
| import org.objectweb.asm.MethodVisitor | ||
|
|
||
| class AndroidXSQLiteDriver : ClassInstrumentable { | ||
|
|
||
| override fun getVisitor( | ||
| instrumentableContext: ClassContext, | ||
| apiVersion: Int, | ||
| originalVisitor: ClassVisitor, | ||
| parameters: SpanAddingClassVisitorFactory.SpanAddingParameters, | ||
| ): ClassVisitor { | ||
| val currentClassName = instrumentableContext.currentClassData.className | ||
|
|
||
| return CommonClassVisitor( | ||
| apiVersion = apiVersion, | ||
| classVisitor = originalVisitor, | ||
| className = currentClassName.substringAfterLast('.'), | ||
| methodInstrumentables = METHOD_INSTRUMENTABLES, | ||
| parameters = parameters, | ||
| ) | ||
| } | ||
|
|
||
| // 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() | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| companion object { | ||
| private val METHOD_INSTRUMENTABLES: List<MethodInstrumentable> = | ||
| listOf(SetDriverCallSiteInstrumentable()) | ||
| } | ||
| } | ||
|
|
||
| class SetDriverCallSiteInstrumentable : MethodInstrumentable { | ||
|
|
||
| override fun getVisitor( | ||
| instrumentableContext: MethodContext, | ||
| apiVersion: Int, | ||
| originalVisitor: MethodVisitor, | ||
| parameters: SpanAddingClassVisitorFactory.SpanAddingParameters, | ||
| ): MethodVisitor = SQLiteDriverCallSiteVisitor(apiVersion, originalVisitor) | ||
|
|
||
| override fun isInstrumentable(data: MethodContext): Boolean = true | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| package io.sentry.android.gradle.instrumentation.androidx.sqlite.driver.visitor | ||
|
|
||
| import org.objectweb.asm.MethodVisitor | ||
| import org.objectweb.asm.Opcodes | ||
|
|
||
| /** | ||
| * Wraps the `SQLiteDriver` argument of `setDriver(SQLiteDriver)` calls with | ||
| * `SentrySQLiteDriver.create(...)`. Targeted at `RoomDatabase.Builder.setDriver()` but will match | ||
| * any `setDriver(Landroidx/sqlite/SQLiteDriver;)...` invocation. Keeps us robust to repackaging of | ||
| * the receiver, which has happened with Room. | ||
| * | ||
| * The SDK protects against duplicate wrappings, allowing the visitor to wrap the driver | ||
| * unconditionally. | ||
| * | ||
| * The injected INVOKESTATIC has net-zero stack effect (pops one SQLiteDriver, pushes one | ||
| * SQLiteDriver), so existing stack-map frames remain valid; COMPUTE_MAXS is sufficient. | ||
| */ | ||
| class SQLiteDriverCallSiteVisitor(apiVersion: Int, originalVisitor: MethodVisitor) : | ||
| MethodVisitor(apiVersion, originalVisitor) { | ||
|
|
||
| override fun visitMethodInsn( | ||
| opcode: Int, | ||
| owner: String?, | ||
| name: String?, | ||
| descriptor: String?, | ||
| isInterface: Boolean, | ||
| ) { | ||
| // setDriver is always an instance call; skip name/descriptor checks on other opcodes. | ||
| if (opcode != Opcodes.INVOKEVIRTUAL && opcode != Opcodes.INVOKEINTERFACE) { | ||
| super.visitMethodInsn(opcode, owner, name, descriptor, isInterface) | ||
| return | ||
| } | ||
|
|
||
| // startsWith b/c it lets us account for today's setDriver signature and accommodate (some) | ||
| // future extensions. We don't support the driver param at arbitrary locations b/c current | ||
| // wrapping logic assumes the driver will be on top of the JVM stack. | ||
| if (name == SET_DRIVER && descriptor?.startsWith(SET_DRIVER_DESCRIPTOR_PREFIX) == true) { | ||
| super.visitMethodInsn( | ||
| Opcodes.INVOKESTATIC, | ||
| SENTRY_SQLITE_DRIVER, | ||
| CREATE, | ||
| SENTRY_CREATE_DESCRIPTOR, | ||
| false, | ||
| ) | ||
| } | ||
|
|
||
| super.visitMethodInsn(opcode, owner, name, descriptor, isInterface) | ||
| } | ||
|
|
||
| companion object { | ||
|
|
||
| private const val CREATE = "create" | ||
| private const val SENTRY_CREATE_DESCRIPTOR = | ||
| "(Landroidx/sqlite/SQLiteDriver;)Landroidx/sqlite/SQLiteDriver;" | ||
| private const val SENTRY_SQLITE_DRIVER = "io/sentry/sqlite/SentrySQLiteDriver" | ||
| private const val SET_DRIVER = "setDriver" | ||
| private const val SET_DRIVER_DESCRIPTOR_PREFIX = "(Landroidx/sqlite/SQLiteDriver;)" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,6 +33,7 @@ internal object SentryVersions { | |
| 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) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: Update version number once we ship the SentrySQLiteDriver in sentry-android. |
||
| internal val VERSION_ANDROID_OKHTTP_LISTENER = SemVer(6, 20, 0) | ||
| internal val VERSION_OKHTTP = SemVer(7, 0, 0) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| package com.example; | ||
|
|
||
| import androidx.room.RoomDatabase; | ||
| import androidx.sqlite.SQLiteDriver; | ||
| import androidx.sqlite.driver.bundled.BundledSQLiteDriver; | ||
|
|
||
| public class FactoryReturn { | ||
|
|
||
| public void build() { | ||
| new RoomDatabase.Builder().setDriver(provideDriver()); | ||
| } | ||
|
|
||
| private SQLiteDriver provideDriver() { | ||
| return new BundledSQLiteDriver(); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| package com.example; | ||
|
|
||
| import androidx.room.RoomDatabase; | ||
| import androidx.sqlite.driver.bundled.BundledSQLiteDriver; | ||
|
|
||
| /** setDriver argument loaded from an instance field. */ | ||
| public class FieldLoad { | ||
|
|
||
| private BundledSQLiteDriver driver; | ||
|
|
||
| public void build() { | ||
| new RoomDatabase.Builder().setDriver(driver); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| package com.example; | ||
|
|
||
| import androidx.room.RoomDatabase; | ||
| import androidx.sqlite.SQLiteDriver; | ||
| import androidx.sqlite.driver.bundled.BundledSQLiteDriver; | ||
|
|
||
| /** | ||
| * A {@code checkcast} to {@code SQLiteDriver} sits immediately before the {@code setDriver} call | ||
| * site (here forced via an {@code Object}-typed local). This mirrors the bytecode kotlinc emits for | ||
| * an inferred-type local and verifies the visitor still inserts the wrap when a {@code checkcast} | ||
| * precedes the invocation. | ||
| */ | ||
| public class InferredLocal { | ||
|
|
||
| public void build() { | ||
| Object driver = new BundledSQLiteDriver(); | ||
| new RoomDatabase.Builder().setDriver((SQLiteDriver) driver); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| import androidx.room.RoomDatabase | ||
| import androidx.sqlite.driver.bundled.BundledSQLiteDriver | ||
|
|
||
| /** | ||
| * Kotlin reference showing the idiomatic equivalent of [InferredLocal.java]. The compiled `.class` | ||
| * used by tests comes from the Java source; this file is kept for documentation only (it shows the | ||
| * `val driver = BundledSQLiteDriver()` pattern that makes kotlinc emit a `checkcast` to | ||
| * `SQLiteDriver` at the `setDriver` call site). | ||
| */ | ||
| class InferredLocal { | ||
| fun configure(builder: RoomDatabase.Builder<RoomDatabase>) { | ||
| val driver = BundledSQLiteDriver() | ||
| builder.setDriver(driver) | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| package com.example; | ||
|
|
||
| import androidx.room.RoomDatabase; | ||
| import androidx.sqlite.db.SupportSQLiteOpenHelper; | ||
| import androidx.sqlite.driver.SupportSQLiteDriver; | ||
|
|
||
| /** SupportSQLiteDriver bridge constructed inline: a SupportSQLiteDriver wrapping a SupportSQLiteOpenHelper. */ | ||
| public class InlineBridge { | ||
|
|
||
| private SupportSQLiteOpenHelper helper; | ||
|
|
||
| public void build() { | ||
| new RoomDatabase.Builder().setDriver(new SupportSQLiteDriver(helper)); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| package com.example; | ||
|
|
||
| import androidx.room.RoomDatabase; | ||
| import androidx.sqlite.driver.bundled.BundledSQLiteDriver; | ||
|
|
||
| /** Driver constructed inline as the setDriver argument. */ | ||
| public class InlineConstruction { | ||
|
|
||
| public void build() { | ||
| new RoomDatabase.Builder().setDriver(new BundledSQLiteDriver()); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,12 @@ | ||
| package com.example; | ||
|
|
||
| import androidx.room.RoomDatabase; | ||
| import androidx.sqlite.driver.bundled.BundledSQLiteDriver; | ||
|
|
||
| public class InvokeInterface { | ||
|
|
||
| public void build() { | ||
| RoomDatabase.SetDriverReceiver receiver = new RoomDatabase.Builder(); | ||
| receiver.setDriver(new BundledSQLiteDriver()); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| package com.example; | ||
|
|
||
| import androidx.room.RoomDatabase; | ||
| import androidx.sqlite.SQLiteDriver; | ||
| import androidx.sqlite.db.SupportSQLiteOpenHelper; | ||
| import androidx.sqlite.driver.SupportSQLiteDriver; | ||
|
|
||
| /** Erased-local SupportSQLiteDriver bridge: SQLiteDriver-typed local holding a SupportSQLiteDriver instance. */ | ||
| public class LocalTypedAsBridge { | ||
|
|
||
| private final SupportSQLiteOpenHelper helper; | ||
|
|
||
| public LocalTypedAsBridge(SupportSQLiteOpenHelper helper) { | ||
| this.helper = helper; | ||
| } | ||
|
|
||
| public void build() { | ||
| SQLiteDriver driver = new SupportSQLiteDriver(helper); | ||
| new RoomDatabase.Builder().setDriver(driver); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| package com.example; | ||
|
|
||
| import androidx.room.RoomDatabase; | ||
| import androidx.sqlite.driver.bundled.BundledSQLiteDriver; | ||
|
|
||
| /** Local typed as the concrete BundledSQLiteDriver impl, not the SQLiteDriver interface. */ | ||
| public class LocalTypedAsImpl { | ||
|
|
||
| public void build() { | ||
| BundledSQLiteDriver driver = new BundledSQLiteDriver(); | ||
| new RoomDatabase.Builder().setDriver(driver); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: Update version number once we ship the SentrySQLiteDriver in sentry-android.