Initialize native crash monitoring in a background thread

[bidetofevil]

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.

[bidetofevil]

• Add fallback start time values for cases where process creation time is unavailable #564
• Initialize native crash monitoring in a background thread #540 👈
• Defer aspects of API service load until necessary #539
• Defer session properties load until it's really needed #538
• Defer loading of user info until when it's really necessary #537
• Synchronize, optimize, and safely delay native module load #536
• Add NDK service tracing #535
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @bidetofevil and the rest of your teammates on Graphite

[codecov]

Codecov Report

Attention: Patch coverage is 69.44444% with 22 lines in your changes are missing coverage. Please review.

Project coverage is 80.12%. Comparing base (8210d0d) to head (0c8dd3a).

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     

Files	Coverage Δ
...roid/embracesdk/config/behavior/SdkModeBehavior.kt	80.95% <100.00%> (+2.00%)	⬆️
...e/android/embracesdk/config/remote/RemoteConfig.kt	100.00% <100.00%> (ø)
.../io/embrace/android/embracesdk/ndk/NativeModule.kt	100.00% <100.00%> (ø)
...dk/session/orchestrator/SessionOrchestratorImpl.kt	93.69% <100.00%> (ø)
...ce/android/embracesdk/worker/WorkerThreadModule.kt	100.00% <100.00%> (ø)
...oid/embracesdk/injection/ModuleInitBootstrapper.kt	94.59% <94.11%> (-0.06%)	⬇️
...mbrace/android/embracesdk/ndk/EmbraceNdkService.kt	49.04% <86.95%> (+0.48%)	⬆️
...bracesdk/capture/startup/AppStartupTraceEmitter.kt	73.61% <28.00%> (-6.80%)	⬇️

... and 1 file with indirect coverage changes

[fractalwrench]

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.

[bidetofevil]

Chatted with Jamie off line - I'll write a doc to make the case and likely put this behind a local feature flag

[bidetofevil]

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.

[bidetofevil]

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.

[bidetofevil]

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

[bidetofevil]

Will log this interval as a child span of the currently-private startup trace

[bidetofevil]

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.

[bidetofevil]

Making embraceNdkService a spy means the object references won't match. Changed to verify that a listener exists

[bidetofevil]

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.

[bidetofevil]

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.

[bidetofevil]

Verifying the method call on the delegate because there's no good way to inspect the side effects otherwise. Ah well.

[fractalwrench]

LGTM

[bidetofevil]

Merge activity

• Mar 14, 2:38 AM EDT: @bidetofevil started a stack merge that includes this pull request via Graphite.
• Mar 14, 2:38 AM EDT: @bidetofevil merged this pull request with Graphite.