1143888 - Integrate the Adjust campaign tracking SDK into Firefox

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

We need to start work to integrate the Adjust SDK into Firefox.

1. Needs a specific build flag so we can disable at any time.
2. Will likely be RELEASE only, but we'll need to test in custom builds.
3. We only send a single "ping", on the first use via the INSTALL_REFERRER
4. We pull the code into our repo in third-parties. We should always have strict control over the version of the SDK we are using.


Code can be found here:
https://github.com/adjust/android_sdk

Comment 1

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Finkle (:mfinkle) from comment #0)
> We need to start work to integrate the Adjust SDK into Firefox.
> 
> 1. Needs a specific build flag so we can disable at any time.
> 2. Will likely be RELEASE only, but we'll need to test in custom builds.
> 3. We only send a single "ping", on the first use via the INSTALL_REFERRER
> 4. We pull the code into our repo in third-parties. We should always have
> strict control over the version of the SDK we are using.

I anticipate handling 1,2,4 shortly, although I'm happy for someone else to do the dirty and me to review.  I would prefer not to be involved with 3, since I know nothing about distributions and am fully engaged with Firefox for iOS.  I suggest filing 3 as a separate ticket.

Comment 2

[Richard Newman [:rnewman]]

I'm gonna put this out there: distributions should be able to turn this off completely via a trivial file existence check (so we don't have to parse prefs).

Might as well add this now, 'cos it's easy to do.

Scream if you disagree.

Comment 3

[Richard Newman [:rnewman]]

I'm happy to guide the integration with distribution and referrer handling code, because I have accepted that I own that now. Such is life.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

Since we have our own INSTALL_REFERRER handler, we'll need to follow these instructions: https://github.com/adjust/android_sdk/blob/master/doc/referrer.md

Also note that the SDK requires android.permission.INTERNET and android.permission.ACCESS_WIFI_STATE permissions. I think we already ask for those.

Lastly, the SDK uses the Google Advertising ID to uniquely identify devices and requires Google Play Services "Ads" library.

Comment 5

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #4)

> and requires Google Play Services "Ads" library.

Would anyone object if we didn't ship this in our resource-constrained build?

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: install_tracking WIP

Basics. This will get a build with Install Tracking and the SDK seems to do something.

I need to get the Google Play build parts cleaned up. I also need to use the MOZ_INSTALL_TRACKING var in the code.

Comment 8

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8583130 [details] [diff] [review]
install_tracking WIP

Review of attachment 8583130 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a great start!

::: build/autoconf/android.m4
@@ +288,5 @@
>  
> +if test -n "$MOZ_INSTALL_TRACKING" ; then
> +    AC_SUBST(MOZ_INSTALL_TRACKING)
> +
> +    dnl We may have already setup the Play Services library

It seems like MOZ_INSTALL_TRACKING and MOZ_NATIVE_DEVICES overlap here, and it's time to grow a MOZ_ANDROID_GOOGLE_PLAY_SERVICES or similar.

::: mobile/android/base/GeckoApplication.java
@@ +157,5 @@
>  
>          super.onCreate();
> +
> +        // Initialize the Adjust SDK
> +        String appToken = "ABCDEFGHIJKL";

nit: final all the things!

Extract an AppConstant for this?

::: mobile/android/confvars.sh
@@ +70,5 @@
>    MOZ_NATIVE_DEVICES=1
>  fi
>  
> +# Enable install tracking SDK
> +MOZ_INSTALL_TRACKING=1

This is an inflamatory name :)  But it does reveal the intent, but not the tech.

And this, defaulted on, means that local developer builds will require Play Services and will build Adjust, possibly tracking.  That seems wrong.

Comment 9

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #8)

> Extract an AppConstant for this?

Presumably this should actually be a build define…


> And this, defaulted on, means that local developer builds will require Play
> Services and will build Adjust, possibly tracking.  That seems wrong.

I think this is mfinkle's working patch (hence the ABCDEFGHIJKL!). When landed this should be

MOZ_INSTALL_TRACKING=RELEASE_BUILD

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: adjust-data-post.txt

Attaching an example of the data posted via the SDK by just starting Firefox. No INSTALL_REFERRER was used. Anyone starting Firefox would have this data posted.

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8588260 [details]
adjust-data-post.txt

Presumably this is happening immediately after launch?

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: install_tracking v0.1

This patch:
1. Adds the Adjust SDK source code, if MOZ_INSTALL_TRACKING is set
2. Inits the Adjust SDK in GeckoApplication and handles the INSTALL_REFERRER, if MOZ_INSTALL_TRACKING is set
3. Sets MOZ_INSTALL_TRACKING if RELEASE_BUILD and MOZ_NATIVE_DEVICES is set

We depend on MOZ_NATIVE_DEVICES as a simple, lazy way to make sure the Google Play Services libs are included, without needing to create a separate configure path for multiple features depending on Google Play Services.

We can add such a path in the future. This simple patch will be easier to uplift to Beta (ugh). We also need to decide how to handle the | appToken | which I have just left hardcoded in GeckoApplication.

Comment 13

[Richard Newman [:rnewman]]

Comment on attachment 8588319 [details] [diff] [review]
install_tracking v0.1

Review of attachment 8588319 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoApplication.java
@@ +16,5 @@
>  import org.mozilla.gecko.util.ThreadUtils;
>  
> +import com.adjust.sdk.Adjust;
> +import com.adjust.sdk.AdjustConfig;
> +import com.adjust.sdk.LogLevel;

Did you test a build with MOZ_INSTALL_TRACKING not set? You don't seem to unconditionally include any of these files in moz.build, so I would expect this to break.

@@ +159,5 @@
>          super.onCreate();
> +
> +        if (AppConstants.MOZ_INSTALL_TRACKING) {
> +            // Initialize the Adjust SDK
> +            final String appToken = "ABCDEFGHIJKL";

This should be a build define. (Probably the environment, too.)

::: mobile/android/base/distribution/ReferrerReceiver.java
@@ +55,5 @@
>          if (!TextUtils.equals(referrer.source, MOZILLA_UTM_SOURCE)) {
> +            if (AppConstants.MOZ_INSTALL_TRACKING) {
> +                // Allow the Adjust handler to process
> +                new AdjustReferrerReceiver().onReceive(context, intent);
> +            }

Let's do this *after* our own code runs -- we want to block distro stuff as little as possible.

Did you determine whether there's any chance of their onReceive method throwing?

Also, why did you opt to init the Adjust SDK in GeckoApplication rather than right here? This seems like it'd be the best place.

Comment 14

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Richard Newman [:rnewman] from comment #13)
> Comment on attachment 8588319 [details] [diff] [review]
> install_tracking v0.1
> 
> Review of attachment 8588319 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/base/GeckoApplication.java
> @@ +16,5 @@
> >  import org.mozilla.gecko.util.ThreadUtils;
> >  
> > +import com.adjust.sdk.Adjust;
> > +import com.adjust.sdk.AdjustConfig;
> > +import com.adjust.sdk.LogLevel;
> 
> Did you test a build with MOZ_INSTALL_TRACKING not set? You don't seem to
> unconditionally include any of these files in moz.build, so I would expect
> this to break.

I did test with and without MOZ_INSTALL_TRACKING set. Changing the RELEASE_BUILD to NIGHTLY_BUILD in confvars will do it. I was also expecting failure, but the imports did not break the build _and_ Adjust data was not sent. Because no data was sent, I concluded MOZ_RELEASE_TRACKING was working correctly.

Maybe I needed a full clobber when switching?

If we get the process failing, as we expect it should, I'll probably just always add the source files to the 3rd party JAR and just use the MOZ_INSTALL_TRACKING in the code to control using/not_using the SDK.

> > +        if (AppConstants.MOZ_INSTALL_TRACKING) {
> > +            // Initialize the Adjust SDK
> > +            final String appToken = "ABCDEFGHIJKL";
> 
> This should be a build define. (Probably the environment, too.)

Sure. I don't see the pattern for making that happen, and I don't care too much about it for now.

> ::: mobile/android/base/distribution/ReferrerReceiver.java
> >          if (!TextUtils.equals(referrer.source, MOZILLA_UTM_SOURCE)) {
> > +            if (AppConstants.MOZ_INSTALL_TRACKING) {
> > +                // Allow the Adjust handler to process
> > +                new AdjustReferrerReceiver().onReceive(context, intent);
> > +            }
> 
> Let's do this *after* our own code runs -- we want to block distro stuff as
> little as possible.

This spot should be the flow where we know it's not a Distribution or Campaign event. We are early bailing.

> Did you determine whether there's any chance of their onReceive method
> throwing?

I do not know.

> Also, why did you opt to init the Adjust SDK in GeckoApplication rather than
> right here? This seems like it'd be the best place.

I just followed the suggested practices in the Adjust docs. We can tweak that later. If some advertisers don't use INSTALL_REFERRER, we'll need to use GeckoApplication anyway or the ad/install will not be tracked.

Comment 15

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #14)

> Maybe I needed a full clobber when switching?

You definitely need to re-configure. Just changing the mozconfig or confvars and doing an incremental build won't be enough.


> If we get the process failing, as we expect it should, I'll probably just
> always add the source files to the 3rd party JAR and just use the
> MOZ_INSTALL_TRACKING in the code to control using/not_using the SDK.

Sounds fine to me. I'm happy to spend time (when we're all less in a crunch) on a follow-up to make this more compartmentalized, given that we'll be turning this off altogether when there's no campaign running.


> > Did you determine whether there's any chance of their onReceive method
> > throwing?
> 
> I do not know.

Best to stick a try..catch around it just in case. I really really don't want to find that we're crashing on first run in some odd configuration.


> > Also, why did you opt to init the Adjust SDK in GeckoApplication rather than
> > right here? This seems like it'd be the best place.
> 
> I just followed the suggested practices in the Adjust docs. We can tweak
> that later. If some advertisers don't use INSTALL_REFERRER, we'll need to
> use GeckoApplication anyway or the ad/install will not be tracked.

I think their recommendation is based on tracking actions throughout the app; the Application onCreate is guaranteed to run before any other code. In our case we want it to be as late as possible, because we know we'll only be sending one ping.

If we don't think INSTALL_REFERRER will always apply, then let's come up with a place to put this that won't negatively affect every cold startup -- a post-distribution task (like our tasks for adding default bookmarks) might be a good option.

Again, a follow-up will do for now if you can't spend the time.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: install_tracking v0.2

I found I had a piece of this patch stuck in a different patch. Yes, "qnew before qref" disease.

This patch has all the pieces in it. That part in question was from confvars.sh which now looks like:

+# Enable install tracking SDK
+# if we have Google Play support, provided by MOZ_NATIVE_DEVICES
+if test "$RELEASE_BUILD"; then
+if test "$MOZ_NATIVE_DEVICES"; then
+  MOZ_INSTALL_TRACKING=1
+fi
+fi

Because using | MOZ_INSTALL_TRACKING=$(RELEASE_BUILD) | never worked right. Also tried | MOZ_INSTALL_TRACKING=RELEASE_BUILD | but didn't work. I think it has to do with "being defined vs being defined as a value".

Anyway, the above code block worked right for nightly and release.

Comment 17

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8588695 [details] [diff] [review]
install_tracking v0.2

Review of attachment 8588695 [details] [diff] [review]:
-----------------------------------------------------------------

Unfortunately this won't work when the flag is off.  There are other cases like this: the crash reporter and the native devices stuff.  Preprocessing an always-included helper is quick and dirty and regresses our nice preprocessed story.  Including either a real or a stub helper is slightly nicer but makes IDE integration slightly harder.  I prefer the latter but leave it to your schedule.

::: configure.in
@@ +3955,5 @@
>  MOZ_ANDROID_SEARCH_ACTIVITY=
>  MOZ_ANDROID_DOWNLOADS_INTEGRATION=
>  MOZ_ANDROID_MLS_STUMBLER=
>  MOZ_ANDROID_SHARE_OVERLAY=
> +MOZ_INSTALL_TRACKING=

I have a mild preference for marking this as Android only, and a mild preference to have the flag describe the tech rather than the behaviour: include Adjust SDK.  But I can't be bothered.

::: mobile/android/base/distribution/ReferrerReceiver.java
@@ +9,5 @@
>  import org.mozilla.gecko.GeckoAppShell;
>  import org.mozilla.gecko.GeckoEvent;
>  import org.mozilla.gecko.mozglue.RobocopTarget;
>  
> +import com.adjust.sdk.AdjustReferrerReceiver;

This integration style will never work if Adjust is not compiled in.  I guess you can add an optionally compiled file, or maybe we have to pre-process for this?  Or we do it dynamically with introspection?

I think I'd lean to a preprocessed helper file that has empty methods where appropriate :(  Better than preprocessing big files.

::: mobile/android/config/proguard/proguard.cfg
@@ +218,5 @@
>  # Don't print spurious warnings from the support library.
>  # See: http://stackoverflow.com/questions/22441366/note-android-support-v4-text-icucompatics-cant-find-dynamically-referenced-cl
>  -dontnote android.support.**
> +
> +# Needed to keep some constants in the install tracking library

Move this stuff into adjust-keeps.cfg, like play-services-keeps.cfg.

Comment 18

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: bz://1143888/nalexander

/r/6771 - Bug 1143888 - "Integrate the Adjust campaign tracking SDK into Firefox" [r=mark.finkle]
/r/6773 - Bug 1143888 - Build system updates.

Pull down these commits:

hg pull -r b29ecf386eff09830e6b291af901dda9d45baf0a https://reviewboard-hg.mozilla.org/gecko/

Comment 19

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8590078 [details]
MozReview Request: bz://1143888/nalexander

/r/6771 - Bug 1143888 - "Integrate the Adjust campaign tracking SDK into Firefox" [r=mark.finkle]
/r/6773 - Bug 1143888 - Build system updates.

Pull down these commits:

hg pull -r b29ecf386eff09830e6b291af901dda9d45baf0a https://reviewboard-hg.mozilla.org/gecko/

Comment 20

[Nick Alexander :nalexander [he/him]]

mfinkle: for testing, you don't need a full clobber.  You get enough with just:

(touch confvars.sh)
rm -rf objdir-droid/mobile/android/base && ./mach configure
./mach build mobile/android

You can verify sensible things happen at build time by trying, for example

find objdir-droid/mobile/android/base -iname '*Adjust*.class'

You can inspect the variables from the build system by interpreting

objdir-droid/config.status

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8590078 [details]
MozReview Request: bz://1143888/nalexander

Does review-board no longer set r+? Very frustrated review-board as a "reviewer".

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

See comments about duplicate google-api code that I added in 3 places now. Very frustrated with review-board.

Comment 23

[Nick Alexander :nalexander [he/him]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4a41c1193d9

Comment 24

[Nick Alexander :nalexander [he/him]]

https://hg.mozilla.org/integration/fx-team/rev/f76f02793f7a

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f76f02793f7a

Comment 26

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8590078 [details]
MozReview Request: bz://1143888/nalexander

Approval Request Comment
[Feature/regressing bug #]: 38 campaign tracking.
[User impact if declined]: very little, but marketing will be disappointed that they can't track their investment.
[Describe test coverage new/current, TreeHerder]: essentially none.  We need to get this into a build, with secrets (see https://bugzilla.mozilla.org/show_bug.cgi?id=1152871) as soon as possible for QA and mfinkle to test. 
[Risks and why]: quite low.  Builds in both configurations (on and off).
[String/UUID change made/needed]: none.

We'll want the single patch uplifted to aurora and beta; since we can't flag a landed HG commit, I've flagged the RB request.

Comment 27

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8590078 [details]
MozReview Request: bz://1143888/nalexander

Approval Request Comment
[Feature/regressing bug #]: 38 campaign tracking.
[User impact if declined]: very little, but marketing will be disappointed that they can't track their investment.
[Describe test coverage new/current, TreeHerder]: essentially none.  We need to get this into a build, with secrets (see https://bugzilla.mozilla.org/show_bug.cgi?id=1152871) as soon as possible for QA and mfinkle to test. 
[Risks and why]: quite low.  Builds in both configurations (on and off).
[String/UUID change made/needed]: none.

We'll want the single patch uplifted to aurora and beta; since we can't flag a landed HG commit, I've flagged the RB request.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

(In reply to Nick Alexander :nalexander from comment #27)
> Comment on attachment 8590078 [details]
> MozReview Request: bz://1143888/nalexander
> 
> Approval Request Comment
> [Feature/regressing bug #]: 38 campaign tracking.
I guess you are talking about the "spring" release (38.0.5), not 38.0.0.
If it is the later, this patch seems very big for that late in the cycle (even if it ships thirdparty code)...

Comment 29

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Sylvestre Ledru [:sylvestre] from comment #28)
> (In reply to Nick Alexander :nalexander from comment #27)
> > Comment on attachment 8590078 [details]
> > MozReview Request: bz://1143888/nalexander
> > 
> > Approval Request Comment
> > [Feature/regressing bug #]: 38 campaign tracking.
> I guess you are talking about the "spring" release (38.0.5), not 38.0.0.
> If it is the later, this patch seems very big for that late in the cycle
> (even if it ships thirdparty code)...

This code is required for the "spring" release. We had to do privacy and data audits before moving ahead. Yes, it's late, but it's a required addition.

Comment 30

[Benjamin Smedberg]

For data collection, I still need the final review in the form of a patch to in-tree docs that enumerate the data that this collects.

Comment 31

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #30)
> For data collection, I still need the final review in the form of a patch to
> in-tree docs that enumerate the data that this collects.

Got an example of what you mean?

Comment 32

[Benjamin Smedberg]

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/docs/uitelemetry.rst (which becomes https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/mobile/android/base/fennec/uitelemetry.html)

And also http://mxr.mozilla.org/mozilla-central/source/services/healthreport/docs/identifiers.rst

This needs to document the endpoints that data is sent to, the circumstances under which it is sent, and the datapoints. Normally this would also include the wire format, but since the SDK controls that I don't think we need that in this case.

Comment 33

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8590078 [details]
MozReview Request: bz://1143888/nalexander

OK, we have time for the 38.0.5 cycle then.

Comment 34

[Nick Alexander :nalexander [he/him]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #32)
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/docs/
> uitelemetry.rst (which becomes
> https://ci.mozilla.org/job/mozilla-central-docs/Tree_Documentation/mobile/
> android/base/fennec/uitelemetry.html)
> 
> And also
> http://mxr.mozilla.org/mozilla-central/source/services/healthreport/docs/
> identifiers.rst
> 
> This needs to document the endpoints that data is sent to, the circumstances
> under which it is sent, and the datapoints. Normally this would also include
> the wire format, but since the SDK controls that I don't think we need that
> in this case.

rnewman, mfinkle: in the course of your investigations did you already determine the data sent from device to the Adjust endpoint?  If so, can you provide it to me?  If not, I'll collect this myself and start a documentation patch.

Comment 35

[Nick Alexander :nalexander [he/him]]

rnewman, mfinkle: https://bugzilla.mozilla.org/show_bug.cgi?id=1143888#c34

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e831b9c65796

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/fd7a566bec99

Comment 38

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #35)
> rnewman, mfinkle: https://bugzilla.mozilla.org/show_bug.cgi?id=1143888#c34

Sent via email.

Comment 39

[Nick Alexander :nalexander [he/him]]

Hi lmandel: this is the main landing for integrating the Adjust SDK, which tracks Firefox for Android installs.  As you can see from https://bugzilla.mozilla.org/show_bug.cgi?id=1143888#c37, it has tracked 38.0.5 but not 38.0 (and I believe that all sub-tickets have done the same).

To get an idea of the feature, I recommend reading the documentation at

https://gecko.readthedocs.org/en/latest/mobile/android/base/fennec/adjust.html

Comment 40

[Lawrence Mandel [:lmandel] (use needinfo)]

Thanks for flagging Nick. Sylvestre is already on top of this for the 38.0.5 release.

Release Note Request (optional, but appreciated)
[Why is this notable]: Addition of campaign tracking to Fennec. I don't think this has a direct user benefit but is the type of change that we should be transparent about making.
[Suggested wording]: Support for Android campaign tracking with the Adjust SDK 
[Links (documentation, blog post, etc)]:
Would be good to include a link to documentation (perhaps on Sumo) that describes why we're making this change, the impact on user privacy, and how a user can opt-out (if this is possible).

Nick, Mark - Can one of you please help with the wording for the note and the documentation?

Comment 41

[Mark Finkle (:mfinkle) (use needinfo?)]

Winston Bowden has a mailing list / blog post ready to go.

Comment 42

[Nick Alexander :nalexander [he/him]]

(In reply to Mark Finkle (:mfinkle) from comment #41)
> Winston Bowden has a mailing list / blog post ready to go.

Sweet!  Will Winston also handle the relnote?

For everybody's situational awareness, Bug 1159277 landed some technically focused documentation, which is at

https://gecko.readthedocs.org/en/latest/mobile/android/base/fennec/adjust.html

Comment 43

[Liz Henry (:lizzard) (relman/hg->git project)]

Release Note Request (optional, but appreciated)
[Why is this notable]:
[Suggested wording]:
[Links (documentation, blog post, etc)]:

Sylvestre, I didn't notice this is going to be released in 38.0.5.  I removed the relnote from 39 on Lawrence's suggestion that it should be in 38.0.5 instead.

Comment 44

[Sylvestre Ledru [:Sylvestre]]

Yes. I asked for a wording for this as I am not sure what to write.

Comment 45

[Sylvestre Ledru [:Sylvestre]]

Added to the release notes with "Integrated Adjust SDK to measure aggregate installs" as wording.
We will also probably add a blog post to explain how we respect our user privacy.

Comment 47

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1143888 - "Integrate the Adjust campaign tracking SDK into Firefox" [r=mark.finkle]

Comment 48

[Nick Alexander :nalexander [he/him]]

Attachment: MozReview Request: Bug 1143888 - Build system updates.