[Android] Add Pre-Init memory buffer - #1208
FranAguilera wants to merge 2 commits into
Conversation
Android Size Comparison Report (x86_64)
|
Android Benchmark Results
Allocations
Timing
|
6fbb1e0 to
6ca9807
Compare
| @JvmStatic | ||
| val sessionUrl: String? | ||
| get() = logger()?.sessionUrl | ||
| get() = logger()?.takeUnless { it is PreInitInMemoryLogger }?.sessionUrl |
There was a problem hiding this comment.
ignores the pre init buffer value as we are not initialized. Also choose to not break the non-nullable contract on the ILogger api
There was a problem hiding this comment.
I think this means that we'll break anyone doing
Capture.start()
Capture.sessionUrl()
yeah?
There was a problem hiding this comment.
i think most folks may be checking nullability to infer valid session url, so here there is no much change from that logic to keep this to be non-null until sdk is fully started
One thing we could do is to mark those (Capture.Logger.sessionUrl, Capture.Logger.sessionId, etc) as deprecated and direct people from the deprecation note to rely on the existing StartResult callback that returns an object with the non-nullable sessionId,url, etc when there is a successful start
wdyt?
6ca9807 to
5d746a5
Compare
109ba17 to
769d6d2
Compare
1a47142 to
8d95674
Compare
5503734 to
fc5ac26
Compare
snowp
left a comment
There was a problem hiding this comment.
Mostly makes sense to me, a few points and a few possible inconsistencies I spotted. We should decide whether we are okay with where this approach will get things slightly wrong in some cases or if we need to put in more effort for some of the edge cases
| * Attaches the real logger to a span created before one existed yet (i.e. while the SDK was | ||
| * still starting), emitting its deferred start log and letting a subsequent end() call work. | ||
| */ | ||
| internal fun attachLogger(logger: IInternalLogger) { |
There was a problem hiding this comment.
Would we want some kind of defensive programming here to avoid duplicate emission? Like a deferred flag or something that tells us whether we need to do this? The coordination here seems pretty loose
| @JvmStatic | ||
| val sessionUrl: String? | ||
| get() = logger()?.sessionUrl | ||
| get() = logger()?.takeUnless { it is PreInitInMemoryLogger }?.sessionUrl |
There was a problem hiding this comment.
I think this means that we'll break anyone doing
Capture.start()
Capture.sessionUrl()
yeah?
| logger()?.also { | ||
| it.createTemporaryDeviceCode { | ||
| mainThreadHandler.run { completion(it) } | ||
| logger()?.takeUnless { it is PreInitInMemoryLogger }?.also { logger -> |
There was a problem hiding this comment.
should we just have something like initalizedLogger() to avoid this predicate check in multiple places?
There was a problem hiding this comment.
good call, will add it
| val flushToNativeDuration = | ||
| measureTime { | ||
| preInitInMemoryLogger.flushToNative(loggerImpl) | ||
| } | ||
| default.set(LoggerState.Started(loggerImpl)) |
There was a problem hiding this comment.
I think this is technically racy as we could get inbound logs after flushToNative ends and default.set is called? I think we'd need a lock or other work to make this better if we care about this narrow log drop chance
|
|
||
| startResult.invokeCatchingOrThrowOnDebug(CaptureResult.Success(loggerImpl)) | ||
| } catch (throwable: Throwable) { | ||
| preInitInMemoryLogger.clear() |
There was a problem hiding this comment.
Is this necessary? won't it be deallocated as part of updating default? Seems fine but not sure if we need it
If something can retain a handle to it then I imagine we'd want to make it drop inbounds as well if we wanted to avoid it doing all the log insertion and accounting
There was a problem hiding this comment.
good point, will clean up
| override fun setFeatureFlagExposure( | ||
| name: String, | ||
| variant: String, | ||
| ) = add(BufferedCall.StringFeatureFlagExposure(name, variant)) |
There was a problem hiding this comment.
technically this ought to also capture the timestamp but maybe not worth the complexity to add
| ) : BufferedCall { | ||
| override val sizeBytes = sized(request.toString()) | ||
|
|
||
| override fun replay(logger: IInternalLogger) = logger.log(request) |
There was a problem hiding this comment.
wouldn't this also need a timestamp override? or is that captured in the request info? same for the other convenience functions
|
Moving to draft until all feedback is addressed |
Goal
Resolves BIT-9500
Adding a PreInit in memory buffer for the logger calls that occur while the SDK is starting internally.
This is the current flow in prod
And here with the proposal
The implementation does:
Verification
Steps:
Start with simulated DelayandStart SDK on Background ThreadLog Many messages) etcCHANGELOG.md's "Unreleased" section has been updated, if applicable.