-
Notifications
You must be signed in to change notification settings - Fork 10
[andr] Move expensive operations on start to a bg thread #508
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
Conversation
platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/attributes/NetworkAttributes.kt
Outdated
Show resolved
Hide resolved
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.
Pull Request Overview
This PR offloads two startup operations—network monitoring and SDK start logging—to a background thread using an ExecutorService.
- Introduce an
executorparameter inNetworkAttributesand schedulemonitorNetworkType()on it. - Update
LoggerImplto pass its dispatcher’s executor and deferwriteSdkStartLog()onto that executor. - Adjust tests to supply a direct executor for deterministic execution.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| platform/jvm/capture/src/test/kotlin/io/bitdrift/capture/NetworkAttributesTest.kt | Inject MoreExecutors.newDirectExecutorService() into all NetworkAttributes usages for tests |
| platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/attributes/NetworkAttributes.kt | Added ExecutorService parameter and moved monitorNetworkType() into its execute block |
| platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/LoggerImpl.kt | Pass executorService into NetworkAttributes and wrap writeSdkStartLog() in executorService.execute |
Comments suppressed due to low confidence (1)
platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/LoggerImpl.kt:266
- [nitpick] There are no tests verifying that
writeSdkStartLog()is actually scheduled on theexecutorService. Adding a unit test to confirm it runs off the main thread would help prevent regressions.
eventListenerDispatcher.executorService.execute {
platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/attributes/NetworkAttributes.kt
Outdated
Show resolved
Hide resolved
|
|
||
| init { | ||
| monitorNetworkType() | ||
| executor.execute { |
Copilot
AI
Jul 17, 2025
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.
Since monitorNetworkType() is deferred to a background thread, invoke() may return an outdated network_type if it runs before this task completes. Consider synchronizing initial execution or documenting this behavior so consumers understand the potential race window.
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.
This is fine since as a default value we have "unknown" which might happen for the first log or so if there's a race condition. In my tests above that wasn't the case though
platform/jvm/capture/src/main/kotlin/io/bitdrift/capture/attributes/NetworkAttributes.kt
Outdated
Show resolved
Hide resolved
FranAguilera
left a comment
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.
Regarding thread start name
📦 APK Size Report
|
📦 APK Size Report
|

Tackling mainly:
monitorNetworkType()(went from 9.70% to 0.79%)writeSdkStartLog()(went from 2.27% to 0%)This should make
start()perform 11.18% fasterSession example testing the

SDKConfiguredlog with the rightnetwork_typevalue: https://timeline.bitdrift.dev/session/fda02916-f33f-4365-8262-94d6132b2266?utm_source=sdk&pages=1&utilization=0&h=3450357454420969661After the change this is the threading structure: