android_sdk icon indicating copy to clipboard operation
android_sdk copied to clipboard

Adjust using system calls for every single event saved.

Open L7ColWinters opened this issue 7 years ago • 7 comments

After looking into causes for possible performance problems in our app, most notably around binder usage. I've noticed that the Adjust sdk is using Reflection for grabbing information about the advertiser id in gms:

Count: 8 Trace: java.lang.Throwable at android.os.TransactionTracker.addTrace(TransactionTracker.java:47) at android.os.BinderProxy.transact(Binder.java:614) at android.content.pm.IPackageManager$Stub$Proxy.getPackageInfo(IPackageManager.java:2449) at android.app.ApplicationPackageManager.getPackageInfoAsUser(ApplicationPackageManager.java:139) at android.app.ApplicationPackageManager.getPackageInfo(ApplicationPackageManager.java:132) at com.google.android.gms.common.zzp.isGooglePlayServicesAvailable(Unknown Source) at com.google.android.gms.common.zzf.isGooglePlayServicesAvailable(Unknown Source) at com.google.android.gms.ads.identifier.AdvertisingIdClient.zzc(Unknown Source) at com.google.android.gms.ads.identifier.AdvertisingIdClient.start(Unknown Source) at com.google.android.gms.ads.identifier.AdvertisingIdClient.getAdvertisingIdInfo(Unknown Source) at java.lang.reflect.Method.invoke(Native Method) at com.adjust.sdk.Reflection.invokeMethod(Reflection.java:250) at com.adjust.sdk.Reflection.invokeStaticMethod(Reflection.java:233) at com.adjust.sdk.Reflection.getAdvertisingInfoObject(Reflection.java:150) at com.adjust.sdk.Reflection.getPlayAdId(Reflection.java:43) at com.adjust.sdk.Util.getPlayAdId(Util.java:93) at com.adjust.sdk.DeviceInfo.reloadDeviceIds(DeviceInfo.java:102) at com.adjust.sdk.PackageBuilder.injectDeviceInfoIds(PackageBuilder.java:261) at com.adjust.sdk.PackageBuilder.injectDeviceInfo(PackageBuilder.java:232) at com.adjust.sdk.PackageBuilder.getDefaultParameters(PackageBuilder.java:208) at com.adjust.sdk.PackageBuilder.buildEventPackage(PackageBuilder.java:99) at com.adjust.sdk.ActivityHandler.trackEventI(ActivityHandler.java:1004) at com.adjust.sdk.ActivityHandler.access$1000(ActivityHandler.java:32) at com.adjust.sdk.ActivityHandler$4.run(ActivityHandler.java:338) at com.adjust.sdk.CustomScheduledExecutor$RunnableWrapper.run(CustomScheduledExecutor.java:77) at com.adjust.sdk.CustomScheduledExecutor$RunnableWrapper.run(CustomScheduledExecutor.java:77) at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:428) at java.util.concurrent.FutureTask.run(FutureTask.java:237) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:272) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1133) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:607) at java.lang.Thread.run(Thread.java:761)

Also I noticed that other event information was being grabbed:

Count: 4 Trace: java.lang.Throwable at android.os.TransactionTracker.addTrace(TransactionTracker.java:47) at android.os.BinderProxy.transact(Binder.java:614) at android.os.ServiceManagerProxy.getService(ServiceManagerNative.java:123) at android.os.ServiceManager.getService(ServiceManager.java:55) at android.telephony.SubscriptionManager.getDefaultSubscriptionId(SubscriptionManager.java:923) at android.telephony.TelephonyManager.getDefaultPhone(TelephonyManager.java:3663) at android.telephony.TelephonyManager.getNetworkOperator(TelephonyManager.java:1358) at com.adjust.sdk.Util.getMcc(Util.java:607) at com.adjust.sdk.PackageBuilder.injectDeviceInfo(PackageBuilder.java:253) at com.adjust.sdk.PackageBuilder.getDefaultParameters(PackageBuilder.java:208) at com.adjust.sdk.PackageBuilder.buildEventPackage(PackageBuilder.java:99) at com.adjust.sdk.ActivityHandler.trackEventI(ActivityHandler.java:1004) at com.adjust.sdk.ActivityHandler.access$1000(ActivityHandler.java:32) at com.adjust.sdk.ActivityHandler$4.run(ActivityHandler.java:338) at com.adjust.sdk.CustomScheduledExecutor$RunnableWrapper.run(CustomScheduledExecutor.java:77) at com.adjust.sdk.CustomScheduledExecutor$RunnableWrapper.run(CustomScheduledExecutor.java:77) at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:428) at java.util.concurrent.FutureTask.run(FutureTask.java:237) at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:272) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1133) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:607) at java.lang.Thread.run(Thread.java:761)

I was wondering if There was a reason for using non cached values?

L7ColWinters avatar Aug 12 '18 07:08 L7ColWinters

Hi @L7ColWinters

You noticed well, Google Play Advertising ID is a parameter that is being read each time Adjust SDK is about to create a package that is meant to be sent to our SDK. And it is being accessed through the reflection. Reason behind reading it every single time is the fact that advertising identifier is a parameter that is resettable by user and in theory it can happen at any point in time. Having in mind that for us, it is important to operate with current value, this value is being read every single time. Same thing goes for reading parameter that indicates whether user has decided to opt out from tracking. Here're these options in Android (https://img.gadgethacks.com/img/original/74/11/63646943357168/0/636469433571687411.jpg). Decision on whether user decided to opt out or not is also crucial for us to know in it's current state, because we do not want to perform actions for users who chose to opt out.

Our SDK in general works in a way that upon it's initially started and initialised, all the device parameters of interest that are not changeable (or even if they are, for whose changes we don't care in real time) are read only once and then used during SDK lifetime within that app run. But some parameters, like those two described above and referring to your 2nd stack trace from above - MMC and MNC parameters (https://en.wikipedia.org/wiki/Mobile_country_code) as well since switching of networks and mobile carriers is something that does change from time to time (not super often), but we wanted to have that info up to date as well.

Reason behind reflection for obtaining information is because we are trying to keep our SDK free from any 3rd party dependencies. And we consider anything to be 3rd party dependency if it's not part of the standard Android API. For example, advertising ID and information on whether tracking is enabled or not originates from Google Play Services library which is not in user's app by default, but rather needs to be added via Gradle as dependency. If we went with direct referencing, yes, it would work and probably be better/faster, but it would require all Adjust SDK user to drag in Google Play Services as dependency into their app and some (actually in our case quite a few of them) maybe don't really want to (for example, Asian market has tons of app stores which are not Google Play and concept of Google Play Advertising identifier doesn't have almost any value).

Furthermore on the topic, we have gotten advices from our SDK users that we should reconsider accessing certain things with reflection (https://github.com/adjust/android_sdk/issues/332) and we have already stripped everything that didn't make sense to be read with reflection. These changes will be part of our upcoming SDK release.

In case you have any further questions or doubts, feel free to ping us in here.

Cheers

uerceg avatar Aug 12 '18 08:08 uerceg

Thanks for the quick response. I was wondering if you want to ensure that Asian markets like China can still use the ask but not slowing down the library, why not ask for the advertising id and other values that are in this reflection util class? Create an optional class that can inject these values into your library. That way the folks wanting to use play services can and those that want to keep the default of reflection can do that as well.

L7ColWinters avatar Aug 12 '18 18:08 L7ColWinters

That would also give me the ability to cache the advertising id once per app session and since we already are querying it for other sdks it would be more efficient than each library trying to get it's own.

L7ColWinters avatar Aug 12 '18 18:08 L7ColWinters

I see your point in suggestion to allow injection of this parameter, but having in mind all past "experience" we had with all possible ways people can make mistakes while integrating our SDK, we think that allowing that would be shortcut to a disaster. Even though it makes perfect sense beyond any doubt, allowing this would just make us wait for situations in which, due to some errors in client's code, gps_adid value being injected somehow ends up being the same for all the users of particular app. That can cause some real problems later for that client and with this current approach, we're somehow trying to take responsibility for the process of obtaining this parameter and make sure we're doing it right. Also in case of your suggestion, API would need to be extended to allow injection of advertising identifier and information of whether user has opted out from tacking or not. And this would, for some clients, mean additional work and I'd say that is quite often super annoying for clients, regardless of how easy or hard is to perform those actions.

Stressing it one more time, I fully understand you and your suggestion makes sense, but unfortunately we're trying to make library as generic as possible for all the users, regardless of market and that fact sometimes imposes certain constraints that take development into some, for people not affected by edge cases we're trying to cover, not that reasonable directions.

Never the less, even with that in mind, we're always opened for advices and ideas on how to improve our current ways of doing things. And this last comment of yours, even though it's mentioning caching for this suggested injection flow, gives me an idea that might be an improvement of current mechanism. Maybe we can consider building the mechanism (we have all info we need for that) which will keep an eye on the time app is in the foreground - while app is in foreground, once read advertising ID value will not be read anymore, since change of advertising ID requires user to go to settings and reset it, meaning that app in use must enter background. So maybe we can optimise in a way that once app is brought to foreground, once read advertising ID can be reused for all the packages that are created while app is in foreground and only read it again once app is brought to background and then back to foreground.

uerceg avatar Aug 12 '18 20:08 uerceg

Yeah, I was also thinking about that. You already have the onResume / onPause events so that could help immediately by invalidating the values on Adjust.onPause.

L7ColWinters avatar Aug 12 '18 22:08 L7ColWinters

Yep, nice idea. Will be checked and tested and hopefully added to upcoming releases.

Will keep you posted in here.

uerceg avatar Aug 12 '18 22:08 uerceg

Hi @L7ColWinters

Just wanted to ping you in here with some updates from another issue: https://github.com/adjust/android_sdk/issues/332#issuecomment-420400741

Still not dealing with this caching of once read advertising identifier, but hopefully that will be in our SDK release as well.

Cheers

uerceg avatar Sep 11 '18 19:09 uerceg

As of recently, we have introduced the method which you can invoke with true to instruct to SDK to cache values which it reads upon initialization and not to read them with each package creation.

In case you have any further questions / comments on this topic, feel free to comment / reopen.

Cheers

uerceg avatar Dec 14 '23 10:12 uerceg