[Android] Force start to run off main thread #408
Conversation
FranAguilera
changed the title
FranAguilera
force-pushed
the
franjam/move-to-background
branch
6 times, most recently
from
639d69b to
788584f
Compare
FranAguilera
force-pushed
the
franjam/move-to-background
branch
from
3bb8b65 to
b66365b
Compare
FranAguilera
force-pushed
the
franjam/move-to-background
branch
from
9bcdf6b to
80dc8ad
Compare
📦 APK Size Report
|
… temporary memory logger
FranAguilera
force-pushed
the
franjam/move-to-background
branch
from
80dc8ad to
ff8df1e
Compare
📦 APK Size Report
|
|
|
||
| private companion object { | ||
| private const val DEFAULT_NOT_SETUP_MESSAGE = "SDK starting" | ||
| private const val MAX_LOG_CALL_SIZE = 1024 |
There was a problem hiding this comment.
what does this size represent? is it the number of function types stored in the heap?
and how was this size determined? does it match the number in the pre-config buffer?
There was a problem hiding this comment.
that's right, is the max number of logger function calls stored (though will remove oldest entries if value is reached to append new ones at the end). The size was a bit arbitrary but big enough to capture most recent calls. I can do a follow up to investigate an ideal size but i think is good threshold to start testing on the field
There was a problem hiding this comment.
for reference, if you want to use the preconfigbuffer as inspiration: it's bounded not only by count but also by memory size (in bytes), and the current limits are 512, and 1 MB
There was a problem hiding this comment.
awesome thanks, will match current size for now
platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/IPreInitLogFlusher.kt
Outdated
Show resolved
Hide resolved
| val span = Span(null, name, level, fields, startTimeMs, parentSpanId) | ||
| addLoggerCall { it.startSpan(name, level, fields, startTimeMs, parentSpanId) } | ||
| return span |
There was a problem hiding this comment.
did you verify that this correctly closes the span? seems like we might be returning a span with a null logger so it won't ever be closed with a logger?
There was a problem hiding this comment.
oh yeah good catch, adding a follow up task to track this and verify. I think this will be such a short time live that may not break critical spans (🤞 )
There was a problem hiding this comment.
nvm, will try to pass the loggerImpl call here somehow when flushing to native
There was a problem hiding this comment.
Did a minor update and verified the spans works as expected. Here in this session we start a span on initial fragment load and we stopped upon copy session url click. There is also an artificial delay for LoggerImpl creation of 2 seconds
| override fun log( | ||
| level: LogLevel, | ||
| fields: Map<String, String>?, | ||
| throwable: Throwable?, | ||
| message: () -> String, | ||
| ) { | ||
| addLoggerCall { it.log(level, fields, throwable, message) } | ||
| } |
There was a problem hiding this comment.
One potential issue with this is that the timestamp of the logs will be at the time that they were replayed, not the actual time they were logged? Maybe fine for now but something to keep in mind
There was a problem hiding this comment.
yes i agree, it's not perfect as they will be called sequentially but not at the original timestamp. I will create follow up task
There was a problem hiding this comment.
something that we could do is to evaluate the DateProvider eagerly when we receive a log and store the timestamp along with the log function call in the ConcurrentLinkedQueue, but of course this would need our rust logger to have a way to override timestamps
There was a problem hiding this comment.
yup i thought something like that before but will need to tweak the rust layer
| fatalIssueReporter = fatalIssueReporter, | ||
| preInitLogFlusher = preInitInMemoryLogger, | ||
| ) | ||
| default.set(LoggerState.Started(loggerImpl)) |
There was a problem hiding this comment.
From a concurrency perspective it seems like there might be a chance that logs are written to the in memory queue after it gets cleared but before we update the LoggerState, which seems like it could result in dropped logs?
I don't have a good solution for how to avoid this without making it significantly more complicated so maybe it's fine?
There was a problem hiding this comment.
yeah, probably ok i think the init time should be in order of ms (hundred of ms at most). One think to keep in mind is that the backing CommonBackground Thread io.bitdrift.capture.background-thread-worker is a single threaded one and everything within the backgroundThreadHandler.runAsync should be sequential
📦 APK Size Report
|
Verification
Session with a simulated expensive LoggerImpl creation and logs being emitted while start is in process