Adjust SDK is not configured to include play-services-analytics

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcomella, Assigned: nalexander)

Tracking

unspecified
Firefox 47
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44+ fixed, firefox45+ fixed, firefox46+ fixed, firefox47+ fixed)

Details

(Whiteboard: [see comment 10])

Attachments

(2 attachments, 2 obsolete attachments)

When running the patches in bug 1232773, I see:
  12-16 14:17:18.319 E/Adjust  ( 8513): Missing device id's. Please check if Proguard is correctly set with Adjust SDK

in the logcat output.

Nick, is this set up correctly? Is this something that perhaps we disable at compile-time?
Flags: needinfo?(nalexander)
(In reply to Michael Comella (:mcomella) from comment #0)
> When running the patches in bug 1232773, I see:
>   12-16 14:17:18.319 E/Adjust  ( 8513): Missing device id's. Please check if
> Proguard is correctly set with Adjust SDK
> 
> in the logcat output.
> 
> Nick, is this set up correctly? Is this something that perhaps we disable at
> compile-time?

I'm not sure.  I see from https://github.com/adjust/android_sdk that we're supposed to keep some Adjust things:

-keep class com.adjust.sdk.plugin.MacAddressUtil { 
    java.lang.String getMacAddress(android.content.Context); 
}
-keep class com.adjust.sdk.plugin.AndroidIdUtil { 
    java.lang.String getAndroidId(android.content.Context); 
}
-keep class com.google.android.gms.common.ConnectionResult { 
    int SUCCESS; 
}
-keep class com.google.android.gms.ads.identifier.AdvertisingIdClient {
    com.google.android.gms.ads.identifier.AdvertisingIdClient.Info 
        getAdvertisingIdInfo (android.content.Context);
}
-keep class com.google.android.gms.ads.identifier.AdvertisingIdClient.Info {
    java.lang.String getId ();
    boolean isLimitAdTrackingEnabled();
}

I see in https://dxr.mozilla.org/mozilla-central/source/mobile/android/config/proguard/adjust-keeps.cfg#4 that we are doing a lot of this:

# Needed to keep some constants in the install tracking library.
-keep class com.adjust.sdk.** { *; }

# Needed to keep some constants required for the install tracking library.
-keep class com.google.android.gms.common.** { *; }
-keep class com.google.android.gms.ads.identifier.** { *; }

My PG isn't rock solid, but I think those cover us.

mcomella, was this from a moz.build APK?  We should be good in that situation.  We don't PG at all in Gradle/IJ, so I wouldn't expect to see this in that situation.
Flags: needinfo?(nalexander)
(In reply to Nick Alexander :nalexander from comment #1)
> mcomella, was this from a moz.build APK?  We should be good in that
> situation.  We don't PG at all in Gradle/IJ, so I wouldn't expect to see
> this in that situation.

I see this on my moz.build APK. It's possible this is related to us running in a sandbox environment.

NI self to check this on a production environment.
Flags: needinfo?(michael.l.comella)
I don't see this warning in FF44, but it's running in production mode so perhaps these log statements are hidden.

However, this seems to be working as expected for us in the dashboards so I don't think it's worth digging time into.
Flags: needinfo?(michael.l.comella)
The code that throws [1] tries to get some parameters from the DeviceInfo instance. When I stop the debugger at [1], the parameters argument appears to be optimized out, causing the throw, and additionally, the `this` instance I have access to has `macSha1` (or a similarly named variable) equal to null. In order to instantiate this field, we get the String macAddress [2], which calls out to getMacAddress, which uses reflection to call out to one of the fields that's supposed to be protected in the proguard config specified above (comment 1).

I have a sneaking suspicion our wildcard config isn't doing what we think it's doing.

[1]: https://github.com/adjust/android_sdk/blob/1eed6a539fb0a7a61b99a3d06a8aad3020399cb3/Adjust/adjust/src/main/java/com/adjust/sdk/PackageBuilder.java#L196
[2]: https://github.com/adjust/android_sdk/blob/1eed6a539fb0a7a61b99a3d06a8aad3020399cb3/Adjust/adjust/src/main/java/com/adjust/sdk/DeviceInfo.java#L59
I should have read more closely – the branch states we must have one of four parameters [1]:

        if (!parameters.containsKey("mac_sha1")
                && !parameters.containsKey("mac_md5")
                && !parameters.containsKey("android_id")
                && !parameters.containsKey("gps_adid")) {

Looking at the initialization of these parameters, the first three are set if Google Play Services is found to be available, while the last is added in every case. This parameter is set through reflection [2][3], where it throws with:
 java.lang.ClassNotFoundException: com.google.android.gms.ads.identifier.AdvertisingIdClient

which is one of the classes we're supposed to configure to keep with proguard.

[1]: https://github.com/adjust/android_sdk/blob/1eed6a539fb0a7a61b99a3d06a8aad3020399cb3/Adjust/adjust/src/main/java/com/adjust/sdk/PackageBuilder.java#L196
[2]: https://github.com/adjust/android_sdk/blob/1eed6a539fb0a7a61b99a3d06a8aad3020399cb3/Adjust/adjust/src/main/java/com/adjust/sdk/PackageBuilder.java#L168
[3]: https://github.com/adjust/android_sdk/blob/1eed6a539fb0a7a61b99a3d06a8aad3020399cb3/Adjust/adjust/src/main/java/com/adjust/sdk/Util.java#L88
(In reply to Michael Comella (:mcomella) from comment #5)
> Looking at the initialization of these parameters, the first three are set
> if Google Play Services is found to be available,

These are set in DeviceInfo if GPS is found to be available. They get set in deviceInfo, which gets passed around into the function that sets the parameters.

GPS are found to be available in my local build.
(In reply to Michael Comella (:mcomella) from comment #5)
>  java.lang.ClassNotFoundException: com.google.android.gms.ads.identifier.AdvertisingIdClient
> 
> which is one of the classes we're supposed to configure to keep with
> proguard.

Caused by: java.lang.ClassNotFoundException: Didn't find class "com.google.android.gms.ads.identifier.AdvertisingIdClient" on path: DexPathList[[zip file "/data/app/org.mozilla.fennec_mcomella-138.apk"],nativeLibraryDirectories=[/data/app-lib/org.mozilla.fennec_mcomella-138, /vendor/lib, /system/lib]]
(In reply to Michael Comella (:mcomella) from comment #7)
> (In reply to Michael Comella (:mcomella) from comment #5)
> >  java.lang.ClassNotFoundException: com.google.android.gms.ads.identifier.AdvertisingIdClient
> > 
> > which is one of the classes we're supposed to configure to keep with
> > proguard.
> 
> Caused by: java.lang.ClassNotFoundException: Didn't find class
> "com.google.android.gms.ads.identifier.AdvertisingIdClient" on path:
> DexPathList[[zip file
> "/data/app/org.mozilla.fennec_mcomella-138.apk"],nativeLibraryDirectories=[/
> data/app-lib/org.mozilla.fennec_mcomella-138, /vendor/lib, /system/lib]]

We don't build against play-services-ads*.aar, so this will *never* be available in our APK.  I don't recall this being required when we integrated Adjust, but perhaps it was?
It seems this will fail unless Google Play Services is not available (the first three parameters are inited in comment 5) or it is and the Ads component is also available (the fourth parameter is inited).

The deviceInfo fields aren't final though, so it's possible these fields are getting modified somewhere else (*grumble*).

fwiw, they determine if gps is available by calling `com.google.android.gms.common.GooglePlayServicesUtil.isGooglePlayServices` available (via reflection).
via Adjust configuration docs [1]:

Since the 1st of August of 2014, apps in the Google Play Store must use the Google Advertising ID to uniquely identify devices. To allow the adjust SDK to use the Google Advertising ID, you must integrate the Google Play Services. If you haven't done this yet, follow these steps:

    Open the build.gradle file of your app and find the dependencies block. Add the following line:

    compile 'com.google.android.gms:play-services-analytics:8.3.0'

---

Searching for "play-services-ana" [2], we don't appear to do this.

[1]: https://github.com/adjust/android_sdk#4-add-google-play-services
[2]: https://mxr.mozilla.org/mozilla-central/search?string=play-services-ana&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Summary: Investigate Proguard and Adjust SDK → Adjust SDK is not configured to include play-services-analytics
(In reply to Nick Alexander :nalexander from comment #8)
> (In reply to Michael Comella (:mcomella) from comment #7)
> > (In reply to Michael Comella (:mcomella) from comment #5)
> > >  java.lang.ClassNotFoundException: com.google.android.gms.ads.identifier.AdvertisingIdClient
> > > 
> > > which is one of the classes we're supposed to configure to keep with
> > > proguard.
> > 
> > Caused by: java.lang.ClassNotFoundException: Didn't find class
> > "com.google.android.gms.ads.identifier.AdvertisingIdClient" on path:
> > DexPathList[[zip file
> > "/data/app/org.mozilla.fennec_mcomella-138.apk"],nativeLibraryDirectories=[/
> > data/app-lib/org.mozilla.fennec_mcomella-138, /vendor/lib, /system/lib]]
> 
> We don't build against play-services-ads*.aar, so this will *never* be
> available in our APK.  I don't recall this being required when we integrated
> Adjust, but perhaps it was?

mcomella points me at https://github.com/adjust/android_sdk#4-add-google-play-services, so technically, we've been in violation of GPS policy since we landed Adjust.

I think we'll need to take the play-services-ads dependency.  It should be cargo-culted through, like the other GPS AAR files.  See https://dxr.mozilla.org/mozilla-central/source/build/autoconf/android.m4#354, for example.  We could do this in a separate ticket.
It's unclear how this actually affects our recorded data (related to bug 1244930?).

I actually don't know what the point of having a proguard config not recommended by Adjust is (it seems to keep more than necessary too – perhaps Adjust changed their recommendations some point after we landed?) – I filed bug 1245711.
Whiteboard: [see comment 10]
(In reply to Michael Comella (:mcomella) from comment #12)
> It's unclear how this actually affects our recorded data (related to bug
> 1244930?).

Unclear.  Looks bad.

> I actually don't know what the point of having a proguard config not
> recommended by Adjust is (it seems to keep more than necessary too – perhaps
> Adjust changed their recommendations some point after we landed?) – I filed
> bug 1245711.

I agree: we need to address this.  I can help with this.
(In reply to Nick Alexander :nalexander from comment #11)

> mcomella points me at
> https://github.com/adjust/android_sdk#4-add-google-play-services, so
> technically, we've been in violation of GPS policy since we landed Adjust.

The Google policy update is here:
https://support.google.com/googleplay/android-developer/answer/6048248?hl=en

And the restriction to only using the Advertising ID is related to ... advertising. Using unique identifiers for non-advertising purposes is still allowed.

However, if Adjust can't find the advertising ID and is failing to send data, that's a problem.
(In reply to Mark Finkle (:mfinkle) from comment #14)
> However, if Adjust can't find the advertising ID and is failing to send
> data, that's a problem.

More evidence to support this. via [1]:

Keep in mind that adjust will permanently attribute your device the first time our servers see it. If you have previously installed a build with the adjust SDK, a new attribution (i.e. a new install) cannot be triggered on the device. You need to either use a fresh device for each test, or clear our servers’ attribution of your device ahead of each test. Clearing your device can be accomplished by requesting a URL on the below format:

...
Android 	https://app.adjust.io/forget_device?app_token= < app token >&gps_adid= < Advertiser ID of your device >
Android 	https://app.adjust.io/forget_device?app_token= < app token >&android_id= < Android ID of your device >
Android 	https://app.adjust.io/forget_device?app_token= < app token >&mac= < Wifi MAC of your device >
---

As this bug found, we don't send any of these IDs to the server (at least in my local builds). I haven't been able to get data from my local builds to show up in the sandbox dashboard – I'll try adding in the analytics lib or edit the source myself to find out if this gets data onto the sandbox and thus affects our upload ability.

If this is actually a problem, it's strange that we were ever able to record data though...

[1]: https://docs.adjust.com/en/getting-started/#testing-attribution
I edited the source to specify Google Play Services is not available and I got the following server response when I started the application:

Response: {"attribution":{"tracker_token":"...","tracker_name":"Organic","network":"Organic"},"app_token":"...","adid":"...","timestamp":"2016-02-04T21:36:12.878Z+0000","message":"Install tracked","ask_in":...}
---

Looks like we have a way from the logs to track installs.
My data appeared on the Adjust dashboard – I think including gps-analytics is necessary.

A bad alternative would be to hack the code to pretend GPlay is not available (but it's unclear what other side effects might arise from this).
If gps-analytics is necessary, the unanswered question is: why did this work before?

Finkle asked on irc when we sliced up our gps into separate aars and it turns out it's in 44: bug 1115004 comment 5. (Additionally, we updated to gps 8.1.0 in bug 1197147.) That explains why this wasn't necessary before.
Depends on: 1115004
This will let us properly configure the Adjust SDK.

When running build, I get the following error:

0:24.61 Warning: com.google.android.gms.analytics.internal.zzam: can't find referenced class org.apache.http.NameValuePair
0:24.61 Warning: com.google.android.gms.analytics.internal.zzam: can't find referenced class org.apache.http.client.utils.URLEncodedUtils
...

There is precedent for this in bug 1197147, where we fixed it by removing the
google play services which caused the errors.

Unfortunately, it's an official error on Google code too:
  https://code.google.com/p/android-developer-preview/issues/detail?id=3001

Workarounds from the thread are [1]:

1. Use Proguard:

-keep class com.google.android.gms.** { *; }
-dontwarn com.google.android.gms.**

2. Or simply add the library back into your app:

android {
useLibrary 'org.apache.http.legacy'
}
---

The official Google response is [2]:

The development team has fixed the issue that you have reported and
it will be available in a future build.

Workaround for time being: Right the issue is that you need to put
the useLibrary element if you need to compile against it. But if
you don't compile your code against it but you have 3rd party libs
that use it and you run through proguard (Which is picky about
wanting to see all the classes that are used), then we need to pass
it to proguard whether you ask for it for compiling or not.

The short term work-around is to ask for the library for compiling
(which will then also add it to the classpath that we give to
proguard).

[1]: https://code.google.com/p/android-developer-preview/issues/detail?id=3001#c31
[2]: https://code.google.com/p/android-developer-preview/issues/detail?id=3001#c35

Review commit: https://reviewboard.mozilla.org/r/33705/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/33705/
Nick, do you have thoughts on the proper solution for us here?
Flags: needinfo?(nalexander)
Assignee: nobody → michael.l.comella
(In reply to Michael Comella (:mcomella) from comment #20)
> Nick, do you have thoughts on the proper solution for us here?

I traced what "useLibrary" does, and it adds

platforms/android-23/optional/org.apache.http.legacy.jar

to the classpath.  If I'm reading this correctly, it doesn't actually compile against it.  Other folks are saying that doesn't help PG, which actually wants to see the symbols, but perhaps I can make it work.  Will try tomorrow.
Flags: needinfo?(nalexander)
[Tracking Requested - why for this release]: Requesting tracking in case this is the cause of bug 1244930.
status-firefox44: --- → affected
tracking-firefox44: --- → ?
I get the same errors when building so this doesn't appear to work although I
may have set it up incorrectly.
Attachment #8717156 - Attachment is obsolete: true
Looking at solution 2 (comment 19, comment 21), I assume we need to add the library to the compile classpath in our moz.build/Makefile.in infrastructure. Looking around, I found:
* -classpath command arg: https://mxr.mozilla.org/mozilla-central/search?string=-cp&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
* -cp command arg: https://mxr.mozilla.org/mozilla-central/search?string=-cp&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
* JAVA_CLASSPATH env-var: https://mxr.mozilla.org/mozilla-central/search?string=java_classpath&find=mobile%2Fandroid&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

It looks like JAVA_CLASSPATH is used for the target GeneratedJNIWrappers.cpp target, which doesn't sound like it'd build the Java for the whole application, but maybe it does. As my first attempt to fix this, I'd add the jar to that file. However, I'm not quite sure the proper way to get the appropriate path to that jar.

This work should probably be duplicated in gradle (via `useLibrary` I imagine).
Okay, it'd take a lot of time to dig in here so I'm going to pass this off to Nick – NI because this is important and needs to be fixed ASAP.
Flags: needinfo?(nalexander)
(In reply to Michael Comella (:mcomella) from comment #26)
> Okay, it'd take a lot of time to dig in here so I'm going to pass this off
> to Nick – NI because this is important and needs to be fixed ASAP.

Further to our discussion in the mobile product meeting this morning, I'm picking this up right now.
Assignee: michael.l.comella → nalexander
Status: NEW → ASSIGNED
Flags: needinfo?(nalexander)
When we switched to fine-grained Google Play Services bundling (Bug
1115004), we stopped shipping com.google.android.gms.analytics.  That
silently breaks Adjust, which queries the Google Ad ID using
Play Services libraries that Adjust relies on.  (Sadly, this bloats
our APK tremendously.)

There is some hijinkery, however: the Play Services libraries
reference a library (org.apache.http) that is deprecated in Android
23!  However, the library is still present on Android 23 devices,
which buys Google time to replace the offending code.  This compiles
just fine, breaks the Proguard global optimization pass.  To give
Proguard the information, we add the library as a Proguard "library
JAR".  This is equivalent to the Google-provided Gradle `useLibrary`
directive.

Review commit: https://reviewboard.mozilla.org/r/34225/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34225/
Attachment #8717574 - Flags: review?(nalexander)
Attachment #8716087 - Attachment is obsolete: true
OK, I have tested on an Android 23 x86 emulator and my Android 19 Nexus 5.  I see sandbox installs under Firefox Android Beta in the Adjust dashboard.  (Nothing special about Beta here, I just used that app id.)

I r+ mcomella's changes, and see no advantage to getting somebody else to bless my Proguard libraries change.  (ckitching is the only other person I know of who has deep PG knowledge.)

The patch grafts cleanly up to beta.  Release needs to ignore a non-existent Gradle file, which does not impact the build.

We can't see build errors before Beta due to Adjust being build flagged off.  I will repeat this in a release-drivers email now, and work out a landing schedule with Sylvestre (45 release owner) and Ritu (44 release owner).
Comment on attachment 8717574 [details]
MozReview Request: Bug 1233238 - Compile with play-services-{ads,analytics,appindexing} to support Adjust SDK. r=nalexander

https://reviewboard.mozilla.org/r/34225/#review30915

mcomella reverse engineered the library copy-paste.  I added the Proguard library JARs definition and comment.  I see no advantage to having that change rubberstamped, since I know of nobody else who has deep Proguard knowledge.  (See comment on ticket.)

The proof is in the landing and testing.
Attachment #8717574 - Flags: review?(nalexander) → review+
(Assignee)

Updated

3 years ago
See Also: → bug 1247047
Bug 1247047, bumping the GPS dependencies, will reduce the APK bloat that this will produce.  It's very low risk, but for crash landing onto release, very low risk is very high :)
(Assignee)

Updated

3 years ago
Blocks: 1247047
https://hg.mozilla.org/integration/fx-team/rev/e4c297c3a475db2b8b57fc249826fc59daefd288
Bug 1233238 - Compile with play-services-{ads,analytics,appindexing} to support Adjust SDK. r=nalexander
Nick landed this on m-r and it should be in the 44.0.2 release that's building now, and aimed to release tomorrow. 

I have a lot of other things going on right now, but we should make sure by tomorrow that this also lands or has landed on other branches.
status-firefox44: affected → fixed
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
tracking-firefox44: ? → +
tracking-firefox45: --- → +
tracking-firefox46: --- → +
tracking-firefox47: --- → +
Flags: needinfo?(wkocher)
Flags: needinfo?(sledru)
fwiw, if we wanted to verify that this fix corrects bug 1244930 before landing it on release, we could try landing on Beta, verifying that the Adjust install rate matches the data from Google Play (or at least increases the installs at an expected rate) and if so, land on release. The one caveat is that the beta numbers may be so small compared to release, we may not be able to verify if the change in install numbers from the added beta installs is a normal fluctuation in the number of installs.
Looks like this got landed to aurora and beta a few hours ago:

https://hg.mozilla.org/releases/mozilla-aurora/rev/7523d9722a0a
https://hg.mozilla.org/releases/mozilla-beta/rev/6c778ebb6354
status-firefox45: affected → fixed
status-firefox46: affected → fixed
Flags: needinfo?(wkocher)
We have green release and beta builds.  I've manually tested release and see and can verify that my install was counted, as follows:

1) download http://archive.mozilla.org/pub/mobile/tinderbox-builds/mozilla-release-android-x86/1455054439/fennec-44.0.2.en-US.android-i386.apk
2) install on Android 23 x86 emulator
3) witness no Adjust error
4) fetch Google Advertising ID from device using Android Settings > Google > Ads
5) verify that device has been seen as follows:
nalexander@chocho ~/M/gecko> curl "http://app.adjust.io/forget_device?app_token=$TOKEN&gps_adid=$ID"
Forgot device
nalexander@chocho ~/M/gecko> curl "http://app.adjust.io/forget_device?app_token=$TOKEN&gps_adid=$ID"
Device not found

Sadly, this procedure can only be done if you have the app token, which is not public knowledge.  QA could get it directly from me or mfinkle if we need more assurance.
Flags: needinfo?(sledru)

Comment 38

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e4c297c3a475
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47

Comment 39

3 years ago
Can you please guide me on how to test if this is fixed? Thanks
Flags: needinfo?(nalexander)
(In reply to Mihai Pop from comment #39)
> Can you please guide me on how to test if this is fixed? Thanks

Very tricky -- see discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=1233238#c37.  I hate to leave a large and important fix essentially untestable by our QA team, but it is largely untestable.  Sorry!

I just looked at the Adjust dashboard and see non-trivial installs tracked.  I looked at Google Play to correlate, but 44.0.{2,3} are not released yet, and the Beta that was released yesterday (Feb 10) is not reflected in the Play Store numbers yet (they finish at Feb 9).

So I can't say much more than, "it works locally" and all signs are healthy.
Flags: needinfo?(nalexander)
Duplicate of this bug: 1244930
I talked with Mark yesterday and this is verified with the release population.
You need to log in before you can comment on or make changes to this bug.