-
Notifications
You must be signed in to change notification settings - Fork 8
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
Initialize native crash monitoring in a background thread #540
Conversation
e39bf3d
to
a289a42
Compare
47b39aa
to
d37085b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #540 +/- ##
==========================================
- Coverage 80.20% 80.12% -0.08%
==========================================
Files 413 413
Lines 11000 11041 +41
Branches 1618 1624 +6
==========================================
+ Hits 8822 8847 +25
- Misses 1532 1543 +11
- Partials 646 651 +5
|
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.
I don't think this is something we can ship by default. Perhaps it could be included behind an opt-in flag if a customer explicitly wants to make these trade-offs, but I think there's going to be an expectation from most customers that Embrace should capture crashes as soon as possible in the lifecycle.
I'm also pretty concerned about installing on a background thread as we know from ANR monitoring that the amount of CPU time they get can be pretty low in thread-heavy environments.
6c908df
to
4dbec98
Compare
Chatted with Jamie off line - I'll write a doc to make the case and likely put this behind a local feature flag |
e48cbc6
to
936e172
Compare
Added the ability to attach arbitrary intervals to the startup trace which we will use to track the time it takes for a scheduled background job to run when the NDK service is initialized, in addition to the amount of time app startup waits for the current background spans service init to finish. |
Also put the deferral behind a feature flag that doesn't exist yet that will default to false. This ensures this feature will not be enabled in production unless the feature flag is enabled. |
4c57400
to
cf24beb
Compare
@@ -320,8 +328,18 @@ internal class ModuleInitBootstrapper( | |||
} | |||
|
|||
postInit(NativeModule::class) { | |||
val ndkService = nativeModule.ndkService | |||
val initWorkerTaskQueueTime = initModule.clock.now() | |||
workerThreadModule.backgroundWorker(WorkerName.SERVICE_INIT).submit { |
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.
Schedule this to run right after the NDK service initializes, hoping to approximate how a background task on this thread will be scheduled in prod. Looking at the difference between the queue and run times should tell us what to expect
val initWorkerTaskQueueTime = initModule.clock.now() | ||
workerThreadModule.backgroundWorker(WorkerName.SERVICE_INIT).submit { | ||
val taskRuntimeMs = initModule.clock.now() | ||
dataCaptureServiceModule.appStartupTraceEmitter.addTrackedInterval( |
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.
Will log this interval as a child span of the currently-private startup trace
@@ -55,53 +59,12 @@ import java.util.concurrent.ExecutorService | |||
internal class EmbraceNdkServiceTest { |
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.
Not resetting the mocks in this class made it so that certain settings were dependent on state set outside of the test, which isn't great. Rewrote this so things are reset at each iteration so tests will also start from a clean state so will run deterministically irrespective of execution order.
appFramework = Embrace.AppFramework.UNITY | ||
initializeService() | ||
assertTrue(activityService.listeners.contains(embraceNdkService)) | ||
assertEquals(1, activityService.listeners.size) |
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.
Making embraceNdkService a spy means the object references won't match. Changed to verify that a listener exists
@@ -499,13 +512,12 @@ internal class EmbraceNdkServiceTest { | |||
|
|||
appFramework = Embrace.AppFramework.UNITY | |||
initializeService() | |||
val mockedNdkService = spyk(embraceNdkService, recordPrivateCalls = true) |
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.
The previous test is a bit problematic as the invocation could be on either the mock or the actual object the mock wraps. Changing the only reference to the mock removes that potential gap.
every { sharedObjectLoader.loadEmbraceNative() } returns false | ||
every { sharedObjectLoader.loadEmbraceNative() } returns true | ||
initializeService() | ||
verify(exactly = 0) { embraceNdkService["installSignals"]() } |
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.
The previous implementation of this test didn't actually verify that no calls were made to the mock because they were invoked before the mock was even made, so it was always passing. irrespective what the object instantiation did.
Changing this up to actually verified these aren't called.
enableNdk(true) | ||
blockableExecutorService.blockingMode = true | ||
initializeService() | ||
assertNativeSignalHandlerInstalled() |
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.
Verifying the method call on the delegate because there's no good way to inspect the side effects otherwise. Ah well.
cf24beb
to
37473db
Compare
37473db
to
0c8dd3a
Compare
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.
LGTM
Merge activity
|
Goal
Initialize crash monitoring on a background thread. This turns out to be super expensive, so doing it on a background thread, while delaying the signal handler install by ~500ms, dramatically improves SDK startup.
Given this is a bit controversial, I'm putting this up now, but we can talk about whether this is worth it. I personally think so.