Add auto start instrumentation #2098
Conversation
priettt
requested review from
fractalwrench,
fnewberg,
bidetofevil and
a team
as code owners
priettt
force-pushed
the
priettt/autoInstrumentStart
branch
from
2d8e9d5 to
3108bb5
Compare
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
| @@ -1,3 +1,7 @@ | |||
| public final class io/embrace/android/embracesdk/AutoStartInstrumentationHook { | |||
There was a problem hiding this comment.
Ideally this would be under an internal package (but have public visibility) and the function would not contain an underscore
| * @hide | ||
| */ | ||
| @InternalApi | ||
| public final class AutoStartInstrumentationHook { |
| } | ||
| } | ||
|
|
||
| private static void logError(@NonNull Throwable throwable) { |
There was a problem hiding this comment.
This won't do anything if the SDK isn't started so IMO could be deleted, and we just swallow any unexpected exception
| @@ -42,6 +42,11 @@ abstract class EmbraceBytecodeInstrumentation @Inject internal constructor(objec | |||
| val firebasePushNotificationsEnabled: Property<Boolean> = | |||
| objectFactory.property(Boolean::class.java) | |||
|
|
|||
| /** | |||
| * Whether Embrace should automatically start in the Application class. Defaults to true. | |||
There was a problem hiding this comment.
Defaulting this to true feels like a breaking change as folks will already have setup their own place where Embrace is initialized, which will probably be in a different location to where bytecode instrumentation assumes
There was a problem hiding this comment.
This should probably default to false initially until we do a major version bump. But whether we ever flip it to true is probably something product should decide. Having the "no code start" opt-in-able via a gradle property already seems like a big win.
| /** | ||
| * Visits the Application class and returns an [ApplicationMethodAdapter] for the onCreate method. | ||
| */ | ||
| class ApplicationClassAdapter( |
There was a problem hiding this comment.
This current implementation only works if an Application class exists and if the onCreate function has an override. I think that would be a reasonable approach but we will need to document this on the docs site (along with the new gradle DSL property)
There was a problem hiding this comment.
Another approach would be to stick this in a content provider. But yeah I'd just go with this first and document it - this seems much less intrusive and likely covers most production cases anyway.
| * Whether auto-start instrumentation should be enabled or not | ||
| */ | ||
| @Json(name = "auto_start") | ||
| val autoStart: Boolean? = null |
There was a problem hiding this comment.
Why is this required as well as a property on the extension?
There was a problem hiding this comment.
Yeah I would just go with property so there isn't two sources of truth. Going with the property also allows customers to use the SDK without a config file, which will make trying it out easier.
| * @hide | ||
| */ | ||
| @InternalApi | ||
| public final class AutoStartInstrumentationHook { |
There was a problem hiding this comment.
This isn't really instrumentation, but starting the SDK, so maybe call it AutoStartSdkHook or something? And are we still using the word "hook"
And technically this functionality is just wrapping SDK start, and the "auto" part is the result of doing the bytecode instrumentation. Maybe you can even call it SdkInitialization or something, as I think we'll want to add the applicationInitStart/End hooks in there too.
There was a problem hiding this comment.
It's probably worth looking at the conventions I've adopted on this integration branch for some of the bytecode instrumentation. Once 7.3.0 is out I'll merge that down: #2103
|
This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this PR will be closed in 10 days. |
Goal
Add ASM instrumentation to start the Embrace SDK automatically.
Testing
Added a unit test, a InstrumentedBytecodeTestCase, and a BytecodeInstrumentationTest, similar to what we use to test other instrumentation.